Add caching support for IdentityProviderStorageProvider.getForLogin operations

[ShashankFC]

Test 2nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#2

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 38s —— View job

---

Code Review Complete ✅

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files and understand the implementation
• Analyze caching logic for correctness
• Check for bugs, security issues, and performance problems
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR adds caching support for the IdentityProviderStorageProvider.getForLogin() operation, which is frequently called during login flows. The implementation follows the existing caching patterns in Keycloak and includes comprehensive test coverage.

Overall Assessment

✅ Strong implementation - The caching logic is well-designed, properly handles cache invalidation, and includes excellent test coverage. The code follows existing patterns in the codebase.

Detailed Review

1. InfinispanIdentityProviderStorageProvider.java (+95/-1)

Positive aspects:

• The getForLogin() caching implementation (lines 214-255) follows the same pattern as getByOrganization(), maintaining consistency
• Cache key generation properly includes both realm ID and FetchMode (line 77-79)
• Proper cache invalidation logic in registerIDPLoginInvalidation() (lines 382-389) and registerIDPLoginInvalidationOnUpdate() (lines 403-418)
• Smart invalidation: only invalidates login caches when IDPs qualify as login IDPs (line 384)

Issues identified:

🔴 Critical: Potential cache inconsistency on IDP deletion (lines 382-389, 110-112)

• In registerIDPLoginInvalidation(), you invalidate all FetchMode caches when an IDP qualifies for login
• However, in the remove() method (line 111), you call registerIDPLoginInvalidation(storedIdp) which could be null if the IDP doesn't exist
• This could cause a NullPointerException if storedIdp is null and getLoginPredicate().test(idp) is called

Location: InfinispanIdentityProviderStorageProvider.java:111

registerIDPLoginInvalidation(storedIdp); // storedIdp could be null!

Recommendation: Add null check:

if (storedIdp != null) {
    registerIDPLoginInvalidation(storedIdp);
} else {
    // If we couldn't find the IDP, invalidate all login caches to be safe
    for (FetchMode mode : FetchMode.values()) {
        realmCache.registerInvalidation(cacheKeyForLogin(getRealm(), mode));
    }
}

🟡 Medium: Cache invalidation edge case in update (lines 403-418)

• The registerIDPLoginInvalidationOnUpdate() method has sound logic for detecting when to invalidate
• However, there's an edge case: if an IDP's organization link changes but both orgs make it qualify for login, and the organization is different, the cache should still be invalidated
• Line 410 checks Objects.equals(original.getOrganizationId(), updated.getOrganizationId()) but this might not catch all cases where the organization membership changes in ways that affect the cache

Good practices observed:

• Comprehensive Javadoc on registerIDPLoginInvalidationOnUpdate() explaining the invalidation logic (lines 391-402)
• The method properly handles the case where an IDP transitions between login-available states

2. IdentityProviderStorageProvider.java (+1/-0)

Observation:

• Added getForLogin() as a default interface method (line 165)
• The default implementation is functional but relies on getAllStream() which may not be cached in all implementations
• This is acceptable since the Infinispan implementation overrides it with caching

3. OrganizationAwareIdentityProviderBean.java (+2/-2)

Changes:

• Lines 58, 74: Changed from session.identityProviders().getAllStream() with manual filtering to session.identityProviders().getForLogin()
• This is a good refactoring that leverages the new cached method

Positive aspects:

• Simplifies the code by removing manual filtering logic
• Will benefit from caching performance improvements
• The filtering logic is now centralized in the storage provider

4. OrganizationCacheTest.java (+170/-0)

Test coverage:

• The testCacheIDPForLogin() test (lines 369-535) is comprehensive and well-structured
• Tests multiple scenarios:
  1. ✅ Cache population for different FetchModes
  2. ✅ Cache persistence when non-login IDPs are added/removed
  3. ✅ Cache persistence when IDPs are updated without changing login availability
  4. ✅ Cache invalidation when IDP login availability changes
  5. ✅ Cache invalidation when organization links change

Excellent test practices:

• Tests verify cache state directly by accessing the cache (lines 396-397, 435-439)
• Tests verify cache invalidation properly (lines 476-480, 512-516)
• Tests verify re-population after invalidation (lines 482-497, 518-533)

Minor observation:

• Line 303: typo in cleanup registration - uses "alias" instead of the actual alias variable. This should be:

getCleanup().addCleanup(() -> testRealm().identityProviders().get(alias)::remove);

Security Considerations

✅ No security issues identified:

• The caching mechanism properly respects realm boundaries
• Cache keys include realm ID to prevent cross-realm data leakage
• The cache invalidation logic ensures stale data isn't served

Performance Considerations

✅ Positive performance impact:

• Caching getForLogin() will significantly improve login page performance, especially in realms with many IDPs
• The invalidation logic is selective (only invalidates when necessary), minimizing cache churn
• The IdentityProviderListQuery supports multiple search keys, reducing duplicate cache entries

Recommendations

1. Fix the null check issue in remove() method (line 111) - see critical issue above
2. Consider documenting the cache key structure and invalidation strategy in the class-level Javadoc
3. Fix test cleanup registration (line 303) to use the correct alias variable

Conclusion

This is a well-implemented caching enhancement that follows Keycloak's patterns. The critical issue with null handling in the remove() method should be addressed before merging. The test coverage is excellent and demonstrates thorough understanding of the caching behavior.

---