Add Client resource type and its scopes to authorization schema and e… - #1
Open
linxia0415 wants to merge 1 commit into
Open
Add Client resource type and its scopes to authorization schema and e…#1linxia0415 wants to merge 1 commit into
linxia0415 wants to merge 1 commit into
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
📝 WalkthroughWalkthroughThis pull request extends Keycloak's fine-grained authorization system to support client resource management. The changes add a new ChangesClient Authorization Implementation
Test Framework and Authorization Tests
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (1)
262-272: ⚡ Quick winUnused method - consider removing or documenting future intent.
The
getEvaluationContextmethod is not called anywhere in the codebase. SincecanExchangeTothrowsUnsupportedOperationException, this appears to be dead code. Consider either removing it or adding a comment explaining its intended future use.♻️ Suggested cleanup
If this method is not needed for future functionality, remove it:
- 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; - } - }; - } -Alternatively, if planned for future use, add a TODO comment explaining when it will be needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java` around lines 262 - 272, The getEvaluationContext method appears to be dead code (not referenced) while canExchangeTo currently throws UnsupportedOperationException; either remove getEvaluationContext or document its intended future use. Locate the getEvaluationContext method and either delete it to remove unused code or add a concise TODO/Javadoc above it referencing ClientModelIdentity and DefaultEvaluationContext and explaining the expected future scenario (e.g., when canExchangeTo will be implemented) that will require this helper; ensure the comment names getEvaluationContext and the related classes so its purpose and future necessity are clear.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (1)
319-322: 💤 Low valuePassing
nulltoclients().create(null)may assert FORBIDDEN for the wrong reason.A
nullrepresentation can be rejected during request/body handling rather than by the authorization check, masking the intent of this negative test. Consider passing a minimal validClientRepresentationso the 403 unambiguously comes from the missing manage permission.♻️ Suggested change
- // can't create a new client - try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { + // can't create a new client + ClientRepresentation forbiddenClient = new ClientRepresentation(); + forbiddenClient.setClientId("forbiddenClient"); + try (Response response = realmAdminClient.realm(realm.getName()).clients().create(forbiddenClient)) { Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java` around lines 319 - 322, The test passes null to realmAdminClient.realm(realm.getName()).clients().create(null), which can fail during request validation instead of an authorization check; replace the null with a minimal valid ClientRepresentation (e.g., set at least the clientId and enabled fields) so the call reaches the authorization layer and the Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()) unambiguously verifies missing manage permission in PermissionClientTest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java`:
- Around line 77-96: The current event-listener block in AdminPermissions checks
only Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipping cleanup when only
ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled; update the guard to run when either
feature flag is enabled (e.g.,
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) ||
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) so the
existing role/client/group removal branches still call
management(...).roles()/clients()/groups().setPermissionsEnabled(...) and thus
invoke the MgmtPermissionsV2 cleanup path when V2 is active.
---
Nitpick comments:
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java`:
- Around line 262-272: The getEvaluationContext method appears to be dead code
(not referenced) while canExchangeTo currently throws
UnsupportedOperationException; either remove getEvaluationContext or document
its intended future use. Locate the getEvaluationContext method and either
delete it to remove unused code or add a concise TODO/Javadoc above it
referencing ClientModelIdentity and DefaultEvaluationContext and explaining the
expected future scenario (e.g., when canExchangeTo will be implemented) that
will require this helper; ensure the comment names getEvaluationContext and the
related classes so its purpose and future necessity are clear.
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`:
- Around line 319-322: The test passes null to
realmAdminClient.realm(realm.getName()).clients().create(null), which can fail
during request validation instead of an authorization check; replace the null
with a minimal valid ClientRepresentation (e.g., set at least the clientId and
enabled fields) so the call reaches the authorization layer and the
Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus())
unambiguously verifies missing manage permission in PermissionClientTest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc1d7a4f-b734-4555-b4ba-d24fdb9e0f89
📒 Files selected for processing (10)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypePermissionTest.java
Comment on lines
+77
to
96
| 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(); | ||
|
|
||
| } else { | ||
| realm = (RealmModel)role.getContainer(); | ||
| } else { | ||
| realm = (RealmModel) role.getContainer(); | ||
| } | ||
| management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false); | ||
| } else if (event instanceof ClientModel.ClientRemovedEvent) { | ||
| ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent) event; | ||
| management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false); | ||
| } else if (event instanceof GroupModel.GroupRemovedEvent) { | ||
| GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent) event; | ||
| management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false); | ||
| } | ||
| management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false); | ||
| } else if (event instanceof ClientModel.ClientRemovedEvent) { | ||
| ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent)event; | ||
| management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false); | ||
| } else if (event instanceof GroupModel.GroupRemovedEvent) { | ||
| GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent)event; | ||
| management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false); | ||
| } |
There was a problem hiding this comment.
Feature flag check excludes V2, preventing cleanup when only V2 is enabled.
The listener only checks ADMIN_FINE_GRAINED_AUTHZ (V1), but the management() method on lines 66-71 returns MgmtPermissionsV2 when ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled. If V2 is enabled but V1 is disabled, this cleanup logic will not execute, leaving orphaned permission data (policies, resources) in the authorization store when roles, clients, or groups are removed.
Since V1 and V2 appear to be independently enabled features (per context snippet 1, V2 depends on AUTHORIZATION, not V1), the check should gate on either flag.
🔧 Proposed fix to check both feature flags
- if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {
+ if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)
+ || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) {
if (event instanceof RoleContainerModel.RoleRemovedEvent) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java`
around lines 77 - 96, The current event-listener block in AdminPermissions
checks only Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipping cleanup when only
ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled; update the guard to run when either
feature flag is enabled (e.g.,
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) ||
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) so the
existing role/client/group removal branches still call
management(...).roles()/clients()/groups().setPermissionsEnabled(...) and thus
invoke the MgmtPermissionsV2 cleanup path when V2 is active.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…valuation implementation for ClientsPermissionsV2
Closes #35564
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests