Apply suggestions from code review

Co-authored-by: jonathan-r-thorpe <[email protected]>

Development/nmos/authorization_behaviour.cpp

@@ -279,6 +279,8 @@ namespace nmos
                 case authorization_operation:
                     // fetch public keys
                     // fetch access token within 1/2 token life time interval.
+                    // authorization_operation will block until an error occurs, or shutdown
+                    // on shutdown, enclosing for loop will exit
                     details::authorization_operation(model, authorization_state, load_ca_certificates, load_rsa_private_keys, false, gate);
 
                     // reaching here indicates there has been a failure within the authorization operation,
@@ -290,6 +292,9 @@ namespace nmos
                 case authorization_operation_with_immediate_token_fetch:
                     // fetch public keys
                     // immediately fetch access token
+                    // authorization_operation will block until an error occurs, or shutdown
+                    // on shutdown, enclosing for loop will exit
+
                     details::authorization_operation(model, authorization_state, load_ca_certificates, load_rsa_private_keys, true, gate);
 
                     // reaching here indicates there has been a failure within the authorization operation,

Development/nmos/authorization_operation.h

@@ -56,7 +56,7 @@ namespace nmos
             // see https://tools.ietf.org/html/rfc7591#section-3.1
             bool client_registration(nmos::base_model& model, nmos::experimental::authorization_state& authorization_state, nmos::load_ca_certificates_handler load_ca_certificates, nmos::experimental::save_authorization_client_handler client_registered, slog::base_gate& gate);
 
-            // start authorization code workflow
+            // start authorization code flow
             // see https://tools.ietf.org/html/rfc8252#section-4.1
             bool authorization_code_flow(nmos::base_model& model, nmos::experimental::authorization_state& authorization_state, nmos::experimental::request_authorization_code_handler request_authorization_code, slog::base_gate& gate);
 
@@ -65,7 +65,7 @@ namespace nmos
             // fetch the bearer access token for the required scope(s) to access the protected APIs
             // see https://specs.amwa.tv/is-10/releases/v1.0.0/docs/4.2._Behaviour_-_Clients.html#requesting-a-token
             // see https://specs.amwa.tv/is-10/releases/v1.0.0/docs/4.2._Behaviour_-_Clients.html#accessing-protected-resources
-            // fetch the Token Issuer(authorization server)'s public keys fpr validating the incoming bearer access token
+            // fetch the Token Issuer(authorization server)'s public keys for validating the incoming bearer access token
             // see https://specs.amwa.tv/is-10/releases/v1.0.0/docs/4.5._Behaviour_-_Resource_Servers.html#public-keys
             void authorization_operation(nmos::base_model& model, nmos::experimental::authorization_state& authorization_state, nmos::load_ca_certificates_handler load_ca_certificates, load_rsa_private_keys_handler load_rsa_private_keys, bool immediate_token_fetch, slog::base_gate& gate);
 

Development/nmos/authorization_redirect_api.cpp

@@ -203,9 +203,8 @@ namespace nmos
                     client_assertion_lifespan = std::chrono::seconds(nmos::experimental::fields::authorization_request_max(settings));
                 });
 
-                // The Authorization server may redirect error back due to something have went wrong
-                // such as resource owner rejects the request or the developer did something wrong
-                // when creating the Authorization request
+                // The authorization server may redirect an error back to this endpoint due to error conditions
+                // such as resource owner rejecting the request, or invalid authorization request
                 {
                     auto lock = authorization_state.write_lock(); // in order to update shared state
                     try
@@ -220,7 +219,7 @@ namespace nmos
                     }
                     catch (const authorization_flow_exception& e)
                     {
-                        slog::log<slog::severities::error>(gate, SLOG_FLF) << "Authorization flow token request Authorization Flow error: " << utility::us2s(e.error.name) << " description: " << utility::us2s(e.description);
+                        slog::log<slog::severities::error>(gate, SLOG_FLF) << "Authorization flow token request authorization flow error: " << utility::us2s(e.error.name) << " description: " << utility::us2s(e.description);
                         result = details::make_authorization_flow_error_response(status_codes::BadRequest, e.error.name, e.description);
                         authorization_state.authorization_flow = authorization_state::failed;
                     }

Development/nmos/authorization_state.h

