Add Client resource type and scopes to authorization schema #5
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR extends Keycloak's fine-grained authorization permissions (FGAP) system to support client-level permissions alongside the existing user-level permissions. The changes implement a new V2 architecture for client authorization management.
The core addition is the CLIENTS resource type in AdminPermissionsSchema with six permission scopes: CONFIGURE, MANAGE, MAP_ROLES, MAP_ROLES_CLIENT_SCOPE, MAP_ROLES_COMPOSITE, and VIEW. This enables granular control over client management operations - for example, CONFIGURE allows client configuration and secret management, while MAP_ROLES_CLIENT_SCOPE specifically controls client scope role mapping.
A new ClientPermissionsV2 class extends the existing ClientPermissions to integrate with the schema-driven authorization approach. Unlike V1's resource-specific model, V2 uses a centralized permissions evaluation that can check both client-specific permissions and fall back to resource type-level permissions (like "all-clients" permissions).
The MgmtPermissionsV2 class is updated to return the new ClientPermissionsV2 instance, maintaining the lazy initialization pattern. The existing AdminPermissions event listener is wrapped with a feature flag check to ensure permission cleanup only occurs when fine-grained authorization is enabled.
Extensive test coverage validates the new functionality through comprehensive test cases that verify proper permission enforcement for individual clients and realm-wide client operations. The test framework itself was refactored to consolidate permission creation utilities into AbstractPermissionTest, eliminating code duplication and standardizing the API to require explicit client parameters.
This implementation follows the established architectural pattern of the existing USERS resource type, ensuring consistency in the authorization schema design while enabling the same level of granular control for client management that was previously available only for user management.
Confidence score: 3/5
• This PR introduces significant new authorization functionality but has implementation gaps that could cause runtime issues
• The main concern is incomplete client resource name resolution in AdminPermissionsSchema.getResourceName() which handles users but not clients, potentially causing null pointer exceptions or display issues
• Files that need more attention: server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (missing client resource name handling), services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (unused getEvaluationContext method and multiple UnsupportedOperationException methods that may indicate incomplete migration)
10 files reviewed, 6 comments
| private EvaluationContext getEvaluationContext(ClientModel authorizedClient, AccessToken token) { | ||
| ClientModelIdentity identity = new ClientModelIdentity(session, authorizedClient, token); | ||
| return new DefaultEvaluationContext(identity, session) { | ||
| @Override | ||
| public Map<String, Collection<String>> getBaseAttributes() { | ||
| Map<String, Collection<String>> attributes = super.getBaseAttributes(); | ||
| attributes.put("kc.client.id", List.of(authorizedClient.getClientId())); | ||
| return attributes; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
style: The getEvaluationContext method is defined but never used in the current implementation
| ResourceServer server = root.realmResourceServer(); | ||
| if (server == null) return false; | ||
|
|
||
| Resource resource = resourceStore.findByName(server, client.getId(), server.getId()); |
There was a problem hiding this comment.
syntax: Extra space after assignment operator: should be Resource resource = resourceStore.findByName(...)
| /** | ||
| * Returns {@code true} if {@link #canView()} returns {@code true}. | ||
| */ | ||
| void requireView(); |
There was a problem hiding this comment.
syntax: Method signature indicates it should return boolean but documentation says it returns true
| void requireView(); | |
| /** | |
| * Throws ForbiddenException if {@link #canView()} returns {@code false}. | |
| */ | |
| void requireView(); |
| /** | ||
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | ||
| */ | ||
| void requireViewClientScopes(); |
There was a problem hiding this comment.
syntax: Method signature indicates it should return void but documentation says it returns true
| void requireViewClientScopes(); | |
| /** | |
| * Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}. | |
| */ | |
| void requireViewClientScopes(); |
| assertThat(realmAdminClient.realm(realm.getName()).clients().get(myclient.getId()).getProtocolMappers().getMappers(), empty()); | ||
|
|
||
| // can't create a new client | ||
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { |
There was a problem hiding this comment.
logic: Passing null to create() method may not test the intended scenario. Consider using a proper ClientRepresentation object to test forbidden creation.
| //create all-users permission for "myadmin" (so that myadmin can impersonate all users in the realm) | ||
| UserPolicyRepresentation policy = createUserPolicy("Only My Admin User Policy", realm.admin().users().search("myadmin").get(0).getId()); | ||
| createAllUserPermission(policy, Set.of(IMPERSONATE)); | ||
| UserPolicyRepresentation policy = createUserPolicy(realm, client,"Only My Admin User Policy", realm.admin().users().search("myadmin").get(0).getId()); |
There was a problem hiding this comment.
syntax: missing space after comma in method parameters
| UserPolicyRepresentation policy = createUserPolicy(realm, client,"Only My Admin User Policy", realm.admin().users().search("myadmin").get(0).getId()); | |
| UserPolicyRepresentation policy = createUserPolicy(realm, client, "Only My Admin User Policy", realm.admin().users().search("myadmin").get(0).getId()); |
There was a problem hiding this comment.
Greptile Summary
This PR introduces the Clients resource type and its scopes into the fine‑grained admin authorization (FGAP v2) model and wires the admin layer to use it.
-
Authorization schema update
server-spi-private/.../AdminPermissionsSchema.javanow models two first‑class resource types: Users and Clients. It defines Clients scopes (configure, manage, map-roles, map-roles-client-scope, map-roles-composite, view) alongside existing Users scopes and seeds the admin-permissions client with all schema scopes and the "all-resources" entries for each type. It also resolves resource names by id for both users and clients during resource creation.
-
Admin management wiring
services/.../AdminPermissions.javaroutes evaluator/management toMgmtPermissionsV2when theADMIN_FINE_GRAINED_AUTHZ_V2profile feature is enabled. This makes all admin permission checks leverage the v2 schema where applicable.services/.../permissions/MgmtPermissionsV2.javanow exposes client permissions under v2 via a cachedClientPermissionsV2and returnsrealm.getAdminPermissionsClient()for the realm permissions client, aligning v2 management with the admin-permissions client.
-
V2 client permission evaluation
services/.../permissions/ClientPermissionsV2.javais a new v2 implementation that evaluates manage/configure/view and role‑mapping semantics for clients through resource‑based checks. It consults per‑client resources (named by client id) and, when absent, the type‑level "all-clients" resource, and uses the v2 schema scopes for evaluation. Legacy per‑client toggles and policy accessors are intentionally not supported in v2 and are replaced by resource‑type/scoped evaluation.services/.../permissions/ClientPermissionEvaluator.javaupdates Javadoc to clarify how legacy admin roles map to v2 permission checks usingorg.keycloak.authorization.AdminPermissionsSchemawhile keeping the interface signatures unchanged.
-
Test refactors and coverage
- Shared test helpers were centralized and made client‑aware:
tests/.../AbstractPermissionTest.javanow provides static helpers that takeManagedClient, create user/client policies, and build scope permissions for specific resource types, with automatic cleanup. - Existing tests were adapted to operate in the context of a specific client/resource server, ensuring permissions are created and queried against the correct client:
tests/.../PermissionRESTTest.javapasses the client when creating permissions and maintains the expected error semantics around missing or invalid resourceType/scopes.tests/.../UserResourceTypeEvaluationTest.javaandtests/.../UserResourceTypePermissionTest.javaswitch to the shared helpers, explicitly target the Users resource type under v2, and keep coverage for per‑user vs. all‑users permissions, transitive implications, and CRUD/query flows.tests/.../PermissionClientTest.javaadds comprehensive scenarios for the new Clients resource type, validating manage/configure/view and role‑mapping scopes on a single client and at the realm level, plus supported/unsupported policy types in the v2 model.
- Shared test helpers were centralized and made client‑aware:
PR Description Notes:
- The PR body is very terse ("Test 5"). Consider describing the intent (adding Clients resource type and scopes to FGAP v2), the migration notes (legacy per‑client toggles replaced by resource‑based policies), and a brief summary of admin wiring and test changes.
Confidence score: 3/5
- This PR requires careful review due to potential breaking changes
- Score lowered due to feature-flag gating differences and the shift to v2-only operations for clients
- Pay close attention to: services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java; services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java; tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java
10 files reviewed, 1 comment
| if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) { | ||
| if (event instanceof RoleContainerModel.RoleRemovedEvent) { | ||
| RoleContainerModel.RoleRemovedEvent cast = (RoleContainerModel.RoleRemovedEvent) event; | ||
| RoleModel role = cast.getRole(); | ||
| RealmModel realm; | ||
| if (role.getContainer() instanceof ClientModel) { | ||
| realm = ((ClientModel) role.getContainer()).getRealm(); |
There was a problem hiding this comment.
logic: Flag mismatch: using ADMIN_FINE_GRAINED_AUTHZ here while other methods use *_V2; if only V2 is enabled this listener will not run and permissions won’t be cleaned up
2 participants
Test 5