Optimize composite realm role mappings - #51565
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes effective realm-role resolution by computing deep mappings once.
Changes:
- Uses
RoleUtils.getDeepRoleMappings(). - Filters mappings to realm roles.
- Adds user and group-inheritance regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
RoleMapperResource.java |
Optimizes effective realm-role lookup. |
CompositeRealmRoleMappingsTest.java |
Adds endpoint regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return RoleUtils.getDeepRoleMappings(roleMapper).stream() | ||
| .filter(r -> RoleUtils.isRealmRole(r, realm)) |
There was a problem hiding this comment.
Good catch — confirmed GroupAdapter.hasRole() walks the parent chain but RoleUtils.getDeepRoleMappings() doesn't for GroupModel. Fixed by walking the parent chain explicitly in getCompositeRealmRoleMappings() when roleMapper is a GroupModel, then expanding composites via RoleUtils.expandCompositeRoles(). Pushed.
| try (Response r = realm.users().create(UserBuilder.create().username("USER_4").build())) { | ||
| user4Id = ApiUtil.getCreatedId(r); | ||
| } |
There was a problem hiding this comment.
Right, missed that fresh users get default-roles-{realm} (which composites in offline_access/uma_authorization). Added cleanup in @testsetup to strip it from all test users before assigning the roles under test, so the exact-size assertions hold. Pushed.
| @Test | ||
| public void testNoClientRoleLeaksIntoRealmResults() { |
There was a problem hiding this comment.
Fair point, the test wasn't actually exercising the filter. Added a real client + client role assigned to USER_1 in the fixture so testNoClientRoleLeaksIntoRealmResults() now fails if the realm-role filter is removed. Pushed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java:223
- The new group-inheritance test invokes the user endpoint, so
roleMapperis aUserModeland thisGroupModelbranch is never exercised. Add coverage throughgroups().group(childGroupId).roles().realmLevel().listEffective()to verify that a child group's result still includes roles inherited from its parent.
if (roleMapper instanceof GroupModel group) {
// GroupModel.hasRole() (the semantics this endpoint previously relied on)
// walks the parent-group chain, but RoleUtils.getDeepRoleMappings() only
// expands a group's own direct mappings. Walk the chain explicitly here
// to preserve parent-group role inheritance for the group endpoint.
tests/base/src/test/java/org/keycloak/tests/admin/user/CompositeRealmRoleMappingsTest.java:63
- These functional assertions would also pass with the previous per-realm-role
hasRole()implementation, so they do not actually guard the #51531 scalability regression. Add a query/invocation-count test proving that adding unrelated realm roles does not increase endpoint work (for example, by asserting this path does not callrealm.getRolesStream()orroleMapper.hasRole()).
* Regression coverage for #51531: the realm variant previously iterated
* realm.getRolesStream().filter(roleMapper::hasRole), which is O(C x M x D)
* and was especially expensive for group-inherited roles. The fix mirrors
* the client-role fix from #47157 by using RoleUtils.getDeepRoleMappings().
Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
…est fixtures per review Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
59f0e81 to
d1cd163
Compare
RoleMapperResource#getCompositeRealmRoleMappings()currently iterates over all realm roles and callsroleMapper.hasRole()for each role.This can become expensive when a user has a large number of effective role mappings, particularly when roles are inherited through groups or composite roles.
The client-role variant already uses
RoleUtils.getDeepRoleMappings()to resolve effective mappings. This PR applies the same approach to the realm-role variant and filters the resolved mappings to realm roles.Changes
roleMapper.hasRole()lookup withRoleUtils.getDeepRoleMappings().RoleUtils.isRealmRole().Closes #51531