Filter stale role IDs in Infinispan client adapters - #51616
Filter stale role IDs in Infinispan client adapters#51616PRAHLAD09-dev wants to merge 2 commits into
Conversation
Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
There was a problem hiding this comment.
Pull request overview
Filters unresolved cached role IDs to prevent null role mappings and composite-role expansion failures.
Changes:
- Adds null filtering to client and client-scope Infinispan adapters.
- Aligns behavior with JPA adapters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ClientScopeAdapter.java |
Filters unresolved client-scope role mappings. |
ClientAdapter.java |
Filters unresolved client role mappings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return cached.getScope().stream() | ||
| .map(id -> cacheSession.getRoleById(cachedRealm, id)); | ||
| .map(id -> cacheSession.getRoleById(cachedRealm, id)) | ||
| .filter(Objects::nonNull); |
There was a problem hiding this comment.
Thanks for the suggestion. I checked the existing model/cache test infrastructure and confirmed that the existing tests cover the normal cache invalidation path.
For this specific regression, reproducing an unknown role ID in CachedClient deterministically requires bypassing the normal cache invalidation path. The available test infrastructure does not provide a supported API for injecting such a stale cached role ID, and Mockito is not an existing dependency of this module.
I therefore kept the fix limited to the null handling, matching the existing JPA implementation with Objects::nonNull, rather than introducing a new mocking dependency or relying on internal cache-state manipulation.
There was a problem hiding this comment.
Update: @ssilvert provided a concrete Mockito-based approach that instantiates ClientAdapter/ClientScopeAdapter directly with mocked RealmCacheSession/CachedClient/CachedClientScope collaborators — much simpler than I'd assumed, and it doesn't require touching internal cache state. Added junit-jupiter-api, junit-jupiter-engine, and mockito-core as test dependencies to model/infinispan (following the same coordinates already used in operator and ssf/transmitter), and added ClientAdapterTest/ClientScopeAdapterTest covering exactly this scenario. Both pass locally (Tests run: 2, Failures: 0, Errors: 0). Pushed in the latest commit.
| return cached.getScope().stream() | ||
| .map(id -> cacheSession.getRoleById(cachedRealm, id)); | ||
| .map(id -> cacheSession.getRoleById(cachedRealm, id)) | ||
| .filter(Objects::nonNull); |
There was a problem hiding this comment.
Same applies to CachedClientScope. The existing test infrastructure covers normal cache invalidation, but does not provide a supported way to inject an arbitrary stale role ID into the cached scope mappings.
Adding a mocking dependency or manipulating internal cache state solely for this race-condition scenario would add unnecessary test infrastructure to the module. The production change mirrors the existing JPA implementation by filtering unresolved RoleModel instances with Objects::nonNull.
There was a problem hiding this comment.
Update: @ssilvert provided a concrete Mockito-based approach that instantiates ClientAdapter/ClientScopeAdapter directly with mocked RealmCacheSession/CachedClient/CachedClientScope collaborators — much simpler than I'd assumed, and it doesn't require touching internal cache state. Added junit-jupiter-api, junit-jupiter-engine, and mockito-core as test dependencies to model/infinispan (following the same coordinates already used in operator and ssf/transmitter), and added ClientAdapterTest/ClientScopeAdapterTest covering exactly this scenario. Both pass locally (Tests run: 2, Failures: 0, Errors: 0). Pushed in the latest commit.
|
This is a good, minimal fix and it matches the JPA adapters exactly — I (Claude code) traced the NPE through One thing I'd like to see before merge: a regression test. package org.keycloak.models.cache.infinispan;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import java.util.List;
import java.util.Set;
import org.junit.jupiter.api.Test;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleModel;
import org.keycloak.models.cache.infinispan.entities.CachedClient;
class ClientAdapterTest {
@Test
void getScopeMappingsStreamFiltersUnresolvableRoleIds() {
RealmModel realm = mock(RealmModel.class);
RealmCacheSession cacheSession = mock(RealmCacheSession.class);
CachedClient cached = mock(CachedClient.class);
RoleModel liveRole = mock(RoleModel.class);
when(cached.getScope()).thenReturn(Set.of("stale-role-id", "live-role-id"));
// simulates a role deleted on another node whose cache invalidation
// hasn't reached this one yet
when(cacheSession.getRoleById(eq(realm), eq("stale-role-id"))).thenReturn(null);
when(cacheSession.getRoleById(eq(realm), eq("live-role-id"))).thenReturn(liveRole);
ClientAdapter adapter = new ClientAdapter(realm, cached, cacheSession);
List<RoleModel> result = adapter.getScopeMappingsStream().toList();
assertEquals(List.of(liveRole), result,
"a scope mapping pointing at an unresolvable role must be silently dropped, not surfaced as null");
}
}The equivalent test for Two things worth double-checking when you write it:
|
Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
model/infinispan/pom.xml:114
- Adding the Jupiter engine makes Surefire select the JUnit Platform provider, but this module's existing tests use
org.junit.Testand no Vintage engine is present, so those JUnit 4 tests will no longer run. Convert the two new tests to JUnit 4 and remove the Jupiter dependencies, or includejunit-vintage-engineto preserve the existing suite.
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
model/infinispan/pom.xml:103
- This repeats the identical
junit:junitdependency already declared immediately above at lines 93–97, causing Maven's duplicate-dependency model warning. Keep a single declaration.
This issue also appears on line 110 of the same file.
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
|
Thanks for the detailed example — that's exactly what I needed. Added both tests following your pattern (ClientAdapterTest + ClientScopeAdapterTest), and added junit-jupiter-api/junit-jupiter-engine/mockito-core as test dependencies to model/infinispan since they weren't there yet. Verified isUpdated() defaults to false on a fresh instance as you noted, so no extra stubbing was needed. Both tests pass locally. Let me know if this addresses your concern or if you'd like anything adjusted. |
ClientAdapterandClientScopeAdapterin the Infinispan cache currently resolve cached role IDs without handling the case where a role can no longer be resolved.This can result in
nullRoleModelentries being returned fromgetScopeMappingsStream(), which can subsequently cause failures when the mappings are processed by composite-role expansion.The corresponding JPA adapters already filter unresolved roles using
Objects::nonNull. This PR applies the same null-filtering behavior to the Infinispan implementations.Changes
ClientAdapter#getScopeMappingsStream().ClientScopeAdapter#getScopeMappingsStream().Verification
git diff --checkpasses.mvnw.cmd -pl model\infinispan -am -DskipTests compilepasses.Fixes #51589