Skip to content

KTOR-8180 Provide control over tokens to user code for the client Auth #4826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

e5l
Copy link
Member

@e5l e5l commented May 6, 2025

Fix KTOR-8180 Auth: Provide control over tokens to user code

Copy link

coderabbitai bot commented May 6, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This change introduces a new, modular, and asynchronous token storage abstraction for Ktor client's authentication providers. It adds the TokenStorage interface, its implementations, and integrates them into Bearer, Basic, and Digest auth providers. The update includes new documentation, tests, and configuration options for token caching, and marks the old AuthTokenHolder as deprecated.

Changes

Files/Group Change Summary
.gitignore, CLAUDE.md .gitignore fixed syntax and added ignore for .claude/settings.local.json; new CLAUDE.md with code style, build, and workflow guidelines.
CHANGELOG.md Updated to document the addition of the TokenStorage interface and related improvements for version 3.1.3.
docs/auth-token-management.md New documentation explaining token storage, caching, custom storage, clearing, sharing, DI integration, and migration instructions for token management in Ktor client auth.
ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.api,
ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.klib.api
Added/updated public API declarations for TokenStorage, its implementations, new config options, and updated function signatures for auth providers.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/Auth.kt Initialized token storage map during plugin install; added authProvider<T>() extension and documented authProviders property; made AuthProvidersKey internal and moved its location.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/AuthTokenHolder.kt Marked AuthTokenHolder as deprecated and added deprecation guidance.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BasicAuthProvider.kt Integrated TokenStorage into Basic auth provider and config; added custom storage support; clearToken() is now suspendable.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt Integrated TokenStorage into Bearer auth provider and config; added cacheTokens option, custom storage support, and made clearToken() suspendable.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/DigestAuthProvider.kt Replaced internal token holder with TokenStorage and made clearToken() suspendable.
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/TokenStorage.kt New file: defines TokenStorage abstraction, caching/non-caching implementations, factory, and client extension functions for registering/retrieving token storage.
ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/AuthTest.kt Added tests for bearer auth provider: token caching disabled, refresh with/without caching, and correct invocation counts.
ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/BearerProviderTest.kt New test suite: verifies bearer provider's default caching, disabled caching, custom storage, and refresh flow.
ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/TokenStorageTest.kt New test suite: verifies caching and non-caching token storage behavior, concurrent access, updates, and null handling.

Possibly related PRs

  • ktorio/ktor#4829: Both PRs update the changelog for version 3.1.3, indicating a connection regarding release documentation.
  • ktorio/ktor#4825: Both PRs modify .gitignore and add CLAUDE.md, reflecting related changes in project configuration and guidelines.
  • ktorio/ktor#4832: Both PRs address token storage; this PR supersedes the previous approach by introducing a new abstraction and deprecating AuthTokenHolder.

Suggested reviewers

  • osipxd

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@e5l e5l force-pushed the e5l/bearer-cache branch from 82844cf to 4d86995 Compare May 6, 2025 07:13
@osipxd
Copy link
Member

osipxd commented May 6, 2025

We have the same problem in all other AuthProvider implementations using AuthTokenHolder. Could you take a look at KTOR-8180 as a possible general solution?

@e5l e5l changed the base branch from e5l/claude to main May 7, 2025 07:22
@e5l e5l force-pushed the e5l/bearer-cache branch from 4d86995 to de61d4f Compare May 7, 2025 09:14
@e5l e5l requested review from Copilot, osipxd and bjhham May 8, 2025 06:58
@e5l e5l self-assigned this May 8, 2025
@e5l e5l marked this pull request as ready for review May 8, 2025 06:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request updates the token caching mechanism for Bearer authentication to allow disabling caching of the client bearer token. Key changes include:

  • Introduction of a unified TokenStorage interface with both caching and non-caching implementations.
  • Replacement of legacy AuthTokenHolder usage with the new TokenStorage in Digest, Bearer, and Basic auth providers.
  • Updates to tests, documentation, and API files ensuring consistent usage of the new token storage mechanism.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ktor-client-auth/common/test/io/ktor/client/plugins/auth/TokenStorageTest.kt Added several tests covering caching, non-caching, updates, concurrent token loads, and null token scenarios.