@@ -39,7 +39,7 @@ namespace nmos
 
         struct authorization_state
         {
-            // mutex to be used to protect the members of the settings from simultaneous access by multiple threads
+            // mutex to be used to protect the members of the authorization_state from simultaneous access by multiple threads
             mutable nmos::mutex mutex;
 
             // authorization code flow settings
@@ -64,7 +64,7 @@ namespace nmos
 
             // map of issuer (authorization server) to jwt_validator set for access token validation
             nmos::experimental::issuers issuers;
-            // the authorization server which is currently connected to
+            // currently connected authorization server
             web::uri authorization_server_uri;
 
             // OAuth 2.0 bearer token to access authorizaton protected APIs

Development/nmos/authorization_utils.cpp

@@ -77,7 +77,7 @@ namespace nmos
             // get issuer version
             api_version version(const web::uri& issuer)
             {
-                // issuer uri should be like "https://server.example.com/{version}
+                // issuer uri should be of the form "https://server.example.com/{version}"
                 api_version ver{ api_version{} };
                 if (!issuer.is_path_empty())
                 {

Development/nmos/certificate_handlers.cpp

@@ -123,7 +123,7 @@ namespace nmos
     }
 
     // construct callback to load RSA private keys from file based on settings, see nmos/certificate_settings.h
-    // require for OAuth client which is using Private Key JWT as the requested authentication method for the token endpoint
+    // required for OAuth client which is using Private Key JWT as the requested authentication method for the token endpoint
     load_rsa_private_keys_handler make_load_rsa_private_keys_handler(const nmos::settings& settings, slog::base_gate& gate)
     {
         // load the server private keys from files

Development/nmos/client_utils.cpp

@@ -172,7 +172,7 @@ namespace nmos
         return config;
     }
 
-    // construct client config based on settings, e.g. using the specified proxy
+    // construct client config based on settings, e.g. using the specified proxy and OCSP config
     // with the remaining options defaulted, e.g. request timeout
     web::http::client::http_client_config make_http_client_config(const nmos::settings& settings, load_ca_certificates_handler load_ca_certificates, slog::base_gate& gate)
     {

Development/nmos/events_ws_api.cpp

@@ -79,7 +79,7 @@ namespace nmos
                 const auto& settings = model.settings;
 
                 web::uri token_issuer;
-                // note: the ws_validate_authorization returns the token_issuer via function parameter
+                // note: ws_validate_authorization returns the token_issuer via function parameter
                 const auto result = nmos::experimental::ws_validate_authorization(req, nmos::experimental::scopes::events, nmos::get_host_name(settings), token_issuer, access_token_validation, gate_);
                 if (!result)
                 {

Development/nmos/jwk_utils.cpp

@@ -124,7 +124,7 @@ namespace nmos
                 using ssl::experimental::BIO_ptr;
 
                 // supported Elliptic-Curve types
-                // see https://tools.ietf.org/search/rfc4492#appendix-A
+                // see https://tools.ietf.org/html/rfc4492#appendix-A
                 const std::map<utility::string_t, int> curve =
                 {
                     { U("P-256"), NID_X9_62_prime256v1 },

Development/nmos/jwt_validator_impl.cpp

@@ -107,7 +107,7 @@ namespace nmos
                         }
                     }
 
-                    // reaching here, there must be because no matching public key to validate the access token
+                    // reaching here indicates there is no matching public key to validate the access token
 
                     // "Where a Resource Server has no matching public key for a given token, it SHOULD attempt to obtain the missing public key via the the token iss
                     // claim as specified in RFC 8414 section 3. In cases where the Resource Server needs to fetch a public key from a remote Authorization Server it
@@ -186,8 +186,8 @@ namespace nmos
 
                             if (segments.size() >= aud_segments.size() && aud_segments.size())
                             {
-                                // token audience got to be in wildcard domain name format, leftmost is a "*" charcater
-                                // if not it is not going to match
+                                // in order to match the token audience has to be in wildcard domain name format
+                                // with a leftmost "*" character.
                                 // see https://tools.ietf.org/html/rfc4592#section-2.1.1
                                 if (aud_segments[0] != "*")
                                 {
@@ -219,7 +219,7 @@ namespace nmos
                     }
 
                     // scope optional
-                    // If scope claim does not contain the expected scope, the Resource Server reject the token.
+                    // If scope claim does not contain the expected scope, the Resource Server will reject the token.
                     // see https://specs.amwa.tv/is-10/releases/v1.0.0/docs/4.4._Behaviour_-_Access_Tokens.html#scope
                     auto verify_scope = [&decoded_token](const nmos::experimental::scope& scope)
                     {
@@ -384,7 +384,7 @@ namespace nmos
                     using namespace jwt::traits;
 
                     auto decoded_token = jwt::decode<nlohmann_json>(utility::us2s(token));
-                    // token does not guarantee to have client_id
+                    // token is not guaranteed to have a client_id
                     // see https://specs.amwa.tv/is-10/releases/v1.0.0/docs/4.4._Behaviour_-_Access_Tokens.html#client_id
                     if (decoded_token.has_payload_claim("client_id"))
                     {