Add Groups resource type and scopes to authorization schema#5
Add Groups resource type and scopes to authorization schema#5akshayutture-augment wants to merge 1 commit into
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <vramik@redhat.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces Groups as a distinct fine-grained authorization resource type, refactoring permission checks from using group objects to group IDs. It expands permission evaluator APIs for both groups and users with granular permission checks, creates V2 variants of permission classes for fine-grained authorization, and updates existing code paths to use the new APIs. Changes
Sequence DiagramsequenceDiagram
participant Client as Admin Client
participant AdminAPI as Admin API Resource
participant PermEval as Permission Evaluator
participant PermV2 as PermissionsV2 (FGAP)
participant AuthzSchema as Authorization Schema
participant Store as Resource/Policy Store
Client->>AdminAPI: Request (e.g., list/manage groups)
AdminAPI->>PermEval: canView/canManage/canViewMembers
alt Using V1 (Role-based)
PermEval->>PermEval: Check AdminRoles (MANAGE_USERS, VIEW_USERS, etc.)
PermEval-->>AdminAPI: boolean result
else Using V2 (Fine-grained)
PermEval->>PermV2: Delegate to V2 implementation
PermV2->>AuthzSchema: Resolve resource by group/user ID
AuthzSchema->>Store: Look up Resource
PermV2->>Store: Check Policies & scopes (VIEW, MANAGE, etc.)
Store-->>PermV2: Has scope permission?
PermV2-->>PermEval: boolean result
end
PermEval-->>AdminAPI: Grant/Deny
AdminAPI-->>Client: Response (filtered or 403)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Key areas for focused review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 4
🧹 Nitpick comments (6)
test-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.java (1)
11-12: Consider adding defensive null checks for robustness.While the code works for well-formed 201 responses,
response.getLocation()could return null if the Location header is missing. This would produce a less informative NPE instead of a clear assertion failure.Consider adding a null check for clearer test failures:
try (response) { Assertions.assertEquals(201, response.getStatus()); - String path = response.getLocation().getPath(); + var location = response.getLocation(); + Assertions.assertNotNull(location, "Location header is missing in 201 response"); + String path = location.getPath(); return path.substring(path.lastIndexOf('/') + 1); }rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.java (1)
144-153: Align GROUPS-based filtering across all search pathsRight now the
UserModel.GROUPSsession attribute (derived fromauth.groups().getGroupIdsWithViewPermission()) is only set insidesearchForUser(...). Theid:search branch builds a stream and callstoRepresentation(...)directly, so it never sees this attribute:
searchstarting withid:→userModels→toRepresentation(...)without GROUPS set- attribute / generic searches → go through
searchForUser(...), which sets GROUPS and then callstoRepresentation(...)Given that
toRepresentation(...)callsusersEvaluator.grantIfNoPermission(session.getAttribute(UserModel.GROUPS) != null), this means the grant‑if‑pre‑filtered behavior only applies to thesearchForUsercode path.If the intent is that group‑scoped admins should get consistent, GROUPS-constrained behavior for all searches (including
id:), consider:- private Stream<BruteUser> searchForUser(Map<String, String> attributes, RealmModel realm, UserPermissionEvaluator usersEvaluator, Boolean briefRepresentation, Integer firstResult, Integer maxResults, Boolean includeServiceAccounts) { - attributes.put(UserModel.INCLUDE_SERVICE_ACCOUNT, includeServiceAccounts.toString()); - - Set<String> groupIds = auth.groups().getGroupIdsWithViewPermission(); - if (!groupIds.isEmpty()) { - session.setAttribute(UserModel.GROUPS, groupIds); - } - - return toRepresentation(realm, usersEvaluator, briefRepresentation, session.users().searchForUserStream(realm, attributes, firstResult, maxResults)); - } + private void applyGroupsFilterIfAny() { + Set<String> groupIds = auth.groups().getGroupIdsWithViewPermission(); + if (!groupIds.isEmpty()) { + session.setAttribute(UserModel.GROUPS, groupIds); + } + } + + private Stream<BruteUser> searchForUser(Map<String, String> attributes, RealmModel realm, UserPermissionEvaluator usersEvaluator, Boolean briefRepresentation, Integer firstResult, Integer maxResults, Boolean includeServiceAccounts) { + attributes.put(UserModel.INCLUDE_SERVICE_ACCOUNT, includeServiceAccounts.toString()); + applyGroupsFilterIfAny(); + return toRepresentation(realm, usersEvaluator, briefRepresentation, + session.users().searchForUserStream(realm, attributes, firstResult, maxResults)); + }…and invoke
applyGroupsFilterIfAny()once at the top ofsearchUser(...)so that both theid:and non‑id:flows share the same GROUPS filtering semantics.Also worth double‑checking that
getGroupIdsWithViewPermission()is guaranteed non‑null to avoid an NPE onisEmpty().services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionsV2.java (1)
46-62: Clarify intended scope semantics inhasPermissionand V2 user checksThe V2 methods look coherent overall:
canView/canManagenow short-circuit for the expected admin roles (MANAGE_USERS/VIEW_USERS), then fall back to FGAP evaluation viahasPermission(...)pluscanViewByGroup/canManageByGroup.canMapRolesandcanManageGroupMembershipsimilarly short-circuit onMANAGE_USERSand otherwise rely onhasPermission(...)+ group-based manage.canImpersonateadds aDefaultEvaluationContextwithkc.client.idwhen a requester is present, which is a nice improvement for client-aware impersonation evaluation.In
hasPermission(...)you effectively implement “any-of” scope semantics:List<String> expectedScopes = Arrays.asList(scopes); ... for (String scope : permission.getScopes()) { if (expectedScopes.contains(scope)) { return true; } }So calls like:
hasPermission(user, null, AdminPermissionsSchema.MANAGE, AdminPermissionsSchema.MAP_ROLES) hasPermission(user, null, AdminPermissionsSchema.MANAGE, AdminPermissionsSchema.MANAGE_GROUP_MEMBERSHIP)will succeed if either
MANAGEor the more specific scope is granted on the evaluated resource, not necessarily both. That’s likely what you want (globalMANAGEor a narrower per-user permission both being sufficient), but it would be good to explicitly confirm this matches the intended FGAP schema and tests for MAP_ROLES and MANAGE_GROUP_MEMBERSHIP.Also, this method assumes
AdminPermissionsSchema.SCHEMA.getResourceTypeResource(...)always returns a non-null resource; if there is any configuration where that might not be the case, a null-guard beforepolicyStore.findByResource(server, resource)would avoid a potential NPE.Also applies to: 76-92, 94-133
services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java (1)
391-431: Group‑ID based user count filtering looks correct; minor duplication only.Using
auth.groups().getGroupIdsWithViewPermission()in the threegetUsersCountfallback branches correctly aligns count queries with the new groups‑by‑ID permission model. The only nit is that the call is repeated three times; caching to a localSet<String> groupIdsat the top of the method would slightly improve readability and avoid recomputation, but is not required.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (1)
58-307: Strong coverage of GROUPS resource‑type behavior; consider reducing reliance on non‑existent group IDs.This test suite gives solid end‑to‑end coverage for the new
GROUPSresource type and scopes (VIEW, MANAGE, VIEW_MEMBERS, MANAGE_MEMBERS, MANAGE_GROUP_MEMBERSHIP), including fallback from per‑group to all‑groups permissions and interplay with USERS resource‑type. One minor robustness tweak would be intestManageGroupMembership: usingtopGroup.getId()instead of hard‑coded"no-such"for the initialjoinGroupcall would ensure the failure is purely authorization‑driven and not dependent on whether the implementation checks permissions before group existence.server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
328-350: Consider adding a GROUPS branch ingetResourceNamefor better admin UI labels.Currently
getResourceNameonly customizes naming for USERS resources; group resources will fall back toresource.getDisplayName(), which may end up being the raw group ID. To improve readability in the admin console for per‑group permissions, you could mirror the USERS handling for GROUPS, for example:@@ public String getResourceName(KeycloakSession session, Policy policy, Resource resource) { - if (supportsAuthorizationSchema(session, resourceServer)) { - String resourceType = policy.getResourceType(); - - if (USERS.getType().equals(resourceType)) { + if (supportsAuthorizationSchema(session, resourceServer)) { + String resourceType = policy.getResourceType(); + + if (USERS.getType().equals(resourceType)) { if (resource.getName().equals(USERS_RESOURCE_TYPE)) { return "All users"; } @@ - return user.getUsername(); - } + return user.getUsername(); + } + + if (GROUPS.getType().equals(resourceType)) { + if (resource.getName().equals(GROUPS_RESOURCE_TYPE)) { + return "All groups"; + } + + GroupModel group = session.groups().getGroupById(session.getContext().getRealm(), resource.getName()); + + if (group == null) { + throw new ModelIllegalStateException("Group not found for resource [" + resource.getId() + "]"); + } + + return group.getName(); + } }This keeps schema semantics unchanged while giving administrators more meaningful labels.
📜 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)
- 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
🔇 Additional comments (21)
test-framework/core/src/main/java/org/keycloak/testframework/util/ApiUtil.java (1)
9-13: LGTM! Clean resource management with try-with-resources.The refactor to use try-with-resources ensures the Response is properly closed even if the assertion fails or an exception occurs during path extraction.
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (1)
32-98: New group permission API surface looks coherentThe expanded set of
can*/require*methods for listing, managing, and viewing groups is consistent and expressive, and the Javadoc correctly ties legacyAdminRolesto FGAP (AdminPermissionsSchema) permissions. This should make call sites much clearer compared to the previous single entry point.services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java (2)
206-206: Good cleanup removing unnecessary null initialization.The variable
childis always assigned before use (either at line 208 or line 217), so the null initialization was unnecessary. This cleanup improves code clarity.
179-179: Method referenceauth.groups()::canViewis valid.Verification confirms the
canView(GroupModel group)method exists in the GroupPermissionEvaluator interface (line 65) with implementations in both GroupPermissions (line 254) and GroupPermissionsV2 (line 56). The method signature is correct for the intended per-subgroup authorization filtering.services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
74-98: Feature flag choice is intentional and correct.FGAP:V1 feature remains in preview and can be enabled using --features=admin-fine-grained-authz:v1, but V1 may be deprecated and removed in future releases. FGAP:V2 feature is enabled by default.
The listener registration at line 74 uses the base
ADMIN_FINE_GRAINED_AUTHZflag (V1) by design: it handles cleanup events that apply to both V1 and V2 implementations. Themanagement()method internally selects which permission implementation to use based on the V2 flag (lines 67-68). This pattern allows both versions to coexist for backward compatibility while supporting the legacy token exchange features that currently depend on the base flag.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RealmAdminPermissionsConfig.java (1)
34-36: AddingQUERY_GROUPSto the test admin looks correctIncluding
AdminRoles.QUERY_GROUPSalongsideQUERY_USERSensures the test admin can exercise group-related queries under the new GROUPS resource type; this keeps the realm config aligned with the expanded authz schema.services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java (1)
27-31: Consistent lazy wiring ofGroupPermissionsV2The new
groupPermissionsfield andgroups()override follow the same lazy-init pattern asusers()/clients(), exposingGroupPermissionsV2behind theGroupPermissionsinterface without changing callers. This is a clean integration point for the new GROUPS resource handling.Also applies to: 57-62
services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java (1)
300-329: Confirm MANAGE_USERS-only fast-path semantics incanMapRoleSwitching the initial gate in
canMapRoleto:if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) return checkAdminRoles(role);means:
- Any admin with the
MANAGE_USERSadmin role short-circuits intocheckAdminRoles(role).- Admins without
MANAGE_USERSstill go through the full FGAP/resource permission evaluation as before.This replaces the earlier “default manage” gate (e.g.
users().canManageDefault()) with an explicit admin-role check. That’s likely aligned with the broader shift to role-centric admin checks, but it would be good to verify that there is no scenario wherecanManageDefaultcould be true while the admin does not have theMANAGE_USERSrole (e.g., legacy setups or composite roles), which would now lose the fast-path.services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java (1)
99-107: Good alignment of group search with session-backed APIs and evaluatorUsing
session.groups().searchGroupsByAttributes(...)andgetTopLevelGroupsStream(...)makes the data access path explicit and consistent with the group store, and the non-hierarchy branch now cleanly appliesgroupsEvaluator::canViewbefore building representations. This keeps permission checks and group retrieval nicely aligned with the newer GROUPS resource model.Also applies to: 109-115
services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java (3)
219-243: Role‑firstcanManage()implementation is sound and keeps cross‑realm behavior explicit.Short‑circuiting on
AdminRoles.MANAGE_USERSbefore consulting FGAP, and then blocking when!root.isAdminSameRealm(), keeps legacy role behavior intact while avoiding unnecessary policy evaluation. This is a clear and reasonable tightening of the manage‑all‑users check.
271-307: UpdatedcanQuery()/canView()logic aligns with the documented contract.
canQuery()delegating toQUERY_USERS || canView()andcanView()checkingMANAGE_USERS/VIEW_USERSroles before falling back tohasPermission(VIEW_SCOPE, MANAGE_SCOPE)(only when admin is in the same realm) matches the new interface javadocs and keeps the behavior easy to reason about.
582-590: Group hierarchy evaluation using method references is a clean, behavior‑preserving refactor.Switching to
evaluateHierarchy(user, root.groups()::canManageMembers)/::canViewMemberskeeps semantics identical to the prior lambdas while slightly improving readability.server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (2)
55-90: GROUPS resource type and scope definitions are consistent with existing CLIENTS/USERS schema.Defining
GROUPS_RESOURCE_TYPEwith its own scope set (MANAGE, VIEW, MANAGE_MEMBERSHIP, MANAGE_MEMBERS, VIEW_MEMBERS) and wiring it into theSCHEMAmap alongside CLIENTS and USERS cleanly models the new resource type and matches how the tests construct group permissions.
92-131: No issues found; existing callers pass group IDs as expected.Verification confirms that callers of
getOrCreateResourcepass group IDs from representation objects (AbstractPolicyRepresentation.getResources()and policy resource IDs), not group names or paths. Tests explicitly passgroup.getId(), and theresolveGroupimplementation correctly resolves by ID only. Additionally, both callers have fallback logic (findById, thenfindByName) ifgetOrCreateResourcereturns null, providing a safety net for edge cases.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java (1)
19-165: Expanded interface and javadocs clearly define the V2 authorization contract for user operations.Documenting each method in terms of
AdminRolesandAdminPermissionsSchemascopes, and adding the missing requires/queries/impersonation/group‑membership variants, makes the expected behavior ofUserPermissionEvaluatormuch clearer and matches the updatedUserPermissionsimplementation.services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java (1)
445-460: No changes required—session attribute type change is compatible with existing consumers.Verification confirms that
UserModel.GROUPSis safely read byJpaUserProvider.java(line 762) with an explicit cast toSet<String>, which matches the newSet<String>group IDs being stored. Other consumers inUsersResource.javaandBruteForceUsersResource.javaonly check for null, which is type-agnostic. The change is already compatible with the codebase.services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (3)
55-69: Making auth-related stores protected for subclassing looks fineExposing
root,resourceStore, andpolicyStoreasprotected finalenablesGroupPermissionsV2without changing construction semantics; null handling forauthzis preserved. No issues from a correctness perspective.
147-150: Global list/view/manage gating is now role-centric; confirm intended behavior
canList()now allows listing only if the admin hasQUERY_GROUPSor global user view/manage viacanView(), andcanManage()/canView()rely purely onMANAGE_USERS/VIEW_USERS. This tightens global access, leaving granular checks to the group-scoped variants. Behavior looks consistent, but it will change who can list/manage/view groups compared to earlier broader checks, so regression tests and docs should be updated accordingly.Please double-check any callers that previously relied on non-
MANAGE_USERS/VIEW_USERSroles for global group access and ensure tests cover those flows.Also applies to: 273-276, 285-288
297-320: Group ID derivation and member-view semantics are consistent with V1 design
getGroupIdsWithViewPermission()now returns group IDs by scanning"Group"-typed resources and stripping thegroup.resource.prefix, andcanViewMembers()/requireViewMembers()pivot entirely onview-members/manage-membersscopes (or globalusers().canView()). This matches the resource naming established ininitialize()and the intended FGAP semantics for member visibility. No functional issues spotted here.Also applies to: 323-341
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (2)
82-104: Per‑group manage/manage-membership semantics look aligned with FGAP scopesThe per‑group overrides:
canManage(GroupModel)→MANAGE_USERSrole orMANAGEscope for that group.canViewMembers(GroupModel)→users().canView()orVIEW_MEMBERS/MANAGE_MEMBERS.canManageMembers(GroupModel)→users().canManage()orMANAGE_MEMBERS.canManageMembership(GroupModel)→MANAGE_USERSrole orMANAGE/MANAGE_MEMBERSHIP.These follow a consistent pattern of “role shortcut first, then scope-based permission”. No obvious correctness issues here.
It would still be good to have tests that explicitly cover combinations of
VIEW,MANAGE,VIEW_MEMBERS,MANAGE_MEMBERS, andMANAGE_MEMBERSHIPon both all-groups and per-group resources.
167-210: UnsupportedOperationException stubs are acceptable but require call‑site awarenessAll management-style methods (
isPermissionsEnabled,setPermissionsEnabled,viewMembersPermission,manageMembersPermission,manageMembershipPermission,viewPermission,managePermission,resource,getPermissions) are explicitly unsupported in V2 and throwUnsupportedOperationException.That’s fine if V2 usage is strictly limited to the new FGAP flows and no existing endpoints rely on these methods. If any old code paths are switched from
GroupPermissionstoGroupPermissionsV2, you may see unexpected 500s instead of permission checks.Consider grepping for these methods on
GroupPermissionEvaluator/GroupPermissionManagementusages to ensure the V2 instance is never wired into call sites that expect these operations to work.
| /** | ||
| * 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); |
There was a problem hiding this comment.
Javadoc for requireManageMembers references the wrong checker
The Javadoc for requireManageMembers(GroupModel) currently links to canManageMembership(GroupModel) instead of canManageMembers(GroupModel), which is likely a copy‑paste error and can confuse callers.
Please adjust the Javadoc to reference the correct method:
- /**
- * 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.
| /** | |
| * 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 {@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); |
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java
around lines 111-135, the Javadoc for requireManageMembers(GroupModel)
incorrectly links to canManageMembership(GroupModel); update the Javadoc link to
reference canManageMembers(GroupModel) instead so the documentation points to
the correct checker method.
| /** | ||
| * 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 / return type mismatch and ambiguous empty‑set semantics
Two issues here:
- The Javadoc says “Stream of IDs of groups” but the method returns a
Set<String>, which is misleading. - The note “If
UserPermissionEvaluator#canView()evaluates totrue, returns empty set” is non‑obvious: an empty set usually means “no groups”, not “all groups via a global permission”.
I suggest:
- Updating the Javadoc to say “Set of IDs of groups with view permission”.
- Explicitly documenting what an empty set means, e.g., “If
UserPermissionEvaluator#canView()evaluates totrue(global view), this method returns an empty set to signal that all groups are viewable.”
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java
around lines 142 to 147, the Javadoc is inconsistent and ambiguous: it claims to
return a "Stream of IDs of groups" while the method returns Set<String>, and the
note that an empty set means "global view" is unclear. Update the Javadoc to
state "Set of IDs of groups with view permission" and explicitly document the
empty-set semantics — e.g., "If UserPermissionEvaluator#canView() evaluates to
true (global view), this method returns an empty set to signal that all groups
are viewable." Ensure the @return tag matches the Set<String> return type and
clearly explains the special empty-set meaning.
| @Override | ||
| public boolean canView() { | ||
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS, AdminRoles.VIEW_USERS)) { | ||
| return true; | ||
| } | ||
|
|
||
| return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean canView(GroupModel group) { | ||
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS, AdminRoles.VIEW_USERS)) { | ||
| return true; | ||
| } | ||
|
|
||
| return hasPermission(group.getId(), AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); | ||
| } | ||
|
|
||
| @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.
🧩 Analysis chain
canManage() incorrectly treats VIEW scope as sufficient for manage in V2
canManage() falls back to hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE), so an admin with only FGAP VIEW scope over groups will be granted manage rights, which breaks the usual “manage ⊇ view” hierarchy.
Consider requiring only MANAGE for the non‑role path:
@Override
public boolean canManage() {
if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
return true;
}
- return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
+ // Global manage via FGAP must require MANAGE, not just VIEW
+ return hasPermission(null, AdminPermissionsSchema.MANAGE);
}This keeps canView() as (VIEW || MANAGE) while canManage() strictly requires MANAGE.
Please confirm this aligns with the intended AdminPermissionsSchema semantics for global group management.
I'll verify the critical issue by examining the hasPermission() implementation and comparing with V1 behavior.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find hasPermission method implementation in GroupPermissionsV2 or parent classes
rg -nA10 'hasPermission\s*\(' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
Length of output: 2664
🏁 Script executed:
#!/bin/bash
# Find the parent class and hasPermission in parent
rg -nA10 'class GroupPermissionsV2' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.javaLength of output: 342
🏁 Script executed:
#!/bin/bash
# Search for hasPermission in related permission classes
fd -t f 'Permissions.*\.java' services/src/main/java/org/keycloak/services/resources/admin/permissions/ | head -20Length of output: 1155
🏁 Script executed:
#!/bin/bash
# Look for AdminPermissionsSchema definition
fd -t f 'AdminPermissionsSchema' services/src/main/java/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find V1 GroupPermissions for comparison
fd -t f 'GroupPermissions\.java' services/src/main/java/Length of output: 94
I found a critical inconsistency. Let me get the full hasPermission() implementation and compare with V1:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full hasPermission implementation
rg -nA30 'private boolean hasPermission' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
Length of output: 1332
🏁 Script executed:
#!/bin/bash
# Compare: get V1 canManage() implementation
rg -nA10 'public boolean canManage\(\)' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.javaLength of output: 282
🏁 Script executed:
#!/bin/bash
# Find AdminPermissionsSchema constants
rg -rn 'AdminPermissionsSchema\.' services/src/main/java/org/keycloak/services/resources/admin/permissions/ | grep -E '(VIEW|MANAGE)\s*=' | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for AdminPermissionsSchema definition more broadly
rg -rn 'class AdminPermissionsSchema|interface AdminPermissionsSchema' services/Length of output: 0
canManage() incorrectly treats VIEW scope as sufficient for manage in V2
canManage() on line 70 falls back to hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE). Due to the OR logic in hasPermission() (which returns true if ANY scope matches), an admin with only FGAP VIEW scope will be granted manage rights, breaking the permission hierarchy.
The per-group canManage(GroupModel group) on line 79 correctly requires only MANAGE. The global version must align:
@Override
public boolean canManage() {
if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
return true;
}
- return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
+ return hasPermission(null, AdminPermissionsSchema.MANAGE);
}🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
around lines 46-72, canManage() currently falls back to hasPermission(null,
AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) which incorrectly
grants manage rights if VIEW is present; change the fallback to require only
MANAGE (i.e., call hasPermission(null, AdminPermissionsSchema.MANAGE)) so the
global canManage() matches the per-group canManage(GroupModel) behavior.
| @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.
🧩 Analysis chain
Resource ID vs name mismatch and returned IDs in getGroupIdsWithViewPermission
In getGroupIdsWithViewPermission():
resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
granted.add(groupResource.getId());
}
});but hasPermission(String groupId, ...) does:
Resource resource = groupId == null ? null : resourceStore.findByName(server, groupId);Two problems:
- You pass
groupResource.getId()(resource primary key) buthasPermissionlooks it up as a name. In Keycloak’s authorization model, resource IDs and names are distinct, so this will typically not resolve the per‑group resource and will immediately fall back to the “all‑groups” resource. getGroupIdsWithViewPermission()returnsgroupResource.getId(), i.e., resource IDs, whereas the method name and V1 implementation suggest it should return group model IDs (previously derived from resource name).
A more consistent approach is to use the resource’s name as the group ID and keep hasPermission working on that:
- resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
- if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
- granted.add(groupResource.getId());
- }
- });
+ resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
+ // In the GROUPS resource type, the resource name should map to the GroupModel ID
+ String groupId = groupResource.getName();
+ if (hasPermission(groupId, AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
+ granted.add(groupId);
+ }
+ });This aligns lookup in hasPermission() with the naming scheme and preserves the contract of returning group IDs rather than resource IDs.
Please confirm that for GROUPS_RESOURCE_TYPE resources the name indeed stores the GroupModel.getId(). If not, adjust both lookup and returned value to use whatever field is canonically mapped to the group ID.
Also applies to: 130-165
I'll help you verify this review comment by examining the codebase. Let me search for the relevant code patterns and implementations.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the hasPermission() method in the same file
grep -n "hasPermission" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java | head -20
Length of output: 927
🏁 Script executed:
#!/bin/bash
# Search for the full hasPermission method implementation
rg -n -A10 "private.*hasPermission\(" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.javaLength of output: 318
🏁 Script executed:
#!/bin/bash
# Look for GROUPS_RESOURCE_TYPE definition and how resources are created
rg -n "GROUPS_RESOURCE_TYPE" services/src/main/java/org/keycloak/services/resources/admin/permissions/ -A3 -B3Length of output: 2227
🏁 Script executed:
#!/bin/bash
# Examine lines 130-165 mentioned in the review
sed -n '130,165p' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.javaLength of output: 1261
🏁 Script executed:
#!/bin/bash
# Check for V1 implementation to compare
find services/src/main/java/org/keycloak/services/resources/admin/permissions/ -name "*GroupPermissionsV1*" -o -name "*GroupPermissions*" | grep -v V2Length of output: 94
---
🏁 Script executed:
```shell
#!/bin/bash
# Look at V1 implementation of similar method
rg -n "getGroupIdsWithViewPermission" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java -A25
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Check all calls to hasPermission in V2 to see pattern
rg -n "hasPermission\(" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java -B2Length of output: 1281
🏁 Script executed:
#!/bin/bash
# Look for how group resources are created/named in AdminPermissionsSchema
rg -n "GROUPS_RESOURCE_TYPE\|setGroupResourceName\|group.*resource.*name" services/src/main/java/org/keycloak/services/resources/admin/permissions/ -B2 -A2 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for resource creation with group ID
rg -n "resourceStore.create\|Resource.*group" services/src/main/java/org/keycloak/services/resources/admin/permissions/ -B2 -A3 | grep -i group -A3 -B3Length of output: 0
Use group ID instead of resource ID in getGroupIdsWithViewPermission()
The issue is confirmed. V1 implementation returns resource.getName().substring(RESOURCE_NAME_PREFIX.length()) (the group ID extracted from resource name), while V2 at line 122 incorrectly passes groupResource.getId() (the resource primary key) to hasPermission(), which expects to find a resource by name. All other method calls in V2 consistently pass group.getId(), and hasPermission() calls resourceStore.findByName(server, groupId) at line 141.
The fix is:
resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
- if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
- granted.add(groupResource.getId());
+ String groupId = groupResource.getName();
+ if (hasPermission(groupId, AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
+ granted.add(groupId);
}
});This aligns with V1's extraction of group ID from resource name and ensures hasPermission() can resolve the resource by name.
📝 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 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 -> { | |
| String groupId = groupResource.getName(); | |
| if (hasPermission(groupId, AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | |
| granted.add(groupId); | |
| } | |
| }); | |
| return granted; | |
| } |
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
around lines 105-128, the code passes groupResource.getId() (the resource
primary key) into hasPermission(), but hasPermission() expects the group ID
encoded in the resource name; replace that call by extracting the groupId from
groupResource.getName() (strip the RESOURCE_NAME_PREFIX length like V1 does) and
pass that groupId into hasPermission(..., VIEW_MEMBERS, MANAGE_MEMBERS) so the
resourceStore.findByName(server, groupId) can resolve the resource correctly.
2 participants
Test 6
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#6