[BP]When authenticating user, only update the usergroups in the datab…

…ase if they have changed (#7165) * When authenticating user, only update the usergroups in the database if they have changed. This will prevent database locks when multiple authentications from the same user are being done. * Moved code to updateUserGroups in the userGroupRepository * Add unit test testUpdateUserGroups
geonetwork · Jul 19, 2023 · a033ef0 · a033ef0
1 parent 2cd8b77
commit a033ef0
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 17 deletions.
diff --git a/core/src/main/java/org/fao/geonet/kernel/security/keycloak/KeycloakUserUtils.java b/core/src/main/java/org/fao/geonet/kernel/security/keycloak/KeycloakUserUtils.java
@@ -251,8 +251,7 @@ private Map<Profile, List<String>> getProfileGroups(AccessToken accessToken) {
      * @param user to apply the changes to.
      */
     private void updateGroups(Map<Profile, List<String>> profileGroups, User user) {
-        // First we remove all previous groups
-        userGroupRepository.deleteAll(UserGroupSpecs.hasUserId(user.getId()));
+        Set<UserGroup> userGroups =  new HashSet<>();
 
         // Now we add the groups
         for (Profile p : profileGroups.keySet()) {
@@ -293,12 +292,14 @@ private void updateGroups(Map<Profile, List<String>> profileGroups, User user) {
                     ug.setGroup(group);
                     ug.setUser(user);
                     ug.setProfile(Profile.Editor);
-                    userGroupRepository.save(ug);
+                    userGroups.add(ug);
                 }
 
-                userGroupRepository.save(usergroup);
+                userGroups.add(usergroup);
             }
         }
+
+        userGroupRepository.updateUserGroups(user.getId(), userGroups);
     }
 
     /**

diff --git a/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OidcUser2GeonetworkUser.java b/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OidcUser2GeonetworkUser.java
@@ -41,8 +41,10 @@
 import org.springframework.security.oauth2.core.oidc.OidcIdToken;
 import org.springframework.util.StringUtils;
 
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Class for handling Oidc User and the Geonetwork User.
@@ -159,8 +161,7 @@ public UserDetails getUserDetails(OidcIdToken idToken, Map attributes, boolean w
      */
     //from keycloak
     protected void updateGroups(Map<Profile, List<String>> profileGroups, User user) {
-        // First we remove all previous groups
-        userGroupRepository.deleteAll(UserGroupSpecs.hasUserId(user.getId()));
+        Set<UserGroup> userGroups = new HashSet<>();
 
         // Now we add the groups
         for (Profile p : profileGroups.keySet()) {
@@ -201,13 +202,13 @@ protected void updateGroups(Map<Profile, List<String>> profileGroups, User user)
                     ug.setGroup(group);
                     ug.setUser(user);
                     ug.setProfile(Profile.Editor);
-                    userGroupRepository.save(ug);
+                    userGroups.add(ug);
                 }
 
-                userGroupRepository.save(usergroup);
+                userGroups.add(usergroup);
             }
         }
-    }
-
 
+        userGroupRepository.updateUserGroups(user.getId(), userGroups);
+    }
 }
diff --git a/core/src/main/java/org/fao/geonet/kernel/security/shibboleth/ShibbolethUserUtils.java b/core/src/main/java/org/fao/geonet/kernel/security/shibboleth/ShibbolethUserUtils.java
@@ -48,7 +48,9 @@
 import jeeves.component.ProfileManager;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * @author ETj (etj at geo-solutions.it)
@@ -153,9 +155,6 @@ protected UserDetails setupUser(ServletRequest request, ShibbolethUserConfigurat
                 user = (User) authProvider.loadUserByUsername(username);
 
                 if (config.isUpdateGroup()) {
-                    // First we remove all previous groups
-                    userGroupRepository.deleteAll(UserGroupSpecs.hasUserId(user.getId()));
-
                     // Now we add the groups
                     assignGroups(groupRepository, userGroupRepository, roleGroups,
                             roleGroupSeparator, user);
@@ -222,6 +221,9 @@ protected UserDetails setupUser(ServletRequest request, ShibbolethUserConfigurat
 
     private void assignGroups(GroupRepository groupRepository, UserGroupRepository userGroupRepository,
                               String[] role_groups, String separator, User user) {
+
+        Set<UserGroup> userGroups =  new HashSet<>();
+
         // Assign groups
         int i = 0;
 
@@ -258,14 +260,16 @@ private void assignGroups(GroupRepository groupRepository, UserGroupRepository u
                     ug.setGroup(g);
                     ug.setUser(user);
                     ug.setProfile(Profile.Editor);
-                    userGroupRepository.save(ug);
+                    userGroups.add(ug);
                 }
             } else {
                 // Failback if no profile
                 usergroup.setProfile(Profile.Guest);
             }
-            userGroupRepository.save(usergroup);
+            userGroups.add(usergroup);
         }
+
+        userGroupRepository.updateUserGroups(user.getId(), userGroups);
     }
 
     private void assignProfile(String[] role_groups, String roleGroupSeparator, User user) {

diff --git a/domain/src/main/java/org/fao/geonet/domain/UserGroup.java b/domain/src/main/java/org/fao/geonet/domain/UserGroup.java
@@ -32,6 +32,7 @@
 
 import java.io.Serializable;
 import java.util.IdentityHashMap;
+import java.util.Objects;
 
 /**
  * The mapping between user, the groups a user is a part of and the profiles the user has for each
@@ -145,4 +146,19 @@ protected Element asXml(IdentityHashMap<Object, Void> alreadyEncoded) {
             .addContent(new Element("user").setText("" + getId().getUserId()))
             .addContent(new Element("profile").setText(getProfile().name()));
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        UserGroup ug = (UserGroup) o;
+        return (getId().getGroupId() == (ug.getId().getGroupId()) &&
+            getId().getUserId() == (ug.getId().getUserId()) &&
+            getProfile().name().equals(ug.getProfile().name()));
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(getProfile().name(), getId().getUserId(), getId().getGroupId());
+    }
 }
diff --git a/domain/src/main/java/org/fao/geonet/repository/UserGroupRepositoryCustom.java b/domain/src/main/java/org/fao/geonet/repository/UserGroupRepositoryCustom.java
@@ -31,6 +31,7 @@
 
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Custom methods for loading {@link UserGroup} entities.
@@ -61,4 +62,13 @@ public interface UserGroupRepositoryCustom {
      * @return the number of entities deleted
      */
     int deleteAllByIdAttribute(SingularAttribute<UserGroupId, Integer> idAttribute, Collection<Integer> ids);
+
+    /**
+     * Update user with the new list of {@link UserGroup}. 
+     * If the user already has all the groups specified then no change will be made.
+     *
+     * @param userId        user id to have the groups updated
+     * @param newUserGroups the {@link UserGroup} to set
+     */
+    void updateUserGroups(int userId, Set<UserGroup> newUserGroups);
 }
diff --git a/domain/src/main/java/org/fao/geonet/repository/UserGroupRepositoryCustomImpl.java b/domain/src/main/java/org/fao/geonet/repository/UserGroupRepositoryCustomImpl.java
@@ -26,10 +26,12 @@
  */
 package org.fao.geonet.repository;
 
+import org.fao.geonet.ApplicationContextHolder;
 import org.fao.geonet.domain.UserGroup;
 import org.fao.geonet.domain.UserGroupId;
 import org.fao.geonet.domain.UserGroupId_;
 import org.fao.geonet.domain.UserGroup_;
+import org.fao.geonet.repository.specification.UserGroupSpecs;
 import org.springframework.data.jpa.domain.Specification;
 import org.springframework.transaction.annotation.Transactional;
 
@@ -42,7 +44,9 @@
 import javax.persistence.metamodel.SingularAttribute;
 
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Implementation object for methods in {@link UserGroupRepositoryCustom}.
@@ -91,4 +95,19 @@ private List<Integer> findIdsBy(Specification<UserGroup> spec, SingularAttribute
         return _entityManager.createQuery(query).getResultList();
     }
 
+     public void updateUserGroups(int userId, Set<UserGroup> newUserGroups) {
+         UserGroupRepository userGroupRepository = ApplicationContextHolder.get().getBean(UserGroupRepository.class);
+
+         List<UserGroup> currentUserGroupLists = userGroupRepository.findAll(UserGroupSpecs.hasUserId(userId));
+         Set<UserGroup> currentUserGroups = new HashSet<>(currentUserGroupLists);
+
+         // If the new user groups are not the same as what is in the database then update database so that they are the same.
+         if (!newUserGroups.equals(currentUserGroups)) {
+             userGroupRepository.deleteAll(UserGroupSpecs.hasUserId(userId));
+             for (UserGroup ug : newUserGroups) {
+                 userGroupRepository.save(ug);
+             }
+         }
+    }
+
 }
diff --git a/domain/src/test/java/org/fao/geonet/repository/UserGroupRepositoryTest.java b/domain/src/test/java/org/fao/geonet/repository/UserGroupRepositoryTest.java
@@ -33,16 +33,16 @@
 import org.junit.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.jpa.domain.Specification;
-import org.springframework.data.jpa.domain.Specification;
 
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.springframework.data.jpa.domain.Specification.where;
 
@@ -201,6 +201,28 @@ public void testDeleteAllWithUserIdsIn() {
         assertFalse(_repo.findById(ug1.getId()).isPresent());
     }
 
+    @Test
+    public void testUpdateUserGroups() {
+        UserGroup ug1 = _repo.save(newUserGroup());
+        User user = ug1.getUser();
+
+        UserGroup ug2 = newUserGroup();
+        ug2.setUser(user);
+
+        UserGroup ug3 = newUserGroup();
+        ug3.setUser(user);
+
+        Set<UserGroup> userGroups = new HashSet<>();
+        userGroups.add(ug2);
+        userGroups.add(ug3);
+
+        _repo.updateUserGroups(ug1.getUser().getId(), userGroups);
+
+        List<UserGroup> userGroups1 = _repo.findAll(UserGroupSpecs.hasUserId(user.getId()));
+
+        assertEquals(new HashSet<>(userGroups1), userGroups);
+    }
+
     private UserGroup newUserGroup() {
         return getUserGroup(_inc, _userRepo, _groupRepo);
     }