CamundaCloudTokenProvider - Allow disabling of file system credentials cache persistence and add tolerance for token expiration #828
Conversation
…ntials cache persistence and add tolerance for token expiration - Adds a DisableCredentialsCachePersistence() method in client CamundaCloudClientBuilder to prevent persisting token cache in file system to keep only in-memory cache. - Fixes concurrent access issue to AccessToken cache. - Adds AccessToken due date tolerance. Refs: camunda-community-hub#415 camunda-community-hub#642
|
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the CamundaCloudTokenProvider with three key improvements: (1) an option to disable file system persistence for serverless/ephemeral environments, (2) thread-safety improvements using SemaphoreSlim to prevent concurrent access issues, and (3) a configurable tolerance period to refresh tokens before expiration to account for network latency.
- Adds
DisableCredentialsCachePersistence()andUseAccessTokenDueDateTolerance()builder methods to bothICamundaCloudClientBuilderandICamundaCloudTokenProviderBuilderinterfaces - Implements
SemaphoreSlim-based locking inPersistedAccessTokenCacheto protect concurrent access to the token cache dictionary and file system - Refactors token validation logic to support early refresh based on a configurable tolerance period
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Client/Impl/Misc/PersistedAccessTokenCache.cs | Core implementation of concurrency protection, persistence flag, and tolerance-based token validation |
| Client/Impl/Builder/CamundaCloudTokenProviderBuilder.cs | Adds builder methods for new features and passes parameters to token provider |
| Client/Impl/Builder/CamundaCloudClientBuilder.cs | Exposes new builder methods through the client builder interface |
| Client/Impl/Builder/CamundaCloudTokenProvider.cs | Updates constructor to accept and forward new configuration parameters |
| Client/Api/Builder/ICamundaCloudTokenProviderBuilder.cs | Defines interface for new token provider builder methods with documentation |
| Client/Api/Builder/ICamundaCloudClientBuilder.cs | Defines interface for new client builder methods with documentation |
| Client.UnitTests/Impl/Misc/PersistedAccessTokenCacheTest.cs | Adds comprehensive tests for tolerance feature and persistence disabling |
| Client.UnitTests/Impl/Builder/CamundaCloudTokenProviderTest.cs | Adds validation tests for tolerance parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ICamundaCloudClientBuilderFinalStep UsePersistedStoragePath(string path); | ||
|
|
||
| /// <summary> | ||
| /// Disables credentials cache persitence to file system. |
There was a problem hiding this comment.
Spelling error: "persitence" should be "persistence"
| /// Disables credentials cache persitence to file system. | |
| /// Disables credentials cache persistence to file system. |
| public ICamundaCloudClientBuilderFinalStep DisableCredentialsCachePersistence() | ||
| { | ||
| _ = camundaCloudTokenProviderBuilder.DisableCredentialsCachePersistence(); | ||
| return this; | ||
| } | ||
|
|
||
| public ICamundaCloudClientBuilderFinalStep UseAccessTokenDueDateTolerance(TimeSpan tolerance) | ||
| { | ||
| _ = camundaCloudTokenProviderBuilder.UseAccessTokenDueDateTolerance(tolerance); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Missing XML documentation comment. The public methods DisableCredentialsCachePersistence() and UseAccessTokenDueDateTolerance() should have XML documentation comments (/// summary tags) consistent with other methods in this class.
|
|
||
| private readonly string tokenStoragePath; | ||
| private readonly bool persistedCredentialsCacheEnabled; | ||
| private readonly SemaphoreSlim mutex = new SemaphoreSlim(1, 1); |
There was a problem hiding this comment.
The SemaphoreSlim instance should be disposed to free unmanaged resources. The class should implement IDisposable and dispose the semaphore in the Dispose method. SemaphoreSlim implements IDisposable and should be properly disposed to avoid potential resource leaks.
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Net.Http; | ||
| using System.Security.Cryptography; |
There was a problem hiding this comment.
The using System.Security.Cryptography; statement is unused and should be removed. There are no references to any types from this namespace in the file.
| using System.Security.Cryptography; |
| if (this.CachedCredentials.TryGetValue(audience, out var currentAccessToken) | ||
| && this.IsValid(currentAccessToken)) |
There was a problem hiding this comment.
Potential race condition: The dictionary read at line 57-58 (outside the lock) could race with the write at line 147 (inside the lock). Although this optimization attempts to avoid locking for valid tokens, Dictionary<TKey, TValue> is not thread-safe for concurrent reads and writes. Consider using ConcurrentDictionary<TKey, TValue> instead, or performing the initial check inside the lock.
| this.logger?.LogTrace( | ||
| "Access token is no longer valid (now: {Now} > dueTime{DueDateTolerance}: {DueTime}), request new one", | ||
| now, | ||
| tolerance, | ||
| dueDate); |
There was a problem hiding this comment.
The log message shows {Now} which includes the tolerance adjustment, making it potentially confusing. The logged value will be UtcNow + tolerance, not the actual current time. Consider logging the actual UTC time and separately noting the tolerance, or clarifying in the message that this is the adjusted time for comparison purposes.
| ICamundaCloudTokenProviderBuilderFinalStep UsePath(string path); | ||
|
|
||
| /// <summary> | ||
| /// Disables credentials cache persitence to file system. |
There was a problem hiding this comment.
Spelling error: "persitence" should be "persistence"
| /// Disables credentials cache persitence to file system. | |
| /// Disables credentials cache persistence to file system. |
3 participants
DisableCredentialsCachePersistence()builder method in clientCamundaCloudClientBuilderto prevent persisting token cache in file system. The goal is to allow users to setup an in-memory only cache for e.g. serverless usage of the client.AccessTokencache. The introduction of aSemaphoreSlimwill protect concurrent accesses to the dictionary and the file system, and prevent concurrent token fetching. The choice ofSemaphoreSlimis done because it is the lock mechanism that fits the best with async/await model. The implementation is optimized to prevent locking when a valid token is in cache.UseAccessTokenDueDateTolerance(TimeSpan tolerance)builder method in clientCamundaCloudClientBuilder. The goal of this option is to allow the user to refresh theAccessTokenslightly before expiration to prevent that, because of network latency, a token considered valid by the client is expired when validated by the authorization server.This change also solves partially issue 637. But the issue rather mentions introducing different implementation of the
IAccessTokenCache. This approach is more impacting for thePersistedAccessTokenCachefor just an in-memory only alternative, as this implementation already have its in-memory cache. To limit the impact, I chose to only improve the existing implementation.closes #415 #642