From cc2bb96abca44fe575ebd0a0022fd3f528ba8e65 Mon Sep 17 00:00:00 2001 From: Christoph Leistert Date: Thu, 17 Mar 2022 10:50:24 +0100 Subject: [PATCH] Fixes #9482: A user could be assigned to a parent group if he is already assigned to a subgroup. --- .../group/GroupLDAPStorageMapper.java | 2 +- .../org/keycloak/models/jpa/UserAdapter.java | 2 +- .../models/map/user/MapUserAdapter.java | 1 + .../UpdateOnlyChangeUserModelDelegate.java | 5 +- .../org/keycloak/models/utils/RoleUtils.java | 10 +++ .../resources/admin/UserResource.java | 3 +- .../keycloak/testsuite/admin/UserTest.java | 76 ++++++++++++++++++- 7 files changed, 93 insertions(+), 6 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java index 66328fcbec..b0748bb18f 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java @@ -751,7 +751,7 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements @Override public boolean isMemberOf(GroupModel group) { - return getGroupsStream().anyMatch(Predicate.isEqual(group)); + return RoleUtils.isDirectMember(getGroupsStream(),group); } protected Stream getLDAPGroupMappingsConverted() { diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java index 6387b0be64..bca6113c21 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java @@ -392,7 +392,7 @@ public class UserAdapter implements UserModel.Streams, JpaModel { @Override public void joinGroup(GroupModel group) { - if (isMemberOf(group)) return; + if (RoleUtils.isDirectMember(getGroupsStream(), group)) return; joinGroupImpl(group); } diff --git a/model/map/src/main/java/org/keycloak/models/map/user/MapUserAdapter.java b/model/map/src/main/java/org/keycloak/models/map/user/MapUserAdapter.java index 2fb8185fea..ae797199c5 100644 --- a/model/map/src/main/java/org/keycloak/models/map/user/MapUserAdapter.java +++ b/model/map/src/main/java/org/keycloak/models/map/user/MapUserAdapter.java @@ -255,6 +255,7 @@ public abstract class MapUserAdapter extends AbstractUserModel { @Override public void joinGroup(GroupModel group) { + if (RoleUtils.isDirectMember(getGroupsStream(), group)) return; entity.addGroupsMembership(group.getId()); } diff --git a/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java b/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java index 7c3a556f6c..dd97c08dac 100644 --- a/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java +++ b/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java @@ -25,6 +25,7 @@ import java.util.stream.Collectors; import org.keycloak.models.GroupModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; +import org.keycloak.models.utils.RoleUtils; import org.keycloak.models.utils.UserModelDelegate; import static org.keycloak.common.util.ObjectUtil.isEqualOrBothNull; @@ -170,7 +171,7 @@ public class UpdateOnlyChangeUserModelDelegate extends UserModelDelegate { @Override public void joinGroup(GroupModel group) { - if (!isMemberOf(group)) { + if (!RoleUtils.isDirectMember(getGroupsStream(),group)) { delegate.joinGroup(group); } @@ -178,7 +179,7 @@ public class UpdateOnlyChangeUserModelDelegate extends UserModelDelegate { @Override public void leaveGroup(GroupModel group) { - if (isMemberOf(group)) { + if (RoleUtils.isDirectMember(getGroupsStream(),group)) { delegate.leaveGroup(group); } } diff --git a/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java b/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java index 75180a87a3..496b621ab3 100644 --- a/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java +++ b/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java @@ -80,6 +80,16 @@ public class RoleUtils { }); } + /** + * + * @param groups + * @param targetGroup + * @return true if targetGroup is in groups directly + */ + public static boolean isDirectMember(Stream groups, GroupModel targetGroup) { + return groups.anyMatch(g -> targetGroup.getId().equals(g.getId())); + } + /** * @param roles * @param targetRole diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index fae88542e7..fdf972611b 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -50,6 +50,7 @@ import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.models.utils.RepresentationToModel; +import org.keycloak.models.utils.RoleUtils; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.utils.RedirectUtils; import org.keycloak.provider.ProviderFactory; @@ -943,7 +944,7 @@ public class UserResource { throw new NotFoundException("Group not found"); } auth.groups().requireManageMembership(group); - if (!user.isMemberOf(group)){ + if (!RoleUtils.isDirectMember(user.getGroupsStream(),group)){ user.joinGroup(group); adminEvent.operation(OperationType.CREATE).resource(ResourceType.GROUP_MEMBERSHIP).representation(ModelToRepresentation.toRepresentation(group, true)).resourcePath(session.getContext().getUri()).success(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java index da948efe76..436ad4bef5 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -3057,4 +3057,78 @@ public class UserTest extends AbstractAdminTest { testRealm().roles().get("realm-role").remove(); } } -} \ No newline at end of file + + /** + * Test for #9482 + */ + @Test + public void joinParentGroupAfterSubGroup() { + String username = "user-with-sub-and-parent-group"; + String parentGroupName = "parent-group"; + String subGroupName = "sub-group"; + + UserRepresentation userRepresentation = UserBuilder.create().username(username).build(); + + GroupRepresentation subGroupRep = GroupBuilder.create().name(subGroupName).build(); + GroupRepresentation parentGroupRep = GroupBuilder.create().name(parentGroupName).subGroups(List.of(subGroupRep)).build(); + + try (Creator u = Creator.create(realm, userRepresentation); + Creator subgroup = Creator.create(realm, subGroupRep); + Creator parentGroup = Creator.create(realm, parentGroupRep)) { + + UserResource user = u.resource(); + + //when + user.joinGroup(subgroup.id()); + List obtainedGroups = realm.users().get(u.id()).groups(); + + //then + assertEquals(1, obtainedGroups.size()); + assertEquals(subGroupName, obtainedGroups.get(0).getName()); + + //when + user.joinGroup(parentGroup.id()); + obtainedGroups = realm.users().get(u.id()).groups(); + + //then + assertEquals(2, obtainedGroups.size()); + assertEquals(parentGroupName, obtainedGroups.get(0).getName()); + assertEquals(subGroupName, obtainedGroups.get(1).getName()); + } + } + + @Test + public void joinSubGroupAfterParentGroup() { + String username = "user-with-sub-and-parent-group"; + String parentGroupName = "parent-group"; + String subGroupName = "sub-group"; + + UserRepresentation userRepresentation = UserBuilder.create().username(username).build(); + GroupRepresentation subGroupRep = GroupBuilder.create().name(subGroupName).build(); + GroupRepresentation parentGroupRep = GroupBuilder.create().name(parentGroupName).subGroups(List.of(subGroupRep)).build(); + + try (Creator u = Creator.create(realm, userRepresentation); + Creator subgroup = Creator.create(realm, subGroupRep); + Creator parentGroup = Creator.create(realm, parentGroupRep)) { + + UserResource user = u.resource(); + + //when + user.joinGroup(parentGroup.id()); + List obtainedGroups = realm.users().get(u.id()).groups(); + + //then + assertEquals(1, obtainedGroups.size()); + assertEquals(parentGroupName, obtainedGroups.get(0).getName()); + + //when + user.joinGroup(subgroup.id()); + obtainedGroups = realm.users().get(u.id()).groups(); + + //then + assertEquals(2, obtainedGroups.size()); + assertEquals(parentGroupName, obtainedGroups.get(0).getName()); + assertEquals(subGroupName, obtainedGroups.get(1).getName()); + } + } +}