KEYCLOAK-12579: LDAP groups duplicated during UI listing of user groups

olivierlemasle · Mar 11, 2020 · ad3b9fc · ad3b9fc
1 parent bc1146a
commit ad3b9fc
Show file tree

Hide file tree

Showing 17 changed files with 153 additions and 101 deletions.
diff --git a/.../main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java b/.../main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java
@@ -322,12 +322,11 @@ private void updateKeycloakGroupTreeEntry(RealmModel realm, GroupTreeResolver.Gr
             updateAttributesOfKCGroup(kcGroup, ldapGroups.get(kcGroup.getName()));
             syncResult.increaseUpdated();
         } else {
-            kcGroup = realm.createGroup(groupTreeEntry.getGroupName());
             if (kcParent == null) {
-                realm.moveGroup(kcGroup, null);
+                kcGroup = realm.createGroup(groupTreeEntry.getGroupName());
                 logger.debugf("Imported top-level group '%s' from LDAP", kcGroup.getName());
             } else {
-                realm.moveGroup(kcGroup, kcParent);
+                kcGroup = realm.createGroup(groupTreeEntry.getGroupName(), kcParent);
                 logger.debugf("Imported group '%s' from LDAP as child of group '%s'", kcGroup.getName(), kcParent.getName());
             }
 
@@ -406,7 +405,6 @@ protected GroupModel findKcGroupOrSyncFromLDAP(RealmModel realm, LDAPObject ldap
 
                 kcGroup = realm.createGroup(groupName);
                 updateAttributesOfKCGroup(kcGroup, ldapGroup);
-                realm.moveGroup(kcGroup, null);
             }
 
             // Could theoretically happen on some LDAP servers if 'memberof' style is used and 'memberof' attribute of user references non-existing group

diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmAdapter.java
@@ -1352,13 +1352,8 @@ public RequiredActionProviderModel getRequiredActionProviderByAlias(String alias
     }
 
     @Override
-    public GroupModel createGroup(String name) {
-        return cacheSession.createGroup(this, name);
-    }
-
-    @Override
-    public GroupModel createGroup(String id, String name) {
-        return cacheSession.createGroup(this, id, name);
+    public GroupModel createGroup(String id, String name, GroupModel toParent) {
+        return cacheSession.createGroup(this, id, name, toParent);
     }
 
     @Override

diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java
@@ -830,7 +830,7 @@ public GroupModel getGroupById(String id, RealmModel realm) {
     @Override
     public void moveGroup(RealmModel realm, GroupModel group, GroupModel toParent) {
         invalidateGroup(group.getId(), realm.getId(), true);
-        if (toParent != null) invalidateGroup(group.getId(), realm.getId(), false); // Queries already invalidated
+        if (toParent != null) invalidateGroup(toParent.getId(), realm.getId(), false); // Queries already invalidated
         listInvalidations.add(realm.getId());
 
         invalidationEvents.add(GroupMovedEvent.create(group, toParent, realm.getId()));
@@ -993,24 +993,18 @@ public boolean removeGroup(RealmModel realm, GroupModel group) {
         return getRealmDelegate().removeGroup(realm, group);
     }
 
-    @Override
-    public GroupModel createGroup(RealmModel realm, String name) {
-        GroupModel group = getRealmDelegate().createGroup(realm, name);
-        return groupAdded(realm, group);
-    }
-
-    private GroupModel groupAdded(RealmModel realm, GroupModel group) {
+    private GroupModel groupAdded(RealmModel realm, GroupModel group, GroupModel toParent) {
         listInvalidations.add(realm.getId());
-        cache.groupQueriesInvalidations(realm.getId(), invalidations);
-        invalidations.add(group.getId());
+        invalidateGroup(group.getId(), realm.getId(), true);
+        if (toParent != null) invalidateGroup(toParent.getId(), realm.getId(), false); // Queries already invalidated
         invalidationEvents.add(GroupAddedEvent.create(group.getId(), realm.getId()));
         return group;
     }
 
     @Override
-    public GroupModel createGroup(RealmModel realm, String id, String name) {
-        GroupModel group = getRealmDelegate().createGroup(realm, id, name);
-        return groupAdded(realm, group);
+    public GroupModel createGroup(RealmModel realm, String id, String name, GroupModel toParent) {
+        GroupModel group = getRealmDelegate().createGroup(realm, id, name, toParent);
+        return groupAdded(realm, group, toParent);
     }
 
     @Override

diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java
@@ -76,16 +76,13 @@ public void setName(String name) {
 
     @Override
     public GroupModel getParent() {
-        GroupEntity parent = group.getParent();
-        if (parent == null) return null;
-        return realm.getGroupById(parent.getId());
+        String parentId = this.getParentId();
+        return parentId == null? null : realm.getGroupById(parentId);
     }
 
     @Override
     public String getParentId() {
-        GroupEntity parent = group.getParent();
-        if (parent == null) return null;
-        return parent.getId();
+        return GroupEntity.TOP_PARENT_ID.equals(group.getParentId())? null : group.getParentId();
     }
 
     public static GroupEntity toEntity(GroupModel model, EntityManager em) {
@@ -97,13 +94,11 @@ public static GroupEntity toEntity(GroupModel model, EntityManager em) {
 
     @Override
     public void setParent(GroupModel parent) {
-        if (parent == null) group.setParent(null);
-        else if (parent.getId().equals(getId())) {
-            return;
-        }
-        else {
+        if (parent == null) {
+            group.setParentId(GroupEntity.TOP_PARENT_ID);
+        } else if (!parent.getId().equals(getId())) {
             GroupEntity parentEntity = toEntity(parent, em);
-            group.setParent(parentEntity);
+            group.setParentId(parentEntity.getId());
         }
     }
 
@@ -126,7 +121,7 @@ public void removeChild(GroupModel subGroup) {
     @Override
     public Set<GroupModel> getSubGroups() {
         TypedQuery<String> query = em.createNamedQuery("getGroupIdsByParent", String.class);
-        query.setParameter("parent", group);
+        query.setParameter("parent", group.getId());
         List<String> ids = query.getResultList();
         Set<GroupModel> set = new HashSet<>();
         for (String id : ids) {

diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java
@@ -45,6 +45,7 @@
 
 import java.util.*;
 import java.util.stream.Collectors;
+import org.keycloak.models.ModelException;
 
 
 /**
@@ -433,15 +434,16 @@ public List<GroupModel> getGroups(RealmModel realm) {
 
     @Override
     public Long getGroupsCount(RealmModel realm, Boolean onlyTopGroups) {
-        String query = "getGroupCount";
         if(Objects.equals(onlyTopGroups, Boolean.TRUE)) {
-            query = "getTopLevelGroupCount";
+            return em.createNamedQuery("getTopLevelGroupCount", Long.class)
+                    .setParameter("realm", realm.getId())
+                    .setParameter("parent", GroupEntity.TOP_PARENT_ID)
+                    .getSingleResult();
+        } else {
+            return em.createNamedQuery("getGroupCount", Long.class)
+                    .setParameter("realm", realm.getId())
+                    .getSingleResult();
         }
-        Long count = em.createNamedQuery(query, Long.class)
-                .setParameter("realm", realm.getId())
-                .getSingleResult();
-
-        return count;
     }
 
     @Override
@@ -480,7 +482,7 @@ public List<GroupModel> getTopLevelGroups(RealmModel realm) {
         RealmEntity ref = em.getReference(RealmEntity.class, realm.getId());
 
         return ref.getGroups().stream()
-                .filter(g -> g.getParent() == null)
+                .filter(g -> GroupEntity.TOP_PARENT_ID.equals(g.getParentId()))
                 .map(g -> session.realms().getGroupById(g.getId(), realm))
                 .sorted(Comparator.comparing(GroupModel::getName))
                 .collect(Collectors.collectingAndThen(
@@ -491,6 +493,7 @@ public List<GroupModel> getTopLevelGroups(RealmModel realm) {
     public List<GroupModel> getTopLevelGroups(RealmModel realm, Integer first, Integer max) {
         List<String> groupIds =  em.createNamedQuery("getTopLevelGroupIds", String.class)
                 .setParameter("realm", realm.getId())
+                .setParameter("parent", GroupEntity.TOP_PARENT_ID)
                 .setFirstResult(first)
                     .setMaxResults(max)
                     .getResultList();
@@ -501,9 +504,7 @@ public List<GroupModel> getTopLevelGroups(RealmModel realm, Integer first, Integ
                 list.add(group);
             }
         }
-
-        list.sort(Comparator.comparing(GroupModel::getName));
-
+        // no need to sort, it's sorted at database level
         return Collections.unmodifiableList(list);
     }
 
@@ -553,19 +554,19 @@ public KeycloakSession getKeycloakSession() {
     }
 
     @Override
-    public GroupModel createGroup(RealmModel realm, String name) {
-        String id = KeycloakModelUtils.generateId();
-        return createGroup(realm, id, name);
-    }
-
-    @Override
-    public GroupModel createGroup(RealmModel realm, String id, String name) {
-        if (id == null) id = KeycloakModelUtils.generateId();
+    public GroupModel createGroup(RealmModel realm, String id, String name, GroupModel toParent) {
+        if (id == null) {
+            id = KeycloakModelUtils.generateId();
+        } else if (GroupEntity.TOP_PARENT_ID.equals(id)) {
+            // maybe it's impossible but better ensure this doesn't happen
+            throw new ModelException("The ID of the new group is equals to the tag used for top level groups");
+        }
         GroupEntity groupEntity = new GroupEntity();
         groupEntity.setId(id);
         groupEntity.setName(name);
         RealmEntity realmEntity = em.getReference(RealmEntity.class, realm.getId());
         groupEntity.setRealm(realmEntity);
+        groupEntity.setParentId(toParent == null? GroupEntity.TOP_PARENT_ID : toParent.getId());
         em.persist(groupEntity);
         em.flush();
         realmEntity.getGroups().add(groupEntity);

diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java
@@ -1947,13 +1947,8 @@ public RequiredActionProviderModel getRequiredActionProviderByAlias(String alias
     }
 
     @Override
-    public GroupModel createGroup(String name) {
-        return session.realms().createGroup(this, name);
-    }
-
-    @Override
-    public GroupModel createGroup(String id, String name) {
-        return session.realms().createGroup(this, id, name);
+    public GroupModel createGroup(String id, String name, GroupModel toParent) {
+        return session.realms().createGroup(this, id, name, toParent);
     }
 
     @Override

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
@@ -28,17 +28,23 @@
  * @version $Revision: 1 $
  */
 @NamedQueries({
-        @NamedQuery(name="getGroupIdsByParent", query="select u.id from GroupEntity u where u.parent = :parent"),
+        @NamedQuery(name="getGroupIdsByParent", query="select u.id from GroupEntity u where u.parentId = :parent"),
         @NamedQuery(name="getGroupIdsByNameContaining", query="select u.id from GroupEntity u where u.realm.id = :realm and u.name like concat('%',:search,'%') order by u.name ASC"),
-        @NamedQuery(name="getTopLevelGroupIds", query="select u.id from GroupEntity u where u.parent is null and u.realm.id = :realm order by u.name ASC"),
+        @NamedQuery(name="getTopLevelGroupIds", query="select u.id from GroupEntity u where u.parentId = :parent and u.realm.id = :realm order by u.name ASC"),
         @NamedQuery(name="getGroupCount", query="select count(u) from GroupEntity u where u.realm.id = :realm"),
-        @NamedQuery(name="getTopLevelGroupCount", query="select count(u) from GroupEntity u where u.realm.id = :realm and u.parent is null")
+        @NamedQuery(name="getTopLevelGroupCount", query="select count(u) from GroupEntity u where u.realm.id = :realm and u.parentId = :parent")
 })
 @Entity
 @Table(name="KEYCLOAK_GROUP",
         uniqueConstraints = { @UniqueConstraint(columnNames = {"REALM_ID", "PARENT_GROUP", "NAME"})}
 )
 public class GroupEntity {
+
+    /**
+     * ID set in the PARENT column to mark the group as top level.
+     */
+    public static String TOP_PARENT_ID = " ";
+
     @Id
     @Column(name="ID", length = 36)
     @Access(AccessType.PROPERTY) // we do this because relationships often fetch id, but not entity.  This avoids an extra SQL
@@ -48,9 +54,8 @@ public class GroupEntity {
     @Column(name = "NAME")
     protected String name;
 
-    @ManyToOne(fetch = FetchType.LAZY)
-    @JoinColumn(name = "PARENT_GROUP")
-    private GroupEntity parent;
+    @Column(name = "PARENT_GROUP")
+    private String parentId;
 
     @ManyToOne(fetch = FetchType.LAZY)
     @JoinColumn(name = "REALM_ID")
@@ -93,12 +98,12 @@ public void setRealm(RealmEntity realm) {
         this.realm = realm;
     }
 
-    public GroupEntity getParent() {
-        return parent;
+    public String getParentId() {
+        return parentId;
     }
 
-    public void setParent(GroupEntity parent) {
-        this.parent = parent;
+    public void setParentId(String parentId) {
+        this.parentId = parentId;
     }
 
     @Override

diff --git a/model/jpa/src/main/resources/META-INF/jpa-changelog-9.0.1.xml b/model/jpa/src/main/resources/META-INF/jpa-changelog-9.0.1.xml
@@ -23,4 +23,26 @@
         </createIndex>
     </changeSet>
 
+    <changeSet author="keycloak" id="9.0.1-KEYCLOAK-12579-drop-constraints">
+        <preConditions onFail="MARK_RAN" onSqlOutput="TEST">
+            <!-- sql server needs drop and re-create the constraint SIBLING_NAMES -->
+            <dbms type="mssql"/>
+        </preConditions>
+        <dropUniqueConstraint tableName="KEYCLOAK_GROUP" constraintName="SIBLING_NAMES"/>
+    </changeSet>
+
+    <changeSet author="keycloak" id="9.0.1-KEYCLOAK-12579-add-not-null-constraint">
+        <!-- Now the parent group cannot be NULL to make SIBLING_NAMES unique constraint work -->
+        <!-- Top level groups are now marked with the " " (one space) string -->
+        <addNotNullConstraint tableName="KEYCLOAK_GROUP" columnName="PARENT_GROUP" columnDataType="VARCHAR(36)" defaultNullValue=" "/>
+    </changeSet>
+
+    <changeSet author="keycloak" id="9.0.1-KEYCLOAK-12579-recreate-constraints">
+        <preConditions onFail="MARK_RAN" onSqlOutput="TEST">
+            <!-- sql server needs drop and re-create the constraint SIBLING_NAMES -->
+            <dbms type="mssql"/>
+        </preConditions>
+        <addUniqueConstraint columnNames="REALM_ID,PARENT_GROUP,NAME" constraintName="SIBLING_NAMES" tableName="KEYCLOAK_GROUP"/>
+    </changeSet>
+
 </databaseChangeLog>
diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java
@@ -667,13 +667,12 @@ public static void importGroups(RealmModel realm, RealmRepresentation rep) {
     }
 
     public static void importGroup(RealmModel realm, GroupModel parent, GroupRepresentation group) {
-        GroupModel newGroup = realm.createGroup(group.getId(), group.getName());
+        GroupModel newGroup = realm.createGroup(group.getId(), group.getName(), parent);
         if (group.getAttributes() != null) {
             for (Map.Entry<String, List<String>> attr : group.getAttributes().entrySet()) {
                 newGroup.setAttribute(attr.getKey(), attr.getValue());
             }
         }
-        realm.moveGroup(newGroup, parent);
 
         if (group.getRealmRoles() != null) {
             for (String roleString : group.getRealmRoles()) {

diff --git a/server-spi/src/main/java/org/keycloak/models/RealmModel.java b/server-spi/src/main/java/org/keycloak/models/RealmModel.java
@@ -471,8 +471,19 @@ List<ClientStorageProviderModel> getClientStorageProviders() {
     String getDefaultLocale();
     void setDefaultLocale(String locale);
 
-    GroupModel createGroup(String name);
-    GroupModel createGroup(String id, String name);
+    default GroupModel createGroup(String name) {
+        return createGroup(null, name, null);
+    };
+
+    default GroupModel createGroup(String id, String name) {
+        return createGroup(id, name, null);
+    };
+
+    default GroupModel createGroup(String name, GroupModel toParent) {
+        return createGroup(null, name, toParent);
+    };
+
+    GroupModel createGroup(String id, String name, GroupModel toParent);
 
     GroupModel getGroupById(String id);
     List<GroupModel> getGroups();

diff --git a/server-spi/src/main/java/org/keycloak/models/RealmProvider.java b/server-spi/src/main/java/org/keycloak/models/RealmProvider.java
@@ -56,9 +56,19 @@ public interface RealmProvider extends Provider, ClientProvider {
 
     boolean removeGroup(RealmModel realm, GroupModel group);
 
-    GroupModel createGroup(RealmModel realm, String name);
+    default GroupModel createGroup(RealmModel realm, String name) {
+        return createGroup(realm, null, name, null);
+    }
 
-    GroupModel createGroup(RealmModel realm, String id, String name);
+    default GroupModel createGroup(RealmModel realm, String id, String name) {
+        return createGroup(realm, id, name, null);
+    }
+
+    default GroupModel createGroup(RealmModel realm, String name, GroupModel toParent) {
+        return createGroup(realm, null, name, toParent);
+    }
+
+    GroupModel createGroup(RealmModel realm, String id, String name, GroupModel toParent);
 
     void addTopLevelGroup(RealmModel realm, GroupModel subGroup);
 

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
@@ -144,7 +144,7 @@ public Response addChild(GroupRepresentation rep) {
             }
             adminEvent.operation(OperationType.UPDATE);
         } else {
-            child = realm.createGroup(rep.getName());
+            child = realm.createGroup(rep.getName(), group);
             updateGroup(rep, child);
             URI uri = session.getContext().getUri().getBaseUriBuilder()
                                            .path(session.getContext().getUri().getMatchedURIs().get(2))
@@ -154,7 +154,6 @@ public Response addChild(GroupRepresentation rep) {
             adminEvent.operation(OperationType.CREATE);
 
         }
-        realm.moveGroup(child, group);
         adminEvent.resourcePath(session.getContext().getUri()).representation(rep).success();
 
         GroupRepresentation childRep = ModelToRepresentation.toGroupHierarchy(child, true);

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
@@ -164,7 +164,6 @@ public Response addTopLevelGroup(GroupRepresentation rep) {
             rep.setId(child.getId());
             adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), child.getId());
         }
-        realm.moveGroup(child, null);
 
         adminEvent.representation(rep).success();
         return builder.build();