address feedback: more grammar and format

Signed-off-by: Emelia Lei <[email protected]>

Author: emelialei88

src/groups/mqb/mqba/mqba_authenticator.cpp

@@ -43,7 +43,6 @@
 #include <bmqp_protocol.h>
 #include <bmqp_schemaeventbuilder.h>
 #include <bmqu_memoutstream.h>
-#include <bslmt_threadutil.h>
 
 // BDE
 #include <ball_log.h>
@@ -58,6 +57,7 @@
 #include <bsl_string_view.h>
 #include <bsl_vector.h>
 #include <bsla_annotations.h>
+#include <bslmt_threadutil.h>
 #include <bsls_timeinterval.h>
 
 namespace BloombergLP {
@@ -236,9 +236,9 @@ void Authenticator::authenticate(
                                           channel,
                                           context->authenticationContext());
 
-    // Return code processRc is rc_SUCCESS even when authentication fails,
-    // since we still need to continue sending the AuthenticationResponse back
-    // to the client.
+    // Return code processRc is rc_SUCCESS even when plugin authentication
+    // fails, since we still need to continue sending the
+    // AuthenticationResponse back to the client.
     if (processRc != rc_SUCCESS) {
         rc    = (processRc * 10) + rc_PROCESS_AUTHENTICATION_FAILED;
         error = processErrStream.str();
@@ -433,22 +433,22 @@ Authenticator::Authenticator(
     BlobSpPool*                         blobSpPool,
     bdlmt::EventScheduler*              scheduler,
     bslma::Allocator*                   allocator)
-: d_threadPool(bmqsys::ThreadUtil::defaultAttributes(),
+: d_allocator_p(allocator)
+, d_authnController_p(authnController)
+, d_threadPool(bmqsys::ThreadUtil::defaultAttributes(),
                k_MIN_THREADS,                                // min threads
                k_MAX_THREADS,                                // max threads
                bsls::TimeInterval(120).totalMilliseconds(),  // idle time
                allocator)
-, d_authnController_p(authnController)
 , d_blobSpPool_p(blobSpPool)
 , d_scheduler_p(scheduler)
 , d_isStarted(false)
-, d_allocator_p(allocator)
 {
     // PRECONDITIONS
+    BSLS_ASSERT_SAFE(d_allocator_p);
     BSLS_ASSERT_SAFE(d_authnController_p);
     BSLS_ASSERT_SAFE(d_blobSpPool_p);
     BSLS_ASSERT_SAFE(d_scheduler_p);
-    BSLS_ASSERT_SAFE(d_allocator_p);
 }
 
 /// Destructor
@@ -461,9 +461,10 @@ Authenticator::~Authenticator()
 
 int Authenticator::start(bsl::ostream& errorDescription)
 {
-    // PRECONDITIONS
-    BSLS_ASSERT_OPT(!d_isStarted &&
-                    "start() can only be called once on this object");
+    if (d_isStarted) {
+        errorDescription << "start() can only be called once on this object";
+        return -1;
+    }
 
     BALL_LOG_INFO << "Starting Authenticator";
 

src/groups/mqb/mqba/mqba_authenticator.h

@@ -75,7 +75,7 @@ namespace mqba {
 class Authenticator : public mqbnet::Authenticator {
   private:
     // CLASS-SCOPE CATEGORY
-    BALL_LOG_SET_CLASS_CATEGORY("MQBNET.AUTHENTICATIONCONTEXT");
+    BALL_LOG_SET_CLASS_CATEGORY("MQBA.AUTHENTICATOR");
 
   public:
     // TYPES
@@ -101,12 +101,15 @@ class Authenticator : public mqbnet::Authenticator {
   private:
     // DATA
 
-    /// Thread pool to run authentication and reauthentication tasks.
-    bdlmt::ThreadPool d_threadPool;
+    /// Allocator to use.
+    bslma::Allocator* d_allocator_p;
 
     /// Authentication Controller.
     mqbauthn::AuthenticationController* d_authnController_p;
 
+    /// Thread pool to run authentication and reauthentication tasks.
+    bdlmt::ThreadPool d_threadPool;
+
     BlobSpPool* d_blobSpPool_p;
 
     /// Used to track the duration of a valid authenticated connection.
@@ -117,9 +120,6 @@ class Authenticator : public mqbnet::Authenticator {
     /// True if this component is started.
     bool d_isStarted;
 
-    /// Allocator to use.
-    bslma::Allocator* d_allocator_p;
-
   private:
     // NOT IMPLEMENTED
 
@@ -181,7 +181,7 @@ class Authenticator : public mqbnet::Authenticator {
                       const bsl::shared_ptr<bmqio::Channel>& channel);
 
     /// Reauthenticate the connection using the `AuthenticationMessage`
-    /// stored in `context`.  If re-authentication fails, invoke
+    /// stored in `context`.  If reauthentication fails, invoke
     /// `initialConnectionCompleteCb` to close the `channel`. Also, update the
     /// state of `context` as appropriate.
     void reauthenticate(const AuthenticationContextSp&         context,
@@ -244,7 +244,7 @@ class Authenticator : public mqbnet::Authenticator {
     int authenticationOutbound(const AuthenticationContextSp& context)
         BSLS_KEYWORD_OVERRIDE;
 
-    /// Schedule a re-authentication job in the thread pool using the
+    /// Schedule a reauthentication job in the thread pool using the
     /// specified `context` and `channel`.  Return 0 on success, or a
     /// non-zero error code and populate the specified `errorDescription`
     /// with a description of the error otherwise.

src/groups/mqb/mqba/mqba_clientsession.h

@@ -593,7 +593,8 @@ class ClientSession : public mqbnet::Session,
 
     /// Process the specified `event` received from the optionally specified
     /// `source` node.  Note that this method is the entry point for all
-    /// incoming events coming from the remote peer.
+    /// incoming events coming from the remote peer.  The behavior is undefined
+    /// unless `event` is not `AuthenticationEvent`.
     void processEvent(const bmqp::Event&   event,
                       mqbnet::ClusterNode* source = 0) BSLS_KEYWORD_OVERRIDE;
 

src/groups/mqb/mqba/mqba_sessionnegotiator.h

@@ -30,7 +30,7 @@
 /// Thread Safety                              {#mqba_sessionnegotiator_thread}
 /// =============
 /// This component is held by `InitialConnectionContext`, and its functions
-/// are called inly from there.  It is not thread safe.
+/// are called only from there.  It is not thread safe.
 
 // MQB
 #include <mqbconfm_messages.h>

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.cpp

@@ -67,9 +67,9 @@ AnonPassAuthenticationResult::lifetimeMs() const
 AnonPassAuthenticator::AnonPassAuthenticator(
     const mqbcfg::AuthenticatorPluginConfig* config,
     bslma::Allocator*                        allocator)
-: d_authenticatorConfig_p(config)
+: d_allocator_p(allocator)
+, d_authenticatorConfig_p(config)
 , d_isStarted(false)
-, d_allocator_p(allocator)
 {
     if (!config) {
         // No config is provided for an anonymous authenticator.
@@ -110,11 +110,12 @@ int AnonPassAuthenticator::authenticate(
     return 0;
 }
 
-int AnonPassAuthenticator::start(BSLA_UNUSED bsl::ostream& errorDescription)
+int AnonPassAuthenticator::start(bsl::ostream& errorDescription)
 {
-    // PRECONDITIONS
-    BSLS_ASSERT_OPT(!d_isStarted &&
-                    "start() can only be called once on this object");
+    if (d_isStarted) {
+        errorDescription << "start() can only be called once on this object";
+        return -1;
+    }
 
     d_isStarted = true;
 

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.h

@@ -21,10 +21,10 @@
 ///
 /// @brief Provide an default anonymous pass authenticator plugin.
 ///
-/// @bbref{mqbauthn::AnonPassAuthenticator} provides an authenticator plugin
-/// that always authenticates successfully, regardless of the input provided.
-/// It's used as a default authenticator for the anonymous credential when no
-/// other authenticator is configured.
+/// @bbref{mqbauthn::AnonPassAuthenticator} provides a built-in authenticator
+/// plugin that always authenticates successfully, regardless of the input
+/// provided. It's used as a default authenticator for the anonymous credential
+/// when no other authenticator is configured.
 /// @bbref{mqbauthn::AnonPassAuthenticationResult} and
 /// @bbref{mqbauthn::AnonPassAuthenticatorPluginFactory} are the corresponding
 /// result and factory classes for the authenticator plugin.
@@ -99,11 +99,12 @@ class AnonPassAuthenticator : public mqbplug::Authenticator {
     BALL_LOG_SET_CLASS_CATEGORY("MQBAUTHN.ANONPASSAUTHENTICATOR");
 
     // DATA
-    const mqbcfg::AuthenticatorPluginConfig* d_authenticatorConfig_p;
+    bslma::Allocator* d_allocator_p;
 
-    bool d_isStarted;
+    BSLA_UNUSED const mqbcfg::AuthenticatorPluginConfig*
+                      d_authenticatorConfig_p;
 
-    bslma::Allocator* d_allocator_p;
+    bool d_isStarted;
 
   private:
     // NOT IMPLEMENTED
@@ -114,7 +115,8 @@ class AnonPassAuthenticator : public mqbplug::Authenticator {
 
   public:
     // TRAITS
-    BSLMF_NESTED_TRAIT_DECLARATION(Authenticator, bslma::UsesBslmaAllocator)
+    BSLMF_NESTED_TRAIT_DECLARATION(AnonPassAuthenticator,
+                                   bslma::UsesBslmaAllocator)
 
     // CREATORS
 

src/groups/mqb/mqbauthn/mqbauthn_anonpasspluginlibrary.h

@@ -19,7 +19,8 @@
 
 /// @file mqbauthn_anonpasspluginlibrary.h
 ///
-/// @brief Provide library of anonymous pass authenticator plugin for broker.
+/// @brief Provide library of anonymous pass built-in authenticator plugin for
+/// the broker.
 ///
 /// @bbref{mqbauthn::PluginLibrary} provides the definition for the
 /// 'PluginLibrary' class, which represents and publishes anonymous pass

src/groups/mqb/mqbauthn/mqbauthn_authenticationcontroller.cpp

@@ -30,6 +30,7 @@
 
 // BDE
 #include <ball_log.h>
+#include <bdlb_string.h>
 #include <bsl_string.h>
 #include <bsl_string_view.h>
 #include <bsl_unordered_set.h>
@@ -41,6 +42,14 @@ namespace {
 
 typedef bsl::unordered_set<mqbplug::PluginFactory*> PluginFactories;
 
+inline bsl::string normalizeMechanism(bsl::string_view  mech,
+                                      bslma::Allocator* allocator = 0)
+{
+    bsl::string out(mech.begin(), mech.end(), allocator);
+    bdlb::String::toUpper(&out);
+    return out;
+}
+
 }  // close unnamed namespace
 
 // ------------------------------
@@ -58,17 +67,19 @@ AuthenticationController::AuthenticationController(
 
 int AuthenticationController::start(bsl::ostream& errorDescription)
 {
-    // PRECONDITIONS
-    BSLS_ASSERT_OPT(!d_isStarted &&
-                    "start() can only be called once on this object");
-
     enum RcEnum {
         // Enum for the various RC error categories
         rc_SUCCESS             = 0,
-        rc_DUPLICATE_MECHANISM = -1,
-        rc_INVALID_CONFIG      = -2
+        rc_ALREADY_STARTED     = -1,
+        rc_DUPLICATE_MECHANISM = -2,
+        rc_INVALID_CONFIG      = -3
     };
 
+    if (d_isStarted) {
+        errorDescription << "start() can only be called once on this object";
+        return rc_ALREADY_STARTED;
+    }
+
     bmqu::MemOutStream errorStream(d_allocator_p);
 
     BALL_LOG_INFO << "Starting AuthenticationController";
@@ -118,9 +129,12 @@ int AuthenticationController::start(bsl::ostream& errorDescription)
                 dynamic_cast<mqbplug::AuthenticatorPluginFactory*>(*factoryIt);
             AuthenticatorMp authenticator = factory->create(d_allocator_p);
 
+            const bsl::string normMech =
+                normalizeMechanism(authenticator->mechanism(), d_allocator_p);
+
             // Check if there's an authenticator with duplicate mechanism
             AuthenticatorMap::const_iterator cit = d_authenticators.find(
-                authenticator->mechanism());
+                normMech);
             if (cit != d_authenticators.cend()) {
                 errorDescription << "Attempting to create duplicate "
                                     "authenticator with mechanism '"
@@ -141,7 +155,7 @@ int AuthenticationController::start(bsl::ostream& errorDescription)
 
             // Add the authenticator into the collection
             d_authenticators.emplace(
-                authenticator->mechanism(),
+                normMech,
                 bslmf::MovableRefUtil::move(authenticator));
         }
 
@@ -167,14 +181,15 @@ int AuthenticationController::start(bsl::ostream& errorDescription)
             }
 
             // Add the authenticator into the collection
+            const bsl::string normMech =
+                normalizeMechanism(authenticator->mechanism(), d_allocator_p);
             d_authenticators.emplace(
-                authenticator->mechanism(),
+                normMech,
                 bslmf::MovableRefUtil::move(authenticator));
         }
     }
 
     d_isStarted = true;
-
     return rc_SUCCESS;
 }
 
@@ -214,7 +229,8 @@ int AuthenticationController::authenticate(
     int                rc = rc_SUCCESS;
     bmqu::MemOutStream errorStream(d_allocator_p);
 
-    AuthenticatorMap::const_iterator cit = d_authenticators.find(mechanism);
+    const bsl::string normMech = normalizeMechanism(mechanism, d_allocator_p);
+    AuthenticatorMap::const_iterator cit = d_authenticators.find(normMech);
     if (cit != d_authenticators.cend()) {
         const AuthenticatorMp& authenticator = cit->second;
         BALL_LOG_DEBUG << "AuthenticationController: "

src/groups/mqb/mqbauthn/mqbauthn_authenticationcontroller.h

@@ -17,6 +17,17 @@
 #ifndef INCLUDED_MQBAUTHN_AUTHENTICATIONCONTROLLER
 #define INCLUDED_MQBAUTHN_AUTHENTICATIONCONTROLLER
 
+/// @file mqbauthn_authenticationcontroller.h
+///
+/// @brief Provide a small utility class that orchestrates authenticator
+/// plugins.
+///
+/// @bbref{AuthenticationController} provides a small utility class that
+/// orchestrates authenticator plugins. Responsibilities include creating and
+/// holding plugin instances, tracking an optional anonymous credential, and
+/// offering start/stop and authenticate operations used by higher-level
+/// components.
+
 // MQB
 #include <mqbcfg_messages.h>
 #include <mqbplug_authenticator.h>
@@ -43,20 +54,23 @@ class AuthenticationController {
 
   private:
     // PRIVATE TYPES
-    typedef bslma::ManagedPtr<mqbplug::Authenticator> AuthenticatorMp;
-    typedef bsl::unordered_map<bsl::string_view, AuthenticatorMp>
-        AuthenticatorMap;
+    typedef bslma::ManagedPtr<mqbplug::Authenticator>        AuthenticatorMp;
+    typedef bsl::unordered_map<bsl::string, AuthenticatorMp> AuthenticatorMap;
 
     // DATA
 
-    /// Registered authenticators
-    /// Mapping an authentication mechanism to a mqbplug::Authenticator
+    /// Registered authenticator plugins.
+    /// Mapping an authentication mechanism to a mqbplug::Authenticator.
+    /// The mechanisms are stored in upper case to make the lookup
+    /// case-insensitive.
     AuthenticatorMap d_authenticators;
 
-    /// Anonymous credential
+    /// Anonymous credential. If set, this credential will be used for
+    /// authentication when the client does not provide any credential.
+    /// Default anonymous credential will be used if not set.
     bsl::optional<mqbcfg::Credential> d_anonymousCredential;
 
-    /// Used to instantiate 'Authenticator'
+    /// Used to instantiate Authenticator
     /// plugins at start-time.
     mqbplug::PluginManager* d_pluginManager_p;