Add caching support for IdentityProviderStorageProvider.getForLogin operations#9
Open
akshayutture-augment wants to merge 1 commit into
Open
Conversation
Closes #32573 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
Author
|
@coderabbitai review |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces login-aware caching for identity providers in the Infinispan storage provider, adding cache key generation and retrieval methods with organization-level broker public flag validation. Enabled provider filtering is added to the login bean, and a new test validates cache behavior across multiple scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java(6 hunks)server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java(1 hunks)services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java(1 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java(3 hunks)
🔇 Additional comments (4)
services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java (1)
64-82: ExplicitisEnabledfilter aftergetForLoginis correct with wrapped IDPsRe-checking
idp.isEnabled()on theORG_ONLYandALLlogin streams is appropriate givencreateOrganizationAwareIdentityProviderModelcan dynamically report disabled for org-bound providers; this avoids exposing IDPs that are no longer effectively enabled while still leveraging the shared login cache.server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java (1)
252-255: Org-aware guard ingetLoginPredicatecorrectly restricts login IDPsThe added condition
idp.getOrganizationId() == null || BROKER_PUBLIC == truealigns the in-memory login predicate with the search options used ingetForLogin, ensuring only public org-linked IDPs are considered login-eligible while preserving realm-level behavior. This also gives the Infinispan layer a precise signal for when login caches must be invalidated.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (1)
20-37: New imports correctly tie test to login cache APIStatic import of
cacheKeyForLoginand theFetchModeimport keep the login-cache assertions readable and consistent with existing cache key usage in this test class.model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (1)
19-22: Login-aware caching and invalidation logic are consistent with SPI semanticsThe new login cache key (
IDP_LOGIN_SUFFIX+cacheKeyForLogin), thegetForLoginoverride, and theregisterIDPLoginInvalidation*helpers integrate cleanly with existingIdentityProviderListQuerypatterns:
getForLogincaches internal IDs per(realm, FetchMode, organizationId)and re-hydrates throughsession.identityProviders().getById, so the organization-awareisEnabledoverride is honoured at lookup time.registerIDPLoginInvalidationandregisterIDPLoginInvalidationOnUpdateboth rely onLoginFilter.getLoginPredicate, keeping the definition of “login-eligible” aligned between filtering and invalidation and avoiding unnecessary cache churn when updates don’t affect login availability or org linkage.Overall, the design matches the behavior asserted in
OrganizationCacheTest.testCacheIDPForLogin.Also applies to: 40-48, 77-80, 213-255, 382-418
Comment on lines
97
to
113
| @Override | ||
| public boolean remove(String alias) { | ||
| String cacheKey = cacheKeyIdpAlias(getRealm(), alias); | ||
| IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias); | ||
| if (isInvalid(cacheKey)) { | ||
| //lookup idp by alias in cache to be able to invalidate its internalId | ||
| registerIDPInvalidation(idpDelegate.getByAlias(alias)); | ||
| registerIDPInvalidation(storedIdp); | ||
| } else { | ||
| CachedIdentityProvider cached = realmCache.getCache().get(cacheKey, CachedIdentityProvider.class); | ||
| if (cached != null) { | ||
| registerIDPInvalidation(cached.getIdentityProvider()); | ||
| } | ||
| } | ||
| registerCountInvalidation(); | ||
| registerIDPLoginInvalidation(storedIdp); | ||
| return idpDelegate.remove(alias); | ||
| } |
There was a problem hiding this comment.
Guard against possible NPE when removing a non-existent IDP with an invalidated alias key
In remove(String alias), if storedIdp = idpDelegate.getByAlias(alias) returns null while isInvalid(cacheKey) is true, registerIDPInvalidation(storedIdp) will dereference idp.getInternalId() and throw an NPE. Although this situation is unlikely, it’s a correctness risk.
Consider guarding the call:
String cacheKey = cacheKeyIdpAlias(getRealm(), alias);
- IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias);
+ IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias);
if (isInvalid(cacheKey)) {
- //lookup idp by alias in cache to be able to invalidate its internalId
- registerIDPInvalidation(storedIdp);
+ // lookup idp by alias in cache to be able to invalidate its internalId
+ if (storedIdp != null) {
+ registerIDPInvalidation(storedIdp);
+ }
} else {
CachedIdentityProvider cached = realmCache.getCache().get(cacheKey, CachedIdentityProvider.class);
if (cached != null) {
registerIDPInvalidation(cached.getIdentityProvider());
}
}
registerCountInvalidation();
- registerIDPLoginInvalidation(storedIdp);
+ if (storedIdp != null) {
+ registerIDPLoginInvalidation(storedIdp);
+ }
return idpDelegate.remove(alias);This keeps behavior unchanged when the IDP exists but avoids an NPE when it doesn’t.
🤖 Prompt for AI Agents
In
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java
around lines 97–113, guard against a possible NPE by checking for null before
calling invalidation helpers: only call registerIDPInvalidation(storedIdp) when
storedIdp != null (i.e., wrap that call in a null-check) and likewise ensure
registerIDPLoginInvalidation(storedIdp) is only invoked if storedIdp is
non-null; leave other logic unchanged.
Comment on lines
+369
to
+535
| @Test | ||
| public void testCacheIDPForLogin() { | ||
| // create 20 providers, and associate 10 of them with an organization. | ||
| for (int i = 0; i < 20; i++) { | ||
| IdentityProviderRepresentation idpRep = new IdentityProviderRepresentation(); | ||
| idpRep.setAlias("idp-alias-" + i); | ||
| idpRep.setEnabled((i % 2) == 0); // half of the IDPs will be disabled and won't qualify for login. | ||
| idpRep.setDisplayName("Broker " + i); | ||
| idpRep.setProviderId("keycloak-oidc"); | ||
| if (i >= 10) | ||
| idpRep.getConfig().put(OrganizationModel.BROKER_PUBLIC, Boolean.TRUE.toString()); | ||
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); | ||
| } | ||
|
|
||
| String orgaId = testRealm().organizations().getAll().get(0).getId(); | ||
| for (int i = 10; i < 20; i++) { | ||
| testRealm().organizations().get(orgaId).identityProviders().addIdentityProvider("idp-alias-" + i); | ||
| } | ||
|
|
||
| getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { | ||
| RealmModel realm = session.getContext().getRealm(); | ||
| RealmCacheSession realmCache = (RealmCacheSession) session.getProvider(CacheRealmProvider.class); | ||
|
|
||
| // check all caches for login don't exist yet | ||
| for (FetchMode fetchMode : IdentityProviderStorageProvider.FetchMode.values()) { | ||
| String cachedKey = cacheKeyForLogin(realm, fetchMode); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cachedKey, IdentityProviderListQuery.class); | ||
| assertNull(identityProviderListQuery); | ||
| } | ||
|
|
||
| // perform some login IDP searches and ensure they are cached. | ||
| session.identityProviders().getForLogin(FetchMode.REALM_ONLY, null); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.REALM_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(5, identityProviderListQuery.getIDPs("").size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ORG_ONLY, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ORG_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(5, identityProviderListQuery.getIDPs(orgaId).size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ALL, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ALL), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(10, identityProviderListQuery.getIDPs(orgaId).size()); | ||
| }); | ||
|
|
||
| // 1- add/remove IDPs that are not available for login - none of these operations should invalidate the login caches. | ||
| IdentityProviderRepresentation idpRep = new IdentityProviderRepresentation(); | ||
| idpRep.setAlias("idp-alias-" + 20); | ||
| idpRep.setEnabled(true); | ||
| idpRep.setHideOnLogin(true); // this will make the new IDP not available for login. | ||
| idpRep.setDisplayName("Broker " + 20); | ||
| idpRep.setProviderId("keycloak-oidc"); | ||
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); | ||
|
|
||
| // remove one IDP that was not available for login. | ||
| testRealm().identityProviders().get("idp-alias-1").remove(); | ||
|
|
||
| getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { | ||
| RealmModel realm = session.getContext().getRealm(); | ||
| RealmCacheSession realmCache = (RealmCacheSession) session.getProvider(CacheRealmProvider.class); | ||
|
|
||
| // check all caches for login are still there. | ||
| for (FetchMode fetchMode : IdentityProviderStorageProvider.FetchMode.values()) { | ||
| String cachedKey = cacheKeyForLogin(realm, fetchMode); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cachedKey, IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| } | ||
| }); | ||
|
|
||
| // 2- update a couple of idps (one not available for login, one available), but don't change their login-availability status | ||
| // none of these operations should invalidate the login caches. | ||
| idpRep = testRealm().identityProviders().get("idp-alias-20").toRepresentation(); | ||
| idpRep.getConfig().put("somekey", "somevalue"); | ||
| idpRep.setTrustEmail(true); | ||
| testRealm().identityProviders().get("idp-alias-20").update(idpRep); // should still be unavailable for login | ||
|
|
||
| idpRep = testRealm().identityProviders().get("idp-alias-0").toRepresentation(); | ||
| idpRep.getConfig().put("somekey", "somevalue"); | ||
| idpRep.setTrustEmail(true); | ||
| testRealm().identityProviders().get("idp-alias-0").update(idpRep); // should still be available for login | ||
|
|
||
| getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { | ||
| RealmModel realm = session.getContext().getRealm(); | ||
| RealmCacheSession realmCache = (RealmCacheSession) session.getProvider(CacheRealmProvider.class); | ||
|
|
||
| // check all caches for login are still there. | ||
| for (FetchMode fetchMode : IdentityProviderStorageProvider.FetchMode.values()) { | ||
| String cachedKey = cacheKeyForLogin(realm, fetchMode); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cachedKey, IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| } | ||
| }); | ||
|
|
||
| // 3- update an IDP, changing the availability for login - this should invalidate the caches. | ||
| idpRep = testRealm().identityProviders().get("idp-alias-20").toRepresentation(); | ||
| idpRep.setHideOnLogin(false); | ||
| testRealm().identityProviders().get("idp-alias-20").update(idpRep); | ||
|
|
||
| getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { | ||
| RealmModel realm = session.getContext().getRealm(); | ||
| RealmCacheSession realmCache = (RealmCacheSession) session.getProvider(CacheRealmProvider.class); | ||
|
|
||
| // check all caches have been cleared. | ||
| for (FetchMode fetchMode : IdentityProviderStorageProvider.FetchMode.values()) { | ||
| String cachedKey = cacheKeyForLogin(realm, fetchMode); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cachedKey, IdentityProviderListQuery.class); | ||
| assertNull(identityProviderListQuery); | ||
| } | ||
|
|
||
| // re-do searches to populate the caches again and check the updated results. | ||
| session.identityProviders().getForLogin(FetchMode.REALM_ONLY, null); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.REALM_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(6, identityProviderListQuery.getIDPs("").size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ORG_ONLY, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ORG_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(5, identityProviderListQuery.getIDPs(orgaId).size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ALL, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ALL), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(11, identityProviderListQuery.getIDPs(orgaId).size()); | ||
|
|
||
| }); | ||
|
|
||
| // 4- finally, change one of the realm-level login IDPs, linking it to an org - although it still qualifies for login, it is now | ||
| // linked to an org, which should invalidate all login caches. | ||
| idpRep = testRealm().identityProviders().get("idp-alias-20").toRepresentation(); | ||
| idpRep.getConfig().put(OrganizationModel.BROKER_PUBLIC, Boolean.TRUE.toString()); | ||
| testRealm().identityProviders().get("idp-alias-20").update(idpRep); | ||
| testRealm().organizations().get(orgaId).identityProviders().addIdentityProvider("idp-alias-20"); | ||
|
|
||
| getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { | ||
| RealmModel realm = session.getContext().getRealm(); | ||
| RealmCacheSession realmCache = (RealmCacheSession) session.getProvider(CacheRealmProvider.class); | ||
|
|
||
| // check all caches have been cleared. | ||
| for (FetchMode fetchMode : IdentityProviderStorageProvider.FetchMode.values()) { | ||
| String cachedKey = cacheKeyForLogin(realm, fetchMode); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cachedKey, IdentityProviderListQuery.class); | ||
| assertNull(identityProviderListQuery); | ||
| } | ||
|
|
||
| // re-do searches to populate the caches again and check the updated results. | ||
| session.identityProviders().getForLogin(FetchMode.REALM_ONLY, null); | ||
| IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.REALM_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(5, identityProviderListQuery.getIDPs("").size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ORG_ONLY, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ORG_ONLY), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(6, identityProviderListQuery.getIDPs(orgaId).size()); | ||
|
|
||
| session.identityProviders().getForLogin(FetchMode.ALL, orgaId); | ||
| identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.ALL), IdentityProviderListQuery.class); | ||
| assertNotNull(identityProviderListQuery); | ||
| assertEquals(11, identityProviderListQuery.getIDPs(orgaId).size()); | ||
|
|
||
| }); | ||
| } |
There was a problem hiding this comment.
Fix cleanup alias and consider making login-count assertions less brittle
The test flow and assertions around cache presence and sizes look correct for the modeled scenarios, but there are two minor issues:
-
Cleanup uses literal
"alias"
getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove);will not remove the providers created as"idp-alias-" + i. This matches an existing pattern intestCacheIDPByOrgbut is almost certainly unintended and leaves extra IDPs behind.A minimal fix inside this test would be:
- testRealm().identityProviders().create(idpRep).close(); - getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); + testRealm().identityProviders().create(idpRep).close(); + final String alias = idpRep.getAlias(); + getCleanup().addCleanup(testRealm().identityProviders().get(alias)::remove);
(You may want to apply the same pattern to
testCacheIDPByOrgfor consistency.) -
Hard-coded expected counts assume a fixed realm baseline
The expectations5/5/10, then6/5/11, then5/6/11are correct given only the IDPs created in this test are login-eligible, but they will fail if the test realm configuration or other tests introduce additional login-capable IDPs. If future changes make the base realm less minimal, consider deriving the expected counts from the created representations (e.g., via local calculations) rather than relying on a fixed global total.
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java
around lines 369-535, the cleanup registration incorrectly uses the literal
"alias" and the test uses hard-coded expected IDP counts; fix by registering
cleanup with the actual alias created (e.g., capture the alias string per
iteration and call
getCleanup().addCleanup(testRealm().identityProviders().get(alias)::remove)) so
each created provider is removed, and replace brittle hard-coded assertions with
computed expected counts derived from the representations you created in the
test (count login-eligible providers for realm/organization based on
enabled/hideOnLogin/BROKER_PUBLIC flags) so assertions adapt if the realm
baseline changes; also apply the same cleanup fix to testCacheIDPByOrg for
consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 2
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#2