Add Groups resource type and scopes to authorization schema #6
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <[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 implements Groups resource type and scopes in Keycloak's authorization schema as part of the Fine-Grained Admin Permissions (FGAP) V2 system. The changes introduce a new authorization paradigm that moves from traditional role-based permissions to a more granular resource-type and scope-based system.
Key architectural changes:
- New Groups Resource Type: Added
GROUPS_RESOURCE_TYPEtoAdminPermissionsSchemawith specific scopes (manage-membership,manage-members,view-members) alongside existingmanageandviewscopes - V2 Permission Classes: Introduced
GroupPermissionsV2and updatedUserPermissionsV2that extend existing permission classes but use the new schema-based authorization model - API Method Changes: Renamed methods like
getGroupsWithViewPermission()togetGroupIdsWithViewPermission()to work with string IDs rather than full objects for better performance - Permission Evaluation Refactoring: Simplified permission checks by removing helper methods and inlining logic, moving from conditional checks to direct role evaluations combined with schema-based permissions
Integration changes:
- Updated
MgmtPermissionsV2to include group permissions support through lazy-initializedGroupPermissionsV2 - Modified resource classes (
GroupsResource,UsersResource,GroupResource) to use the new permission evaluation methods - Enhanced test coverage with comprehensive
GroupResourceTypeEvaluationTestthat validates various group permission scenarios - Added extensive JavaDoc documentation explaining the dual authorization approach (traditional AdminRoles + new AdminPermissionsSchema)
The implementation follows established patterns from the existing Users and Clients resource types, ensuring consistency across the authorization system. The changes maintain backward compatibility while enabling more granular permission control for group management operations.
Confidence score: 3/5
- This PR introduces significant authorization system changes that could impact security and access control if not properly implemented
- There's a critical bug in
AdminPermissionsSchema.getResourceName()method that doesn't handle the newly added Groups resource type, which could cause runtime exceptions - Several files have minor issues like syntax errors (double semicolon in test file) and logic inconsistencies (empty set return when all groups should be accessible)
- Files that need more attention:
AdminPermissionsSchema.java,GroupPermissionsV2.java,GroupResourceTypeEvaluationTest.java
19 files reviewed, 6 comments
| /** | ||
| * If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set. | ||
| * | ||
| * @return Stream of IDs of groups with view permission. |
There was a problem hiding this comment.
syntax: Documentation incorrectly states '@return Stream of IDs' but method signature returns Set<String>
| * @return Stream of IDs of groups with view permission. | |
| * @return Set of IDs of groups with view permission. |
| Keycloak realmAdminClient; | ||
|
|
||
| private final String groupName = "top_group"; | ||
| private final GroupRepresentation topGroup = new GroupRepresentation();; |
There was a problem hiding this comment.
syntax: Double semicolon syntax error
| private final GroupRepresentation topGroup = new GroupRepresentation();; | |
| private final GroupRepresentation topGroup = new GroupRepresentation(); |
| /** | ||
| * Returns {@code true} if the caller has {@link AdminRoles#MANAGE_USERS} role. | ||
| * <p/> | ||
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or |
There was a problem hiding this comment.
syntax: Missing space after 'or' in the JavaDoc comment
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or |
| /** | ||
| * Returns {@code true} if the caller has {@link AdminRoles#MANAGE_USERS} role. | ||
| * <p/> | ||
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or |
There was a problem hiding this comment.
syntax: Missing space after 'or' in the JavaDoc comment
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the user or |
| public Set<String> getGroupIdsWithViewPermission() { | ||
| if (root.users().canView()) return Collections.emptySet(); |
There was a problem hiding this comment.
logic: Logic error: when users can view all groups globally, this should return all group IDs, not an empty set
| return true; | ||
| } | ||
|
|
||
| return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); |
There was a problem hiding this comment.
logic: Inconsistent scope check: canManage() checks VIEW and MANAGE scopes, but should only check MANAGE scope
2 participants
Test 6