some changes after review

Author: zvonand

docs/en/operations/external-authenticators/tokens.md

@@ -8,30 +8,30 @@ import SelfManaged from '@site/docs/en/_snippets/_self_managed_only_no_roadmap.m
 
 ClickHouse users can be authenticated using tokens. This works in two ways:
 
-- Existing users (defined in `users.xml` or in local access control paths) can be authenticated with a token if this user can be `IDENTIFIED WITH jwt`. 
+- An existing user (defined in `users.xml` or in local access control paths) can be authenticated with a token if this user can be `IDENTIFIED WITH jwt`. 
 - Use the information from the token or from an external Identity Provider (IdP) as a source of user definitions and allow locally undefined users to be authenticated with a valid token.
 
 Although not all tokens are JWTs, under the hood both ways are treated as the same authentication method to maintain better compatibility.
 
 # Token Processors
 
 ## Configuration
-To use token-based authentication, add `token_processors` section to `config.xml` and define at least one token processor in it. 
+To use token-based authentication, add `token_processors` section to `config.xml` and define at least one token processor in it.
 Its contents are different for different token processor types.
 
 **Common parameters**
 - `type` -- type of token processor. Supported values: "JWT", "Azure", "OpenID". Mandatory. Case-insensitive.
 - `token_cache_lifetime` -- maximum lifetime of cached token (in seconds). Optional, default: 3600.
 - `username_claim` -- name of claim (field) that will be treated as ClickHouse username. Optional, default: "sub".
-- `groups_claim` -- Name of claim (field) that contains list of groups user belongs to. This claim will be looked up in the token itself (in case token is a valid JWT, e.g. in Keycloak) or in response from `/userinfo`. Optional, default: "groups".
+- `groups_claim` -- name of claim (field) that contains list of groups user belongs to. This claim will be looked up in the token itself (in case token is a valid JWT, e.g. in Keycloak) or in response from `/userinfo`. Optional, default: "groups".
 
-For each type, there are additional specific parameters. 
+For each type, there are additional specific parameters.
 If some parameters that are not required for current processor type are specified, they are ignored. 
 If there are conflicting parameters (e.g `algo` is specified together with `jwks_uri`), an exception will be thrown.
 
 ## JWT (JSON Web Token)
 
-JWT itself is a source of information about user. 
+JWT itself is a source of information about user.
 It is decoded locally and its integrity is verified using either static key or JWKS (JSON Web Key Set), either local or remote.
 
 `algo`, `static_jwks`/`static_jwks_file` and `jwks_uri` are defining different JWT processing workflows, and they cannot be specified together.
@@ -48,7 +48,7 @@ It is decoded locally and its integrity is verified using either static key or J
 </clickhouse>
 ```
 **Parameters:**
-- `algo` - Algorithm for validate signature. Mandatory. Supported values:
+- `algo` - Algorithm for signature validation. Mandatory. Supported values:
 
   | HMAC  | RSA   | ECDSA  | PSS   | EdDSA   |
     |-------| ----- | ------ | ----- | ------- |
@@ -168,7 +168,7 @@ Either `configuration_endpoint` or both `userinfo_endpoint` and `token_introspec
 Sometimes a token is a valid JWT. In that case token will be decoded and validated locally if configuration endpoint returns JWKS URI (or `jwks_uri` is specified alongside `userinfo_endpoint` and `token_introspection_endpoint`).
 
 ### Tokens cache
-To reduce number of requests to IdP, tokens are cached internally for no longer then `token_cache_lifetime` seconds.
+To reduce number of requests to IdP, tokens are cached internally for a maximum period of `token_cache_lifetime` seconds.
 If token expires sooner than `token_cache_lifetime`, then cache entry for this token will only be valid while token is valid.
 If token lifetime is longer than `token_cache_lifetime`, cache entry for this token will be valid for `token_cache_lifetime`. 
 
