Keycloak / PR #2

Add caching support for IdentityProviderStorageProvider.getForLogin

keycloak/keycloak#32918
Golden recall: 50% Extra findings: 9

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Recursive caching call using session instead of delegate — session.identityProviders().getById(id) in getForLogin() goes through the Infinispan wrapper instead of using idpDelegate directly HIGH ✗ Missed FR found related cache issues (race condition in cache-miss, missing invalidation in removeAll(), potential NPE) but did not specifically identify that session.identityProviders().getById(id) bypasses the delegate, creating an inefficient recursive call through the Infinispan wrapper.
#2 Cleanup reference uses incorrect alias — getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove) should use "idp-alias-" + i instead of "alias" HIGH ✓ Found FR: "Test cleanup uses literal string 'alias' instead of actual IDP alias. Cleanup will silently fail to remove created IDPs, causing test pollution." Upgraded from Medium to HIGH.

Supplementary Findings (12 findings)

Finding File Severity Legitimate?
Potential NullPointerException if storedIdp is null in remove() — registerIDPInvalidation(storedIdp) called without null guard InfinispanIdentityProviderStorageProvider.java:97 HIGH ✓ Real bug — NPE if alias does not exist
Unnecessary DB round-trip on every remove() — idpDelegate.getByAlias(alias) called even when cache is valid InfinispanIdentityProviderStorageProvider.java:94 MEDIUM ✓ Performance optimization
removeAll() does not invalidate login caches — may leave stale references InfinispanIdentityProviderStorageProvider.java:113 MEDIUM ✓ Consistency issue
registerIDPLoginInvalidation uses getLoginPredicate() with org-aware isEnabled() — may cause excessive invalidations InfinispanIdentityProviderStorageProvider.java:385 MEDIUM ✓ Potential cache thrashing
getLoginPredicate() org-aware logic not applied in the default getForLogin implementation IdentityProviderStorageProvider.java:258 MEDIUM ✓ Semantic gap in interface
REALM_ONLY path does not filter with idp.isEnabled() unlike other paths OrganizationAwareIdentityProviderBean.java:79 MEDIUM ✓ Inconsistent filtering
Test only validates count of cached IDPs, not their identity OrganizationCacheTest.java:430 MEDIUM ✓ Insufficiently precise tests
No coverage for org-disabled scenario OrganizationCacheTest.java:520 MEDIUM ✓ Missing coverage
No test for removing a login-qualifying IDP OrganizationCacheTest.java:416 MEDIUM ✓ Missing coverage
Missing @Override on getForLogin InfinispanIdentityProviderStorageProvider.java:221 LOW ✓ Java convention
Race window in cache-miss-for-searchKey: cache.invalidateObject(cacheKey) before building the new query InfinispanIdentityProviderStorageProvider.java:246 LOW ✓ Potential race condition
No test for IDP transition from one org to another OrganizationCacheTest.java:534 LOW ✓ Missing coverage

Honest Analysis

FriendlyReviewer detected 1 of 2 golden bugs. The cleanup alias bug (#2) was found and upgraded from Medium to HIGH. The recursive caching bug (#1) was missed — it is a subtle issue where session.identityProviders().getById() goes through the Infinispan wrapper instead of the JPA delegate. FriendlyReviewer found related problems (race condition in cache-miss, missing invalidation) but not the recursion itself.

The 12 supplementary findings are where FriendlyReviewer compensated. The NPE in remove() (HIGH) is a legitimate crash risk. The unnecessary DB round-trip, missing invalidation in removeAll(), and inconsistent org-aware filtering are all real performance or correctness concerns.

A pattern worth noting: three findings flag missing test coverage for specific scenarios (org-disabled, login-qualified IDP removal, org-to-org transition). FriendlyReviewer does not just review the implementation — it evaluates whether the tests are adequate for the code being changed.

The missing @Override annotation is minor but illustrates how FriendlyReviewer catches style issues that escape human review.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
2 high, 8 medium, 3 low

This MR introduces caching for getForLogin in the Infinispan IdentityProviderStorageProvider, along with organization-aware filtering. The overall design is sound, but several issues were identified: unnecessary database round-trips, null-pointer risks, missing invalidation for removeAll(), missed @Override annotations, race windows in cache-miss handling, and inconsistent isEnabled() re-checking in the organization-aware bean. Additionally, the tests suffer from cleanup bugs and lack coverage for critical scenarios such as org-disabled states, removal of login-qualifying IDPs, and org-to-org transitions. These issues should be addressed to ensure correctness, performance, and test reliability.

These might need a close look

  • 🔴 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L97)
    Potential NullPointerException: If storedIdp is null (alias does not exist), registerIDPInvalidation(storedIdp) will throw NPE. Add a null guard before calling registerIDPInvalidation.
  • 🔴 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L399)
    Test cleanup uses literal string "alias" instead of actual IDP alias. Cleanup will silently fail to remove created IDPs, causing test pollution. Use dynamic alias variable.

Worth checking

  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L94)
    Unnecessary database round-trip: idpDelegate.getByAlias(alias) is called even when storedIdp from cache is non-null. Cache hit should skip the DB call entirely.
  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L113)
    Missing cache invalidation for login caches in removeAll(). After removing all IDPs, registerIDPInvalidation is not called, leaving stale entries in login caches.
  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L385)
    registerIDPLoginInvalidation uses getLoginPredicate() which includes organization-aware isEnabled() re-checking. This means any cache invalidation re-evaluates the predicate against the current (potentially changed) state, which can cause excessive or incorrect invalidations. Consider separating the cache key construction from the predicate evaluation.
  • 🟡 services/src/main/java/org/keycloak/models/utils/IdentityProviderStorageProvider.java (L258)
    The default getForLogin implementation does not apply the organization-aware getLoginPredicate() logic, creating an inconsistency between the cached and uncached paths.
  • 🟡 services/src/main/java/org/keycloak/models/jpa/OrganizationAwareIdentityProviderBean.java (L79)
    The REALM_ONLY path does not filter with idp.isEnabled(), unlike the ORGANIZATION and ANY paths. This inconsistency means disabled IDPs may be returned in the REALM_ONLY case.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L430)
    Test only validates the count of IDPs returned from cache, not their identity. A cache returning stale/wrong IDPs would still pass the count-based assertion.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L520)
    No test coverage for the scenario where an IDP is disabled for an organization while cached. The cache may return stale enabled state.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L416)
    No test for the scenario where an IDP that was previously qualifying for login is removed, ensuring the cache reflects the removal.

Small things (take or leave)

  • 🔵 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L221)
    Missing @Override annotation on getForLogin method. Add it for consistency and compile-time checking.
  • 🔵 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L246)
    Potential race window: cache.invalidateObject(cacheKey) is called before the new query result is fully constructed. If a concurrent read arrives between invalidation and population, it triggers a redundant DB query.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L534)
    No test coverage for the scenario where an IDP is transitioned from one organization to another while cached.