Add caching support for IdentityProviderStorageProvider.getForLogin operations#2
Conversation
Closes #32573 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces login-aware caching for identity providers in Keycloak's Infinispan cache layer. New methods enable per-login cache key generation and retrieval with fallback logic, nuanced invalidation handling for cache consistency, and enhanced filtering to enforce enabled status and organization constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InfinispanCache as Infinispan Cache
participant IDPProvider as IDPStorageProvider
participant Delegate as Delegate Storage
Client->>IDPProvider: getForLogin(fetchMode, orgId)
IDPProvider->>InfinispanCache: retrieve with cacheKeyForLogin()
alt Cache Hit
InfinispanCache-->>IDPProvider: cached IDPs
IDPProvider-->>Client: return cached result
else Cache Miss
IDPProvider->>Delegate: getForLogin(fetchMode, orgId)
Delegate-->>IDPProvider: IDPs from delegate
IDPProvider->>InfinispanCache: store in login cache
IDPProvider-->>Client: return result
end
sequenceDiagram
participant Admin
participant IDPProvider as IDPStorageProvider
participant Cache as Login Cache
Admin->>IDPProvider: update IDP (login-eligibility change)
IDPProvider->>IDPProvider: registerIDPLoginInvalidationOnUpdate()
alt Login-eligibility Changed
IDPProvider->>Cache: invalidate login caches
Note over Cache: caches cleared for affected FetchModes
else Only Config Changed
Note over IDPProvider: no invalidation needed
Cache-->>Cache: remains valid
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java:
- Around line 380-381: The cleanup registration uses a hardcoded "alias" string
so removals fail; change the cleanup to reference the actual identity provider
alias from the created representation (e.g., use idpRep.getAlias() or capture a
final String alias = idpRep.getAlias()) when calling get(...) in the lambda
added via getCleanup().addCleanup(...) so the removal targets the real provider
created by testRealm().identityProviders().create(idpRep).close().
- Around line 424-425: The cleanup currently uses a hardcoded "alias" when
registering removal: replace the literal "alias" with the actual identity
provider alias used when creating the provider (e.g., use idpRep.getAlias() or
the local alias variable) so getCleanup().addCleanup(...) calls
testRealm().identityProviders().get(actualAlias)::remove; ensure the same alias
value passed to testRealm().identityProviders().create(idpRep) is used for the
subsequent get(...) call.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.javaserver-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.javaservices/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (1)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (1)
InfinispanIdentityProviderStorageProvider(42-447)
🔇 Additional comments (11)
server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java (1)
252-256: LGTM - Login predicate properly extended for organization-aware filtering.The added condition correctly mirrors the logic in
getForLogin()(lines 173-181), ensuring consistent filtering whether applied via search options or in-memory predicate.services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java (2)
74-77: LGTM - Defensive re-check for organization-aware IDP enablement.The
isEnabled()re-check correctly handles the case where the cached IDP is wrapped with organization-aware logic that checks the organization's enabled state at runtime. This ensures stale cache entries don't return disabled organization IDPs.
79-82: LGTM - Consistent defensive filtering for the ALL fetch mode.Same rationale as the
ORG_ONLYpath. The comment clearly explains the wrapper consideration.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (2)
369-415: Test setup and initial cache verification look correct.The test comprehensively validates:
- Initial empty cache state for all FetchModes
- Cache population with correct IDP counts (5 realm-only enabled, 5 org-linked enabled, 10 total for ALL mode)
466-498: Good coverage of login-availability change triggering cache invalidation.Setting
hideOnLogin=falseon an IDP correctly triggers cache invalidation and the subsequent re-query shows the updated count (6 realm-only IDPs).model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (6)
77-79: LGTM - Cache key method follows established patterns.The
cacheKeyForLoginmethod is correctly made public static for test access, and the key format is consistent with other cache key methods in this class.
213-255: LGTM - Login caching implementation follows established patterns.The implementation correctly:
- Checks for invalidation before using cache
- Uses organizationId (or empty string) as a sub-key for different org contexts
- Falls back to delegate on cache miss or stale cached IDP reference
- Wraps returned models with
createOrganizationAwareIdentityProviderModelNote: The use of
HashSetmeans ordering is not preserved, but callers likeOrganizationAwareIdentityProviderBeanapply their own sorting (IDP_COMPARATOR_INSTANCE), so this is acceptable.
382-418: LGTM - Well-designed selective cache invalidation.The invalidation logic is correctly optimized:
registerIDPLoginInvalidation: Only invalidates if the IDP actually qualifies for login, avoiding unnecessary invalidation for non-login IDPsregisterIDPLoginInvalidationOnUpdate: Smart logic that preserves caches when:
- A non-login IDP remains non-login after update
- A login IDP remains login with the same organization link
This minimizes cache churn while ensuring correctness for all state transitions (enabled/disabled, org link changes, visibility changes).
98-113: LGTM - Correct invalidation handling on remove.The change to fetch
storedIdpfrom the delegate (line 100) before cache checks is necessary to properly determine if login caches need invalidation. The trade-off of an additional delegate call on remove is acceptable for ensuring cache consistency.
82-86: LGTM - Login cache invalidation properly integrated into create flow.
88-95: LGTM - Update correctly uses original vs updated state for invalidation decision.
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); |
There was a problem hiding this comment.
Bug: Cleanup registrations use hardcoded "alias" instead of actual alias.
The cleanup lambda references "alias" instead of the actual IDP alias (e.g., "idp-alias-" + i). This will fail to clean up the created IDPs properly since no IDP with alias "alias" exists.
🐛 Proposed fix
- testRealm().identityProviders().create(idpRep).close();
- getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove);
+ String alias = "idp-alias-" + i;
+ testRealm().identityProviders().create(idpRep).close();
+ getCleanup().addCleanup(testRealm().identityProviders().get(alias)::remove);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testRealm().identityProviders().create(idpRep).close(); | |
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); | |
| String alias = "idp-alias-" + i; | |
| testRealm().identityProviders().create(idpRep).close(); | |
| getCleanup().addCleanup(testRealm().identityProviders().get(alias)::remove); |
🤖 Prompt for AI Agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java
around lines 380 - 381, The cleanup registration uses a hardcoded "alias" string
so removals fail; change the cleanup to reference the actual identity provider
alias from the created representation (e.g., use idpRep.getAlias() or capture a
final String alias = idpRep.getAlias()) when calling get(...) in the lambda
added via getCleanup().addCleanup(...) so the removal targets the real provider
created by testRealm().identityProviders().create(idpRep).close().
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); |
There was a problem hiding this comment.
Same cleanup bug - hardcoded "alias" instead of actual alias.
🐛 Proposed fix
- testRealm().identityProviders().create(idpRep).close();
- getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove);
+ testRealm().identityProviders().create(idpRep).close();
+ getCleanup().addCleanup(testRealm().identityProviders().get("idp-alias-20")::remove);🤖 Prompt for AI Agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java
around lines 424 - 425, The cleanup currently uses a hardcoded "alias" when
registering removal: replace the literal "alias" with the actual identity
provider alias used when creating the provider (e.g., use idpRep.getAlias() or
the local alias variable) so getCleanup().addCleanup(...) calls
testRealm().identityProviders().get(actualAlias)::remove; ensure the same alias
value passed to testRealm().identityProviders().create(idpRep) is used for the
subsequent get(...) call.
2 participants
Test 2
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#2