From 4bc11bdf7f925ce83ff6b996531328888efa91da Mon Sep 17 00:00:00 2001 From: rmartinc Date: Tue, 27 Jun 2023 16:07:26 +0200 Subject: [PATCH] Do not return an error when moving a group to the current parent Closes https://github.com/keycloak/keycloak/issues/21242 --- .../resources/admin/GroupResource.java | 66 ++++++++++--------- .../resources/admin/GroupsResource.java | 4 +- .../testsuite/admin/group/GroupTest.java | 11 ++++ 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java b/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java index f5a9556649..1659c7a7df 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java @@ -24,6 +24,7 @@ import org.keycloak.events.admin.ResourceType; import org.keycloak.models.Constants; import org.keycloak.models.GroupModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.ModelToRepresentation; @@ -31,7 +32,6 @@ import org.keycloak.representations.idm.GroupRepresentation; import org.keycloak.representations.idm.ManagementPermissionReference; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.services.ErrorResponse; -import org.keycloak.services.Urls; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; import org.keycloak.services.resources.admin.permissions.AdminPermissionManagement; import org.keycloak.services.resources.admin.permissions.AdminPermissions; @@ -156,39 +156,41 @@ public class GroupResource { if (ObjectUtil.isBlank(groupName)) { throw ErrorResponse.error("Group name is missing", Response.Status.BAD_REQUEST); } - boolean childExists = group.getSubGroupsStream().anyMatch(s -> Objects.equals(s.getName(), groupName)); - if (childExists) { + + try { + Response.ResponseBuilder builder = Response.status(204); + GroupModel child = null; + if (rep.getId() != null) { + child = realm.getGroupById(rep.getId()); + if (child == null) { + throw new NotFoundException("Could not find child by id"); + } + if (!Objects.equals(child.getParentId(), group.getId())) { + realm.moveGroup(child, group); + } + adminEvent.operation(OperationType.UPDATE); + } else { + child = realm.createGroup(groupName, group); + updateGroup(rep, child, realm, session); + URI uri = session.getContext().getUri().getBaseUriBuilder() + .path(AdminRoot.class) + .path(AdminRoot.class, "getRealmsAdmin") + .path(RealmsAdminResource.class, "getRealmAdmin") + .path(RealmAdminResource.class, "getGroups") + .path(GroupsResource.class, "getGroupById") + .build(realm.getName(), child.getId()); + builder.status(201).location(uri); + rep.setId(child.getId()); + adminEvent.operation(OperationType.CREATE); + + } + adminEvent.resourcePath(session.getContext().getUri()).representation(rep).success(); + + GroupRepresentation childRep = ModelToRepresentation.toGroupHierarchy(child, true); + return builder.type(MediaType.APPLICATION_JSON_TYPE).entity(childRep).build(); + } catch (ModelDuplicateException e) { throw ErrorResponse.exists("Sibling group named '" + groupName + "' already exists."); } - - Response.ResponseBuilder builder = Response.status(204); - GroupModel child = null; - if (rep.getId() != null) { - child = realm.getGroupById(rep.getId()); - if (child == null) { - throw new NotFoundException("Could not find child by id"); - } - realm.moveGroup(child, group); - adminEvent.operation(OperationType.UPDATE); - } else { - child = realm.createGroup(groupName, group); - updateGroup(rep, child, realm, session); - URI uri = session.getContext().getUri().getBaseUriBuilder() - .path(AdminRoot.class) - .path(AdminRoot.class, "getRealmsAdmin") - .path(RealmsAdminResource.class, "getRealmAdmin") - .path(RealmAdminResource.class, "getGroups") - .path(GroupsResource.class, "getGroupById") - .build(realm.getName(), child.getId()); - builder.status(201).location(uri); - rep.setId(child.getId()); - adminEvent.operation(OperationType.CREATE); - - } - adminEvent.resourcePath(session.getContext().getUri()).representation(rep).success(); - - GroupRepresentation childRep = ModelToRepresentation.toGroupHierarchy(child, true); - return builder.type(MediaType.APPLICATION_JSON_TYPE).entity(childRep).build(); } public static void updateGroup(GroupRepresentation rep, GroupModel model, RealmModel realm, KeycloakSession session) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java index 5e59413fea..f191791b4e 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java @@ -157,7 +157,9 @@ public class GroupsResource { if (child == null) { throw new NotFoundException("Could not find child by id"); } - realm.moveGroup(child, null); + if (child.getParentId() != null) { + realm.moveGroup(child, null); + } adminEvent.operation(OperationType.UPDATE).resourcePath(session.getContext().getUri()); } else { child = realm.createGroup(groupName); 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 70cc2bdaa0..2d747ae62e 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 @@ -183,10 +183,16 @@ public class GroupTest extends AbstractGroupTest { assertSameNameNotAllowed(response,"Top level group named 'top' already exists."); response.close(); + // allow moving the group to top level (nothing is done) + response = realm.groups().add(topGroup); + assertEquals(Response.Status.NO_CONTENT, response.getStatusInfo()); + response.close(); + GroupRepresentation level2Group = new GroupRepresentation(); level2Group.setName("level2"); response = realm.groups().group(topGroup.getId()).subGroup(level2Group); assertEquals(201, response.getStatus()); // created status + level2Group.setId(ApiUtil.getCreatedId(response)); response.close(); GroupRepresentation anotherlevel2Group = new GroupRepresentation(); @@ -194,6 +200,11 @@ public class GroupTest extends AbstractGroupTest { response = realm.groups().group(topGroup.getId()).subGroup(anotherlevel2Group); assertSameNameNotAllowed(response,"Sibling group named 'level2' already exists."); response.close(); + + // allow moving the group to the same parent (nothing is done) + response = realm.groups().group(topGroup.getId()).subGroup(level2Group); + assertEquals(Response.Status.NO_CONTENT, response.getStatusInfo()); + response.close(); } @Test