Eliminate token cache timeout setting in most cases #126
Conversation
WalkthroughThe changes in this pull request involve multiple modifications across various classes within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
src/Arc4u.Standard.OAuth2/Token/ITokenCache.cs (1)
12-20: LGTM! Improved method signature and added documentation.The changes to the
Putmethod signature and the addition of XML documentation are well-implemented and align with the PR objectives. The newtimeoutparameter allows for more flexible expiration times, which is a good improvement over the previous fixed timeout approach.Consider a minor improvement to the XML documentation for consistency:
/// <summary> /// Set or overwrite the item with key <paramref name="key"/> with <paramref name="data"/>. -/// The item expires after <paramref name="timeout"/> +/// The item expires after <paramref name="timeout"/>. /// </summary> /// <typeparam name="T"></typeparam> /// <param name="key"></param> /// <param name="timeout"></param> /// <param name="data"></param>This change adds a period at the end of the sentence for consistency with other comments.
src/Arc4u.Standard.OAuth2.AspNetCore/Options/ClaimsFillerOptions.cs (1)
11-14: LGTM! Consider enhancing the documentation slightly.The addition of the
MaxTimeproperty is well-implemented and properly documented. It provides a clear purpose for caching extra claims whenLoadClaimsFromClaimsFillerProvideris true.Consider adding a brief explanation of why 20 minutes was chosen as the default value. This could help future maintainers understand the reasoning behind this specific duration. For example:
/// <summary> -/// If <see cref="LoadClaimsFromClaimsFillerProvider"/> is true, the extra claims are cached for <see cref="MaxTime"/>."/> +/// If <see cref="LoadClaimsFromClaimsFillerProvider"/> is true, the extra claims are cached for <see cref="MaxTime"/>. +/// The default value of 20 minutes provides a balance between performance and data freshness for most use cases. /// </summary> public TimeSpan MaxTime { get; set; } = TimeSpan.FromMinutes(20);src/Arc4u.Standard.OAuth2/Token/ApplicationLocalDataCache.cs (1)
Line range hint
24-31: Approve simplification ofClearmethod with a suggestionThe removal of the empty try-catch block in the
Clearmethod improves code clarity. This change allows exceptions to propagate, which is generally a good practice for better error handling and debugging.Consider adding a log statement to capture any exceptions that may occur during the
Cache.Removeoperation:public void Clear(string key) { - Cache.Remove(key); - _token = null; + try + { + Cache.Remove(key); + _token = null; + } + catch (Exception ex) + { + Logger.Technical().From<ApplicationLocalDataCache>().Exception(ex).Message("Failed to clear cache").Log(); + } }This will provide visibility into any issues that may occur during cache clearing operations.
src/Arc4u.Standard.OAuth2.AspNetCore/Extensions/AddClaimsFillerExtension.cs (2)
14-28: Improved configuration validation, with minor suggestions.The additional validation logic enhances the robustness of the
ClaimsFillerOptionsconfiguration. This is a positive change that will help catch configuration errors early. However, consider the following suggestions for further improvement:
Use a
List<string>orStringBuilderfor accumulating error messages instead of string concatenation. This would be more efficient, especially if more validations are added in the future.The
MaxTimecheck could be more explicit about the expected value. Consider changing the condition tovalidate.MaxTime <= TimeSpan.Zeroto catch negative values as well.Here's a suggested refactoring that addresses these points:
if (validate.LoadClaimsFromClaimsFillerProvider) { - string? configErrors = null; + var configErrors = new List<string>(); if (null == validate.SettingsKeys || !validate.SettingsKeys.Any()) { - configErrors += "Settings key must be provided." + System.Environment.NewLine; + configErrors.Add("Settings key must be provided."); } - if (validate.LoadClaimsFromClaimsFillerProvider && validate.MaxTime == TimeSpan.Zero) + if (validate.MaxTime <= TimeSpan.Zero) { - configErrors += $"If {nameof(validate.LoadClaimsFromClaimsFillerProvider)} is true, then {nameof(validate.MaxTime)} must be provided." + System.Environment.NewLine; + configErrors.Add($"{nameof(validate.MaxTime)} must be a positive TimeSpan when {nameof(validate.LoadClaimsFromClaimsFillerProvider)} is true."); } - if (configErrors is not null) + if (configErrors.Any()) { - throw new ConfigurationException(configErrors); + throw new ConfigurationException(string.Join(Environment.NewLine, configErrors)); } }This refactoring improves efficiency and clarity while maintaining the intended functionality.
52-56: Formatting improvement with suggestion for consistency.The formatting changes improve the readability of the lambda expression, which is beneficial. However, to maintain consistency with the changes made in the first
AddClaimsFillermethod, consider adding theMaxTimeproperty to this method as well.Here's a suggested addition to include the
MaxTimeproperty:AddClaimsFiller(services, o => { o.LoadClaimsFromClaimsFillerProvider = options.LoadClaimsFromClaimsFillerProvider; o.SettingsKeys = options.SettingsKeys; + o.MaxTime = options.MaxTime; });This addition ensures that the
MaxTimeproperty is properly propagated when theAddClaimsFillermethod is called with anIConfigurationobject.src/Arc4u.Standard.OAuth2/Token/ApplicationCache.cs (1)
50-50: LGTM! Consider adding timeout validation.The update to use the
timeoutparameter in the_cache.Putcall is correct and aligns with the PR objectives. This change allows for more dynamic and flexible timeout management.Consider adding a validation check for the
timeoutparameter to ensure it's not negative or zero:public void Put<T>(string key, TimeSpan timeout, T data) { + if (timeout <= TimeSpan.Zero) + { + throw new ArgumentException("Timeout must be a positive value.", nameof(timeout)); + } if (null == data) { _logger.Technical().From<ApplicationCache>().System("A null token data information was provided to the cache. We skip this data from the cache."); return; } _logger.Technical().From<ApplicationCache>().System($"Adding token data information to the cache: {key}.").Log(); _cache.Put(GetKey(key), timeout, data); _logger.Technical().From<ApplicationCache>().System($"Added token data information to the cache: {key}.").Log(); }src/Arc4u.Standard.UnitTest/Security/TokenCacheOptionsTests.cs (3)
Line range hint
28-45: LGTM: MaxTime assertion removed as per PR objectivesThe removal of the MaxTime assertion aligns with the PR's goal of eliminating the explicit timeout in most scenarios. The test still correctly verifies the CacheName setting.
Consider renaming the test method from
TockeCacheOption_From_Action_ShouldtoTokenCacheOption_From_Action_Shouldto fix the typo in "Token".
Line range hint
47-71: LGTM: MaxTime configuration and assertion removedThe removal of the MaxTime configuration entry and its corresponding assertion is in line with the PR objectives. The test continues to verify the correct setting of CacheName from the configuration.
As before, consider renaming the test method from
TockeCacheOption_From_Config_ShouldtoTokenCacheOption_From_Config_Shouldto correct the typo in "Token".
Line range hint
73-98: Approved, but test method name needs updateThe removal of MaxTime-related configurations and assertions aligns with the PR objectives. However, the test method name no longer accurately reflects its purpose.
Rename the test method from
TockeCacheOption_From_Config_With_Default_MaxTime_ShouldtoTokenCacheOption_From_Config_Should_UseProvidedCacheNameor a similar name that better describes its current functionality. This change will improve the clarity of the test suite and prevent confusion for future developers.src/Arc4u.Standard.OAuth2.AspNetCore/Middleware/BasicAuthenticationMiddleware.cs (1)
89-89: Approve change with suggestions for improvementThis change aligns well with the PR objectives by removing the fixed timeout for the token cache. The new approach of using the token's expiration time is more dynamic and efficient. Good job!
A few suggestions to further improve this:
- Verify that
tokenInfo.ExpiresOnUtcis always set and valid before using it in this calculation.- Consider adding a comment explaining the rationale behind this change for future maintainers.
Here's a suggested improvement:
- tokenCache?.Put(cacheKey, tokenInfo.ExpiresOnUtc - DateTime.UtcNow, tokenInfo); + // Cache the token with its remaining validity time instead of a fixed timeout + if (tokenInfo.ExpiresOnUtc > DateTime.UtcNow) + { + tokenCache?.Put(cacheKey, tokenInfo.ExpiresOnUtc - DateTime.UtcNow, tokenInfo); + } + else + { + _logger.Technical().LogWarning("Attempted to cache an expired token"); + }This change adds a safety check and logging, which could help catch any unexpected scenarios where the token might already be expired.
src/Arc4u.Standard.UnitTest/Security/JwtHttpHandlerTests.cs (6)
274-276: Consider using a more realistic expiration time for cache entriesWhile the mock setup for the TokenCache's Put method is correct, using
TimeSpan.MaxValueas the expiration time might not reflect real-world scenarios accurately. Consider using a more realistic expiration time that aligns with typical token lifetimes or the specific requirements of your application.You could replace
TimeSpan.MaxValuewith a constant or configuration value that represents a typical token lifetime, for example:-mockTokenCache.Setup(m => m.Put<TokenInfo>(It.IsAny<string>(), TimeSpan.MaxValue, It.IsAny<TokenInfo>())); +mockTokenCache.Setup(m => m.Put<TokenInfo>(It.IsAny<string>(), TimeSpan.FromHours(1), It.IsAny<TokenInfo>()));
Line range hint
1-1000: Consider consistent use of var keywordThroughout the file, there's a mix of explicit type declarations and the var keyword. For better readability and consistency, consider using var more consistently where the type is obvious from the right side of the assignment.
For example, you could change instances like:
-IConfiguration configuration = new ConfigurationRoot(new List<IConfigurationProvider>(config.Providers)); +var configuration = new ConfigurationRoot(new List<IConfigurationProvider>(config.Providers)); -IServiceCollection services = new ServiceCollection(); +var services = new ServiceCollection();This change would make the code more consistent and potentially more readable, especially for complex types.
Line range hint
1-1000: Consider refining test method naming conventionThe test method names in this file are descriptive but quite long. Additionally, there's some inconsistency in the naming convention, with some using underscores and others ending with "Should". Consider adopting a more consistent and concise naming convention for improved readability.
Here are a few suggestions:
- Use a consistent format, e.g., "Should_[ExpectedBehavior]When[Condition]"
- Keep names concise while still being descriptive
- Use underscores consistently for readability
For example:
-public async Task Jwt_With_OAuth2_And_Principal_With_OIDC_Token_Should() +public async Task Should_InjectBearerToken_When_UsingOAuth2WithOIDCToken() -public async Task Jwt_With_ClientSecet_Should() +public async Task Should_InjectBearerToken_When_UsingClientSecret()This approach maintains descriptiveness while improving consistency and readability.
Line range hint
1-1000: Consider reorganizing tests for improved structureThe file contains multiple test methods covering various scenarios, but there's no clear separation or grouping of related tests. To improve readability and maintainability, consider reorganizing the tests into nested classes based on the scenarios they're testing.
Here's an example of how you could restructure the tests:
public class JwtHttpHandlerTests { public class OAuth2Tests { [Fact] public async Task Should_InjectBearerToken_When_UsingOAuth2WithOIDCToken() { ... } [Fact] public async Task Should_InjectBearerToken_When_UsingOAuth2WithBearerToken() { ... } } public class ClientSecretTests { [Fact] public async Task Should_InjectBearerToken_When_UsingClientSecret() { ... } } public class RemoteSecretTests { [Fact] public async Task Should_InjectBasicAuth_When_UsingRemoteSecretWithBasicAuthorization() { ... } [Fact] public async Task Should_InjectCustomHeader_When_UsingRemoteSecret() { ... } } // ... other nested classes for different scenarios }This structure would group related tests together, making it easier to navigate and maintain the test suite. It also allows for shared setup methods within each nested class if needed.
Line range hint
1-1000: Consider adding tests for error handling and edge casesThe current test suite covers various successful scenarios, but it may be beneficial to add tests that explicitly check error conditions and edge cases. This would improve the robustness of the test suite and ensure that the JwtHttpHandler behaves correctly in exceptional situations.
Consider adding tests for scenarios such as:
- Expired tokens
- Invalid tokens
- Missing required configuration
- Network failures when refreshing tokens
- Concurrent token refresh requests
For example, you could add a test like this:
[Fact] public async Task Should_ThrowException_When_TokenIsExpired() { // Arrange var expiredJwt = new JwtSecurityToken("issuer", "audience", new List<Claim> { new Claim("key", "value") }, expires: DateTime.UtcNow.AddHours(-1)); var expiredAccessToken = new JwtSecurityTokenHandler().WriteToken(expiredJwt); // ... set up the test with the expired token ... // Act & Assert await Assert.ThrowsAsync<SecurityTokenExpiredException>(async () => { await invoker.SendAsync(httpRequestMessage, CancellationToken.None); }); }Adding such tests would increase confidence in the error handling capabilities of the JwtHttpHandler.
Line range hint
1-1000: Consider centralizing and improving test data setupCurrently, test data such as tokens and configuration is set up individually in each test method, leading to some repetition across the test suite. To improve maintainability and readability, consider centralizing the test data setup and potentially implementing a test data builder pattern.
Here are some suggestions for improvement:
- Create helper methods for common setup tasks:
private IConfiguration CreateTestConfiguration(Dictionary<string, string> additionalSettings = null) { var baseSettings = new Dictionary<string, string> { ["Authentication:DefaultAuthority:Url"] = "https://login.microsoft.com" // ... other common settings ... }; if (additionalSettings != null) { foreach (var setting in additionalSettings) { baseSettings[setting.Key] = setting.Value; } } return new ConfigurationBuilder().AddInMemoryCollection(baseSettings).Build(); } private string CreateTestToken(DateTime? expirationTime = null) { var jwt = new JwtSecurityToken("issuer", "audience", new List<Claim> { new Claim("key", "value") }, notBefore: DateTime.UtcNow.AddHours(-1), expires: expirationTime ?? DateTime.UtcNow.AddHours(1)); return new JwtSecurityTokenHandler().WriteToken(jwt); }
- Implement a test data builder for complex objects:
public class TestPrincipalBuilder { private string _authenticationType = Constants.BearerAuthenticationType; private string _accessToken = null; public TestPrincipalBuilder WithAuthenticationType(string type) { _authenticationType = type; return this; } public TestPrincipalBuilder WithAccessToken(string token) { _accessToken = token; return this; } public AppPrincipal Build() { var identity = new ClaimsIdentity(_authenticationType); if (_accessToken != null) { identity.BootstrapContext = _accessToken; } return new AppPrincipal(new Arc4u.Security.Principal.Authorization(), identity, "S-1-0-0"); } }
- Use these in your tests:
[Fact] public async Task Should_InjectBearerToken_When_UsingOAuth2WithBearerToken() { var config = CreateTestConfiguration(new Dictionary<string, string> { ["Authentication:OAuth2.Settings:Audiences:0"] = "urn://audience.com", ["Authentication:OAuth2.Settings:Scopes"] = "user.read user.write" }); var accessToken = CreateTestToken(); var principal = new TestPrincipalBuilder() .WithAuthenticationType(Constants.BearerAuthenticationType) .WithAccessToken(accessToken) .Build(); // ... rest of the test setup and execution ... }These changes would make the tests more readable, reduce repetition, and make it easier to maintain and extend the test suite in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- src/Arc4u.Standard.OAuth2.AspNetCore.Adal/Token/Cache.cs (2 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (5 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/Extensions/AddClaimsFillerExtension.cs (2 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/Middleware/BasicAuthenticationMiddleware.cs (1 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/Options/ClaimsFillerOptions.cs (1 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/TokenProvider/CredentialTokenCacheTokenProvider.cs (1 hunks)
- src/Arc4u.Standard.OAuth2/Extensions/TokenCacheExtension.cs (0 hunks)
- src/Arc4u.Standard.OAuth2/Options/TokenCacheOptions.cs (0 hunks)
- src/Arc4u.Standard.OAuth2/Token/ApplicationCache.cs (2 hunks)
- src/Arc4u.Standard.OAuth2/Token/ApplicationLocalDataCache.cs (1 hunks)
- src/Arc4u.Standard.OAuth2/Token/ITokenCache.cs (2 hunks)
- src/Arc4u.Standard.UnitTest/Security/JwtHttpHandlerTests.cs (4 hunks)
- src/Arc4u.Standard.UnitTest/Security/TokenCacheOptionsTests.cs (3 hunks)
- src/Arc4u.Standard.UnitTest/Security/gRpcInterceptorTests.cs (5 hunks)
💤 Files with no reviewable changes (2)
- src/Arc4u.Standard.OAuth2/Extensions/TokenCacheExtension.cs
- src/Arc4u.Standard.OAuth2/Options/TokenCacheOptions.cs
🧰 Additional context used
🔇 Additional comments (16)
src/Arc4u.Standard.OAuth2/Token/ApplicationLocalDataCache.cs (2)
Line range hint
1-85: Overall assessment of ApplicationLocalDataCache changesThe changes to this class partially implement the PR objectives by introducing a
timeoutparameter in thePutmethod. However, there are a few points that need attention:
- The
timeoutparameter in thePutmethod is not currently used.- The
Clearmethod has been simplified, which improves code clarity but may affect error handling.Please address the comments provided for each method to ensure the changes fully meet the PR objectives.
To ensure consistency across the codebase, let's verify the usage of the
Putmethod:#!/bin/bash # Description: Check for calls to ApplicationLocalDataCache.Put method rg --type csharp -A 3 'ApplicationLocalDataCache.*Put\('This will help identify any places where the method is called and ensure they are updated to provide the new
timeoutparameter.
Line range hint
39-52: Implement thetimeoutparameter in thePutmethodThe
Putmethod signature has been updated to include aTimeSpan timeoutparameter, which aligns with the PR objectives. However, this parameter is currently not utilized within the method implementation.To address this, consider updating the
Cache.Putcall to include thetimeoutparameter:-Cache.Put(key, data); +Cache.Put(key, data, timeout);Also, ensure that the
ICacheinterface and its implementation support this new signature.Let's verify the
ICacheinterface definition:src/Arc4u.Standard.OAuth2/Token/ApplicationCache.cs (2)
Line range hint
1-85: Overall, the changes improve cache flexibility and align with PR objectives.The modifications to the
ApplicationCacheclass, particularly in thePutmethod, enhance the flexibility of the caching mechanism by allowing dynamic timeout settings. This aligns well with the PR objective of eliminating the fixed token cache timeout.The removal of unused namespaces (
SystemandSystem.Collections.Generic) is a good practice for code cleanliness.These changes contribute to a more efficient and maintainable caching system for OAuth2 tokens.
41-41: LGTM! Verify usage across the codebase.The addition of the
TimeSpan timeoutparameter to thePutmethod signature is a good improvement, allowing for more flexible cache management.To ensure all calls to this method have been updated accordingly, run the following script:
src/Arc4u.Standard.UnitTest/Security/TokenCacheOptionsTests.cs (2)
1-10: LGTM: Using statements updated to reflect new dependenciesThe changes in the using statements align well with the PR objectives. The addition of OAuth2-related namespaces (Arc4u.OAuth2.Extensions and Arc4u.OAuth2.Options) is consistent with the modifications to token caching mechanisms mentioned in the PR summary. The removal of unused namespaces helps keep the code clean and focused.
Line range hint
100-138: Review the relevance of this test caseThe removal of the MaxTime configuration entry is consistent with the PR objectives. However, the test's name and expected behavior (throwing an exception for MaxTime set to zero) may no longer be relevant.
Please clarify the following:
- Is this test still necessary given the removal of the MaxTime setting from TokenCacheOptions?
- If it is still needed, should it be updated to test the new timeout mechanism in ClaimsFillerOptions mentioned in the PR objectives?
- If it's no longer relevant, should it be removed entirely?
Consider running the following script to check for any remaining references to MaxTime in the TokenCacheOptions class:
Based on the results, we can determine if this test needs to be updated, replaced, or removed.
src/Arc4u.Standard.OAuth2.AspNetCore/TokenProvider/CredentialTokenCacheTokenProvider.cs (1)
84-84: Improved token caching with dynamic expirationThis change enhances the token caching mechanism by using the token's actual expiration time instead of a fixed timeout. This approach is more accurate and efficient, aligning well with the PR objectives.
Benefits:
- More precise cache management based on each token's specific lifetime.
- Potential reduction in unnecessary token refreshes.
- Improved security by ensuring tokens are not kept in cache beyond their actual expiration time.
The implementation looks correct and doesn't introduce any apparent issues.
src/Arc4u.Standard.UnitTest/Security/gRpcInterceptorTests.cs (5)
11-13: LGTM: New dependencies added to support updated test scenarios.The additional using statements for OAuth2, gRPC, and ASP.NET Core components are appropriate for the context of this test file. These imports align with the purpose of testing gRPC interceptors in an OAuth2 environment.
Also applies to: 22-22, 24-24
Line range hint
233-237: LGTM: Improved flexibility in test configuration setup.The addition of the foreach loop to dynamically populate the configuration dictionary with scopes enhances the test's flexibility. This change allows for easier modification of the number of scopes in the test setup, which is a good practice for maintaining and extending tests.
266-266: LGTM: Token cache timeout removal implemented correctly.The modification to use
TimeSpan.MaxValuein the TokenCache mock setup aligns perfectly with the PR objective of removing the fixed timeout for the token cache. This change effectively relies on the token's inherent expiration, streamlining the caching process as intended.
539-539: LGTM: Improved comment placement for better readability.The relocation of the comment "Register the different TokenProvider and CredentialTokenProviders." provides better context for the following code block. This minor change enhances the readability of the test setup.
Line range hint
1-585: Overall changes align well with PR objectives and maintain test integrity.The modifications to this test file successfully implement the changes required by the PR objectives, particularly regarding the removal of fixed token cache timeouts. The new dependencies and updated test scenarios reflect these changes while maintaining the overall structure and coverage of the original tests. The implementation appears sound and consistent throughout the file.
src/Arc4u.Standard.UnitTest/Security/JwtHttpHandlerTests.cs (3)
112-118: LGTM: Improved test setup with token informationThe addition of TokenRefreshInfo setup enhances the test scenario by providing necessary refresh and access token information. This change improves the completeness of the test case.
198-199: LGTM: Enhanced AppPrincipal setup with bearer tokenThe modification improves the test scenario by setting up an AppPrincipal with a BootstrapContext containing an access token. This change provides a more realistic representation of an authenticated principal with a bearer token.
Line range hint
244-248: LGTM: Dynamic scope configurationThe addition of this foreach loop enhances the test setup by dynamically adding scopes to the configuration dictionary. This approach allows for flexible and maintainable scope configuration in the test scenario.
src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (1)
102-102: Improved performance with HashSet for claim exclusionConverting
ClaimsToExcludeto aHashSet<string>withStringComparer.OrdinalIgnoreCaseenhances performance during claim exclusion checks due to faster lookup times compared to arrays or lists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (3)
127-133: Improved caching logic: Simplified and potentially more efficient.The updated code for handling cached claims is a good improvement. It simplifies the logic by removing the explicit expiration check and directly adds the cached claims to the identity. This change improves readability and potentially performance.
However, there's a minor optimization opportunity:
Consider using
!identity.HasClaim(c => StringComparer.OrdinalIgnoreCase.Equals(c.Type, cachedClaim.ClaimType))instead of!identity.Claims.Any(...)to potentially improve performance. Here's the suggested change:identity.AddClaims(cachedClaims.Where(c => !identity.Claims.Any(c1 => StringComparer.OrdinalIgnoreCase.Equals(c1.Type, c.ClaimType))) - .Select(c => new Claim(c.ClaimType, c.Value))); + .Where(c => !identity.HasClaim(c1 => StringComparer.OrdinalIgnoreCase.Equals(c1.Type, c.ClaimType))) + .Select(c => new Claim(c.ClaimType, c.Value)));This change uses the
HasClaimmethod, which might be more efficient than querying the entireClaimscollection.
152-162: Improved expiration date handling: More robust and comprehensive.The new logic for parsing and handling expiration dates is a significant improvement. It checks for expiration claims from multiple sources and provides a fallback mechanism. This approach ensures that an expiration date is always available for caching purposes.
To further improve readability and maintainability, consider the following refactoring:
private DateTimeOffset GetExpirationDate(IEnumerable<ClaimDto> claims, ClaimsIdentity identity) { var expClaim = claims.FirstOrDefault(c => StringComparer.OrdinalIgnoreCase.Equals(c.ClaimType, tokenExpirationClaimType)) ?? identity.Claims.FirstOrDefault(c => StringComparer.OrdinalIgnoreCase.Equals(c.Type, tokenExpirationClaimType)); if (expClaim != null && long.TryParse(expClaim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var ticks)) { return DateTimeOffset.FromUnixTimeSeconds(ticks); } return DateTimeOffset.UtcNow.Add(_options.MaxTime); }Then, you can simplify the main method:
var expDate = GetExpirationDate(claims, identity); SaveClaimsToCache(claims, cacheKey, expDate - DateTimeOffset.UtcNow);This refactoring encapsulates the expiration date logic in a separate method, improving readability and making the code easier to maintain.
Line range hint
194-218: Improved cache saving logic with better error handling.The updates to the
SaveClaimsToCachemethod are good improvements:
- The method now accepts a
TimeSpanparameter for more flexible cache timeout management.- Proper null checks and argument validation have been added.
- The method checks if the timeout is valid before saving to cache, preventing potential issues.
- Error handling has been improved with try-catch blocks and appropriate logging.
These changes enhance the robustness and reliability of the caching mechanism.
For consistency and to follow best practices, consider the following minor improvement:
Move the
ArgumentNullException.ThrowIfNull(claims);check to the beginning of the method, before thecacheKeycheck. This ensures that all argument validations are performed upfront:private void SaveClaimsToCache([DisallowNull] IEnumerable<ClaimDto> claims, string? cacheKey, TimeSpan timeout) { ArgumentNullException.ThrowIfNull(claims); if (string.IsNullOrWhiteSpace(cacheKey)) { return; } // Rest of the method... }This change improves the method's consistency and adheres to the general practice of validating arguments at the beginning of a method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Arc4u.Standard.OAuth2.AspNetCore.Adal/Token/Cache.cs (2 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (5 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/Options/ClaimsFillerOptions.cs (1 hunks)
- src/Arc4u.Standard.OAuth2/Token/ITokenCache.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Arc4u.Standard.OAuth2.AspNetCore.Adal/Token/Cache.cs
- src/Arc4u.Standard.OAuth2.AspNetCore/Options/ClaimsFillerOptions.cs
- src/Arc4u.Standard.OAuth2/Token/ITokenCache.cs
🧰 Additional context used
🔇 Additional comments (4)
src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (4)
13-13: LGTM: Added necessary namespace for dependency injection.The addition of
using Microsoft.Extensions.DependencyInjection;is appropriate, likely to support the use of dependency injection features in the class.
102-102: Great optimization: Improved claim filtering efficiency.Changing
ClaimsToExcludefrom an array to aHashSet<string>withStringComparer.OrdinalIgnoreCaseis an excellent optimization. This change offers two main benefits:
- Faster membership checks due to HashSet's O(1) average time complexity.
- Case-insensitive comparisons, ensuring consistent filtering regardless of claim type casing.
These improvements will enhance the overall performance of claim filtering operations.
145-150: Improved claim filtering and addition logic.The updates to claim filtering and addition are good improvements:
- Using
ClaimsToExclude.Contains()for filtering is more efficient due to the HashSet implementation.- The logic for adding claims to the identity has been simplified and optimized.
However, there's still a redundant filtering operation that was mentioned in a previous review.
As suggested in a previous review, the second
Whereclause on line 149 is redundant since the claims are already filtered usingClaimsToExcludeon line 145. Consider removing this redundant filtering to further optimize the code:identity.AddClaims(claims.Where(c => !StringComparer.OrdinalIgnoreCase.Equals(c.ClaimType, tokenExpirationClaimType)) - .Where(c => !identity.Claims.Any(c1 => StringComparer.OrdinalIgnoreCase.Equals(c1.Type, c.ClaimType))) .Select(c => new Claim(c.ClaimType, c.Value)));
166-176: Well-implemented expiration date parsing method.The
TryParseExpirationDatemethod is a good addition to the class. It provides a clean and reusable way to parse expiration dates from claim values. The use of theoutparameter pattern is appropriate for this scenario, allowing the method to return both a boolean indicating success and the parsed value.Some positive aspects of this implementation:
- It's private and static, which is correct for its intended use within the class.
- It handles null values and parsing errors gracefully.
- It uses
CultureInfo.InvariantCulturefor parsing, ensuring consistent behavior across different locales.
…im names (a.k.a. "types") are case-sensitive. Claims manipulations have been factored out in a separate utility class.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (3 hunks)
- src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs (2 hunks)
- src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs (2 hunks)
- src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/Arc4u.Standard.OAuth2.AspNetCore/Security/AppServicePrincipalFactory.cs (4)
11-11: LGTM: New namespace import aligns with PR objectives.The addition of
using Arc4u.Standard.OAuth2.Security;is consistent with the changes described in the PR objectives, particularly regarding token caching and claim handling modifications.
102-102: LGTM: Improved method signature and variable scoping.The addition of the
settingsResolveNameparameter enhances flexibility in configuration resolution, aligning with the PR's goal of streamlining the caching process. Additionally, declaring thetokenvariable inline within thetryblock is a good practice that limits the variable's scope.
Line range hint
1-146: Overall assessment: Changes align with PR objectives and improve code quality.The modifications to
AppServicePrincipalFactory.csalign well with the PR objectives. Key improvements include:
- Enhanced flexibility through the addition of the
settingsResolveNameparameter.- Streamlined claim handling with the new
MergeClaimsandSanitizemethods.- Optimized variable scoping for the
tokenvariable.These changes contribute to a more efficient and maintainable codebase. However, ensure thorough testing of the new claim handling methods to maintain security and correctness.
105-105: Verify implementation of new claim handling methods.The new approach using
identity.MergeClaims(jwtToken.Claims.Sanitize())appears to streamline claim handling, which aligns with the PR objectives. However, please ensure that theMergeClaimsandSanitizemethods are properly implemented and thoroughly tested to maintain security and correctness.To verify the implementation of these methods, please run the following script:
src/Arc4u.Standard.OAuth2/Security/ClaimsUtility.cs (2)
27-37: Verify the handling of the "exp" claim inSanitizemethodsIn the
Sanitizemethod forIEnumerable<ClaimDto>(lines 27-37), the "exp" claim is included in the output claims. Conversely, in theSanitizemethod forIEnumerable<Claim>(lines 46-56), the "exp" claim is explicitly excluded. Please verify if this difference is intentional and aligns with the desired behavior.Also applies to: 46-56
19-19:⚠️ Potential issueFix syntax error in
ClaimsToExcludeinitializationThere is a syntax error in the initialization of the
ClaimsToExcludeHashSet. In C#, you should initialize aHashSet<string>usingnew HashSet<string> { ... }.Apply this diff to fix the syntax error:
-private static readonly HashSet<string> ClaimsToExclude = ["aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope"]; +private static readonly HashSet<string> ClaimsToExclude = new HashSet<string> { "aud", "iss", "iat", "nbf", "acr", "aio", "appidacr", "ipaddr", "scp", "sub", "tid", "uti", "unique_name", "apptype", "appid", "ver", "http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant", "http://schemas.microsoft.com/identity/claims/scope" };Likely invalid or redundant comment.
src/Arc4u.Standard.OAuth2.AspNetCore/AppPrincipalTransform.cs (1)
151-160: Ensure cache timeout is non-negative to prevent errorsCalculating the timeout as
DateTimeOffset.FromUnixTimeSeconds(ticks) - DateTimeOffset.UtcNowmay result in a negativeTimeSpanif the expiration time is in the past. Ensure the timeout is non-negative before proceeding.src/Arc4u.Standard.OAuth2.Client/Security/Principal/AppPrincipalFactory.cs (3)
13-13: ImportingArc4u.Standard.OAuth2.Securityis necessaryThe added using directive
using Arc4u.Standard.OAuth2.Security;is essential for accessing methods likeTryGetExpiration,Sanitize(), andMergeClaimsused later in the code.
126-129: Efficiently determine if claims can be copied from cacheThe updated logic accurately checks if the token's expiration matches the cached claims' expiration. This ensures that cached claims are reused only when valid, reducing unnecessary backend calls.
149-154: Properly retrieve and merge additional claimsThe code correctly retrieves additional claims using
claimFiller.GetAsync, sanitizes them, and merges them into the identity. Logging the number of claims added enhances traceability and debugging capabilities.
| return _cacheHelper.GetCache().Get<List<ClaimDto>>(cacheKey); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Technical().Exception(ex).Log(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching general exceptions
Catching the general Exception type may mask specific issues and make debugging more difficult. Consider catching more specific exceptions that are expected from the cache retrieval operation.
| _cacheHelper.GetCache().Put(cacheKey, timeout, claims); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Technical().Exception(ex).Log(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching general exceptions
In the cache save operation, catching the general Exception type can hide specific errors and complicate error handling. It's advisable to catch specific exceptions that you anticipate from the cache put operation.
| identity.AddClaims(jwtToken.Claims.Sanitize().Select(c => new Claim(c.Type, c.Value))); | ||
| // there is no guarantee that expTokenClaim is not null, so we need to check it. | ||
| if (expTokenClaim is not null) | ||
| { | ||
| identity.AddClaim(expTokenClaim); | ||
| } |
There was a problem hiding this comment.
Verify potential duplication of the exp claim
Since Sanitize() might not remove the exp claim from jwtToken.Claims, adding expTokenClaim separately could result in duplicate exp claims in the identity. Please verify whether Sanitize() removes the exp claim to prevent duplication.
To confirm, check the implementation of Sanitize():
#!/bin/bash
# Description: Verify if the `exp` claim is removed by the Sanitize() method.
# Search for the Sanitize() method implementation
fd --type f --extension cs --exec rg --type cs 'ClaimExtensions\.Sanitize\(\)'If Sanitize() preserves the exp claim, consider avoiding adding expTokenClaim again or modify Sanitize() accordingly.
This pull request eliminates the reliance on a fixed timeout for the token cache.
In other words, the
MaxTimesettings in the following section:... isn't used anymore and can be eliminated.
The reasoning behind the modification is that the implicit validity of the cached information is never longer than the expiration date of the stored token. And in all use cases except one, the expiration date can be obtained from the token information being stored (i.e. the
expclaim). This makes the explicit setting in the configuration unnecessary.The only exception is the "extra claims" than can optionally be loaded. These extra claims are cached. It is expected that the
IClaimsFillerimplementation returns anexpclaim. If it doesn't, the code now looks for anexpclaim in the existingClaimsPrincipal. And if for some reason that fails, we need a timeout. This timeout is now part of theClaimsFillerOptions. It is calledMaxTimeas before, and defaults to 20 minutes if it isn't specified (since in existing code, it would not be).The code
AppPrincipalTransform.cshas been optimized to avoid needless list copying, and to systematically compare claim types case-insensitively.What is still missing in this PR is a way to explicitly delete cached entries when the user logs off. It also doesn't change the way cache keys are computed.
According to https://www.rfc-editor.org/rfc/rfc7519 section 10.1, claim names (a.k.a. "types") are case-sensitive: this has been enacted throughout the code.
Claims manipulations have been factored out in a separate utility class.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation