From 17da3ee8d94784bbf201824b813528f51ae1679b Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Tue, 8 Jun 2021 10:53:20 +0200 Subject: [PATCH] KEYCLOAK-18380 Fix Groups search by name returns unwanted groups Previously the group search did not apply a given search query as filter for groups along the group path. We now filter the found groups with the given group search query if present. --- .../models/utils/ModelToRepresentation.java | 20 +++++- .../testsuite/admin/group/GroupTest.java | 61 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java index 45b6f20511..a076ed2109 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java @@ -38,6 +38,7 @@ import org.keycloak.provider.ProviderConfigProperty; import org.keycloak.representations.idm.*; import org.keycloak.representations.idm.authorization.*; import org.keycloak.storage.StorageId; +import org.keycloak.utils.StringUtil; import java.util.ArrayList; import java.util.HashMap; @@ -144,7 +145,7 @@ public class ModelToRepresentation { public static Stream searchForGroupByName(RealmModel realm, boolean full, String search, Integer first, Integer max) { return realm.searchForGroupByNameStream(search, first, max) - .map(g -> toGroupHierarchy(g, full)); + .map(g -> toGroupHierarchy(g, full, search)); } public static Stream searchForGroupByName(UserModel user, boolean full, String search, Integer first, Integer max) { @@ -173,13 +174,28 @@ public class ModelToRepresentation { } public static GroupRepresentation toGroupHierarchy(GroupModel group, boolean full) { + return toGroupHierarchy(group, full, null); + } + + public static GroupRepresentation toGroupHierarchy(GroupModel group, boolean full, String search) { GroupRepresentation rep = toRepresentation(group, full); List subGroups = group.getSubGroupsStream() - .map(subGroup -> toGroupHierarchy(subGroup, full)).collect(Collectors.toList()); + .filter(g -> groupMatchesSearchOrIsPathElement(g, search)) + .map(subGroup -> toGroupHierarchy(subGroup, full, search)).collect(Collectors.toList()); rep.setSubGroups(subGroups); return rep; } + private static boolean groupMatchesSearchOrIsPathElement(GroupModel group, String search) { + if (StringUtil.isBlank(search)) { + return true; + } + if (group.getName().contains(search)) { + return true; + } + return group.getSubGroupsStream().findAny().isPresent(); + } + public static UserRepresentation toRepresentation(KeycloakSession session, RealmModel realm, UserModel user) { UserRepresentation rep = new UserRepresentation(); rep.setId(user.getId()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java index b1169d586c..3f1e3a14bd 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java @@ -60,6 +60,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.stream.Collectors; import javax.ws.rs.ClientErrorException; import javax.ws.rs.core.Response.Status; import static org.hamcrest.Matchers.*; @@ -839,6 +840,66 @@ public class GroupTest extends AbstractGroupTest { } } + /** + * Groups search with query returns unwanted groups + * @link https://issues.redhat.com/browse/KEYCLOAK-18380 + */ + @Test + public void searchForGroupsShouldOnlyReturnMatchingElementsOrIntermediatePaths() { + + /* + * /g1/g1.1-bubu + * /g1/g1.2-test1234 + * /g2-test1234 + * /g3/g3.1-test1234/g3.1.1 + */ + String needle = "test1234"; + GroupRepresentation g1 = GroupBuilder.create().name("g1").build(); + GroupRepresentation g1_1 = GroupBuilder.create().name("g1.1-bubu").build(); + GroupRepresentation g1_2 = GroupBuilder.create().name("g1.2-" + needle).build(); + GroupRepresentation g2 = GroupBuilder.create().name("g2-" + needle).build(); + GroupRepresentation g3 = GroupBuilder.create().name("g3").build(); + GroupRepresentation g3_1 = GroupBuilder.create().name("g3.1-" + needle).build(); + GroupRepresentation g3_1_1 = GroupBuilder.create().name("g3.1.1").build(); + + String realmName = AuthRealm.TEST; + RealmResource realm = adminClient.realms().realm(realmName); + + createGroup(realm, g1); + createGroup(realm, g2); + createGroup(realm, g3); + addSubGroup(realm, g1, g1_1); + addSubGroup(realm, g1, g1_2); + addSubGroup(realm, g3, g3_1); + addSubGroup(realm, g3_1, g3_1_1); + + try { + // we search for "test1234" and expect only /g1/g1.2-test1234, /g2-test1234 and /g3/g3.1-test1234 as a result + List result = realm.groups().groups(needle, 0, 100); + // ensure stable sorting to make tests pass + result = result.stream().sorted(Comparator.comparing(GroupRepresentation::getName)).collect(Collectors.toList()); + assertEquals(3, result.size()); + assertEquals("g1", result.get(0).getName()); + assertEquals(1, result.get(0).getSubGroups().size()); + assertEquals("g1.2-" + needle, result.get(0).getSubGroups().get(0).getName()); + assertEquals("g2-" + needle, result.get(1).getName()); + assertEquals("g3", result.get(2).getName()); + assertEquals(1, result.get(2).getSubGroups().size()); + assertEquals("g3.1-" + needle, result.get(2).getSubGroups().get(0).getName()); + } finally { + if (g1.getId() != null) { + realm.groups().group(g1.getId()).remove(); + } + + if (g2.getId() != null) { + realm.groups().group(g2.getId()).remove(); + } + + if (g3.getId() != null) { + realm.groups().group(g3.getId()).remove(); + } + } + } /** * Verifies that the role assigned to a user's group is correctly handled by Keycloak Admin endpoint.