From 9c8f35f2c3d42f409e749e041fabb91c11f6b811 Mon Sep 17 00:00:00 2001 From: netroms Date: Mon, 6 Nov 2023 13:22:35 +0800 Subject: [PATCH] fix: revert acl check on user group update (#15443) * fix: revert acl check on user group update (#15131) (cherry picked from commit a69533020c36e92a63a95e7d3027bcfe4097fc24) * fix: revert acl check on user group update (#15131) (cherry picked from commit a69533020c36e92a63a95e7d3027bcfe4097fc24) * fix: revert acl check on user group update --- .../java/org/hisp/dhis/user/UserGroup.java | 6 -- .../org/hisp/dhis/user/UserGroupService.java | 22 +------- .../dhis/user/DefaultUserGroupService.java | 55 ++++++++----------- .../webapi/controller/UserControllerTest.java | 40 ++++++++++++++ 4 files changed, 66 insertions(+), 57 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroup.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroup.java index d340567f7781..be8039a94a53 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroup.java +++ b/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"; diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroupService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroupService.java index 5b638ad15e56..f55847f44877 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserGroupService.java +++ b/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 uids); - void addUserToGroups(User user, Collection uids, User currentUser); void removeUserFromGroups(User user, Collection uids); - void updateUserGroups(User user, Collection 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 userGroupIds, User currentUser); + void updateUserGroups(User user, Collection uids, User currentUser); List getAllUserGroups(); @@ -83,10 +69,6 @@ public interface UserGroupService { List 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); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserGroupService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserGroupService.java index d390e1feddc6..69feaae3bb2b 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserGroupService.java +++ b/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 uids) { - addUserToGroups(user, uids, currentUserService.getCurrentUser()); - } - @Override @Transactional public void addUserToGroups(User user, Collection uids, User currentUser) { @@ -171,30 +171,35 @@ public void removeUserFromGroups(User user, Collection uids) { } } - @Override - @Transactional - public void updateUserGroups(User user, Collection uids) { - updateUserGroups(user, uids, currentUserService.getCurrentUser()); - } - @Override @Transactional public void updateUserGroups(User user, Collection uids, User currentUser) { Collection updates = getUserGroupsByUid(uids); + Map 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 getUserGroupsByUid(Collection uids) { @@ -207,18 +212,6 @@ public List 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 getUserGroupsBetween(int first, int max) { diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java index 6201c17670c5..7eabe4ee5574 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java +++ b/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");