Keycloak / PR #22

Add caching support for IdentityProviderStorageProvider.getForLogin operations

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

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Recursive caching call using session instead of delegate HIGH ✗ Not found The review covers the race on the shared cache key (InfinispanIdentityProviderStorageProvider L214) and the invalidation asymmetry (L405), but no candidate mentions the recursive call — the caching wrapper re-enters the same caching path through the session instead of reading through the delegate. See model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java:246
#2 Cleanup reference uses incorrect alias - should be 'idp-alias-' + i instead of 'alias'. MEDIUM ✓ Found "Cleanup registers the wrong IDP alias: both cleanup entries use the literal "alias" instead of the created idp-alias-<i> names. As a result, none of the 21 created IDPs are ever removed, leaving the realm polluted and breaking exact-count assertions in sibling tests (e.g. testCacheIDPByOrg)" (organization/cache/OrganizationCacheTest.java L381)

Supplementary Findings (5 findings)

Finding File Severity Legitimate?
Concurrency race: getForLogin uses a single cache key per (realm, fetch-mode) shared across all organizations, distinguished only by an internal searchKey — concurrent first-load calls can each add their own query and the last write drops the other org's entry InfinispanIdentityProviderStorageProvider.java:214 MEDIUM ✓ Real race under concurrent first-load; lands in the golden's area without naming the recursion
Behavior change to the public getLoginPredicate() API — this MR ANDs in an org/public filter, a contract change with 18 dependent callers (e.g. IdentityProviderBean.federatedProviderPredicate() now excludes org-linked non-public IDPs) IdentityProviderStorageProvider.java:252 MEDIUM ✓ Real contract change to a public SPI method, user-visible; needs confirmation and tests
Invalidation asymmetry: original is obtained via org-aware getById (its isEnabled() returns false when the org is disabled) while updated is the raw model — spurious invalidation of all login caches when an org-linked IDP is updated while its org is disabled InfinispanIdentityProviderStorageProvider.java:405 LOW ✓ Real but only over-invalidates; no consistency bug
Test does not cover the org disable/remove scenario that the bean's isEnabled() re-check exists for — a regression where the re-check is dropped would not be caught OrganizationCacheTest.java:370 LOW ✓ Real coverage gap for the motivating case
Test does not exercise the multi-searchKey reload/merge branch (the IdentityProviderListQuery(..., previous) constructor) — only single-searchKey paths are covered, leaving the most fragile logic unverified OrganizationCacheTest.java:395 LOW ✓ Real coverage gap on the new caching path

Honest Analysis

FriendlyReviewer found 1 of the 2 golden bugs of this PR — recall 50%. G2 (the cleanup registering literal "alias" instead of idp-alias-<i>) is found and made the single blocking "close look" finding, with the consequences spelled out: none of the 21 created IDPs are ever removed, polluting the realm and breaking exact-count assertions in sibling tests. That is the right call — a broken @After cleanup is a test-isolation defect that can corrupt unrelated suites.

The miss is the interesting part. G1 is a recursive call — the caching wrapper reads back through the session instead of the delegate, so each cache miss re-enters the same caching path. The review worked in the exact area: it describes the shared cache key at L214, the self-heal branch, and the re-materialization with fallback, but every candidate describes a race or a consistency issue, never the recursion itself. Structurally, that is the most damaging defect of the PR — a cache-aside layer that calls back into the cache — and the review's closest approximation (L214) is about concurrent first loads, not recursion.

The five supplementary findings are the most substantive of the run: the getLoginPredicate() contract change with 18 dependent callers is a genuine API-level concern with a concrete visible effect, and the L405 asymmetry is correctly classified as over-invalidation only. The two missing test scenarios are honest coverage gaps. Nothing here is filler.

