Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Co-authored-by: jonathan-r-thorpe <[email protected]>
  • Loading branch information
@lo-simon @jonathan-r-thorpe
lo-simon and jonathan-r-thorpe authored Dec 8, 2023
1 parent 311bed6 commit 5ee5b30
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 30 deletions.
2 changes: 1 addition & 1 deletion Development/cpprest/access_token_error.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions Development/nmos/api_utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion Development/nmos/authorization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ namespace nmos
slog::log<slog::severities::warning>(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;
}

Expand Down
22 changes: 11 additions & 11 deletions Development/nmos/authorization_behaviour.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions Development/nmos/authorization_handlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Development/nmos/authorization_handlers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
18 changes: 9 additions & 9 deletions Development/nmos/authorization_operation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<web::http::oauth2::experimental::response_type> 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; }))
{
Expand All @@ -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);
Expand All @@ -336,7 +336,7 @@ namespace nmos
});
if (!supported)
{
slog::log<slog::severities::error>(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<slog::severities::error>(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();
}
}
Expand All @@ -353,7 +353,7 @@ namespace nmos
});
if (!supported)
{
slog::log<slog::severities::error>(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<slog::severities::error>(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();
}
}
Expand Down

0 comments on commit 5ee5b30

Please sign in to comment.