@@ -190,7 +190,7 @@ Example (goes into `users.xml`):
 </clickhouse>
 ```
 
-Here, the JWT payload must contain `["view-profile"]` on path `resource_access.account.roles`, otherwise authentication will not succeed even with a valid JWT. 
+Here, the JWT payload must contain `["view-profile"]` on path `resource_access.account.roles`, otherwise authentication will not succeed even with a valid JWT.
 
 :::note
 If `claims` is defined, this user will not be able to authenticate using opaque tokens, so, only JWT-based authentication will be available.
@@ -209,7 +209,7 @@ If `claims` is defined, this user will not be able to authenticate using opaque
 ```
 
 :::note
-JWT authentication cannot be used together with any other authentication method. The presence of any other sections like `password` alongside `jwt` will force ClickHouse to shut down.
+A user cannot have JWT authentication together with any other authentication method. The presence of any other sections like `password` alongside `jwt` will force ClickHouse to shut down.
 :::
 
 ## Enabling token authentication using SQL {#enabling-jwt-auth-using-sql}
@@ -219,10 +219,10 @@ Users with "JWT" authentication type cannot be created using SQL now.
 ## Identity Provider as an External User Directory {#idp-external-user-directory}
 
 If there is no suitable user pre-defined in ClickHouse, authentication is still possible: Identity Provider can be used as source of user information.
-To allow this, add `token` section to the `users_directories` section of the `config.xml` file. 
+To allow this, add `token` section to the `users_directories` section of the `config.xml` file.
 
 At each login attempt, ClickHouse tries to find the user definition locally and authenticate it as usual.
-If the user is not defined, ClickHouse will treat the user as externally defined and will try to validate the token and get user information from the specified processor.
+If a token is provided but the user is not defined, ClickHouse will treat the user as externally defined and will try to validate the token and get user information from the specified processor.
 If validated successfully, the user will be considered existing and authenticated. The user will be assigned roles from the list specified in the `roles` section. 
 All this implies that the SQL-driven [Access Control and Account Management](/docs/en/guides/sre/user-management/index.md#access-control) is enabled and roles are created using the [CREATE ROLE](/docs/en/sql-reference/statements/create/role.md#create-role-statement) statement.
 
@@ -232,7 +232,7 @@ All this implies that the SQL-driven [Access Control and Account Management](/do
 <clickhouse>
     <user_directories>
         <token>
-            <processor>processor_name</processor>
+            <processor>token_processor_name</processor>
             <common_roles>
                 <token_test_role_1 />
             </common_roles>

src/Access/Common/JWKSProvider.cpp

@@ -31,28 +31,28 @@ jwt::jwks<jwt::traits::kazuho_picojson> JWKSClient::getJWKS()
     }
 
     Poco::Net::HTTPResponse response;
-    std::ostringstream responseString;
+    std::string response_string;
 
     Poco::Net::HTTPRequest request{Poco::Net::HTTPRequest::HTTP_GET, jwks_uri.getPathAndQuery()};
 
     if (jwks_uri.getScheme() == "https")
     {
         Poco::Net::HTTPSClientSession session = Poco::Net::HTTPSClientSession(jwks_uri.getHost(), jwks_uri.getPort());
         session.sendRequest(request);
-        std::istream & responseStream = session.receiveResponse(response);
-        if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK || !responseStream)
+        std::istream & response_stream = session.receiveResponse(response);
+        if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK || !response_stream)
             throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "Failed to get user info by access token, code: {}, reason: {}",
                 response.getStatus(), response.getReason());
-        Poco::StreamCopier::copyStream(responseStream, responseString);
+        Poco::StreamCopier::copyToString(response_stream, response_string);
     }
     else
     {
         Poco::Net::HTTPClientSession session = Poco::Net::HTTPClientSession(jwks_uri.getHost(), jwks_uri.getPort());
         session.sendRequest(request);
-        std::istream & responseStream = session.receiveResponse(response);
-        if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK || !responseStream)
+        std::istream & response_stream = session.receiveResponse(response);
+        if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK || !response_stream)
             throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "Failed to get user info by access token, code: {}, reason: {}", response.getStatus(), response.getReason());
