From 667928cda9f8cef593140d6509ada170f97f9cac Mon Sep 17 00:00:00 2001
From: felmer <franz-josef.elmer@id.ethz.ch>
Date: Sat, 17 Jun 2023 08:20:12 +0200
Subject: [PATCH] SSDM-13716: Fix bug that user cannot be moved to other group
but didn't show up in global group, tests extended
---
.../generic/server/task/UserManager.java | 58 +++++--
.../systemtest/task/UserManagerTest.java | 142 +++++++++++++++---
2 files changed, 171 insertions(+), 29 deletions(-)
diff --git a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/task/UserManager.java b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/task/UserManager.java
index 6da46d503ac..76cf029ae4c 100644
--- a/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/task/UserManager.java
+++ b/server-application-server/source/java/ch/systemsx/cisd/openbis/generic/server/task/UserManager.java
@@ -273,6 +273,7 @@ public class UserManager
manageGroup(sessionToken, groupCode, users, currentState, report);
}
updateHomeSpaces(sessionToken, currentState, report);
+ removeUsersFromGlobalGroup(sessionToken, currentState, report);
} catch (Throwable e)
{
report.addErrorMessage("Error: " + e.toString());
@@ -426,6 +427,21 @@ public class UserManager
return personUpdate;
}
+ private void removeUsersFromGlobalGroup(String sessionToken, CurrentState currentState, UserManagerReport report)
+ {
+ AuthorizationGroup globalGroup = currentState.getGlobalGroup();
+ if (globalGroup != null)
+ {
+ Context context = new Context(sessionToken, service, currentState, report);
+
+ for (String userId : currentState.getUsersToBeRemovedFromGlobalGroup())
+ {
+ removePersonFromAuthorizationGroup(context, globalGroup.getCode(), userId);
+ }
+ context.executeOperations();
+ }
+ }
+
private void manageInstanceAdmins(String sessionToken, CurrentState currentState, UserManagerReport report)
{
if (instanceAdmins != null)
@@ -814,9 +830,9 @@ public class UserManager
private void manageUsers(Context context, String groupCode, Map<String, Principal> groupUsers)
{
UserGroup group = groupsByCode.get(groupCode);
- Map<String, Person> currentUsersOfGroup = context.currentState.getCurrentUsersOfGroup(groupCode);
+ Map<String, Person> currentUsersOfGroup = context.getCurrentState().getCurrentUsersOfGroup(groupCode);
Set<String> usersToBeRemoved = new TreeSet<>(currentUsersOfGroup.keySet());
- AuthorizationGroup globalGroup = context.currentState.getGlobalGroup();
+ AuthorizationGroup globalGroup = context.getCurrentState().getGlobalGroup();
String adminGroupCode = createAdminGroupCode(groupCode);
boolean createUserSpace = group == null || group.isCreateUserSpace();
boolean useEmailAsUserId = group != null && group.isUseEmailAsUserId();
@@ -833,6 +849,7 @@ public class UserManager
addPersonToAuthorizationGroup(context, groupCode, userId);
if (globalGroup != null)
{
+ context.getCurrentState().addPersonToGlobalGroup(userId);
addPersonToAuthorizationGroup(context, globalGroup.getCode(), userId);
}
if (isAdmin(userId, groupCode))
@@ -850,10 +867,10 @@ public class UserManager
private void handleRoleAssignmentForUserSpaces(Context context, String groupCode)
{
UserGroup group = groupsByCode.get(groupCode);
- Map<String, Person> currentUsersOfGroup = context.currentState.getCurrentUsersOfGroup(groupCode);
+ Map<String, Person> currentUsersOfGroup = context.getCurrentState().getCurrentUsersOfGroup(groupCode);
Set<String> allUserSpaces = getAllUserSpaces(context, groupCode, currentUsersOfGroup);
Role userSpaceRole = group.getUserSpaceRole();
- AuthorizationGroup authorizationGroup = context.currentState.groupsByCode.get(groupCode);
+ AuthorizationGroup authorizationGroup = context.getCurrentState().groupsByCode.get(groupCode);
AuthorizationGroupPermId groupId = new AuthorizationGroupPermId(groupCode);
for (String spaceCode : allUserSpaces)
@@ -880,7 +897,7 @@ public class UserManager
}
if (roleAssignments.isEmpty() && userSpaceRole != null)
{
- Space space = context.currentState.getSpace(spaceCode);
+ Space space = context.getCurrentState().getSpace(spaceCode);
createRoleAssignment(context, groupId, userSpaceRole, space.getPermId());
}
}
@@ -975,12 +992,12 @@ public class UserManager
{
removePersonFromAuthorizationGroup(context, groupCode, userId);
removePersonFromAuthorizationGroup(context, adminGroupCode, userId);
- AuthorizationGroup globalGroup = context.currentState.getGlobalGroup();
+ AuthorizationGroup globalGroup = context.getCurrentState().getGlobalGroup();
if (globalGroup != null)
{
- removePersonFromAuthorizationGroup(context, globalGroup.getCode(), userId);
+ context.getCurrentState().removeUserFromGlobalGroup(userId);
}
- Person user = context.currentState.getUser(userId);
+ Person user = context.getCurrentState().getUser(userId);
for (RoleAssignment roleAssignment : user.getRoleAssignments())
{
Space space = roleAssignment.getSpace();
@@ -1038,7 +1055,8 @@ public class UserManager
private void addPersonToAuthorizationGroup(Context context, String groupCode, String userId)
{
- if (context.currentState.getCurrentUsersOfGroup(groupCode).keySet().contains(userId) == false)
+ System.err.println(context.getCurrentState().getCurrentUsersOfGroup(groupCode));
+ if (context.getCurrentState().getCurrentUsersOfGroup(groupCode).keySet().contains(userId) == false)
{
AuthorizationGroupUpdate groupUpdate = new AuthorizationGroupUpdate();
groupUpdate.setAuthorizationGroupId(new AuthorizationGroupPermId(groupCode));
@@ -1050,7 +1068,7 @@ public class UserManager
private void removePersonFromAuthorizationGroup(Context context, String groupCode, String userId)
{
- if (context.currentState.getCurrentUsersOfGroup(groupCode).keySet().contains(userId))
+ if (context.getCurrentState().getCurrentUsersOfGroup(groupCode).keySet().contains(userId))
{
AuthorizationGroupUpdate groupUpdate = new AuthorizationGroupUpdate();
groupUpdate.setAuthorizationGroupId(new AuthorizationGroupPermId(groupCode));
@@ -1121,6 +1139,10 @@ public class UserManager
private Set<String> newUsers = new TreeSet<>();
+ private Set<String> usersAddedToGlobalGroup = new TreeSet<>();
+
+ private Set<String> usersRemovedFromGlobalGroup = new TreeSet<>();
+
private AuthorizationGroup globalGroup;
CurrentState(List<AuthorizationGroup> authorizationGroups, AuthorizationGroup globalGroup, List<Space> spaces, List<Person> users)
@@ -1132,6 +1154,22 @@ public class UserManager
users.forEach(user -> usersById.put(user.getUserId(), user));
}
+ public void removeUserFromGlobalGroup(String userId)
+ {
+ usersRemovedFromGlobalGroup.add(userId);
+ }
+
+ public void addPersonToGlobalGroup(String userId)
+ {
+ usersAddedToGlobalGroup.add(userId);
+ }
+
+ public Set<String> getUsersToBeRemovedFromGlobalGroup()
+ {
+ usersRemovedFromGlobalGroup.removeAll(usersAddedToGlobalGroup);
+ return usersRemovedFromGlobalGroup;
+ }
+
public Map<String, Person> getCurrentUsersOfGroup(String groupCode)
{
Map<String, Person> result = new TreeMap<>();
diff --git a/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/task/UserManagerTest.java b/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/task/UserManagerTest.java
index d0e7aeefc1d..3f7f94a15f8 100644
--- a/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/task/UserManagerTest.java
+++ b/server-application-server/sourceTest/java/ch/systemsx/cisd/openbis/systemtest/task/UserManagerTest.java
@@ -214,8 +214,8 @@ public class UserManagerTest extends AbstractTest
// Then
assertEquals(report.getErrorReport(), "");
assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G1, user: u2\n"
- + "1970-01-01 01:00:01 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
- + "1970-01-01 01:00:02 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G1_U2\n");
+ + "1970-01-01 01:00:01 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G1_U2\n"
+ + "1970-01-01 01:00:02 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n");
UserManagerExpectationsBuilder builder = createBuilder();
builder.groups("G1").commonSpaces(commonSpaces).users(U1, U2);
builder.space("A").observer(U1).non(U2);
@@ -751,8 +751,8 @@ public class UserManagerTest extends AbstractTest
// Then
assertEquals(report.getErrorReport(), "");
assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u2\n"
- + "1970-01-01 01:00:01 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
- + "1970-01-01 01:00:02 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n");
+ + "1970-01-01 01:00:01 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:02 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n");
UserManagerExpectationsBuilder builder = createBuilder();
builder.groups("G2").commonSpaces(commonSpaces).users(U1, U3);
builder.usersWithoutAuthentication(U2);
@@ -770,6 +770,110 @@ public class UserManagerTest extends AbstractTest
builder.assertExpectations();
}
+ @Test
+ public void testMoveUserToAnotherGroup()
+ {
+ // Given
+ // 1. create group G1 with users U1 (admin), U2 and group G2 with user U3 (admin)
+ MockLogger logger = new MockLogger();
+ Map<Role, List<String>> commonSpaces = commonSpaces();
+ UserManager userManager = new UserManagerBuilder(v3api, logger, report()).commonSpaces(commonSpaces).get();
+ List<String> globalSpaces = Arrays.asList("A", "B");
+ userManager.setGlobalSpaces(globalSpaces);
+ userManager.addGroup(new UserGroupAsBuilder("G1").admins(U1), users(U1, U2));
+ userManager.addGroup(new UserGroupAsBuilder("G2").admins(U3), users(U3));
+ assertEquals(manage(userManager).getErrorReport(), "");
+ // 2. move user U2 from group G1 to group G2
+ userManager = new UserManagerBuilder(v3api, logger, report()).commonSpaces(commonSpaces).get();
+ userManager.setGlobalSpaces(globalSpaces);
+ userManager.addGroup(new UserGroupAsBuilder("G1").admins(U1), users(U1));
+ userManager.addGroup(new UserGroupAsBuilder("G2").admins(U3), users(U2, U3));
+
+ // When
+ UserManagerReport report = manage(userManager);
+
+ // Then
+ assertEquals(report.getErrorReport(), "");
+ assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G1, user: u2\n"
+ + "1970-01-01 01:00:01 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G1_U2\n"
+ + "1970-01-01 01:00:02 [ADD-SPACE] G2_U2\n"
+ + "1970-01-01 01:00:03 [ASSIGN-ROLE-TO-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:04 [ASSIGN-ROLE-TO-AUTHORIZATION-GROUP] group: G2_ADMIN, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:05 [ADD-USER-TO-AUTHORIZATION-GROUP] group: G2, user: u2\n"
+ + "1970-01-01 01:00:06 [ASSIGN-HOME-SPACE-FOR-USER] user: u2, home space: G2_U2\n");
+ UserManagerExpectationsBuilder builder = createBuilder();
+ builder.groups("G1").commonSpaces(commonSpaces).users(U1);
+ builder.groups("G2").commonSpaces(commonSpaces).users(U2, U3);
+ builder.usersWithoutAuthentication();
+ builder.space("A").observer(U1, U2, U3);
+ builder.space("B").observer(U1, U2, U3);
+ builder.space("G1_ALPHA").admin(U1).non(U2, U3);
+ builder.space("G1_BETA").admin(U1).non(U2, U3);
+ builder.space("G1_GAMMA").admin(U1).non(U2, U3);
+ builder.space("G2_ALPHA").admin(U3).user(U2).non(U1);
+ builder.space("G2_BETA").admin(U3).user(U2).non(U1);
+ builder.space("G2_GAMMA").admin(U3).observer(U2).non(U1);
+ builder.homeSpace(U1, "G1_U1");
+ builder.homeSpace(U2, "G2_U2");
+ builder.homeSpace(U3, "G2_U3");
+ builder.assertExpectations();
+ }
+
+
+ @Test
+ public void testMoveUserToAnotherGroupAndRemovePreviousGroup()
+ {
+ // Given
+ // 1. create group G1 with users U1 (admin), U2 and group G2 with user U3 (admin)
+ MockLogger logger = new MockLogger();
+ Map<Role, List<String>> commonSpaces = commonSpaces();
+ UserManager userManager = new UserManagerBuilder(v3api, logger, report()).commonSpaces(commonSpaces).get();
+ List<String> globalSpaces = Arrays.asList("A", "B");
+ userManager.setGlobalSpaces(globalSpaces);
+ userManager.addGroup(new UserGroupAsBuilder("G1").admins(U1), users(U1, U2));
+ userManager.addGroup(new UserGroupAsBuilder("G2").admins(U3), users(U3));
+ assertEquals(manage(userManager).getErrorReport(), "");
+ // 2. move user U2 from group G1 to group G2 and remove group G1
+ userManager = new UserManagerBuilder(v3api, logger, report()).commonSpaces(commonSpaces).get();
+ userManager.setGlobalSpaces(globalSpaces);
+ userManager.addGroup(new UserGroupAsBuilder("G2").admins(U3), users(U2, U3));
+
+ // When
+ UserManagerReport report = manage(userManager);
+
+ // Then
+ assertEquals(report.getErrorReport(), "");
+ assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G1, user: u1\n"
+ + "1970-01-01 01:00:01 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G1_ADMIN, user: u1\n"
+ + "1970-01-01 01:00:02 [UNASSIGN-ROLE-FORM-USER] user: u1, role: SPACE_ADMIN for G1_U1\n"
+ + "1970-01-01 01:00:03 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G1, user: u2\n"
+ + "1970-01-01 01:00:04 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G1_U2\n"
+ + "1970-01-01 01:00:05 [REMOVE-AUTHORIZATION-GROUP] G1\n"
+ + "1970-01-01 01:00:06 [REMOVE-AUTHORIZATION-GROUP] G1_ADMIN\n"
+ + "1970-01-01 01:00:07 [ADD-SPACE] G2_U2\n"
+ + "1970-01-01 01:00:08 [ASSIGN-ROLE-TO-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:09 [ASSIGN-ROLE-TO-AUTHORIZATION-GROUP] group: G2_ADMIN, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:10 [ADD-USER-TO-AUTHORIZATION-GROUP] group: G2, user: u2\n"
+ + "1970-01-01 01:00:11 [ASSIGN-HOME-SPACE-FOR-USER] user: u2, home space: G2_U2\n"
+ + "1970-01-01 01:00:12 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u1\n");
+ UserManagerExpectationsBuilder builder = createBuilder();
+ builder.groups("G1").commonSpaces(commonSpaces).users();
+ builder.groups("G2").commonSpaces(commonSpaces).users(U2, U3);
+ builder.usersWithoutAuthentication(U1);
+ builder.space("A").observer(U2, U3);
+ builder.space("B").observer(U2, U3);
+ builder.space("G1_ALPHA").non(U2, U3);
+ builder.space("G1_BETA").non(U2, U3);
+ builder.space("G1_GAMMA").non(U2, U3);
+ builder.space("G2_ALPHA").admin(U3).user(U2);
+ builder.space("G2_BETA").admin(U3).user(U2);
+ builder.space("G2_GAMMA").admin(U3).observer(U2);
+ builder.homeSpace(U1, "G1_U1");
+ builder.homeSpace(U2, "G2_U2");
+ builder.homeSpace(U3, "G2_U3");
+ builder.assertExpectations();
+ }
+
@Test
public void testDisableAGroup()
{
@@ -794,14 +898,14 @@ public class UserManagerTest extends AbstractTest
assertEquals(report.getErrorReport(), "");
assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u1\n"
+ "1970-01-01 01:00:01 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2_ADMIN, user: u1\n"
- + "1970-01-01 01:00:02 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u1\n"
- + "1970-01-01 01:00:03 [UNASSIGN-ROLE-FORM-USER] user: u1, role: SPACE_ADMIN for G2_U1\n"
- + "1970-01-01 01:00:04 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u2\n"
- + "1970-01-01 01:00:05 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
- + "1970-01-01 01:00:06 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
- + "1970-01-01 01:00:07 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u3\n"
- + "1970-01-01 01:00:08 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u3\n"
- + "1970-01-01 01:00:09 [UNASSIGN-ROLE-FORM-USER] user: u3, role: SPACE_ADMIN for G2_U3\n");
+ + "1970-01-01 01:00:02 [UNASSIGN-ROLE-FORM-USER] user: u1, role: SPACE_ADMIN for G2_U1\n"
+ + "1970-01-01 01:00:03 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u2\n"
+ + "1970-01-01 01:00:04 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:05 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u3\n"
+ + "1970-01-01 01:00:06 [UNASSIGN-ROLE-FORM-USER] user: u3, role: SPACE_ADMIN for G2_U3\n"
+ + "1970-01-01 01:00:07 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u1\n"
+ + "1970-01-01 01:00:08 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
+ + "1970-01-01 01:00:09 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u3\n");
UserManagerExpectationsBuilder builder = createBuilder();
builder.groups("G2").users();
builder.usersWithoutAuthentication(U1, U2, U3);
@@ -902,13 +1006,13 @@ public class UserManagerTest extends AbstractTest
assertEquals(report.getErrorReport(), "");
assertEquals(report.getAuditLog(), "1970-01-01 01:00:00 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u2\n"
+ "1970-01-01 01:00:01 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2_ADMIN, user: u2\n"
- + "1970-01-01 01:00:02 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
- + "1970-01-01 01:00:03 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
- + "1970-01-01 01:00:04 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u3\n"
- + "1970-01-01 01:00:05 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u3\n"
- + "1970-01-01 01:00:06 [UNASSIGN-ROLE-FORM-USER] user: u3, role: SPACE_ADMIN for G2_U3\n"
- + "1970-01-01 01:00:07 [REMOVE-AUTHORIZATION-GROUP] G2\n"
- + "1970-01-01 01:00:08 [REMOVE-AUTHORIZATION-GROUP] G2_ADMIN\n");
+ + "1970-01-01 01:00:02 [UNASSIGN-ROLE-FORM-USER] user: u2, role: SPACE_ADMIN for G2_U2\n"
+ + "1970-01-01 01:00:03 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: G2, user: u3\n"
+ + "1970-01-01 01:00:04 [UNASSIGN-ROLE-FORM-USER] user: u3, role: SPACE_ADMIN for G2_U3\n"
+ + "1970-01-01 01:00:05 [REMOVE-AUTHORIZATION-GROUP] G2\n"
+ + "1970-01-01 01:00:06 [REMOVE-AUTHORIZATION-GROUP] G2_ADMIN\n"
+ + "1970-01-01 01:00:07 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u2\n"
+ + "1970-01-01 01:00:08 [REMOVE-USER-FROM-AUTHORIZATION-GROUP] group: ALL_GROUPS, user: u3\n");
UserManagerExpectationsBuilder builder = createBuilder();
builder.groups("G1").commonSpaces(commonSpaces).users(U1);
builder.usersWithoutAuthentication(U2, U3);
--
GitLab