From d46e00b2d68933a4570f5b5278115dd087b5d972 Mon Sep 17 00:00:00 2001 From: vramik Date: Fri, 17 Jul 2026 11:16:02 +0200 Subject: [PATCH] User partial filters ignore ancestor group membership denies Closes #51158 Signed-off-by: vramik --- .../jpa/JpaUserPartialEvaluationProvider.java | 37 +++++++++++++++++-- .../models/jpa/entities/GroupEntity.java | 1 + .../fgap/UserResourceTypeFilteringTest.java | 35 ++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserPartialEvaluationProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserPartialEvaluationProvider.java index 874e9a2dac3c..9350fa8fa717 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserPartialEvaluationProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserPartialEvaluationProvider.java @@ -18,6 +18,7 @@ package org.keycloak.models.jpa; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -99,7 +100,7 @@ private Predicate getAllowedGroupFilters(PartialEvaluationContext context) { return cb.exists(createUserMembershipSubquery(context)); } - return cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(allowedGroups))); + return cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(expandGroupsToDescendants(allowedGroups)))); } private Predicate getDeniedGroupsFilters(PartialEvaluationContext context) { @@ -123,11 +124,11 @@ private Predicate getDeniedGroupsFilters(PartialEvaluationContext context) { return notMembers; } - Predicate onlySpecificGroups = cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(allowedGroups))); + Predicate onlySpecificGroups = cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(expandGroupsToDescendants(allowedGroups)))); return cb.and(cb.or(notMembers, onlySpecificGroups)); } - return cb.not(cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(context.getDeniedGroupIds())))); + return cb.not(cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(expandGroupsToDescendants(context.getDeniedGroupIds()))))); } if (context.getAllowedResources().isEmpty() && (allowedGroups.isEmpty() || context.deniedResources().contains(USERS_RESOURCE_TYPE))) { @@ -138,7 +139,7 @@ private Predicate getDeniedGroupsFilters(PartialEvaluationContext context) { return null; } - return cb.not(cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(deniedGroups)))); + return cb.not(cb.exists(createUserMembershipSubquery(context, root -> root.get("groupId").in(expandGroupsToDescendants(deniedGroups))))); } private Subquery createUserMembershipSubquery(PartialEvaluationContext context) { @@ -168,6 +169,34 @@ private Subquery createUserMembershipSubquery(PartialEvaluationContext contex return subquery; } +// One query per level of group nesting (N + 1 queries). For typical Keycloak deployments (2-4 levels deep) this is fine, but worth noting. +// A recursive CTE would be a single query, though it isn't used elsewhere and portability across DB vendors might be a concern. The iterative +// approach is pragmatic here. + private Set expandGroupsToDescendants(Set groupIds) { + if (groupIds.isEmpty()) return groupIds; + + EntityManager em = getEntityManager(); + String realmId = getSession().getContext().getRealm().getId(); + Set expanded = new HashSet<>(groupIds); + Set currentLevel = new HashSet<>(groupIds); + + while (!currentLevel.isEmpty()) { + List children = em.createNamedQuery("getChildGroupIdsByParentIds", String.class) + .setParameter("realm", realmId) + .setParameter("parentIds", currentLevel) + .getResultList(); + + currentLevel = new HashSet<>(); + for (String childId : children) { + if (expanded.add(childId)) { + currentLevel.add(childId); + } + } + } + + return expanded; + } + /** * @deprecated remove once FGAP v1 is removed */ diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java index 686cc3e6409e..ea0eea73f244 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java @@ -43,6 +43,7 @@ */ @NamedQueries({ @NamedQuery(name="getGroupIdsByParent", query="select u.id from GroupEntity u where u.realm = :realm and u.type = 0 and u.parentId = :parent order by u.name ASC"), + @NamedQuery(name="getChildGroupIdsByParentIds", query="select g.id from GroupEntity g where g.realm = :realm and g.parentId in :parentIds"), @NamedQuery(name="deleteGroupsByRealm", query="delete from GroupEntity g where g.realm = :realm") }) @Entity 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..33e993196417 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 @@ -568,6 +570,39 @@ public void testRoleMemberFilteringByViewPermission() { assertThat(roleMembers, hasItems(allowedUsers.toArray(new String[0]))); } + @Test + public void testParentGroupDenyExcludesChildMemberFromSearch() { + 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-denied-parent-" + KeycloakModelUtils.generateId()); + GroupRepresentation childGroup = new GroupRepresentation(); + childGroup.setName("fgap-denied-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)); + } + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + realm.admin().users().get(deniedUser.getId()).joinGroup(childGroup.getId()); + + UserPolicyRepresentation denyMyAdmin = createUserPolicy( + Logic.NEGATIVE, realm, adminPermissionsClient, "Deny My Admin User Policy", myadmin.getId()); + createPermission(adminPermissionsClient, parentGroup.getId(), GROUPS_RESOURCE_TYPE, Set.of(VIEW_MEMBERS), denyMyAdmin); + + 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 testViewGroupMembersPolicyUsingAggregatedPolicy() { List search = realmAdminClient.realm(realm.getName()).users().search(null, 0, 10);