-        Poco::StreamCopier::copyStream(responseStream, responseString);
+        Poco::StreamCopier::copyToString(response_stream, response_string);
     }
 
     last_request_send = std::chrono::high_resolution_clock::now();
@@ -61,9 +61,9 @@ jwt::jwks<jwt::traits::kazuho_picojson> JWKSClient::getJWKS()
 
     try
     {
-        parsed_jwks = jwt::parse_jwks(responseString.str());
+        parsed_jwks = jwt::parse_jwks(response_string);
     }
-    catch (const Exception & e)
+    catch (const std::exception & e)
     {
         throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "Failed to parse JWKS: {}", e.what());
     }
@@ -72,7 +72,7 @@ jwt::jwks<jwt::traits::kazuho_picojson> JWKSClient::getJWKS()
     return cached_jwks;
 }
 
-StaticJWKSParams::StaticJWKSParams(const std::string &static_jwks_, const std::string &static_jwks_file_)
+StaticJWKSParams::StaticJWKSParams(const std::string & static_jwks_, const std::string & static_jwks_file_)
 {
     if (static_jwks_.empty() && static_jwks_file_.empty())
         throw Exception(ErrorCodes::INVALID_CONFIG_PARAMETER,
@@ -85,16 +85,23 @@ StaticJWKSParams::StaticJWKSParams(const std::string &static_jwks_, const std::s
     static_jwks_file = static_jwks_file_;
 }
 
-StaticJWKS::StaticJWKS(const StaticJWKSParams &params)
+StaticJWKS::StaticJWKS(const StaticJWKSParams & params)
 {
     String content = String(params.static_jwks);
     if (!params.static_jwks_file.empty())
     {
         std::ifstream ifs(params.static_jwks_file);
-        content = String((std::istreambuf_iterator<char>(ifs)), (std::istreambuf_iterator<char>()));
+        Poco::StreamCopier::copyToString(ifs, content);
+    }
+    try
+    {
+        auto keys = jwt::parse_jwks(content);
+        jwks = std::move(keys);
+    }
+    catch (const std::exception & e)
+    {
+        throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "Failed to parse JWKS: {}", e.what());
     }
-    auto keys = jwt::parse_jwks(content);
-    jwks = std::move(keys);
 }
 
 }

src/Access/ExternalAuthenticators.cpp

@@ -616,6 +616,9 @@ bool ExternalAuthenticators::checkCredentialsAgainstProcessor(const ITokenProces
         {
             if (credentials.getExpiresAt().value() < default_expiration_ts)
                 cache_entry.expires_at = credentials.getExpiresAt().value();
+            else
+                LOG_TRACE(getLogger("AccessTokenAuthentication"), "Attempt to authenticate user {} with expired access token by {}", credentials.getUserName(), processor.getProcessorName());
+
         }
         else
         {

src/Access/TokenAccessStorage.h

@@ -40,8 +40,7 @@ class TokenAccessStorage : public IAccessStorage
     bool isReadOnly() const override { return true; }
     bool exists(const UUID & id) const override;
 
-private: // IAccessStorage implementations.
-
+private:
     mutable std::recursive_mutex mutex; // Note: Reentrance possible by internal role lookup via access_control
     AccessControl & access_control;
     const Poco::Util::AbstractConfiguration & config;
@@ -56,10 +55,9 @@ class TokenAccessStorage : public IAccessStorage
     mutable std::map<String, std::set<String>> roles_per_users; // user name -> role names (...that should be granted to it; may but don't have to include common roles)
     mutable std::map<UUID, String> granted_role_names;          // (currently granted) role id -> its name
     mutable std::map<String, UUID> granted_role_ids;            // (currently granted) role name -> its id
-    scope_guard role_change_subscription;
     mutable MemoryAccessStorage memory_storage;
+    scope_guard role_change_subscription;
 
-//    void setConfiguration();
     void processRoleChange(const UUID & id, const AccessEntityPtr & entity);
 
     bool areTokenCredentialsValidNoLock(const User & user, const Credentials & credentials, const ExternalAuthenticators & external_authenticators) const;