From 24a819eabc6ba88f1259c852ebefdccf8a61f9ee Mon Sep 17 00:00:00 2001 From: Oluwatobi Mustapha Date: Mon, 9 Mar 2026 16:01:17 +0100 Subject: [PATCH] Fix FGAP deny evaluation for manage-group-membership Add the missing Users FGAP scope alias from manage-group-membership to Groups manage-membership so deny permissions on group members apply during user membership updates. Add a regression test covering a protected group member, an unrelated user, and the no-mutation postcondition after a forbidden request. Closes keycloak#46693 --- .../release_notes/topics/26_6_0.adoc | 7 ++ .../fine-grain-v2.adoc | 10 ++- .../fgap/AdminPermissionsSchema.java | 7 +- .../fgap/UserResourceTypeEvaluationTest.java | 66 +++++++++++-------- 4 files changed, 58 insertions(+), 32 deletions(-) diff --git a/docs/documentation/release_notes/topics/26_6_0.adoc b/docs/documentation/release_notes/topics/26_6_0.adoc index 45f54cf175b..f41857bda72 100644 --- a/docs/documentation/release_notes/topics/26_6_0.adoc +++ b/docs/documentation/release_notes/topics/26_6_0.adoc @@ -114,3 +114,10 @@ bin/kc.sh start --log="console,file" --log-file-rotation-enabled=false ---- For more information, see the https://www.keycloak.org/server/logging/file[File logging] guide. + += Fine-Grained Admin Permissions new Groups scope for user membership changes + +Fine-Grained Admin Permissions (FGAP) now includes a new `Groups` scope: `manage-membership-of-members`. + +This scope is now used as the group-side bridge for evaluating user-side `manage-group-membership` permissions based on a user's current group memberships. +The existing `manage-membership` scope keeps its current behavior for target group membership management operations. diff --git a/docs/documentation/server_admin/topics/admin-console-permissions/fine-grain-v2.adoc b/docs/documentation/server_admin/topics/admin-console-permissions/fine-grain-v2.adoc index 1924ad6c45b..2d4c1a4c29f 100644 --- a/docs/documentation/server_admin/topics/admin-console-permissions/fine-grain-v2.adoc +++ b/docs/documentation/server_admin/topics/admin-console-permissions/fine-grain-v2.adoc @@ -86,7 +86,7 @@ set of scopes: | *view* | Defines if a realm administrator can view users. This scope should be set whenever you want to make users available from queries. | `view-members` | *manage* | Defines if a realm administrator can manage users. | `manage-members` -| *manage-group-membership* | Defines if a realm administrator can assign or unassign users to/from groups. | None +| *manage-group-membership* | Defines if a realm administrator can assign or unassign users to/from groups. | `manage-membership-of-members` | *map-roles* | Defines if a realm administrator can assign or unassign roles to/from users. | None | *impersonate* | Defines if a realm administrator can impersonate other users. | `impersonate-members` | *reset-password* | Defines if a realm administrator can reset user passwords. If no permission with `reset-password` is found, it falls back to check `manage` scope. | None @@ -97,6 +97,11 @@ The user resource type has a strong relationship with some of the permissions yo users are members of groups and granting access to `view-members` or `manage-members` of a group should also allow a realm administrator to `view` and `manage` members of that group. +To assign or unassign a specific user to or from a group, both sides of the relationship are required: `manage-group-membership` +on the user and `manage-membership` on the target group. +The `manage-membership-of-members` scope can be used to grant or deny `manage-group-membership` based on the groups that +the target user currently belongs to. + [NOTE] ==== This feature does not support enforcing access to federated resource, however, this limitation is being considered @@ -125,6 +130,7 @@ set of management operations: This operation applies to any child group in the group hierarchy. This can be prevented by explicitly denying permission for specific subgroups. | *manage-membership* | Defines if a realm administrator can add or remove members from groups. +| *manage-membership-of-members* | Defines if a realm administrator can grant or deny `manage-group-membership` for members of a group. |=== ===== Clients Resource Type @@ -374,7 +380,7 @@ if a respective admin role is assigned to a realm administrator, permission eval It does not grant the ability to *view* clients. | *view-users* | A realm administrator can *view* all users and groups in the realm. | *manage-users* | A realm administrator can *view*, *map-roles*, *manage-group-membership* and *manage* all users in the realm, - as well as *view*, *manage-membership* and *manage* groups in the realm. + as well as *view*, *manage-membership*, *manage-membership-of-members* and *manage* groups in the realm. | *impersonation* | A realm administrator can *impersonate* all users in the realm. | *view-clients* | A realm administrator can *view* all clients in the realm. | *manage-clients* | A realm administrator can *view* and *manage* all clients and client scopes in the realm. diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/fgap/AdminPermissionsSchema.java b/server-spi-private/src/main/java/org/keycloak/authorization/fgap/AdminPermissionsSchema.java index 81e7e7dd407..67a7dae0046 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/fgap/AdminPermissionsSchema.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/fgap/AdminPermissionsSchema.java @@ -85,6 +85,7 @@ public class AdminPermissionsSchema extends AuthorizationSchema { // group specific scopes public static final String MANAGE_MEMBERSHIP = "manage-membership"; + public static final String MANAGE_MEMBERSHIP_OF_MEMBERS = "manage-membership-of-members"; public static final String MANAGE_MEMBERS = "manage-members"; public static final String VIEW_MEMBERS = "view-members"; public static final String IMPERSONATE_MEMBERS = "impersonate-members"; @@ -101,9 +102,9 @@ public class AdminPermissionsSchema extends AuthorizationSchema { public static final String MANAGE_GROUP_MEMBERSHIP = "manage-group-membership"; public static final ResourceType CLIENTS = new ResourceType(CLIENTS_RESOURCE_TYPE, Set.of(MANAGE, MAP_ROLES, MAP_ROLES_CLIENT_SCOPE, MAP_ROLES_COMPOSITE, VIEW)); - public static final ResourceType GROUPS = new ResourceType(GROUPS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, MANAGE_MEMBERSHIP, MANAGE_MEMBERS, VIEW_MEMBERS, IMPERSONATE_MEMBERS)); + public static final ResourceType GROUPS = new ResourceType(GROUPS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, MANAGE_MEMBERSHIP, MANAGE_MEMBERSHIP_OF_MEMBERS, MANAGE_MEMBERS, VIEW_MEMBERS, IMPERSONATE_MEMBERS)); public static final ResourceType ROLES = new ResourceType(ROLES_RESOURCE_TYPE, Set.of(MAP_ROLE, MAP_ROLE_CLIENT_SCOPE, MAP_ROLE_COMPOSITE)); - public static final ResourceType USERS = new ResourceType(USERS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, IMPERSONATE, MAP_ROLES, MANAGE_GROUP_MEMBERSHIP, RESET_PASSWORD), Map.of(VIEW, Set.of(VIEW_MEMBERS), MANAGE, Set.of(MANAGE_MEMBERS), IMPERSONATE, Set.of(IMPERSONATE_MEMBERS)), GROUPS.getType()); + public static final ResourceType USERS = new ResourceType(USERS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, IMPERSONATE, MAP_ROLES, MANAGE_GROUP_MEMBERSHIP, RESET_PASSWORD), Map.of(VIEW, Set.of(VIEW_MEMBERS), MANAGE, Set.of(MANAGE_MEMBERS), IMPERSONATE, Set.of(IMPERSONATE_MEMBERS), MANAGE_GROUP_MEMBERSHIP, Set.of(MANAGE_MEMBERSHIP_OF_MEMBERS)), GROUPS.getType()); private static final String SKIP_EVALUATION = "kc.authz.fgap.skip"; public static final AdminPermissionsSchema SCHEMA = new AdminPermissionsSchema(); @@ -533,4 +534,4 @@ public class AdminPermissionsSchema extends AuthorizationSchema { return Boolean.parseBoolean(session.getAttributeOrDefault(SKIP_EVALUATION, Boolean.FALSE.toString())); } -} \ No newline at end of file +} diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java index cc23e987752..98ae5650302 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java @@ -54,6 +54,8 @@ import org.junit.jupiter.api.Test; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.IMPERSONATE; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.MANAGE; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.MANAGE_GROUP_MEMBERSHIP; +import static org.keycloak.authorization.fgap.AdminPermissionsSchema.MANAGE_MEMBERSHIP; +import static org.keycloak.authorization.fgap.AdminPermissionsSchema.MANAGE_MEMBERSHIP_OF_MEMBERS; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.MAP_ROLES; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.RESET_PASSWORD; import static org.keycloak.authorization.fgap.AdminPermissionsSchema.VIEW; @@ -309,50 +311,60 @@ public class UserResourceTypeEvaluationTest extends AbstractPermissionTest { @Test public void testManageGroupMembership() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation allowMyAdminPermission = createUserPolicy(realm, client, "Only My Admin User Policy", myadmin.getId()); + GroupRepresentation target = createGroup("target"); + UserRepresentation userBob = createUser("bob"); + // myadmin shouldn't be able to manage group membership of the user just yet try { - realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup("no-such"); + realmAdminClient.realm(realm.getName()).users().get(userBob.getId()).joinGroup(target.getId()); fail("Expected Exception wasn't thrown."); } catch (Exception ex) { assertThat(ex, instanceOf(ForbiddenException.class)); } - //create all-users permission for "myadmin" (so that myadmin can add users into a group) - UserPolicyRepresentation policy = createUserPolicy(realm, client,"Only My Admin User Policy", realm.admin().users().search("myadmin").get(0).getId()); - ScopePermissionRepresentation allUsersPermission = createAllPermission(client, usersType, policy, Set.of(MANAGE_GROUP_MEMBERSHIP)); - - //check myadmin can manage membership using all-users permission + // only the user-side permission is not enough + createAllPermission(client, usersType, allowMyAdminPermission, Set.of(MANAGE_GROUP_MEMBERSHIP)); try { - realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup("no-such"); - fail("Expected Exception wasn't thrown."); - } catch (Exception ex) { - // expecting here NotFoundException: https://github.com/keycloak/keycloak/blob/b5c95e9f1c58bc500316dd5c0f2d3bb5e197ca99/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java#L1060 - assertThat(ex, instanceOf(NotFoundException.class)); - } - - // remove all-users permissions to test user-permission - allUsersPermission = getScopePermissionsResource(client).findByName(allUsersPermission.getName()); - getScopePermissionsResource(client).findById(allUsersPermission.getId()).remove(); - - // now myadmin cannot manage membership - try { - realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup("no-such"); + realmAdminClient.realm(realm.getName()).users().get(userBob.getId()).joinGroup(target.getId()); fail("Expected Exception wasn't thrown."); } catch (Exception ex) { assertThat(ex, instanceOf(ForbiddenException.class)); } - // create userPermission - createPermission(client, userAlice.getId(), usersType, Set.of(MANAGE_GROUP_MEMBERSHIP), policy); + // target group must allow membership changes + createAllPermission(client, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, allowMyAdminPermission, Set.of(MANAGE_MEMBERSHIP)); + realmAdminClient.realm(realm.getName()).users().get(userBob.getId()).joinGroup(target.getId()); + assertTrue(realm.admin().users().get(userBob.getId()).groups().stream().anyMatch(group -> target.getId().equals(group.getId()))); + } + + @Test + public void testManageGroupMembershipDeniedForProtectedGroupMembers() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation allowMyAdminPermission = createUserPolicy(realm, client, "Only My Admin User Policy", myadmin.getId()); + UserPolicyRepresentation denyMyAdminPermission = createUserPolicy(Logic.NEGATIVE, realm, client, "Not My Admin User Policy", myadmin.getId()); + GroupRepresentation vault = createGroup("vault"); + GroupRepresentation target = createGroup("target"); + UserRepresentation userBob = createUser("bob"); + + realm.admin().users().get(userAlice.getId()).joinGroup(vault.getId()); + + createAllPermission(client, usersType, allowMyAdminPermission, Set.of(MANAGE_GROUP_MEMBERSHIP)); + createAllPermission(client, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, allowMyAdminPermission, Set.of(MANAGE_MEMBERSHIP)); + createAllPermission(client, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, allowMyAdminPermission, Set.of(MANAGE_MEMBERSHIP_OF_MEMBERS)); + createPermission(client, vault.getId(), AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, Set.of(MANAGE_MEMBERSHIP_OF_MEMBERS), denyMyAdminPermission); + + realmAdminClient.realm(realm.getName()).users().get(userBob.getId()).joinGroup(target.getId()); + assertTrue(realm.admin().users().get(userBob.getId()).groups().stream().anyMatch(group -> target.getId().equals(group.getId()))); - //check myadmin can manage membership using individual permission try { - realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup("no-such"); + realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup(target.getId()); fail("Expected Exception wasn't thrown."); - } catch (Exception ex) { - // expecting here NotFoundException: https://github.com/keycloak/keycloak/blob/b5c95e9f1c58bc500316dd5c0f2d3bb5e197ca99/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java#L1060 - assertThat(ex, instanceOf(NotFoundException.class)); + } catch (ForbiddenException expected) { } + + assertFalse(realm.admin().users().get(userAlice.getId()).groups().stream().anyMatch(group -> target.getId().equals(group.getId()))); } @Test