From b2af6d6aa260729aca0c3678a0cbb1422d72c6eb Mon Sep 17 00:00:00 2001 From: vramik Date: Fri, 17 Jul 2026 11:26:29 +0200 Subject: [PATCH] Partial evaluation misses ancestor group policies with extendChildren Closes #51144 Signed-off-by: vramik --- .../provider/group/GroupPolicyProvider.java | 15 +++- .../fgap/UserResourceTypeFilteringTest.java | 86 +++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java index d76eb749072d..a3363698e76a 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java @@ -16,6 +16,7 @@ */ package org.keycloak.authorization.policy.provider.group; +import java.util.ArrayList; import java.util.List; import java.util.function.BiFunction; import java.util.stream.Collectors; @@ -111,7 +112,19 @@ public Stream getPermissions(KeycloakSession session, ResourceType resou StoreFactory storeFactory = provider.getStoreFactory(); ResourceServer resourceServer = storeFactory.getResourceServerStore().findByClient(adminPermissionsClient); PolicyStore policyStore = storeFactory.getPolicyStore(); - List groupIds = user.getGroupsStream().map(GroupModel::getId).toList(); + // getParent() issues a query per ancestor; may need optimization for deep hierarchies + List groupIds = user.getGroupsStream() + .flatMap(group -> { + List ids = new ArrayList<>(); + GroupModel current = group; + while (current != null) { + ids.add(current.getId()); + current = current.getParent(); + } + return ids.stream(); + }) + .distinct() + .toList(); return policyStore.findDependentPolicies(resourceServer, resourceType.getType(), groupResourceType == null ? null : groupResourceType.getType(), GroupPolicyProviderFactory.ID, "groups", groupIds); } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java index 060a2323882a..2e6df16e4a76 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java @@ -23,6 +23,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.client.Client; import jakarta.ws.rs.client.WebTarget; import jakarta.ws.rs.core.GenericType; @@ -78,6 +79,7 @@ import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @KeycloakIntegrationTest @@ -466,6 +468,90 @@ public void testViewUserUsingRoleInheritedFromCompositeRole() { assertEquals(1, search.size()); } + @Test + public void testExtendChildrenGroupDenyExcludesDeniedUserFromSearch() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation allowMyAdmin = createUserPolicy( + realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId()); + createAllPermission(adminPermissionsClient, usersType, allowMyAdmin, Set.of(VIEW)); + + GroupRepresentation parentGroup = createGroup("fgap-parent-" + KeycloakModelUtils.generateId()); + GroupRepresentation childGroup = new GroupRepresentation(); + childGroup.setName("fgap-child-" + KeycloakModelUtils.generateId()); + try (Response response = realm.admin().groups().group(parentGroup.getId()).subGroup(childGroup)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + childGroup.setId(ApiUtil.getCreatedId(response)); + } + realm.admin().users().get(myadmin.getId()).joinGroup(childGroup.getId()); + + GroupPolicyRepresentation denyParentSubtree = new GroupPolicyRepresentation(); + denyParentSubtree.setName("Deny Parent Subtree Policy"); + denyParentSubtree.setLogic(Logic.NEGATIVE); + denyParentSubtree.addGroup(parentGroup.getId(), true); + try (Response response = adminPermissionsClient.authorization().policies().group() + .create(denyParentSubtree)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + } + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), denyParentSubtree); + + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(deniedUser.getUsername(), 0, 10); + assertThat(search.stream().map(UserRepresentation::getId).toList(), + not(hasItems(deniedUser.getId()))); + assertThat(realmAdminClient.realm(realm.getName()).users() + .count(null, null, null, deniedUser.getUsername()), is(0)); + } + + @Test + public void testExtendChildrenGroupDenyThreeLevelHierarchy() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation allowMyAdmin = createUserPolicy( + realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId()); + createAllPermission(adminPermissionsClient, usersType, allowMyAdmin, Set.of(VIEW)); + + GroupRepresentation grandparentGroup = createGroup("fgap-grandparent-" + KeycloakModelUtils.generateId()); + GroupRepresentation parentGroup = new GroupRepresentation(); + parentGroup.setName("fgap-parent-" + KeycloakModelUtils.generateId()); + try (Response response = realm.admin().groups().group(grandparentGroup.getId()).subGroup(parentGroup)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + parentGroup.setId(ApiUtil.getCreatedId(response)); + } + GroupRepresentation childGroup = new GroupRepresentation(); + childGroup.setName("fgap-child-" + KeycloakModelUtils.generateId()); + try (Response response = realm.admin().groups().group(parentGroup.getId()).subGroup(childGroup)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + childGroup.setId(ApiUtil.getCreatedId(response)); + } + realm.admin().users().get(myadmin.getId()).joinGroup(childGroup.getId()); + + GroupPolicyRepresentation denyGrandparentSubtree = new GroupPolicyRepresentation(); + denyGrandparentSubtree.setName("Deny Grandparent Subtree Policy"); + denyGrandparentSubtree.setLogic(Logic.NEGATIVE); + denyGrandparentSubtree.addGroup(grandparentGroup.getId(), true); + try (Response response = adminPermissionsClient.authorization().policies().group() + .create(denyGrandparentSubtree)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + } + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), denyGrandparentSubtree); + + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(deniedUser.getUsername(), 0, 10); + assertThat(search.stream().map(UserRepresentation::getId).toList(), + not(hasItems(deniedUser.getId()))); + assertThat(realmAdminClient.realm(realm.getName()).users() + .count(null, null, null, deniedUser.getUsername()), is(0)); + } + @Test public void testSessionEndpointRespectsUserViewPermission() { UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0);