The caveat is the verdict's confidence: the review asserts the design is "sound" and the cache "functional" while its own findings describe a shared-key race, a spurious-invalidation path, and untested merge logic. The recursion golden sits precisely in that gap between the positive summary and the collected caveats — the review validated the caching design at the level of keys and invalidation and never questioned the read path itself.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
1 high, 2 medium, 3 low 1 mitigated

This MR introduces a caching layer for IdentityProviderStorageProvider.getForLogin in the Infinispan provider, using a per-(realm, fetch-mode) cache key with per-organization search keys. The overall design is sound: invalidation correctly tracks login-relevant changes, org-link changes propagate through the update path, and cache consistency is maintained via re-materialization with fallback. However, the review surfaces several issues that should be addressed before merge: a concurrency race due to a shared cache key across organizations, a spurious-invalidation asymmetry in the update logic, a behavior change to the public getLoginPredicate() API that affects dependent callers, and test-quality problems including incorrect cleanup registrations and missing coverage of the org-disable and multi-searchKey reload paths. The test cleanup defect is particularly concerning as it leaves 21 IDPs registered and can corrupt count assertions in sibling tests.

These might need a close look

  • 🔴 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L381)
    Cleanup registers the wrong IDP alias: both cleanup entries use the literal "alias" instead of the created idp-alias-<i> names. As a result, none of the 21 created IDPs are ever removed, leaving the realm polluted and breaking exact-count assertions in sibling tests (e.g., testCacheIDPByOrg). Fix by registering per-created-alias cleanup or removing org links in cleanup.

Worth checking

  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L214)
    Concurrency race: getForLogin uses a single cache key per (realm, fetch-mode) shared across all organizations, distinguished only by an internal searchKey. Concurrent first-load calls for different orgs can both see query == null and each addRevisioned their own query, with the last write dropping the other org's entry. The self-heal branch also calls invalidateObject on the shared key, which can clobber a just-written entry for another org, causing cache thrashing under load. Recommend keying by (realm, mode, organizationId), as getByOrganization does, or documenting the intentional behavior.
  • 🟡 server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java (L252)
    Behavior change to the public getLoginPredicate(): this MR ANDs in an org/public filter (orgId == null || BROKER_PUBLIC == true). This is a contract change to an existing SPI method with 18 dependent callers. For example, IdentityProviderBean.federatedProviderPredicate() now excludes org-linked non-public IDPs from the federated provider list. This may be intended, but it is a user-visible change to an existing path and should be confirmed and covered by tests.
Small things (take or leave)
  • 🔵 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (L405)
    Asymmetry in update invalidation: original is obtained via getById and is org-aware wrapped (its isEnabled() returns false when the linked org is disabled/deleted), while updated is the raw model. When an org-linked login IDP is updated while its org is disabled, the predicate on original fails and causes a spurious invalidation of all login caches, negating the no-op optimization. This is not a consistency bug (only over-invalidates), but it makes the logic fragile and the predicates inconsistent; evaluate against a raw representation or document the behavior.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L370)
    Test does not cover the org disable/remove scenario that the bean's isEnabled() re-check exists for. The motivating case (org disabled/removed without invalidating the login cache) is not exercised, so a regression where the re-check is dropped would not be caught. Add a step disabling/re-enabling the org and re-asserting filtered results.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (L395)
    Test does not exercise the multi-searchKey reload/merge branch (the IdentityProviderListQuery(..., previous) constructor) of the new caching path. Only single-searchKey paths are covered, leaving the most fragile logic unverified. Consider issuing a second getForLogin with a different org id in the same session and asserting both search keys are retained.

✅ Feature-level checklist

Partially covered

  • ⚠️ Add cache for IdentityProviderStorageProvider.getForLogin (as stated in the PR summary).
    The cache is implemented and functional, but the concurrency race on the shared cache key, spurious invalidations, and behavior change to the public getLoginPredicate() mean the implementation is not fully robust or transparent. Test coverage gaps (org-disable and multi-searchKey branch) also leave critical paths unverified. The core requirement is satisfied, but with caveats that should be addressed or explicitly accepted.