Add Groups resource type and scopes to authorization schema#6
Add Groups resource type and scopes to authorization schema#6ShashankFC wants to merge 1 commit into
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <vramik@redhat.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request refactors Keycloak's admin authorization system to introduce fine-grained group-based permissions. It adds a GROUPS resource type with dedicated scopes (MANAGE, VIEW, MANAGE_MEMBERS, MANAGE_MEMBERSHIP, VIEW_MEMBERS), replaces group model-based permission lookups with group ID-based approaches, introduces a V2 permission evaluation system that coexists with V1, and expands permission evaluator interfaces with granular permission-checking methods across users and groups. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
73-98: Listener guard is incomplete—should check both feature flags.The listener registration guard only checks
ADMIN_FINE_GRAINED_AUTHZ, but this misses the V2 case. TheMgmtPermissionsclass (line 85 ofMgmtPermissions.java) uses an OR condition to initialize when either flag is enabled:if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2))The test configuration enables only
ADMIN_FINE_GRAINED_AUTHZ_V2without the base feature, meaning the listener will never register in that scenario. The cleanup logic for removed roles, clients, and groups would be skipped entirely when V2 is enabled alone.Update the guard to:
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2))
🤖 Fix all issues with AI agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java:
- Around line 99-148: The Javadoc for requireManageMembers incorrectly
references canManageMembership; update the Javadoc on the
requireManageMembers(GroupModel) method to reference
canManageMembers(GroupModel) instead (use {@link #canManageMembers(GroupModel)})
so the documentation matches the method naming convention and behavior.
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java:
- Around line 64-71: The canManage() method wrongly calls hasPermission(null,
AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) — remove the VIEW
scope so it only checks for MANAGE; update the hasPermission invocation in
canManage() to pass only AdminPermissionsSchema.MANAGE (matching
canManage(GroupModel)) while keeping the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check.
- Around line 119-128: The code incorrectly uses groupResource.getId() (the
internal Resource DB id) when calling hasPermission and when adding to the
result set; replace both uses with groupResource.getName() so the lookup and
returned identifiers use the resource name (which per
AdminPermissionsSchema.resolveGroup() is set to the actual group ID). Update the
lambda in resourceStore.findByType to call
hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS,
AdminPermissionsSchema.MANAGE_MEMBERS) and to
granted.add(groupResource.getName()).
In
@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java:
- Line 68: In GroupResourceTypeEvaluationTest remove the stray extra semicolon
after the GroupRepresentation constructor in the declaration of the field
topGroup (private final GroupRepresentation topGroup = new
GroupRepresentation();;) so it becomes a single-semicolon termination; update
only the declaration to use a single semicolon to fix the syntax/style issue.
🧹 Nitpick comments (3)
test-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.java (1)
9-13: Good use of try-with-resources for automatic resource management.This refactor ensures the
Responseis properly closed even if an assertion fails or an exception occurs during path extraction, which is cleaner than explicitclose()calls.Optional improvement: Consider adding a null-check or assertion for
response.getLocation()to provide a clearer error message if the Location header is unexpectedly missing:💡 Optional null-safety enhancement
try (response) { Assertions.assertEquals(201, response.getStatus()); - String path = response.getLocation().getPath(); + Assertions.assertNotNull(response.getLocation(), "Expected Location header in 201 response"); + String path = response.getLocation().getPath(); return path.substring(path.lastIndexOf('/') + 1); }server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
181-186: Consider adding fallback resolution for groups by path.
resolveUserandresolveClientattempt secondary lookups (by username/clientId) when the initial ID lookup fails.resolveGrouponly resolves by ID. If consistency is desired, consider adding a fallback lookup by group path, though this may be intentional if groups should only be referenced by ID in the permission context.services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (1)
298-320: Clarify the semantic of returning an empty set whencanView()is true.When
root.users().canView()returnstrue, this method returns an empty set. Callers must understand that an empty set means "view all" rather than "view none." This inverted semantic could lead to bugs if callers interpret an empty set as no permissions.Consider adding a clarifying comment or using an
Optional/ dedicated result type to distinguish between "all groups" and "no groups."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 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)
- 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 context used
🧬 Code graph analysis (2)
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (1)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
AdminPermissionsSchema(53-351)
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (1)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
AdminPermissionsSchema(53-351)
🔇 Additional comments (38)
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RealmAdminPermissionsConfig.java (1)
34-36: LGTM!The addition of
AdminRoles.QUERY_GROUPSalongsideQUERY_USERSis a necessary change to support testing the new GROUPS resource type and scopes introduced in this PR. The test admin user now has the appropriate permissions to query both users and groups.services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java (2)
177-181: Clean simplification of per-item permission filtering.The method reference
auth.groups()::canViewis a cleaner approach that properly delegates the permission check to theGroupPermissionEvaluator. This aligns well with the broader PR goal of fine-grained per-group permissions.
204-234: LGTM - Definite assignment is guaranteed.Removing the explicit
nullinitialization is safe here. The control flow ensureschildis assigned in both branches before use at line 233:
- If branch: assigned at line 208 (or throws
NotFoundExceptionif null)- Else branch: assigned at line 217
This is a minor improvement as the compiler's definite assignment analysis will catch any future refactoring that inadvertently breaks this invariant.
services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java (1)
301-312: The change is aligned with codebase design for global admin roles.The short-circuit for
MANAGE_USERSrole holders bypassingisAdminSameRealm()is intentional and consistent with how other global admin roles are handled throughout the codebase (see UserPermissions.java lines 234, 298; GroupPermissionsV2.java; ClientPermissionsV2.java). Global admin roles likeMANAGE_USERSare designed to operate across realms without fine-grained authorization service checks. ThecheckAdminRoles(role)call still enforces privilege escalation prevention by validating that admins cannot map roles with higher permissions than they currently hold, maintaining the security boundary.services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java (2)
102-102: LGTM!The direct call to
session.groups().searchGroupsByAttributes()aligns with the broader refactor to use session-based search rather than delegating throughModelToRepresentation.
112-114: LGTM!The per-item visibility filtering via
groupsEvaluator::canViewbefore mapping to representation is correct and aligns with the fine-grained permission model introduced in this PR.server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (3)
54-80: LGTM!The new
GROUPS_RESOURCE_TYPEand associated scopes (MANAGE_MEMBERSHIP,MANAGE_MEMBERS,VIEW_MEMBERS) are well-defined and consistent with the existing pattern forCLIENTSandUSERSresource types.
85-89: LGTM!The constructor registration of all three resource types (
CLIENTS,GROUPS,USERS) provides a complete schema for admin permissions.
105-112: LGTM!The switch expression cleanly handles resource type resolution with appropriate error handling for unknown types.
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (4)
56-58: LGTM!Changing field visibility to
protectedenables the V2 subclass (GroupPermissionsV2) to access these fields, following a proper extension pattern.
148-150: LGTM!The updated
canListlogic correctly prioritizes theQUERY_GROUPSadmin role before falling back tocanView(), which aligns with the expected permission hierarchy.
274-288: LGTM!Simplifying
canManage()andcanView()to delegate directly tohasOneAdminRolewith the appropriate roles (MANAGE_USERSandMANAGE_USERS/VIEW_USERSrespectively) is a cleaner implementation that aligns with the admin role-based permission model.
323-341: LGTM!The updated
requireViewMembersandcanViewMembersmethods correctly check bothVIEW_MEMBERS_SCOPEandMANAGE_MEMBERS_SCOPEsince manage permission implies view permission.services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java (2)
27-27: LGTM!The new
groupPermissionsfield follows the same pattern asuserPermissionsandclientPermissions.
57-62: LGTM!The lazy initialization pattern for
GroupPermissionsV2is consistent with the existing patterns forusers()andclients()methods. The unsynchronized lazy initialization is appropriate for request-scoped instances.services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java (4)
398-398: LGTM!The switch to
getGroupIdsWithViewPermission()aligns with the ID-based permission model introduced in this PR.
425-425: LGTM!Consistent use of
getGroupIdsWithViewPermission()for the parameterized user count query.
430-430: LGTM!Consistent use of
getGroupIdsWithViewPermission()for the default user count query.
448-453: Semantics ofUserModel.GROUPSsession attribute are consistent across the codebase.The empty set semantics you identified are correctly implemented:
null→ user has global view permission (whengetGroupIdsWithViewPermission()returns an empty set, the attribute is not set)- non-empty set → user can only view members of these specific groups
All consumers interpret this correctly:
UsersResourceandBruteForceUsersResourcepass!= nulltograntIfNoPermission()to determine permission scopeJpaUserProvideruses the null check to conditionally apply group-based filtering viagroupsWithPermissionsSubquery()No inconsistencies found.
rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.java (1)
147-152: LGTM - Clean migration to group ID-based permission checks.The refactor from group models to group IDs aligns with the broader V2 permission system changes. The simplified stream handling directly passing to
toRepresentationis cleaner.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java (3)
47-53: LGTM - V2 permission check logic is correct.The cascading permission evaluation with admin role check first, then permission-based check, and finally group-based fallback follows a sensible and secure pattern.
56-62: LGTM - canManage implementation is consistent with V2 semantics.
77-92: LGTM - canMapRoles and canManageGroupMembership properly include both MANAGE and specific scopes.The OR logic in
hasPermissioncorrectly allows either the broad MANAGE scope or the specific scope (MAP_ROLES / MANAGE_GROUP_MEMBERSHIP) to grant access, aligning with the documented behavior inUserPermissionEvaluator.services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (2)
46-62: LGTM - View permission checks are correctly implemented.The
canView()andcanView(GroupModel)methods properly check admin roles first, then fall back to permission-based evaluation with both VIEW and MANAGE scopes.
82-103: LGTM - Member-related permission checks are correctly implemented.The
canViewMembers,canManageMembers, andcanManageMembershipmethods properly delegate to user permissions or check group-specific scopes.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java (4)
233-243: LGTM - SimplifiedcanManage()with direct admin role check.The refactor to check
AdminRoles.MANAGE_USERSfirst before falling back to realm-based permission evaluation is cleaner and follows the V2 pattern.
271-274: LGTM - SimplifiedcanQuery()implementation.The simplified expression
root.hasOneAdminRole(AdminRoles.QUERY_USERS) || canView()is more readable.
296-307: LGTM -canView()follows consistent admin role prioritization.
582-590: LGTM - Method references improve readability.Using
root.groups()::canManageMembersandroot.groups()::canViewMembersis cleaner than equivalent lambda expressions.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java (2)
19-25: LGTM - New imports support the enhanced permission documentation.The imports for
AdminPermissionsSchema,AdminRoles, andImpersonationConstantsare correctly added to support the detailed Javadoc references.
32-165: LGTM - Comprehensive Javadoc documentation added.The detailed documentation for each method clearly explains the permission evaluation logic, including which admin roles grant access and which permission scopes are checked. This significantly improves API discoverability and maintainability.
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (5)
90-136: LGTM - Comprehensive test coverage for VIEW_MEMBERS and MANAGE_MEMBERS permissions.The tests properly verify:
- Users are hidden when no permissions exist
- VIEW_MEMBERS grants visibility to group members
- MANAGE_MEMBERS grants both visibility and update capabilities
138-187: LGTM - testManageAllGroups thoroughly exercises all-groups permission.The test validates creating groups, adding children, and role mapping with the MANAGE scope on all groups.
189-247: LGTM - testManageGroup correctly validates per-group MANAGE permission isolation.The test confirms that MANAGE permission on one group doesn't leak to other groups.
249-281: LGTM - testViewGroups validates VIEW permission semantics correctly.
283-319: LGTM - testManageGroupMembership tests the combination of user and group permissions.Good test coverage for the MANAGE_GROUP_MEMBERSHIP and MANAGE_MEMBERSHIP scopes working together to allow group membership management.
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (2)
19-25: LGTM - Appropriate imports added for documentation references.
31-97: LGTM - Well-documented interface methods for group permission evaluation.The Javadoc clearly specifies which admin roles and permission scopes grant access for each method.
| /** | ||
| * Throws ForbiddenException if {@link #canViewMembers(GroupModel)} returns {@code false}. | ||
| */ | ||
| void requireViewMembers(GroupModel group); | ||
|
|
||
| /** | ||
| * Returns {@code true} if {@link UserPermissionEvaluator#canManage()} evaluates to {@code true}. | ||
| * <p/> | ||
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | ||
| */ | ||
| boolean canManageMembers(GroupModel group); | ||
|
|
||
| /** | ||
| * Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} role. | ||
| * <p/> | ||
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the group or | ||
| * {@link AdminPermissionsSchema#MANAGE_MEMBERSHIP} of the group. | ||
| */ | ||
| boolean canManageMembership(GroupModel group); | ||
|
|
||
|
|
||
| /** | ||
| * Returns {@code true} if {@link UserPermissionEvaluator#canView()} evaluates to {@code true}. | ||
| * <p/> | ||
| * Or if it has a permission to {@link AdminPermissionsSchema#VIEW_MEMBERS} or | ||
| * {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | ||
| */ | ||
| boolean canViewMembers(GroupModel group); | ||
|
|
||
|
|
||
| /** | ||
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | ||
| */ | ||
| void requireManageMembership(GroupModel group); | ||
|
|
||
| /** | ||
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | ||
| */ | ||
| void requireManageMembers(GroupModel group); | ||
|
|
||
| /** | ||
| * Returns Map with information what access the caller for the provided group has. | ||
| */ | ||
| Map<String, Boolean> getAccess(GroupModel group); | ||
|
|
||
| Set<String> getGroupsWithViewPermission(); | ||
| /** | ||
| * If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set. | ||
| * | ||
| * @return Stream of IDs of groups with view permission. | ||
| */ | ||
| Set<String> getGroupIdsWithViewPermission(); | ||
| } |
There was a problem hiding this comment.
Javadoc inconsistency: requireManageMembers documentation references wrong method.
At lines 132-134, the Javadoc for requireManageMembers states it throws if canManageMembership returns false, but it should reference canManageMembers based on the method naming convention used elsewhere in this interface.
📝 Proposed fix
/**
- * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}.
+ * Throws ForbiddenException if {@link #canManageMembers(GroupModel)} returns {@code false}.
*/
void requireManageMembers(GroupModel group);📝 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.
| /** | |
| * Throws ForbiddenException if {@link #canViewMembers(GroupModel)} returns {@code false}. | |
| */ | |
| void requireViewMembers(GroupModel group); | |
| /** | |
| * Returns {@code true} if {@link UserPermissionEvaluator#canManage()} evaluates to {@code true}. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | |
| */ | |
| boolean canManageMembers(GroupModel group); | |
| /** | |
| * Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} role. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the group or | |
| * {@link AdminPermissionsSchema#MANAGE_MEMBERSHIP} of the group. | |
| */ | |
| boolean canManageMembership(GroupModel group); | |
| /** | |
| * Returns {@code true} if {@link UserPermissionEvaluator#canView()} evaluates to {@code true}. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#VIEW_MEMBERS} or | |
| * {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | |
| */ | |
| boolean canViewMembers(GroupModel group); | |
| /** | |
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | |
| */ | |
| void requireManageMembership(GroupModel group); | |
| /** | |
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | |
| */ | |
| void requireManageMembers(GroupModel group); | |
| /** | |
| * Returns Map with information what access the caller for the provided group has. | |
| */ | |
| Map<String, Boolean> getAccess(GroupModel group); | |
| Set<String> getGroupsWithViewPermission(); | |
| /** | |
| * If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set. | |
| * | |
| * @return Stream of IDs of groups with view permission. | |
| */ | |
| Set<String> getGroupIdsWithViewPermission(); | |
| } | |
| /** | |
| * Throws ForbiddenException if {@link #canViewMembers(GroupModel)} returns {@code false}. | |
| */ | |
| void requireViewMembers(GroupModel group); | |
| /** | |
| * Returns {@code true} if {@link UserPermissionEvaluator#canManage()} evaluates to {@code true}. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | |
| */ | |
| boolean canManageMembers(GroupModel group); | |
| /** | |
| * Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} role. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the group or | |
| * {@link AdminPermissionsSchema#MANAGE_MEMBERSHIP} of the group. | |
| */ | |
| boolean canManageMembership(GroupModel group); | |
| /** | |
| * Returns {@code true} if {@link UserPermissionEvaluator#canView()} evaluates to {@code true}. | |
| * <p/> | |
| * Or if it has a permission to {@link AdminPermissionsSchema#VIEW_MEMBERS} or | |
| * {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group. | |
| */ | |
| boolean canViewMembers(GroupModel group); | |
| /** | |
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | |
| */ | |
| void requireManageMembership(GroupModel group); | |
| /** | |
| * Throws ForbiddenException if {@link #canManageMembers(GroupModel)} returns {@code false}. | |
| */ | |
| void requireManageMembers(GroupModel group); | |
| /** | |
| * Returns Map with information what access the caller for the provided group has. | |
| */ | |
| Map<String, Boolean> getAccess(GroupModel group); | |
| /** | |
| * If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set. | |
| * | |
| * @return Stream of IDs of groups with view permission. | |
| */ | |
| Set<String> getGroupIdsWithViewPermission(); | |
| } |
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java
around lines 99 - 148, The Javadoc for requireManageMembers incorrectly
references canManageMembership; update the Javadoc on the
requireManageMembers(GroupModel) method to reference
canManageMembers(GroupModel) instead (use {@link #canManageMembers(GroupModel)})
so the documentation matches the method naming convention and behavior.
| @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.
Bug: canManage() incorrectly includes VIEW scope.
The canManage() method at line 70 checks for both VIEW and MANAGE scopes, but it should only check for MANAGE. Compare with canManage(GroupModel) at line 79 which correctly uses only MANAGE.
This appears to be a copy-paste error from canView().
🐛 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.
| @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
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
around lines 64 - 71, The canManage() method wrongly calls hasPermission(null,
AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) — remove the VIEW
scope so it only checks for MANAGE; update the hasPermission invocation in
canManage() to pass only AdminPermissionsSchema.MANAGE (matching
canManage(GroupModel)) while keeping the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check.
| 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.
Bug: Using groupResource.getId() instead of groupResource.getName() for group ID lookup.
The code uses groupResource.getId() which returns the Resource entity's internal database ID. However:
- At line 122,
hasPermission(groupResource.getId(), ...)will fail becausehasPermissionat line 141 doesresourceStore.findByName(server, groupId), expecting the group ID (which is the resource name). - At line 123,
granted.add(groupResource.getId())adds the wrong identifier to the result set.
Based on AdminPermissionsSchema.resolveGroup(), the resource name is set to group.getId(), so you should use groupResource.getName().
🐛 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());
}
});🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
around lines 119 - 128, The code incorrectly uses groupResource.getId() (the
internal Resource DB id) when calling hasPermission and when adding to the
result set; replace both uses with groupResource.getName() so the lookup and
returned identifiers use the resource name (which per
AdminPermissionsSchema.resolveGroup() is set to the actual group ID). Update the
lambda in resourceStore.findByType to call
hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS,
AdminPermissionsSchema.MANAGE_MEMBERS) and to
granted.add(groupResource.getName()).
| Keycloak realmAdminClient; | ||
|
|
||
| private final String groupName = "top_group"; | ||
| private final GroupRepresentation topGroup = new GroupRepresentation();; |
There was a problem hiding this comment.
Minor: Double semicolon in variable initialization.
There's an extra semicolon after the GroupRepresentation constructor call.
🧹 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.
| private final GroupRepresentation topGroup = new GroupRepresentation();; | |
| private final GroupRepresentation topGroup = new GroupRepresentation(); |
🤖 Prompt for AI Agents
In
@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java
at line 68, In GroupResourceTypeEvaluationTest remove the stray extra semicolon
after the GroupRepresentation constructor in the declaration of the field
topGroup (private final GroupRepresentation topGroup = new
GroupRepresentation();;) so it becomes a single-semicolon termination; update
only the declaration to use a single semicolon to fix the syntax/style issue.
2 participants
Test 6
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#6