ktor-client-auth/common/test/io/ktor/client/plugins/auth/BearerProviderTest.kt Added tests verifying token caching behavior and non-caching mode for Bearer auth.
ktor-client-auth/common/test/io/ktor/client/plugins/auth/AuthTest.kt Added tests verifying token caching disabling and refresh flows.
ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/TokenStorage.kt Introduced TokenStorage, CachingTokenStorage, and NonCachingTokenStorage implementations.
ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/DigestAuthProvider.kt Replaced legacy token holder with new TokenStorage usage.
ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt Updated Bearer auth provider to use the new token storage and a custom tokenStorage parameter.
ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BasicAuthProvider.kt Updated Basic auth provider to use the new token storage implementation.
Remaining files (Auth.kt, API files, docs, CLAUDE.md, CHANGELOG.md) Updated to reflect changes in token management and provide guidance for usage.
Comments suppressed due to low confidence (1)

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt:143

  • [nitpick] Consider renaming 'loadTokensCallback' to 'loadTokens' for consistency with other auth providers and to simplify the naming convention.
private val loadTokensCallback: suspend () -> BearerTokens?,

* [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.client.plugins.auth.providers.BearerAuthProvider.clearToken)
*/
public fun clearToken() {
tokensHolder.clearToken()
public suspend fun clearToken() {
Copy link
Preview

Copilot AI May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the API documentation and code comments to indicate that clearToken is now a suspend function and may involve asynchronous operations.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

❌ Junie can be launched only by a repository collaborator

@bjhham
Copy link
Contributor

bjhham commented May 8, 2025

Looks like your Claude branch snuck in here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (10)
CLAUDE.md (1)

1-53: Suggest linking to CLAUDE.md in the project README
To improve discoverability, consider adding a pointer to CLAUDE.md in the repository’s main README.md.

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/AuthTokenHolder.kt (1)

17-20: Consider strengthening the deprecation level
By default, @Deprecated emits a warning. If you want to enforce migration more strictly, specify level = DeprecationLevel.ERROR:

@Deprecated(
    "Use TokenStorage interface instead",
    level = DeprecationLevel.ERROR,
    ReplaceWith("CachingTokenStorage(loadTokens)", "io.ktor.client.plugins.auth.providers.CachingTokenStorage")
)
CHANGELOG.md (1)

5-5: Align changelog bullet style

Most entries in “### Improvements” use the imperative mood (e.g., “Implement”, “Don’t send”), whereas this one is in past tense (“Added”). For consistency, consider changing to:

-* Added TokenStorage interface for flexible auth token caching (...)
+* Add TokenStorage interface for flexible auth token caching (...)
docs/auth-token-management.md (1)

62-62: Minor grammatical issue.

Consider adding a comma before "or" to improve readability.

-When users log out or you need to refresh tokens proactively, you can clear the cached tokens:
+When users log out, or you need to refresh tokens proactively, you can clear the cached tokens:
🧰 Tools
🪛 LanguageTool

[uncategorized] ~62-~62: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... ## Clearing Tokens When users log out or you need to refresh tokens proactively,...

(COMMA_COMPOUND_SENTENCE)

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BasicAuthProvider.kt (2)

112-114: Property and DSL method share the same name – risk of shadowing

BasicAuthConfig defines both the internal var tokenStorage and a public DSL function tokenStorage(...) with the same identifier.
Although the scopes differ, this can confuse users and hampers IDE code-completion (the function is hidden by the property reference until the user adds ()).

Consider renaming the backing property to _tokenStorage (or similar) and keeping the public builder name as is.

- internal var tokenStorage: TokenStorage<BasicAuthCredentials>? = null
+ internal var _tokenStorage: TokenStorage<BasicAuthCredentials>? = null
...
- public fun tokenStorage(storage: TokenStorage<BasicAuthCredentials>) {
-     tokenStorage = storage
+ public fun tokenStorage(storage: TokenStorage<BasicAuthCredentials>) {
+     _tokenStorage = storage
   }

154-156: No way to disable caching for Basic Auth while Bearer has one

BearerAuthProvider exposes the cacheTokens flag whereas BasicAuthProvider silently defaults to TokenStorageFactory.withCache(...).
For API consistency and to avoid surprising users who expect similar behaviour across providers, please expose the same toggle (or forward a nonCaching storage when cacheTokens = false).

ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/TokenStorageTest.kt (1)

14-17: Super-fluous TestScope wrapper

The test creates its own TestScope and then immediately calls runTest on it.
kotlinx.coroutines.test.runTest { ... } already provides an isolated test scope, so the extra indirection is unnecessary.

-private val testScope = TestScope()
-
-@Test
-fun testCachingTokenStorage() = testScope.runTest {
+@Test
+fun testCachingTokenStorage() = runTest {

Apply similarly to other tests in the file.

ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/AuthTest.kt (2)

761-770: Remove println debugging noise from unit tests

The added debug statements (println(...)) clutter CI logs and slow test execution.
Rely on assertions for verification instead of console output.


840-854: cacheTokens = true is redundant

BearerAuthProvider caches tokens by default; explicitly setting cacheTokens = true is unnecessary and can be dropped to keep the test concise.

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt (1)

155-161: Consider isolating token-storage selection logic from the provider constructor

BearerAuthProvider decides at runtime which TokenStorage implementation to use.
Moving this decision into BearerAuthConfig (and passing the final instance to the provider) would:

  1. Keep all configuration-related concerns in one place.
  2. Let unit tests exercise the selection logic without instantiating the full provider.
  3. Avoid duplicating the cacheTokens / tokenStorage precedence rules in two classes.

Not blocking, but worth considering to reduce cognitive load and promote single-responsibility.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c281e1 and de61d4f.

📒 Files selected for processing (15)
  • .gitignore (2 hunks)
  • CHANGELOG.md (1 hunks)
  • CLAUDE.md (1 hunks)
  • docs/auth-token-management.md (1 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.api (7 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.klib.api (7 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/Auth.kt (3 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/AuthTokenHolder.kt (1 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BasicAuthProvider.kt (7 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt (6 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/DigestAuthProvider.kt (4 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/TokenStorage.kt (1 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/AuthTest.kt (1 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/BearerProviderTest.kt (1 hunks)
  • ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/TokenStorageTest.kt (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/BearerProviderTest.kt (2)
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.kt (6)
  • bearer (20-26)
  • loadTokens (90-92)
  • tokenStorage (113-115)
  • refreshTokens (59-125)
  • refreshTokens (80-82)
  • refreshTokens (141-247)
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/TokenStorage.kt (2)
  • loadTokens (125-204)
  • loadTokens (213-245)
ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/Auth.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
  • AttributeKey (19-21)
🪛 LanguageTool
docs/auth-token-management.md

[uncategorized] ~62-~62: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... ## Clearing Tokens When users log out or you need to refresh tokens proactively,...

(COMMA_COMPOUND_SENTENCE)

🔇 Additional comments (23)
.gitignore (2)

16-16: Inconsistent with summary: backtick not removed
The AI summary indicates the extraneous backtick in `.vscode`` was removed, but this line still contains a backtick. Please update it to:

.vscode

Likely an incorrect or invalid review comment.


45-46: Add ignore pattern for Claude settings
Excluding **/.claude/settings.local.json ensures local Claude configuration files aren't tracked, aligning with the new CLAUDE.md guidance.

CLAUDE.md (1)

1-53: Comprehensive Claude integration guide
The new CLAUDE.md provides clear, actionable instructions for builds, coding conventions, and API compatibility.

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/AuthTokenHolder.kt (1)

14-20: Deprecate AuthTokenHolder in favor of TokenStorage
The addition of KDoc deprecation notice and the @Deprecated annotation with a ReplaceWith suggestion effectively guides users toward migrating to the new TokenStorage API.

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/DigestAuthProvider.kt (4)

126-126: Good implementation of token storage integration.

The addition of TokenStorageFactory.withCache(credentials) properly integrates the DigestAuthProvider with the new token storage abstraction, aligning it with the other authentication providers.


174-174: Token loading properly migrated to use the new TokenStorage API.

This change correctly uses the asynchronous loadToken() method from the TokenStorage interface to retrieve credentials, replacing the previous direct token access approach.


211-212: Token refresh flow correctly integrated with TokenStorage.

The token refresh mechanism now properly uses the updateToken method from the TokenStorage interface, ensuring consistent token refresh behavior across authentication providers.


234-236: Token clearing now uses asynchronous API.

The clearToken() method is now properly implemented as a suspending function that delegates to the TokenStorage interface, allowing for proper asynchronous token clearing.

docs/auth-token-management.md (4)

1-5: Excellent documentation for the new token management system.

The documentation provides a clear overview of the new token management functionality, which will help users understand the changes and how to use them effectively.


24-39: Well-documented token caching configuration.

The documentation clearly explains how to disable token caching, which is the primary feature introduced in this PR (KTOR-6569).


41-58: Comprehensive coverage of custom token storage options.

The documentation provides clear examples of how to use custom token storage, including the built-in implementations and how to provide a fully custom implementation.


156-204: Excellent migration guidance.

The documentation provides clear instructions for migrating from previous versions, with before-and-after code examples that illustrate the improvements in the API.

ktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/Auth.kt (4)

91-92: Good initialization of token storage infrastructure.

This initialization properly sets up the token storage map in the client's attributes, which is necessary for the token storage system to work across multiple authentication providers.


209-217: Useful extension function for retrieving auth providers.

The authProvider<T>() extension function provides a type-safe and convenient way to retrieve a specific auth provider, which is particularly useful for clearing tokens or other provider-specific operations.


218-222: Good public API for accessing all auth providers.

The authProviders property provides a clean way to access all registered auth providers, which is useful for operations that need to work with multiple providers.


224-225: Appropriate encapsulation of internal attribute key.

Marking AuthProvidersKey as @PublishedApi internal is a good practice that maintains encapsulation while still allowing it to be used in inline functions.

ktor-client/ktor-client-plugins/ktor-client-auth/common/test/io/ktor/client/plugins/auth/BearerProviderTest.kt (4)

18-71: Excellent test coverage for default token caching behavior.

This test thoroughly verifies the default caching behavior of the Bearer auth provider, ensuring that tokens are cached and reused across requests, and that the cache can be cleared when needed.


73-116: Good test coverage for disabled token caching.

This test verifies the behavior when token caching is disabled, ensuring that tokens are freshly loaded for each request, which is the primary feature introduced in this PR (KTOR-6569).


118-184: Comprehensive test for custom token storage.

This test verifies that users can provide their own token storage implementation, which is important for flexibility and advanced use cases.


186-233: Thorough test for token refresh flow.

This test verifies the token refresh behavior when a 401 Unauthorized response is received, ensuring that the token is properly refreshed and the request is retried.

ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.klib.api (2)

8-114: Generated ABI dump – no manual changes needed.


153-184: Generated ABI dump – no manual changes needed.

ktor-client/ktor-client-plugins/ktor-client-auth/api/ktor-client-auth.api (1)

70-90: Generated API dump – no manual changes required.

Comment on lines 186 to 189
override suspend fun refreshToken(response: HttpResponse): Boolean {
tokensHolder.setToken(credentials)
tokenStorage.updateToken(credentials)
return true
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

refreshToken always returns true even when no token is produced

updateToken(credentials) might return null (e.g., credential provider yielded null).
Returning true in that case informs the Auth pipeline that a refresh occurred, potentially causing an endless retry loop.

-        tokenStorage.updateToken(credentials)
-        return true
+        val updated = tokenStorage.updateToken(credentials)
+        return updated != null
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
override suspend fun refreshToken(response: HttpResponse): Boolean {
tokensHolder.setToken(credentials)
tokenStorage.updateToken(credentials)
return true
}
override suspend fun refreshToken(response: HttpResponse): Boolean {
val updated = tokenStorage.updateToken(credentials)
return updated != null
}

Comment on lines +187 to +193
if (newValue != null) {
value = newValue
}
}

value
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Stale token kept when refresh returns null

CachingTokenStorage.updateToken only replaces the cached value when the new token is not null.
If the refresh callback returns null (e.g., credentials revoked), the old token survives and will still be used.

-                if (newValue != null) {
-                    value = newValue
-                }
+                // Replace the cached value even when the new value is null
+                // so that callers fall back to `loadTokens()` on the next attempt.
+                value = newValue

Comment on lines +213 to +245
public class NonCachingTokenStorage<T>(
private val loadTokens: suspend () -> T?
) : TokenStorage<T> {
// This token is used only during the refresh process
@Volatile private var currentRefreshingToken: T? = null
private val mutex = Mutex()

override suspend fun loadToken(): T? {
mutex.withLock {
// If we have a token from an ongoing refresh operation, use that
if (currentRefreshingToken != null) {
return currentRefreshingToken
}
}

// Otherwise, load a fresh token
return loadTokens()
}

override suspend fun updateToken(block: suspend () -> T?): T? {
return mutex.withLock {
val newToken = block()
currentRefreshingToken = newToken
newToken
}
}

override suspend fun clearToken() {
mutex.withLock {
currentRefreshingToken = null
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

NonCachingTokenStorage unintentionally caches refreshed tokens

Documentation promises “doesn't cache tokens”, but currentRefreshingToken is returned on every subsequent loadToken() until explicitly cleared, effectively caching it.

-            // If we have a token from an ongoing refresh operation, use that
-            if (currentRefreshingToken != null) {
-                return currentRefreshingToken
+            // If a refresh is in progress, use the intermediate token once
+            currentRefreshingToken?.let {
+                currentRefreshingToken = null   // ensure no long-term caching
+                return it
             }

Additionally, consider resetting currentRefreshingToken = null at the end of updateToken once other callers can observe the new value.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public class NonCachingTokenStorage<T>(
private val loadTokens: suspend () -> T?
) : TokenStorage<T> {
// This token is used only during the refresh process
@Volatile private var currentRefreshingToken: T? = null
private val mutex = Mutex()
override suspend fun loadToken(): T? {
mutex.withLock {
// If we have a token from an ongoing refresh operation, use that
if (currentRefreshingToken != null) {
return currentRefreshingToken
}
}
// Otherwise, load a fresh token
return loadTokens()
}
override suspend fun updateToken(block: suspend () -> T?): T? {
return mutex.withLock {
val newToken = block()
currentRefreshingToken = newToken
newToken
}
}
override suspend fun clearToken() {
mutex.withLock {
currentRefreshingToken = null
}
}
}
public class NonCachingTokenStorage<T>(
private val loadTokens: suspend () -> T?
) : TokenStorage<T> {
// This token is used only during the refresh process
@Volatile private var currentRefreshingToken: T? = null
private val mutex = Mutex()
override suspend fun loadToken(): T? {
mutex.withLock {
// If a refresh is in progress, use the intermediate token once
currentRefreshingToken?.let {
currentRefreshingToken = null // ensure no long-term caching
return it
}
}
// Otherwise, load a fresh token
return loadTokens()
}
override suspend fun updateToken(block: suspend () -> T?): T? {
return mutex.withLock {
val newToken = block()
currentRefreshingToken = newToken
newToken
}
}
override suspend fun clearToken() {
mutex.withLock {
currentRefreshingToken = null
}
}
}

Comment on lines +199 to 201
val token = tokenStorage.loadToken() ?: return
LOGGER.trace("Using token for request: ${token.accessToken}")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Do not log raw access tokens – mask or omit them

LOGGER.trace("Using token for request: ${token.accessToken}") and
LOGGER.trace("Token refreshed: ${newToken.accessToken}") leak full bearer tokens into application logs.
Even at TRACE level this poses a high security & compliance risk (PII / credentials disclosure).

Suggested remediation (mask token or drop it entirely):

-LOGGER.trace("Using token for request: ${token.accessToken}")
+LOGGER.trace("Using bearer token for request") // do not print sensitive data

-LOGGER.trace("Token refreshed: ${newToken.accessToken}")
+LOGGER.trace("Bearer token refreshed successfully")

If some visibility is truly required, log only a hash or the first N characters:

private fun mask(token: String) = token.take(6) + ""
LOGGER.trace("Using bearer token (id=${mask(token.accessToken)})")

Also applies to: 221-223

Copy link
Contributor

❌ Junie can be launched only by a repository collaborator

@e5l
Copy link
Member Author

e5l commented May 8, 2025

yep, will rebase

@e5l e5l changed the title KTOR-6569 Add option for Bearer auth to not cache client token KTOR-8180 Provide control over tokens to user code for the client Auth May 8, 2025
@e5l e5l force-pushed the e5l/bearer-cache branch from de61d4f to 5ff5050 Compare May 8, 2025 07:20
@e5l e5l requested a review from Mr3zee May 8, 2025 07:32
@e5l e5l marked this pull request as draft May 8, 2025 08:27
@e5l e5l removed request for osipxd, Mr3zee and bjhham May 8, 2025 08:27
Copy link
Member

@Mr3zee Mr3zee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks LLMish to me 🤔 Please ask your agents to consider comments

Comment on lines +54 to +60
/**
* Returns the current token value without loading it.
* Mostly used for testing purposes.
*
* @return the current token or null if not available
*/
public fun getCurrentToken(): T?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdym without loading it?

Comment on lines +233 to +237
return mutex.withLock {
val newToken = block()
currentRefreshingToken = newToken
newToken
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be noop

private val loadTokens: suspend () -> T?
) : TokenStorage<T> {
// This token is used only during the refresh process
@Volatile private var currentRefreshingToken: T? = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed

Comment on lines +221 to +226
mutex.withLock {
// If we have a token from an ongoing refresh operation, use that
if (currentRefreshingToken != null) {
return currentRefreshingToken
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, call loadTokens each time

Comment on lines +241 to +243
mutex.withLock {
currentRefreshingToken = null
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noop

*
* @param T the type of token to store
*/
public interface DefaultTokenStorage<T> : TokenStorage<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move getCurrentToken to TokenStorage interface

Comment on lines +187 to +189
if (newValue != null) {
value = newValue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this if is correct

Also, considering that AuthTokenHolder was the template for the impl, the behavior is different here. AuthTokenHolder didn't do assignment. Which impl is right?


// Get token from storage
val token = tokenStorage.loadToken() ?: return
LOGGER.trace("Using token for request: ${token.accessToken}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no god please no

private val sendWithoutRequestCallback: (HttpRequestBuilder) -> Boolean = { true },
private val realm: String?
private val realm: String?,
private val cacheTokens: Boolean = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like cacheTokens, customTokenStorage (or better tokenStorage = TokenStorageFactory.withCache(loadTokensCallback) is enough. Also loadTokensCallback is questionable is this case. Better do different constructor for multiple scenarios

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants