Skip to content

Conversation

@mynameispathak
Copy link

Summary

This PR introduces enhanced token caching capabilities for confidential clients, providing thread-safe token storage with automatic renewal to improve performance and reduce unnecessary network requests.

Changes Made

New Features

  • EnhancedClient: New client type with built-in token caching
  • Thread-safe token cache: Concurrent access protection with sync.RWMutex
  • Auto-renewal: Configurable renewal buffer (default: 2 minutes before expiry)
  • Cache management: Methods for clearing, checking, and getting cache statistics

New Methods

  • NewEnhancedClient() - Creates enhanced client with default settings
  • NewEnhancedClientWithOptions() - Creates enhanced client with custom renewal buffer
  • AcquireTokenByCredentialWithCaching() - Token acquisition with automatic caching
  • ForceRefreshToken() - Force refresh and cache new token
  • ClearTokenCache() - Clear all cached tokens
  • IsTokenCached() - Check if valid token exists in cache
  • GetCacheStats() - Get cache statistics

Implementation Details

  • TokenCache: New internal package for token storage and management
  • CachedTokenData: Structure storing token, expiry, scopes, and tenant info
  • Thread safety: All cache operations protected with read/write locks
  • Expiry handling: Proper token expiry tracking and validation

Benefits

  • Performance: Reduces network calls by reusing valid cached tokens
  • Thread safety: Safe for concurrent access from multiple goroutines
  • Automatic renewal: Proactive token refresh before expiry
  • Backward compatibility: Existing Client remains unchanged

Usage Example

// Create enhanced client
client, err := confidential.NewEnhancedClient(
    "https://login.microsoftonline.com/tenant",
    "client-id",
    credential,
)

// Acquire token with caching
result, err := client.AcquireTokenByCredentialWithCaching(
    ctx,
    []string{"https://graph.microsoft.com/.default"},
)

Testing

  • Unit tests for cache operations
  • Concurrent access testing
  • Expiry validation testing
  • Integration tests with real token acquisition

Related Issues

Closes #587

Breaking Changes

None - this is purely additive functionality.

- Add TokenCache struct with RWMutex for concurrent access
- Implement automatic token renewal with configurable buffer
- Add token validation and cache statistics
- Support cache management operations (clear, stats)
- Generate deterministic cache keys from scopes and tenant
- Add EnhancedClient struct with built-in token caching
- Add EnhancedClientOptions for configurable renewal buffer
- Import cache package for token management
- Add NewEnhancedClient with default 2-minute renewal buffer
- Add NewEnhancedClientWithOptions for custom configuration
- Integrate token cache with existing client functionality
- Add AcquireTokenByCredentialWithCaching with automatic token reuse
- Add ForceRefreshToken for explicit token refresh
- Add cache management methods (ClearTokenCache, IsTokenCached)
- Add GetCacheStats for monitoring and debugging
- Integrate with existing token acquisition flow
- Test token caching and reuse functionality
- Test force refresh capability
- Test cache expiry with configurable renewal buffer
- Verify cache statistics and management operations
- Demonstrate automatic token caching and renewal
- Show force refresh capability
- Display cache statistics and management
- Provide production-ready usage patterns
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2025

@mynameispathak
Copy link
Author

@microsoft-github-policy-service agree company="Sprinklr"

@bgavrilMS bgavrilMS requested a review from 4gust October 7, 2025 11:52
@bgavrilMS
Copy link
Member

Hi @mynameispathak , thanks for the PR. Today, MSAL Go caches tokens in memory, associated with each ConfidentialClientApplication object. What are you trying to achieve that you cannot do with MSAL Go today?

Note:

  • We can add a "WithForceRefresh()" modifier, like other MSAL libs, but we would like to understand your scenario first.
  • Tokens are refreshed at 5 min interval.

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Not consistent with other MSALs

@4gust
Copy link
Collaborator

4gust commented Oct 15, 2025

Hi @mynameispathak , apologies for the delayed response — I wanted to ensure I reviewed everything thoroughly.

Regarding issue #587, here are my thoughts:

  1. Thread-safe caching:
    We already have thread-safe caching in place. I believe the confusion may come from the fact that AcquireTokenByCredential intentionally does not perform a cache lookup. For cache usage, AcquireTokenSilent should be used instead.

  2. Automatic token renewal:
    This is also handled internally. A silent call (AcquireTokenSilent) will attempt to refresh the token when needed.
    The second point you raised relates to proactive token refresh — in some scenarios, the token will contain information indicating when the client should proactively refresh it.

  3. Token reuse:
    AcquireTokenSilent will return a cached token when the parameters match, ensuring reuse.

  4. Force refresh capability:
    If you want to bypass the cache and force a new token, AcquireTokenByCredential gives you that control.

  5. Cache statistics:
    The client supports this via the WithCacheAccessor method, which accepts your custom cache implementation. You can run various statistics on the cache without requiring the client itself to handle the analysis.

  6. As Bogdan mentioned that this does not consistent with other MSAL's

Lastly, regarding the issues you linked at the bottom in #587#569 and #570 — please note that these are related to Managed Identity, not Confidential Client. They don't share the same implementation.

@mynameispathak
Copy link
Author

mynameispathak commented Nov 20, 2025

Hi @4gust,

Thank you for the detailed response. I appreciate you taking the time to review this thoroughly.

However, I'd like to respectfully clarify a key point about consistency across MSAL implementations:

MSAL.NET Behavior

According to Microsoft's official documentation, MSAL.NET's AcquireTokenForClient() method automatically manages the application token cache, checking for existing tokens and refreshing them as needed:

"In MSAL.NET, the AcquireTokenForClient method automatically manages the application token cache, checking for existing tokens and refreshing them as needed. Therefore, it's unnecessary to call AcquireTokenSilent before AcquireTokenForClient, as the latter inherently performs cache lookups and token refreshes."

Source: Microsoft Learn - MSAL.NET Best Practices

The existence of the WithForceRefresh() option in the .NET API documentation further confirms that by default, the method checks the cache first - you only use WithForceRefresh(true) when you explicitly want to bypass the cache.

The Inconsistency

This creates a significant difference in developer experience:

MSAL.NET (One call handles everything):

var result = await app.AcquireTokenForClient(scopes).ExecuteAsync();
// Automatically checks cache, refreshes if needed

MSAL Go (Manual two-step pattern required):

result, err := client.AcquireTokenSilent(ctx, scopes)
if err != nil {
    result, err = client.AcquireTokenByCredential(ctx, scopes)
}
// Developer must manually implement cache checking

My Proposal

I understand that AcquireTokenByCredential() intentionally bypasses the cache in MSAL Go. My proposed EnhancedClient was meant to provide consistency with MSAL.NET's behavior, where a single method call handles both cache lookup and token acquisition automatically.

Would you consider:

  1. Modifying AcquireTokenByCredential() to check cache by default (matching MSAL.NET's behavior), with an option to force refresh if needed?
  2. Adding a separate convenience method that provides MSAL.NET-like behavior (e.g., AcquireTokenForClient())?
  3. Or is the current two-call pattern the intended design for MSAL Go?

Summary

I'm not questioning that the current implementation works as designed. Rather, I'm highlighting that:

  • ✅ MSAL.NET's AcquireTokenForClient() checks cache automatically
  • ✅ MSAL Go's AcquireTokenByCredential() does not
  • ✅ This creates an inconsistency in the MSAL family of libraries
  • ✅ This makes it harder for developers migrating between implementations

Regarding managed identity issues (#569, #570): I understand they're separate implementations. I mentioned them only as examples of where automatic caching/renewal could improve developer experience across different client types.

Thank you for considering this feedback. I'm happy to discuss the design rationale further if there are specific reasons MSAL Go chose a different approach.

@mynameispathak
Copy link
Author

Hey @4gust, @bgavrilMS,
Can you please review my above comment and let me know your thoughts?

@4gust
Copy link
Collaborator

4gust commented Nov 25, 2025

@mynameispathak Thank you for the response.

I want to confirm that the primary requirement you are pointing is the automatic cache check within the acquire call itself. This functionality has already been added for the confidential client. You can review it here: #590. It should now work similar to .NET's implementation. Its released in version v1.6.0

If there is anything else you would like addressed, please let me know.

@mynameispathak
Copy link
Author

Hi @4gust, @bgavrilMS,

Thank you for confirming that v1.6.0 includes automatic cache lookup in AcquireTokenByCredential.

I do want to note that I opened [PR #588] on October 7th addressing this exact issue, a week before [PR #590] was created on October 14th.

While I'm glad to see this functionality is now available and that my analysis was validated, I'm disappointed that my contribution wasn't acknowledged or that I wasn't given the opportunity to collaborate on the implementation.

Could you clarify the contribution process for this project? Specifically:

  • Was there something wrong with my approach that led to reimplementing it internally?
  • Could my PR have been considered if structured differently?
  • How can external contributors better align with the team's expectations?

I'm interested in contributing further to this project, but I'd like to understand how to do so effectively.

Thank you,
Aniket Pathak

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.

[Feature Request] Thread-safe token caching with auto-renewal for confidential clients

3 participants