Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The unpaginated by-name endpoint now uses a search overload that adds a database query containing all composite IDs on every request, even when the role cache is warm.
1 open issue
What changed in this PR
Extracts shared role-management logic from the v1 REST resources so the v2 client API can manage roles without depending on v1 resources, supporting #47631.
Changes:
- Introduces a shared
RoleServicefor role operations and permission checks. - Migrates v1 resources and v2 client role reconciliation to the service, keeping admin-event handling with callers.
- Adds regression coverage for role errors, composite permissions, and v2 client events.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/admin/realm/RealmRolesCRUDTest.java |
Tests duplicate names, rename conflicts, and unknown composites. |
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java |
Tests composite-mapping permissions during role creation. |
services/src/main/java/org/keycloak/services/ServiceException.java |
Adds shared service errors with suggested HTTP statuses. |
services/src/main/java/org/keycloak/services/roles/RoleService.java |
Centralizes role operations and authorization checks. |
services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java |
Adds shared helpers and v1 error translation. |
services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java |
Delegates name-based role operations to the service. |
services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java |
Delegates ID-based role operations to the service. |
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AdminEventV2Test.java |
Verifies role changes emit only a client event. |
rest/admin-v2/services/src/main/java/org/keycloak/services/RolesService.java |
Removes the wrapper around v1 role resources. |
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientResourceTypeProvider.java |
Uses the shared service for declarative client roles. |
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
When search is omitted, the roles-by-ID endpoint paginates an unordered HashSet stream, producing arbitrary pages instead of the name-ordered results expected by existing tests.
1 open issue
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
A whitespace-only search now returns all composites when pagination is omitted but remains filtered when pagination is supplied, so the cache optimization changes existing search semantics.
1 open issue
1 resolved since last review
Review effort: Balanced
closes: keycloak#47631 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
🟡 Changes recommended
A nonempty composite-role search without pagination incorrectly returns all visible composites instead of matching roles, breaking the existing search assertion.
1 open issue
1 resolved since last review
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
closes: #47631
Adds a common role service. Since the role classes hold very little state, this is just directly passing the values to the methods. If we want a better way to transfer state, then we could consider adding directly creatable versions of the model classes (like we are already doing for client models) or a service specific representation.