Add Groups resource type and scopes to authorization schema and evalu… - #1
Open
linxia0415 wants to merge 1 commit into
Open
Add Groups resource type and scopes to authorization schema and evalu…#1linxia0415 wants to merge 1 commit into
linxia0415 wants to merge 1 commit into
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <vramik@redhat.com>
📝 WalkthroughWalkthroughThis pull request extends Keycloak's admin fine-grained authorization to support group-scoped permissions. It adds a GROUPS resource type to the authorization schema with membership-related scopes, refactors group permission evaluation from simple role checks to policy-based evaluation via a new GroupPermissionsV2 implementation, and updates related admin resources and user permission checks to use the new group ID-based visibility APIs. ChangesGroup-Level Fine-Grained Authorization System
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 4
🤖 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/GroupPermissionsV2.java`:
- Around line 105-128: In getGroupIdsWithViewPermission (GroupPermissionsV2),
you're using the authorization resource's internal ID instead of the actual
group ID: replace usages of groupResource.getId() in the hasPermission(...) call
and the granted.add(...) call with groupResource.getName() so permission checks
and returned IDs use the resource name (which is set to the group ID in
AdminPermissionsSchema.getOrCreateResource()) rather than the internal resource
ID.
- Around line 64-71: The canManage() method in GroupPermissionsV2 improperly
treats VIEW as sufficient for manage; update the hasPermission call in
canManage() so it only checks for AdminPermissionsSchema.MANAGE (remove
AdminPermissionsSchema.VIEW) while keeping the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check; this makes canManage()
consistent with canManage(GroupModel group) and ensures only the MANAGE scope
grants manage rights.
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java`:
- Line 68: The declaration of the field topGroup in class
GroupResourceTypeEvaluationTest has an extra semicolon ("private final
GroupRepresentation topGroup = new GroupRepresentation();;"); remove the
duplicate semicolon so the field reads with a single terminating semicolon;
locate the topGroup field in GroupResourceTypeEvaluationTest and update its
declaration to end with just one semicolon.
- Around line 73-77: The test double-closes Response objects because the
Response returned from realm.admin().groups().add(...) is wrapped in an outer
try-with-resources while ApiUtil.handleCreatedResponse(...) now also closes the
Response; remove the outer try-with-resources and the redundant status assertion
and instead assign the response id from ApiUtil.handleCreatedResponse(...)
directly (as done by topGroup.setId(ApiUtil.handleCreatedResponse(response)));
apply the same change to the other occurrences around the blocks that call
realm.admin().groups().add(...) at the locations mirrored by the existing
pattern so only ApiUtil.handleCreatedResponse(...) manages closing the Response.
🪄 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: a9a12f31-7d52-4d3e-9113-dc602d44f374
📒 Files selected for processing (19)
rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.javaserver-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.javaserver-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.javaservices/src/main/java/org/keycloak/services/resources/admin/GroupResource.javaservices/src/main/java/org/keycloak/services/resources/admin/GroupsResource.javaservices/src/main/java/org/keycloak/services/resources/admin/UsersResource.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.javatest-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RealmAdminPermissionsConfig.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java
💤 Files with no reviewable changes (2)
- server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java
- tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java
Comment on lines
+64
to
+71
| @Override | ||
| public boolean canManage() { | ||
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) { | ||
| return true; | ||
| } | ||
|
|
||
| return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); | ||
| } |
There was a problem hiding this comment.
canManage() incorrectly includes VIEW scope.
The method checks for both VIEW and MANAGE scopes, but canManage() should only grant manage permission when the user has the MANAGE scope. Compare with canManage(GroupModel group) at line 79 which correctly checks only for MANAGE.
🐛 Proposed fix
`@Override`
public boolean canManage() {
if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
return true;
}
- return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
+ return hasPermission(null, AdminPermissionsSchema.MANAGE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public boolean canManage() { | |
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) { | |
| return true; | |
| } | |
| return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); | |
| } | |
| `@Override` | |
| public boolean canManage() { | |
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) { | |
| return true; | |
| } | |
| return hasPermission(null, AdminPermissionsSchema.MANAGE); | |
| } |
🤖 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/GroupPermissionsV2.java`
around lines 64 - 71, The canManage() method in GroupPermissionsV2 improperly
treats VIEW as sufficient for manage; update the hasPermission call in
canManage() so it only checks for AdminPermissionsSchema.MANAGE (remove
AdminPermissionsSchema.VIEW) while keeping the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check; this makes canManage()
consistent with canManage(GroupModel group) and ensures only the MANAGE scope
grants manage rights.
Comment on lines
+105
to
+128
| @Override | ||
| public Set<String> getGroupIdsWithViewPermission() { | ||
| if (root.users().canView()) return Collections.emptySet(); | ||
|
|
||
| if (!root.isAdminSameRealm()) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| ResourceServer server = root.realmResourceServer(); | ||
|
|
||
| if (server == null) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| Set<String> granted = new HashSet<>(); | ||
|
|
||
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | ||
| if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | ||
| granted.add(groupResource.getId()); | ||
| } | ||
| }); | ||
|
|
||
| return granted; | ||
| } |
There was a problem hiding this comment.
getGroupIdsWithViewPermission() uses resource ID instead of group ID.
The method iterates over authorization resources and uses groupResource.getId() (the authorization resource's internal ID) instead of groupResource.getName() (the actual group ID). The resource name is set to the group ID in AdminPermissionsSchema.getOrCreateResource().
Both line 122 (hasPermission(groupResource.getId(), ...)) and line 123 (granted.add(groupResource.getId())) should use groupResource.getName() instead.
🐛 Proposed fix
resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
- if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
- granted.add(groupResource.getId());
+ if (hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
+ granted.add(groupResource.getName());
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public Set<String> getGroupIdsWithViewPermission() { | |
| if (root.users().canView()) return Collections.emptySet(); | |
| if (!root.isAdminSameRealm()) { | |
| return Collections.emptySet(); | |
| } | |
| ResourceServer server = root.realmResourceServer(); | |
| if (server == null) { | |
| return Collections.emptySet(); | |
| } | |
| Set<String> granted = new HashSet<>(); | |
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | |
| if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | |
| granted.add(groupResource.getId()); | |
| } | |
| }); | |
| return granted; | |
| } | |
| `@Override` | |
| public Set<String> getGroupIdsWithViewPermission() { | |
| if (root.users().canView()) return Collections.emptySet(); | |
| if (!root.isAdminSameRealm()) { | |
| return Collections.emptySet(); | |
| } | |
| ResourceServer server = root.realmResourceServer(); | |
| if (server == null) { | |
| return Collections.emptySet(); | |
| } | |
| Set<String> granted = new HashSet<>(); | |
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | |
| if (hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | |
| granted.add(groupResource.getName()); | |
| } | |
| }); | |
| return granted; | |
| } |
🤖 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/GroupPermissionsV2.java`
around lines 105 - 128, In getGroupIdsWithViewPermission (GroupPermissionsV2),
you're using the authorization resource's internal ID instead of the actual
group ID: replace usages of groupResource.getId() in the hasPermission(...) call
and the granted.add(...) call with groupResource.getName() so permission checks
and returned IDs use the resource name (which is set to the group ID in
AdminPermissionsSchema.getOrCreateResource()) rather than the internal resource
ID.
| Keycloak realmAdminClient; | ||
|
|
||
| private final String groupName = "top_group"; | ||
| private final GroupRepresentation topGroup = new GroupRepresentation();; |
There was a problem hiding this comment.
Remove extra semicolon.
✂️ Proposed fix
- private final GroupRepresentation topGroup = new GroupRepresentation();;
+ private final GroupRepresentation topGroup = new GroupRepresentation();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private final GroupRepresentation topGroup = new GroupRepresentation();; | |
| private final GroupRepresentation topGroup = new GroupRepresentation(); |
🤖 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/GroupResourceTypeEvaluationTest.java`
at line 68, The declaration of the field topGroup in class
GroupResourceTypeEvaluationTest has an extra semicolon ("private final
GroupRepresentation topGroup = new GroupRepresentation();;"); remove the
duplicate semicolon so the field reads with a single terminating semicolon;
locate the topGroup field in GroupResourceTypeEvaluationTest and update its
declaration to end with just one semicolon.
Comment on lines
+73
to
+77
| try (Response response = realm.admin().groups().add(topGroup)) { | ||
| assertThat(response.getStatus(), equalTo(Response.Status.CREATED.getStatusCode())); | ||
| topGroup.setId(ApiUtil.handleCreatedResponse(response)); | ||
| realm.cleanup().add(r -> r.groups().group(topGroup.getId()).remove()); | ||
| } |
There was a problem hiding this comment.
Fix resource management: double-close of Response.
The Response is wrapped in try-with-resources on line 73, then passed to ApiUtil.handleCreatedResponse on line 75. Since ApiUtil.handleCreatedResponse now uses try-with-resources internally (as changed in ApiUtil.java), the Response will be closed twice: once inside handleCreatedResponse and again when the outer try block exits.
Additionally, the status check on line 74 is redundant because handleCreatedResponse already validates the 201 status.
This pattern also appears at lines 195-199 and 261-265.
🔧 Proposed fix
- try (Response response = realm.admin().groups().add(topGroup)) {
- assertThat(response.getStatus(), equalTo(Response.Status.CREATED.getStatusCode()));
- topGroup.setId(ApiUtil.handleCreatedResponse(response));
- realm.cleanup().add(r -> r.groups().group(topGroup.getId()).remove());
- }
+ Response response = realm.admin().groups().add(topGroup);
+ topGroup.setId(ApiUtil.handleCreatedResponse(response));
+ realm.cleanup().add(r -> r.groups().group(topGroup.getId()).remove());Apply the same pattern at lines 195-199 and 261-265:
Lines 195-199:
- try (Response response = realm.admin().groups().add(myGroup)) {
- assertThat(response.getStatus(), equalTo(Response.Status.CREATED.getStatusCode()));
- myGroup.setId(ApiUtil.handleCreatedResponse(response));
- realm.cleanup().add(r -> r.groups().group(myGroup.getId()).remove());
- }
+ Response response = realm.admin().groups().add(myGroup);
+ myGroup.setId(ApiUtil.handleCreatedResponse(response));
+ realm.cleanup().add(r -> r.groups().group(myGroup.getId()).remove());Lines 261-265:
- try (Response response = realm.admin().groups().add(myGroup)) {
- assertThat(response.getStatus(), equalTo(Response.Status.CREATED.getStatusCode()));
- myGroup.setId(ApiUtil.handleCreatedResponse(response));
- realm.cleanup().add(r -> r.groups().group(myGroup.getId()).remove());
- }
+ Response response = realm.admin().groups().add(myGroup);
+ myGroup.setId(ApiUtil.handleCreatedResponse(response));
+ realm.cleanup().add(r -> r.groups().group(myGroup.getId()).remove());🤖 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/GroupResourceTypeEvaluationTest.java`
around lines 73 - 77, The test double-closes Response objects because the
Response returned from realm.admin().groups().add(...) is wrapped in an outer
try-with-resources while ApiUtil.handleCreatedResponse(...) now also closes the
Response; remove the outer try-with-resources and the redundant status assertion
and instead assign the response id from ApiUtil.handleCreatedResponse(...)
directly (as done by topGroup.setId(ApiUtil.handleCreatedResponse(response)));
apply the same change to the other occurrences around the blocks that call
realm.admin().groups().add(...) at the locations mirrored by the existing
pattern so only ApiUtil.handleCreatedResponse(...) manages closing the Response.
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.
…ation implementation
Closes #35562
Summary by CodeRabbit
Release Notes
New Features
Tests