fix: revert acl check on user group update (#15443)

* fix: revert acl check on user group update (#15131)

(cherry picked from commit a695330)

* fix: revert acl check on user group update (#15131)

(cherry picked from commit a695330)

* fix: revert acl check on user group update

dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroup.java

@@ -48,13 +48,7 @@
 @JacksonXmlRootElement(localName = "userGroup", namespace = DxfNamespaces.DXF_2_0)
 public class UserGroup extends BaseIdentifiableObject implements MetadataObject {
   public static final String AUTH_USER_ADD = "F_USER_ADD";
-
-  public static final String AUTH_USER_DELETE = "F_USER_DELETE";
-
-  public static final String AUTH_USER_VIEW = "F_USER_VIEW";
-
   public static final String AUTH_USER_ADD_IN_GROUP = "F_USER_ADD_WITHIN_MANAGED_GROUP";
-
   public static final String AUTH_ADD_MEMBERS_TO_READ_ONLY_USER_GROUPS =
       "F_USER_GROUPS_READ_ONLY_ADD_MEMBERS";
 

dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroupService.java

@@ -45,7 +45,7 @@ public interface UserGroupService {
 
   /**
    * Indicates whether the current user can add or remove members for the user group with the given
-   * UID. To to so the current user must have write access to the group or have read access as well
+   * UID. To do so the current user must have write access to the group or have read access as well
    * as the F_USER_GROUPS_READ_ONLY_ADD_MEMBERS authority.
    *
    * @param uid the user group UID.
@@ -55,25 +55,11 @@ public interface UserGroupService {
 
   boolean canAddOrRemoveMember(String uid, User currentUser);
 
-  void addUserToGroups(User user, Collection<String> uids);
-
   void addUserToGroups(User user, Collection<String> uids, User currentUser);
 
   void removeUserFromGroups(User user, Collection<String> uids);
 
-  void updateUserGroups(User user, Collection<String> uids);
-
-  /**
-   * This method will check and perform add/remove given {@link User} from given {@link UserGroup}
-   * member list. The final result is that given {@link User} will only belong to given {@link
-   * UserGroup} list.
-   *
-   * @param user the {@link User} which will be added or removed from given list of {@link
-   *     UserGroup}
-   * @param userGroupIds List uid of {@link UserGroup}
-   * @param currentUser Current User.
-   */
-  void updateUserGroups(User user, Collection<String> userGroupIds, User currentUser);
+  void updateUserGroups(User user, Collection<String> uids, User currentUser);
 
   List<UserGroup> getAllUserGroups();
 
@@ -83,10 +69,6 @@ public interface UserGroupService {
 
   List<UserGroup> getUserGroupsBetweenByName(String name, int first, int max);
 
-  int getUserGroupCount();
-
-  int getUserGroupCountByName(String name);
-
   /** Get UserGroup's display name by given userGroup uid Return null if UserGroup does not exist */
   String getDisplayName(String uid);
 }

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserGroupService.java

@@ -29,11 +29,17 @@
 
 import static com.google.common.base.Preconditions.*;
 
-import java.util.*;
-import org.hisp.dhis.cache.*;
-import org.hisp.dhis.security.acl.*;
-import org.springframework.stereotype.*;
-import org.springframework.transaction.annotation.*;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import org.hisp.dhis.cache.Cache;
+import org.hisp.dhis.cache.CacheProvider;
+import org.hisp.dhis.cache.HibernateCacheManager;
+import org.hisp.dhis.security.acl.AclService;
+import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
 
 /**
  * @author Lars Helge Overland
@@ -141,12 +147,6 @@ public boolean canAddOrRemoveMember(String uid, User currentUser) {
     return canUpdate || canAddMember;
   }
 
-  @Override
-  @Transactional
-  public void addUserToGroups(User user, Collection<String> uids) {
-    addUserToGroups(user, uids, currentUserService.getCurrentUser());
-  }
-
   @Override
   @Transactional
   public void addUserToGroups(User user, Collection<String> uids, User currentUser) {
@@ -171,30 +171,35 @@ public void removeUserFromGroups(User user, Collection<String> uids) {
     }
   }
 
-  @Override
-  @Transactional
-  public void updateUserGroups(User user, Collection<String> uids) {
-    updateUserGroups(user, uids, currentUserService.getCurrentUser());
-  }
-
   @Override
   @Transactional
   public void updateUserGroups(User user, Collection<String> uids, User currentUser) {
     Collection<UserGroup> updates = getUserGroupsByUid(uids);
 
+    Map<UserGroup, Integer> before = new HashMap<>();
+    updates.forEach(userGroup -> before.put(userGroup, userGroup.getMembers().size()));
+
     for (UserGroup userGroup : new HashSet<>(user.getGroups())) {
       if (!updates.contains(userGroup) && canAddOrRemoveMember(userGroup.getUid(), currentUser)) {
+        before.put(userGroup, userGroup.getMembers().size());
         userGroup.removeUser(user);
-        userGroupStore.update(userGroup, currentUser);
       }
     }
 
     for (UserGroup userGroup : updates) {
       if (canAddOrRemoveMember(userGroup.getUid(), currentUser)) {
         userGroup.addUser(user);
-        userGroupStore.update(userGroup, currentUser);
       }
     }
+
+    // Update user group if members have changed
+    before.forEach(
+        (userGroup, beforeSize) -> {
+          if (beforeSize != userGroup.getMembers().size()) {
+            userGroup.setLastUpdatedBy(currentUser);
+            userGroupStore.updateNoAcl(userGroup);
+          }
+        });
   }
 
   private Collection<UserGroup> getUserGroupsByUid(Collection<String> uids) {
@@ -207,18 +212,6 @@ public List<UserGroup> getUserGroupByName(String name) {
     return userGroupStore.getAllEqName(name);
   }
 
-  @Override
-  @Transactional(readOnly = true)
-  public int getUserGroupCount() {
-    return userGroupStore.getCount();
-  }
-
-  @Override
-  @Transactional(readOnly = true)
-  public int getUserGroupCountByName(String name) {
-    return userGroupStore.getCountLikeName(name);
-  }
-
   @Override
   @Transactional(readOnly = true)
   public List<UserGroup> getUserGroupsBetween(int first, int max) {

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java

@@ -42,6 +42,7 @@
 import com.google.common.collect.Sets;
 import com.google.gson.Gson;
 import com.google.gson.JsonElement;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -230,6 +231,45 @@ private JsonImportSummary updateRolesNonAllAdmin() {
     return response;
   }
 
+  @Test
+  void updateUserHasAccessToUpdateGroups() {
+    systemSettingManager.saveSystemSetting(SettingKey.CAN_GRANT_OWN_USER_ROLES, Boolean.TRUE);
+
+    UserRole roleB = createUserRole("ROLE_B", "F_USER_ADD", "F_USER_GROUPS_READ_ONLY_ADD_MEMBERS");
+    userService.addUserRole(roleB);
+
+    UserGroup userGroupA = createUserGroup('A', Collections.emptySet());
+    manager.save(userGroupA);
+
+    User user = createUserWithAuth("someone", "NONE");
+    user.getUserRoles().add(roleB);
+    userService.updateUser(user);
+
+    switchContextToUser(user);
+
+    assertStatus(
+        HttpStatus.OK,
+        PUT(
+            "/users/" + user.getUid(),
+            " {"
+                + "'name': 'test',"
+                + "'username':'someone',"
+                + "'userRoles': ["
+                + "{"
+                + "'id':'"
+                + roleB.getUid()
+                + "'"
+                + "}"
+                + "],"
+                + "'userGroups': ["
+                + "{"
+                + "'id':'"
+                + userGroupA.getUid()
+                + "'"
+                + "}]"
+                + "}"));
+  }
+
   @Test
   void testUpdateRoles() {
     UserRole userRole = createUserRole("ROLE_B", "ALL");