[PM-23572] Add Persistent Grants to SSO #6636
Conversation
…CachePersistedGrantStore.
… comments on ExtendedCache vs InMemoryCaching for Duende.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6636 +/- ##
==========================================
+ Coverage 52.99% 53.33% +0.34%
==========================================
Files 1910 1910
Lines 84843 85091 +248
Branches 7636 7651 +15
==========================================
+ Hits 44966 45387 +421
+ Misses 38125 37950 -175
- Partials 1752 1754 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @enmande's task —— View job Code Review CompleteSummaryThis PR implements distributed caching for SSO authorization code grants using the ExtendedCache infrastructure, enabling horizontal scaling of the SSO service. The implementation is well-designed, thoroughly tested, and follows established patterns in the codebase. The PR effectively addresses the scaling requirements while maintaining backward compatibility with in-memory-only deployments. FindingsFinding 1: PR description and objective are exemplary💭 The PR has excellent documentation including clear objective, technical details, video demonstrations showing both in-memory and Redis configurations, and explicit verification of TTL behavior. This sets a high standard for PR quality. Finding 2: Use typed constant for cache name in service registration🎨 Following the pattern established in PR #6650 ( Current vs. Suggested PatternCurrent (bitwarden_license/src/Sso/Utilities/ServiceCollectionExtensions.cs:86) services.AddExtendedCache(PersistedGrantsDistributedCacheConstants.CacheKey, globalSettings);Suggested Pattern Status: Already addressed - the code correctly uses the constant. Finding 3: Test coverage gap for service registration
Location: bitwarden_license/src/Sso/Utilities/ServiceCollectionExtensions.cs:86-90 Finding 4: Default FusionCacheEntryOptions may be redundant💭 In Current Implementationawait _cache.SetAsync(
grant.Key,
grant,
new FusionCacheEntryOptions { Duration = duration });The ExtendedCache is already configured with defaults including:
Your test at line 71-86 verifies these defaults are in place, but the implementation doesn't explicitly set them (which is correct - they're inherited from the cache configuration). Recommendation: The current implementation is actually correct. The test is verifying the inherited defaults work as expected. No change needed. Finding 5: Excellent test coverage and quality👍 The test suite is comprehensive with 13 test cases covering:
The tests effectively use NSubstitute and demonstrate good testing practices. Finding 6: Security considerations well-addressed👍 The implementation properly:
This aligns with Bitwarden's zero-knowledge security principles. Finding 7: Clear documentation of design decisions📝 The code includes excellent inline comments explaining:
Finding 8: Backward compatibility maintained👍 The graceful degradation to in-memory caching when Redis is not configured ensures existing deployments continue working without configuration changes. This is verified by the video demonstration. Finding 9: Naming consistency📝 The constant is named Location: bitwarden_license/src/Sso/Utilities/PersistedGrantsDistributedCacheConstants.cs:9 Finding 10: No nullable reference type annotations💭 The new files don't have
Note that Action ItemsRequired (
Recommended (💭):
Optional (🎨):
Good Practices Observed
|
brant-livefront
left a comment
brant-livefront
left a comment
There was a problem hiding this comment.
This looks great and I'm excited to see someone else using extended cache. 🎉
A few mild suggestions, but this looks good to me.
| private readonly IFusionCache _cache; | ||
|
|
||
| public DistributedCachePersistedGrantStore( | ||
| [FromKeyedServices("sso-grants")] IFusionCache cache) |
There was a problem hiding this comment.
One of the patterns I was using (you can see an example in #6650) was to have a cache constants class. The virtue being that the cache name and keys are not strings but are typed to constants. It looks like you already have a consistent way of building keys, so you probably don't need that part. But it still might be worthwhile to make absolutely sure the cache name is consistent.
i.e.
// In constants class
public const string CacheName = "sso-grants";
// In this code
[FromKeyedServices(SsoGrantsCacheConstants.CacheName)] IFusionCache cache)| // Keep distributed cache enabled for multi-instance scenarios | ||
| // When Redis isn't configured, FusionCache gracefully uses only L1 (in-memory) | ||
| }.SetSkipDistributedCache(false, false)); |
There was a problem hiding this comment.
I think these are the default values, so you shouldn't have to overwrite on every cache entry?
| // Provides separation of concerns and automatic Redis/in-memory negotiation | ||
| // .AddInMemoryCaching should still persist above; this handles configuration caching, etc., | ||
| // and is separate from this keyed service, which only serves grant negotiation. | ||
| services.AddExtendedCache("sso-grants", globalSettings); |
There was a problem hiding this comment.
If you adopt my comment above, here's another place to use the CacheName constant
…ove explicit skip distributed cache on set for default configuration.
🎟️ Tracking
PM-23572
📔 Objective
Allow SSO to be configured such that grants (currently only
authorization_codegrants are used) can be distributed beyond the server's in-memory caching to enable horizontal scaling of the application.Clarifying details around this particular approach can be found in the ticket, the linked Tech Breakdown, and in the proposed code.
This approach will use the
ExtendedCacheintroduced by #6591. The following occurs:ExtendedCache, scoped to SSO grants.ExtendedCachewill negotiate the storage mechanism behind the keyed service:After 5 minutes, all storage eventualities had their TTLs verified; inspection of Redis via
KEYS sso-grant*showed an empty array.📸 Screenshots
In-Memory Caching
No redis configuration present. Does not support horizontal scaling of SSO. Represents continued viability of current state.
PM-23572__in-memory.mov
Redis caching
Redis configuration present. Shows startup of the SSO application and connection to Redis with configuration of the keyed backplane. Shows a full SSO login flow proving scoped storage of sso grants to the cache (key conflict possibilities mitigated), with all information encrypted in protected payload.
PM-23572__redis.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes