Add Client resource type and its scopes to authorization schema and e… - #194
Add Client resource type and its scopes to authorization schema and e…#194claudiacodacy wants to merge 1 commit into
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While the PR successfully defines the 'Clients' resource type and its associated scopes within the V2 schema, several critical implementation flaws will prevent the feature from functioning correctly and may cause regressions in existing workflows.
Most significantly, the current implementation will cause a runtime crash (500 error) whenever a client is deleted because an event listener calls an unimplemented method in ClientPermissionsV2. Additionally, logic bugs in how resources are created and looked up (owner ID mismatch and missing type assignments) mean that fine-grained permissions will likely fail to resolve correctly in practice.
Performance is also a concern for larger environments, as the evaluator iterates through all resources of a type rather than using indexed lookups. These issues should be resolved before merging to ensure system stability and feature parity with the V1 implementation.
About this PR
- Multiple methods in ClientPermissionsV2 throw UnsupportedOperationException. This indicates that several management and exchange features available in V1 are not yet supported, which may lead to unexpected failures in the admin console or API.
- The V2 'Clients' resource type currently omits the 'token-exchange' scope which is present in V1. If this is intentional for the current phase, it should be documented as a known gap.
1 comment outside of the diff
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
line 104🔴 HIGH RISK
The created resource must be assigned theresourceTypeto enable type-based lookups and correctly associate it with the schema. Currently,resourceStore.findByTypewill return empty results because the type field remains null.
Test suggestions
- Verify managing a specific client with a granted instance-level permission while denying access to others.
- Verify configuring a specific client (e.g., managing secrets) with the 'configure' scope.
- Verify managing all clients in the realm using a resource-type level permission.
- Verify viewing all clients using the 'view' scope at the resource-type level.
- Verify mapping roles and composite roles for a client using specific functional scopes.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| 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.
🔴 HIGH RISK
Resource lookup will fail due to owner ID mismatch. This method uses server.getId() (UUID), but AdminPermissionsSchema.getOrCreateResource creates resources using resourceServer.getClientId() (String). The identifiers must match for findByName to work.
| } | ||
|
|
||
| @Override | ||
| public void setPermissionsEnabled(ClientModel client, boolean enable) { |
There was a problem hiding this comment.
🔴 HIGH RISK
This method throws UnsupportedOperationException, yet it is called by the AdminPermissions listener during client removal. This will cause client deletion to crash with a 500 error when the V2 feature flag is active. Implement at least a no-op method to prevent this regression.
|
|
||
| @Override | ||
| public boolean canExchangeTo(ClientModel authorizedClient, ClientModel to, AccessToken token) { | ||
| throw new UnsupportedOperationException("Not supported in V2"); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Token exchange functionality is not supported in this V2 implementation and will throw an UnsupportedOperationException, diverging from V1 capabilities.
| } | ||
|
|
||
| Collection<Permission> permissions = root.evaluatePermission(new ResourcePermission(resource, resource.getScopes(), server), server); | ||
| List<String> expectedScopes = Arrays.asList(scope); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The loop for scope validation is duplicated across several private methods. Consolidate this into a shared method (e.g., hasGrantedPermission) and replace Arrays.asList(scope).contains(s) with a direct string comparison to avoid unnecessary allocations.
| return false; | ||
| } | ||
|
|
||
| private EvaluationContext getEvaluationContext(ClientModel authorizedClient, AccessToken token) { |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: This method is unused and should be removed to maintain code cleanliness.
|
|
||
| public class ClientPermissionsV2 extends ClientPermissions { | ||
|
|
||
| private static final Logger logger = Logger.getLogger(ClientPermissionsV2.class); |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Unused private static final Logger logger.
2 participants
…valuation implementation for ClientsPermissionsV2
Closes #35564