[client-v2,jdbc-v2] Change credentials in realtime#2812
Open
[client-v2,jdbc-v2] Change credentials in realtime#2812
Conversation
chernser
changed the title
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
There was a problem hiding this comment.
Pull request overview
Adds runtime credential update support across client-v2 and aligns jdbc-v2 with the newer access-token configuration key, along with new examples and tests to demonstrate/authenticate the behavior.
Changes:
- Introduces a
CredentialsManagerand newClientruntime APIs (setCredentials(),setAccessToken()) to update auth for subsequent requests. - Renames/aliases bearer-token config to preferred
access_tokenand updates JDBC parsing/tests accordingly. - Adds new client-v2 and JDBC examples plus additional integration/unit tests for credential updates.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
client-v2/src/main/java/com/clickhouse/client/api/Client.java |
Adds runtime auth update APIs and routes request settings through CredentialsManager. |
client-v2/src/main/java/com/clickhouse/client/api/internal/CredentialsManager.java |
New component to manage mutable credentials and validate auth config. |
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java |
Adds ACCESS_TOKEN and documents legacy BEARERTOKEN_AUTH alias. |
client-v2/src/test/java/com/clickhouse/client/api/internal/CredentialsManagerTest.java |
Unit tests for auth validation and credential snapshot/apply behavior. |
client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java |
Updates token-auth tests to setAccessToken() and adds integration test for setCredentials(). |
client-v2/src/test/java/com/clickhouse/client/ClientTests.java |
Adds integration test switching users via setCredentials(). |
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java |
Accepts access_token with fallback to legacy bearer token property. |
jdbc-v2/src/main/java/com/clickhouse/jdbc/DataSourceImpl.java |
Uses ClientConfigProperties keys and clears token/SSL-auth when connecting with username/password. |
jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java |
Updates integration test to use ACCESS_TOKEN property and renames test. |
examples/client-v2/src/main/java/com/clickhouse/examples/client_v2/Authentication.java |
New example demonstrating runtime auth updates on an existing client. |
examples/client-v2/src/main/java/com/clickhouse/examples/client_v2/RuntimeCredentialsTwoUsers.java |
New demo creating users and switching credentials at runtime. |
examples/client-v2/README.md |
Documents the runtime credentials switch demo usage. |
examples/jdbc/src/main/java/com/clickhouse/examples/jdbc/Authentication.java |
New JDBC example showing auth changes by reconnecting with new properties. |
docs/features.md |
Documents runtime credential updates as stable behavior/compat trait. |
Comment on lines
+42
to
+49
| public void applyCredentials(Map<String, Object> target) { | ||
| putIfNotNull(target, ClientConfigProperties.USER.getKey(), username); | ||
| putIfNotNull(target, ClientConfigProperties.PASSWORD.getKey(), password); | ||
| putIfNotNull(target, ClientConfigProperties.ACCESS_TOKEN.getKey(), accessToken); | ||
| putIfNotNull(target, AUTHORIZATION_HEADER_KEY, authorizationHeader); | ||
| if (useSslAuth) { | ||
| target.put(ClientConfigProperties.SSL_AUTH.getKey(), Boolean.TRUE); | ||
| } |
There was a problem hiding this comment.
applyCredentials() only adds non-null credential fields but never removes stale authentication keys already present in target. Since Client.buildRequestSettings() starts from a copy of Client.configuration, switching from access token (which sets http_header_AUTHORIZATION) back to username/password via setCredentials() will keep sending the old Bearer Authorization header and ignore the new credentials. Consider explicitly removing all auth-related entries (USER, PASSWORD, ACCESS_TOKEN, AUTHORIZATION header key, SSL_AUTH) from target before applying the current mode, or removing these keys from the base Client.configuration after initializing CredentialsManager so requests always rely on the manager snapshot.
Comment on lines
386
to
+388
| this.configuration.put(ClientConfigProperties.ACCESS_TOKEN.getKey(), accessToken); | ||
| this.configuration.remove(ClientConfigProperties.BEARERTOKEN_AUTH.getKey()); | ||
| this.httpHeader(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken); |
There was a problem hiding this comment.
setAccessToken() always writes an Authorization: Bearer ... header even when accessToken is null/blank, which can result in sending Bearer null (or Bearer ) on requests. It would be safer to validate the token (non-null/non-blank) and otherwise remove/clear both the access_token config entry and the Authorization header entry.
Suggested change
| this.configuration.put(ClientConfigProperties.ACCESS_TOKEN.getKey(), accessToken); | |
| this.configuration.remove(ClientConfigProperties.BEARERTOKEN_AUTH.getKey()); | |
| this.httpHeader(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken); | |
| this.configuration.remove(ClientConfigProperties.BEARERTOKEN_AUTH.getKey()); | |
| if (accessToken == null || accessToken.trim().isEmpty()) { | |
| this.configuration.remove(ClientConfigProperties.ACCESS_TOKEN.getKey()); | |
| this.httpHeaders.remove(HttpHeaders.AUTHORIZATION); | |
| } else { | |
| this.configuration.put(ClientConfigProperties.ACCESS_TOKEN.getKey(), accessToken); | |
| this.httpHeader(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken); | |
| } |
Comment on lines
+2121
to
+2127
| /** | ||
| * Updates the credentials used for subsequent requests. | ||
| * | ||
| * <p>This method is not thread-safe with respect to other credential updates | ||
| * or concurrent request execution. Applications must coordinate access if | ||
| * they require stronger consistency. | ||
| * |
There was a problem hiding this comment.
The class-level javadoc states Client is thread-safe, but the newly added runtime-auth APIs explicitly state they are not thread-safe with concurrent request execution. Either make these credential updates thread-safe (e.g., by synchronizing updates + reads or using an immutable/atomic snapshot) or update the Client thread-safety claim so it remains accurate for callers.
| CLIENT_NETWORK_BUFFER_SIZE("client_network_buffer_size", Integer.class, "300000"), | ||
|
|
||
| /** | ||
| * Preferred client setting for token-based authentication like JWT and Oauth. |
There was a problem hiding this comment.
Doc comment terminology: "Oauth" should be "OAuth".
Suggested change
| * Preferred client setting for token-based authentication like JWT and Oauth. | |
| * Preferred client setting for token-based authentication like JWT and OAuth. |
Comment on lines
+2156
to
+2158
| * | ||
| * @param bearer - token to use | ||
| */ |
There was a problem hiding this comment.
updateBearerToken() is now documented as a legacy alias for setAccessToken(), but it isn't marked @Deprecated (unlike Builder.useBearerTokenAuth() and the BEARERTOKEN_AUTH config key doc). Marking this method as @Deprecated would better signal the intended migration path for API consumers.
Suggested change
| * | |
| * @param bearer - token to use | |
| */ | |
| * | |
| * @deprecated Use {@link #setAccessToken(String)} instead. | |
| * @param bearer - token to use | |
| */ | |
| @Deprecated |
| boolean customHttpHeaders = configuration.containsKey(AUTHORIZATION_HEADER_KEY); | ||
|
|
||
| if (!(useSslAuth || hasAccessToken || hasUser || hasPassword || customHttpHeaders)) { | ||
| throw new ClientMisconfigurationException("Username and password (or access token or SSL authentication or pre-define Authorization header) are required"); |
There was a problem hiding this comment.
Error message grammar: "pre-define Authorization header" should be "pre-defined Authorization header".
Suggested change
| throw new ClientMisconfigurationException("Username and password (or access token or SSL authentication or pre-define Authorization header) are required"); | |
| throw new ClientMisconfigurationException("Username and password (or access token or SSL authentication or pre-defined Authorization header) are required"); |
| boolean customHttpHeaders = configuration.containsKey(AUTHORIZATION_HEADER_KEY); | ||
|
|
||
| if (!(useSslAuth || hasAccessToken || hasUser || hasPassword || customHttpHeaders)) { | ||
| throw new ClientMisconfigurationException("Username and password (or access token or SSL authentication or pre-define Authorization header) are required"); |
There was a problem hiding this comment.
The thrown message says "Username and password ... are required", but the validation allows config with only one of USER or PASSWORD (and also allows only Authorization header). Consider rewording to avoid implying both are mandatory, and to reflect the accepted auth modes more clearly.
Suggested change
| throw new ClientMisconfigurationException("Username and password (or access token or SSL authentication or pre-define Authorization header) are required"); | |
| throw new ClientMisconfigurationException( | |
| "Authentication configuration is required: provide username or password, access token, SSL authentication, or a pre-defined Authorization header"); |
|
|
||
| /** | ||
| * Preferred client setting for token-based authentication like JWT and Oauth. | ||
| * For Http it is translated to Authorization Bearer header. |
There was a problem hiding this comment.
Doc comment casing: "For Http" should be "For HTTP".
Suggested change
| * For Http it is translated to Authorization Bearer header. | |
| * For HTTP it is translated to Authorization Bearer header. |
| createUser(adminClient, firstUser, firstPassword); | ||
| createUser(adminClient, secondUser, secondPassword); | ||
|
|
||
| // Create a client with the first user. (It is recommended to use non-existing users for security reasons) |
There was a problem hiding this comment.
Comment says "recommended to use non-existing users" but the example just created the users with CREATE USER IF NOT EXISTS. This looks misleading—consider rewording to something like "use unique/ephemeral users" (or remove the sentence) so the guidance matches the example behavior.
Suggested change
| // Create a client with the first user. (It is recommended to use non-existing users for security reasons) | |
| // Create a client with the first user. Using unique, ephemeral demo users helps avoid reusing existing accounts. |
Member
|
@claude review |
|
Claude finished @mshustov's task in 2m 28s —— View job Code Review: [client-v2,jdbc-v2] Change credentials in realtime
The PR is well-structured and the Critical Bugs1. Stale auth keys are never removed from request settings [ This is the most serious issue. Failing scenario: Client built with // Current (broken):
private Map<String, Object> buildRequestSettings(Map<String, Object> opSettings) {
Map<String, Object> requestSettings = new HashMap<>(configuration); // includes stale auth keys
credentialsManager.applyCredentials(requestSettings); // only adds, never removes
requestSettings.putAll(opSettings);
return requestSettings;
}The fix is to explicitly remove all auth-related keys from public void applyCredentials(Map<String, Object> target) {
// Clear all auth keys first, then apply only current state
target.remove(ClientConfigProperties.ACCESS_TOKEN.getKey());
target.remove(AUTHORIZATION_HEADER_KEY);
target.remove(ClientConfigProperties.SSL_AUTH.getKey());
target.remove(ClientConfigProperties.USER.getKey());
target.remove(ClientConfigProperties.PASSWORD.getKey());
// now apply current credentials
putIfNotNull(target, ClientConfigProperties.USER.getKey(), username);
...
}Note that this change would break the test 2. this.httpHeader(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken); // "Bearer null" if nullThe Similarly, Minor Issues3. Thread-safety documentation inconsistency [ The class-level Javadoc states "Client is thread-safe" but the new 4.
5. The Javadoc says "Prefer using 6. After 7. Missing test for the stale-key scenario
8. The method removes Nits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2801
Checklist
Delete items not relevant to your PR: