From 5ee5b307c79bc5da5d03ab784fec85d92aa8f570 Mon Sep 17 00:00:00 2001 From: Simon Lo Date: Fri, 8 Dec 2023 01:14:01 +0000 Subject: [PATCH] Apply suggestions from code review Co-authored-by: jonathan-r-thorpe <64410119+jonathan-r-thorpe@users.noreply.github.com> --- Development/cpprest/access_token_error.h | 2 +- Development/nmos/api_utils.cpp | 8 +++---- Development/nmos/authorization.cpp | 2 +- Development/nmos/authorization_behaviour.cpp | 22 ++++++++++---------- Development/nmos/authorization_handlers.cpp | 6 +++--- Development/nmos/authorization_handlers.h | 2 +- Development/nmos/authorization_operation.cpp | 18 ++++++++-------- 7 files changed, 30 insertions(+), 30 deletions(-) diff --git a/Development/cpprest/access_token_error.h b/Development/cpprest/access_token_error.h index 6ed7ceed0..0fa94f2a3 100644 --- a/Development/cpprest/access_token_error.h +++ b/Development/cpprest/access_token_error.h @@ -20,7 +20,7 @@ namespace web // "application/x-www-form-urlencoded" format" // see https://tools.ietf.org/html/rfc6749#section-4.1.2.1 - // for diret error + // for direct error: // If the access token request is invalid or unauthorized // "The authorization server responds with an HTTP 400 (Bad Request) // status code(unless specified otherwise) and includes the following diff --git a/Development/nmos/api_utils.cpp b/Development/nmos/api_utils.cpp index 50351beb8..36602b626 100644 --- a/Development/nmos/api_utils.cpp +++ b/Development/nmos/api_utils.cpp @@ -822,12 +822,12 @@ namespace nmos utility::string_t error_description{}; // If the request lacks any authentication information (e.g., the client // was unaware that authentication is necessary or attempted using an - // unsupported authentication method), the resource server SHOULD NOT - // include an error code or other error information. + // unsupported authentication method), the resource server SHOULD NOT + // include an error code or other error information. // - // For example : + // For example : // - // HTTP / 1.1 401 Unauthorized + // HTTP / 1.1 401 Unauthorized // WWW - Authenticate : Bearer realm = "example" // see https://tools.ietf.org/html/rfc6750#section-3.1 if (error.value != nmos::experimental::authorization_error::without_authentication) diff --git a/Development/nmos/authorization.cpp b/Development/nmos/authorization.cpp index 46bd68c99..0a3510d03 100644 --- a/Development/nmos/authorization.cpp +++ b/Development/nmos/authorization.cpp @@ -43,7 +43,7 @@ namespace nmos slog::log(gate, SLOG_FLF) << "Test token expiry error: " << e.what(); } - // reaching here, token validation has failed, treat it as expired + // reaching here indicates token validation has failed so treat it as expired return true; } diff --git a/Development/nmos/authorization_behaviour.cpp b/Development/nmos/authorization_behaviour.cpp index 1d59c4b53..947ddb38a 100644 --- a/Development/nmos/authorization_behaviour.cpp +++ b/Development/nmos/authorization_behaviour.cpp @@ -80,7 +80,7 @@ namespace nmos std::default_random_engine discovery_backoff_engine(discovery_backoff_seeder); double discovery_backoff = 0; - // load authorization clients metadata to cache + // load authorization client's metadata to cache if (load_authorization_clients) { const auto auth_clients = load_authorization_clients(); @@ -138,10 +138,10 @@ namespace nmos // reterive client metadata from cache const auto client_metadata = nmos::experimental::get_client_metadata(authorization_state); - // is it not a scopeless client (where scopeless client doesn't access any protected APIs, i.e. doesn't require to register to Authorization server) + // does the client have a scope? A client without a scope is one that doesn't access any protected APIs (i.e. client isn't required to register with Authorization server). if (with_read_lock(model.mutex, [&] { return details::scopes(client_metadata, nmos::experimental::authorization_scopes::from_settings(model.settings)).size(); })) { - // is the client already registered to Authorization server, i.e. found it in cache + // is the client already registered to Authorization server? (i.e. found it in cache). if (!client_metadata.is_null()) { // no token or token expired @@ -180,8 +180,8 @@ namespace nmos { // if OpenID Connect Authorization server is used, client status can be obtained via the Client Configuration Endpoint // "The Client Configuration Endpoint is an OAuth 2.0 Protected Resource that MAY be provisioned by the server for a - // specific Client to be able to view and update its registered information." - // see 3.2 of https://openid.net/specs/openid-connect-registration-1_0.html#ClientConfigurationEndpoint + // specific Client to be able to view and update its registered information." + // see https://openid.net/specs/openid-connect-registration-1_0.html#ClientConfigurationEndpoint // registration_access_token // OPTIONAL. Registration Access Token that can be used at the Client Configuration Endpoint to perform subsequent operations upon the // Client registration. @@ -208,7 +208,7 @@ namespace nmos } else { - // no registration_access_token and registration_client_uri found, treat it has connected with a non-OpenID Connect server + // no registration_access_token and registration_client_uri found, treat it as if connected with a non-OpenID Connect server // start grant flow based on what been defined in the settings // hmm, maybe use of the OpenID API to extend the client lifespan instead of re-registration mode = is_client_expired() ? client_registration // client registration @@ -227,13 +227,13 @@ namespace nmos } else { - // client has not been registered to the Authorization server yet + // client has not been registered with the Authorization server yet mode = client_registration; } } else { - // scope-less client, not require to obtain access token + // client does not have a scope therefore not require to obtain access token mode = authorization_operation; } } @@ -278,10 +278,10 @@ namespace nmos case authorization_operation: // fetch public keys - // fetch access token in 1/2 token life time interval + // fetch access token within 1/2 token life time interval. details::authorization_operation(model, authorization_state, load_ca_certificates, load_rsa_private_keys, false, gate); - // reaching here, there must be failure within the authorization operation, + // reaching here indicates there has been a failure within the authorization operation, // start the authorization sequence again on next available Authorization server authorization_service_error = true; mode = request_authorization_server_metadata; @@ -292,7 +292,7 @@ namespace nmos // immediately fetch access token details::authorization_operation(model, authorization_state, load_ca_certificates, load_rsa_private_keys, true, gate); - // reaching here, there must be failure within the authorization operation, + // reaching here indicates there has been a failure within the authorization operation, // start the authorization sequence again on next available Authorization server authorization_service_error = true; mode = request_authorization_server_metadata; diff --git a/Development/nmos/authorization_handlers.cpp b/Development/nmos/authorization_handlers.cpp index 0674dfefc..6b155b4b8 100644 --- a/Development/nmos/authorization_handlers.cpp +++ b/Development/nmos/authorization_handlers.cpp @@ -182,10 +182,10 @@ namespace nmos } // construct callback to start the authorization code flow request on a browser - // it is required for those OAuth client which is using the Authorization Code Flow to obtain the access token + // this is required for OAuth clients which use Authorization Code Flow to obtain the access token // note: as it is not easy to specify the 'content-type' used in the browser programmatically, this can be easily // fixed by installing a browser header modifier - // such extension e.g. ModHeader can be used to add the missing 'content-type' header accordingly + // extensions such as ModHeader can be used to add the missing 'content-type' header: // for Windows https://chrome.google.com/webstore/detail/modheader-modify-http-hea/idgpnmonknjnojddfkpgkljpfnnfcklj // for Linux https://addons.mozilla.org/en-GB/firefox/addon/modheader-firefox/ request_authorization_code_handler make_request_authorization_code_handler(slog::base_gate& gate) @@ -227,7 +227,7 @@ namespace nmos try { - // if jwt_validator has not already set up, treat it as no public keys to validate token + // if jwt_validator is not already set up, assume no public keys to validate token if (issuer->second.jwt_validator.is_initialized()) { // do access token basic validation, including token schema validation and token issuer public keys validation diff --git a/Development/nmos/authorization_handlers.h b/Development/nmos/authorization_handlers.h index 31151e79f..b21a27dd7 100644 --- a/Development/nmos/authorization_handlers.h +++ b/Development/nmos/authorization_handlers.h @@ -36,7 +36,7 @@ namespace nmos { succeeded, without_authentication, // failure: access protected resource request without authentication - insufficient_scope, // failure: access protected resource request higher privileges + insufficient_scope, // failure: access protected resource request requires higher privileges no_matching_keys, // failure: no matching keys for the token validation failed // failure: access protected resource request with authentication but failed }; diff --git a/Development/nmos/authorization_operation.cpp b/Development/nmos/authorization_operation.cpp index 4b9cf3238..c7f14ffcf 100644 --- a/Development/nmos/authorization_operation.cpp +++ b/Development/nmos/authorization_operation.cpp @@ -100,8 +100,8 @@ namespace nmos // code_verifier = high-entropy cryptographic random STRING using the // unreserved characters[A - Z] / [a - z] / [0 - 9] / "-" / "." / "_" / "~" - // from Section 2.3 of[RFC3986], with a minimum length of 43 characters - // and a maximum length of 128 characters + // from Section 2.3 of[RFC3986], with a minimum length of 43 characters + // and a maximum length of 128 characters // see https://tools.ietf.org/html/rfc7636#section-4.1 { utility::nonce_generator generator(128); @@ -136,7 +136,7 @@ namespace nmos return ub.to_uri(); } - // it is used to strip the trailing dot of the FQDN if it is presented + // used to strip the trailing dot of the FQDN if it is presented utility::string_t strip_trailing_dot(const utility::string_t& host_) { auto host = host_; @@ -219,7 +219,7 @@ namespace nmos } else { - // Some services don't return 'token_type' while it's required by OAuth 2.0 spec: + // Some services don't return 'token_type' even though it's required by the OAuth 2.0 spec: // http://tools.ietf.org/html/rfc6749#section-5.1 // As workaround we act as if 'token_type=bearer' was received. result.set_token_type(oauth2_strings::bearer); @@ -301,9 +301,9 @@ namespace nmos // validate server metadata authapi_validator().validate(metadata, experimental::make_authapi_auth_metadata_schema_uri(version)); // may throw json_exception - // hmm, verify Authorization server meeting the minimum client requirement + // hmm, verify Authorization server meets the minimum client requirement. - // is the required response_types supported by the Authorization server + // are the required response_types supported by the Authorization server? std::set response_types = { response_types::code }; if (grants.end() != std::find_if(grants.begin(), grants.end(), [](const web::http::oauth2::experimental::grant_type& grant) { return grant_types::implicit == grant; })) { @@ -327,7 +327,7 @@ namespace nmos // scopes_supported is optional if (scopes.size() && metadata.has_array_field(nmos::experimental::fields::scopes_supported)) { - // is the required scopes supported by the Authorization server + // are the required scopes supported by the Authorization server? const auto supported = std::all_of(scopes.begin(), scopes.end(), [&](const nmos::experimental::scope& scope) { const auto& scopes_supported = nmos::experimental::fields::scopes_supported(metadata); @@ -336,7 +336,7 @@ namespace nmos }); if (!supported) { - slog::log(gate, SLOG_FLF) << "Request authorization server metadata error: server does not supporting all the required scopes: " << [&scopes]() { std::stringstream ss; for (auto scope : scopes) ss << utility::us2s(scope.name) << " "; return ss.str(); }(); + slog::log(gate, SLOG_FLF) << "Request authorization server metadata error: server does not support all the required scopes: " << [&scopes]() { std::stringstream ss; for (auto scope : scopes) ss << utility::us2s(scope.name) << " "; return ss.str(); }(); throw authorization_exception(); } } @@ -353,7 +353,7 @@ namespace nmos }); if (!supported) { - slog::log(gate, SLOG_FLF) << "Request authorization server metadata error: server does not supporting all the required grants: " << [&grants]() { std::stringstream ss; for (auto grant : grants) ss << utility::us2s(grant.name) << " "; return ss.str(); }(); + slog::log(gate, SLOG_FLF) << "Request authorization server metadata error: server does not support all the required grants: " << [&grants]() { std::stringstream ss; for (auto grant : grants) ss << utility::us2s(grant.name) << " "; return ss.str(); }(); throw authorization_exception(); } }