Add Groups resource type and scopes to authorization schema#6
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <vramik@redhat.com>
WalkthroughThese changes introduce fine-grained authorization for group resources, expanding the permission model to include new group-specific scopes and resource types. The update adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant REST as REST Endpoint
participant MgmtPermV2 as MgmtPermissionsV2
participant GroupPermV2 as GroupPermissionsV2
participant Authz as AuthorizationProvider
AdminUI->>REST: Request group resource operation
REST->>MgmtPermV2: groups()
MgmtPermV2->>GroupPermV2: getGroupIdsWithViewPermission()
GroupPermV2->>Authz: Evaluate permissions for group IDs
Authz-->>GroupPermV2: Return allowed group IDs
GroupPermV2-->>MgmtPermV2: Return allowed group IDs
MgmtPermV2-->>REST: Return allowed group IDs
REST-->>AdminUI: Respond with resource data or permission denied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
♻️ Duplicate comments (1)
services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java (1)
86-92: Consistent refactoring pattern maintained.The method follows the same refactoring approach as
canMapRoles, with direct role checking and consolidated return logic.
🧹 Nitpick comments (4)
services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java (1)
163-164: Consider adding deprecation details to the JavaDoc.While the method is marked as
@Deprecated, it would be helpful to add JavaDoc explaining why it's deprecated and what should be used instead.+ /** + * @deprecated Use {@link #canImpersonate(UserModel, ClientModel)} instead. + */ @Deprecated boolean isImpersonatable(UserModel user, ClientModel requester);services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (1)
315-315: Consider adding validation for the substring operation.The code assumes all resource names start with
RESOURCE_NAME_PREFIX. Consider adding a defensive check to prevent potentialStringIndexOutOfBoundsException.- granted.add(resource.getName().substring(RESOURCE_NAME_PREFIX.length())); + String name = resource.getName(); + if (name.startsWith(RESOURCE_NAME_PREFIX)) { + granted.add(name.substring(RESOURCE_NAME_PREFIX.length())); + }tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (2)
68-68: Remove extra semicolon.- private final GroupRepresentation topGroup = new GroupRepresentation();; + private final GroupRepresentation topGroup = new GroupRepresentation();
300-300: Remove trailing whitespace.- //create new user
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.java(1 hunks)server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java(4 hunks)server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java(0 hunks)services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java(3 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java(7 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java(1 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java(1 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java(4 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java(2 hunks)test-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.java(1 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java(1 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RealmAdminPermissionsConfig.java(1 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java(0 hunks)
💤 Files with no reviewable changes (2)
- tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java
- server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java
🔇 Additional comments (47)
test-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.java (1)
9-13: LGTM! Good resource management improvement.The refactoring to use try-with-resources for the
Responseobject ensures proper resource cleanup while maintaining the existing logic.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RealmAdminPermissionsConfig.java (1)
34-36: LGTM! Proper permission alignment for group authorization.Adding
QUERY_GROUPSrole for the admin user aligns with the new group resource type authorization features being introduced in this PR.services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java (1)
302-302: LGTM! Improved permission check consistency.The change from
root.users().canManageDefault()toroot.hasOneAdminRole(AdminRoles.MANAGE_USERS)aligns with the pattern of using direct admin role checks throughout the codebase, improving consistency.services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java (2)
102-102: LGTM! Proper API usage update.The change from
ModelToRepresentation.searchGroupModelsByAttributestosession.groups().searchGroupsByAttributesaligns with the API refactoring mentioned in the AI summary.
113-114: LGTM! Simplified permission filtering.The direct use of
groupsEvaluator::canViewin the stream filter simplifies the permission checking logic and aligns with the new group permission evaluation model.services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
74-98: LGTM! Performance optimization with feature flag gating.Moving the feature flag check outside the listener registration is a good optimization. This prevents unnecessary listener registration and event processing overhead when fine-grained authorization is disabled.
services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java (2)
179-179: LGTM! Simplified permission filtering.The direct use of
auth.groups()::canViewas a filter predicate eliminates the intermediate boolean variable and makes the code more concise and readable.
206-206: LGTM! Appropriate variable declaration.Changing from explicit initialization to declaration-only is correct since the variable is properly assigned in both branches of the subsequent if-else block.
rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.java (1)
147-152: LGTM! Streamlined permission filtering logic.The refactoring successfully simplifies the method by:
- Using group ID sets instead of group model objects
- Removing intermediate variables and conditional checks
- Maintaining the same permission filtering behavior
This aligns well with the broader refactoring effort to use group IDs for permission evaluation.
services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java (2)
27-27: LGTM! Consistent field addition.The new
groupPermissionsfield follows the same pattern as other permission fields in the class.
57-62: LGTM! Proper lazy initialization implementation.The
groups()method correctly implements the lazy initialization pattern used throughout the class, ensuring consistent behavior with other permission evaluators.services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java (4)
398-398: LGTM! Updated to use group ID-based permission check.The change from
getGroupsWithViewPermission()togetGroupIdsWithViewPermission()aligns with the refactoring to use group IDs for permission evaluation, improving performance by avoiding unnecessary group model loading.
425-425: LGTM! Consistent permission check update.Same beneficial change as above - using group IDs instead of group models for permission evaluation.
430-430: LGTM! Consistent permission check update.Same beneficial change as above - using group IDs instead of group models for permission evaluation.
448-453: LGTM! Streamlined user search with group ID filtering.The refactoring successfully:
- Uses group ID sets instead of group model objects
- Simplifies the session attribute setting logic
- Maintains the same permission filtering behavior while improving efficiency
services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java (4)
234-234: LGTM! Simplified permission check.Direct use of
root.hasOneAdminRole(AdminRoles.MANAGE_USERS)removes unnecessary indirection and makes the permission logic more explicit and readable.
273-273: LGTM! Improved condition ordering.Checking the more specific
QUERY_USERSrole before the broadercanView()could improve performance by evaluating the more targeted condition first.
298-298: LGTM! Consistent direct role checking.The simplified approach using direct role checks aligns with the pattern established in
canManage()and improves code consistency.
584-589: LGTM! Cleaner method references.Using method references (
root.groups()::canManageMembersandroot.groups()::canViewMembers) instead of lambda expressions improves code readability and conciseness.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java (4)
52-52: Good simplification of the return logic.The consolidation of permission checks into a single return statement improves readability while maintaining the same logical behavior.
56-62: Consistent simplification pattern applied.The refactoring maintains consistency with other methods by removing intermediate variables and using direct logical OR operations.
48-48: Removal of AdminRoles.ADMIN check is safeThe search shows no other usages of
hasOneAdminRole(AdminRoles.ADMIN)in permission checks—only granular roles (e.g. MANAGE_USERS, VIEW_USERS) are used. Since theADMINrole is a composite of all these finer-grained roles, checking justMANAGE_USERSorVIEW_USERSstill grants full admin users the same access. No further changes are needed.
78-78: Confirm map-roles permission operates independently of manage permissionIn UserPermissionsV2.canMapRoles (services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java lines 75–83), the authorization logic has been restructured:
- Previously (in V1): canMapRoles immediately returned false unless canManage(user) was true (i.e., global MANAGE scope or group-based manage). Only then did it check MAP_ROLES or group mapping.
- Now (in V2):
- It short-circuits on AdminRoles.MANAGE_USERS (same as canManage’s first branch).
- Then allows MAP_ROLES scope OR MANAGE scope OR canMapRolesByGroup, without requiring full canManage(user) first.
This change effectively lets a user with only the MAP_ROLES permission (but without MANAGE or group-manage) map roles. Please verify whether separating MAP_ROLES from the broader manage check is intentional and aligns with your security requirements.
services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java (1)
19-161: Excellent documentation improvements.The comprehensive JavaDoc additions significantly improve the interface contract by clearly specifying:
- Required roles and permissions for each operation
- When exceptions are thrown
- The relationship between different permission levels
This will greatly help developers understand and implement the authorization model correctly.
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (2)
31-140: Comprehensive documentation improvements.The JavaDoc additions provide excellent clarity on permission requirements and method behaviors, maintaining consistency with the UserPermissionEvaluator interface documentation style.
143-147: Significant API change: Method signature and return type modified.The method has changed from
getGroupsWithViewPermission(GroupModel group)togetGroupIdsWithViewPermission(). This is a breaking change that:
- No longer takes a group parameter
- Returns group IDs instead of boolean
- Returns all groups with view permission rather than checking a specific group
This aligns with performance optimizations but will require updates to all implementations and callers.
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (3)
56-57: Well-structured addition of group resource type support.The new group resource type and its associated scopes follow the established pattern and provide appropriate fine-grained permissions for group management operations.
Also applies to: 69-71, 79-79
107-112: Good refactoring to switch statement.The switch statement improves code readability and maintainability compared to an if-else chain, especially as more resource types may be added in the future.
181-186: Consistent implementation of group resolution.The
resolveGroupmethod follows the same pattern asresolveUserandresolveClient, maintaining consistency in the codebase.services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (4)
56-58: Appropriate visibility changes for inheritance.Changing these fields to
protectedallows theGroupPermissionsV2subclass to access them, which is necessary for the V2 implementation pattern.
149-149: Minor optimization in permission check ordering.Checking the specific
QUERY_GROUPSrole before the more complexcanView()check is a good micro-optimization.
274-276: Good simplification of permission checks.Directly checking roles instead of delegating to user permission methods removes unnecessary indirection and improves clarity.
Also applies to: 286-288
298-320: Well-implemented group ID permission checking.The new method efficiently determines which groups the current user can view by:
- Short-circuiting for users with global view permissions
- Checking both VIEW_MEMBERS and MANAGE_MEMBERS scopes
- Properly extracting group IDs from resource names
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (8)
37-44: LGTM!The class structure and constructor are well-implemented.
46-53: LGTM!The method correctly checks global view permissions with appropriate admin role bypass.
55-62: LGTM!The method correctly checks view permissions for a specific group.
74-80: LGTM!The method correctly checks manage permissions for a specific group.
82-103: LGTM!All three member-related permission methods are correctly implemented with proper scope checks and permission delegation.
105-128: LGTM!The method correctly retrieves group IDs with view permissions, with appropriate optimizations for users with global view access.
130-165: LGTM!The core permission evaluation logic is well-implemented with proper fallback to resource type permissions.
167-210: LGTM!The unsupported operations are appropriately handled with clear error messages indicating V2 limitations.
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (6)
90-106: LGTM!The test correctly validates VIEW_MEMBERS permission functionality.
108-136: LGTM!The test thoroughly validates MANAGE_MEMBERS permission for both viewing and managing users.
138-187: LGTM!The test comprehensively validates all-groups MANAGE permission functionality.
189-247: LGTM!The test correctly validates specific group MANAGE permissions with comprehensive scenarios.
249-281: LGTM!The test properly validates VIEW permissions for both specific groups and all groups.
283-320: LGTM!The test correctly validates group membership management permissions, and helper methods are well-structured.
| 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.
Fix incorrect permission check in canManage() method.
The method is checking for VIEW scope when it should only check for MANAGE scope. A user with only VIEW permissions should not be able to manage groups.
Apply this 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.
| 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
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
between lines 65 and 71, the canManage() method incorrectly checks for both VIEW
and MANAGE permissions, allowing users with only VIEW permission to manage
groups. Modify the hasPermission call to check only for the MANAGE permission
scope, removing the VIEW scope from the parameters to ensure only users with
MANAGE permission can manage groups.
2 participants
Test 6
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores