From 788cfe995ba841f44c958b30b5f003bffef18e50 Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Wed, 28 Dec 2022 12:33:54 +0530 Subject: [PATCH] fix: invite user should not update existing member's role (#19254) Fixes #19244 Fixes #17606 If an existing member is invited again from the Share modal with a different role, the user gets assigned the other role as well. This breaks the framework that a member of the workspace can only have a SINGLE role. This breaks other flows like update role/delete member etc because they all run with the assumption that the member can not have more than one role in the workspace. This PR fixes this by throwing an error if a user is being invited again to the workspace. --- .../UserAndAccessManagementServiceCEImpl.java | 33 ++++++++++- .../server/services/WorkspaceServiceTest.java | 56 +++++++++++++++++++ .../ShareWorkspacePermissionTests.java | 27 +++------ 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java index d8180e4c95..f2f4a6251f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.stream.Collectors; import static com.appsmith.server.services.ce.UserServiceCEImpl.INVITE_USER_EMAIL_TEMPLATE; +import static java.lang.Boolean.TRUE; @Slf4j public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManagementServiceCE { @@ -108,18 +109,33 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage // Get workspace from the default group. Mono workspaceMono = permissionGroupMono.flatMap(permissionGroup -> workspaceService.getById(permissionGroup.getDefaultWorkspaceId())).cache(); + // Get all the default permision groups of the workspace + Mono> defaultPermissionGroupsMono = + workspaceMono.flatMap(workspace -> + permissionGroupService.findAllByIds(workspace.getDefaultPermissionGroups()) + .collectList() + ).cache(); + // Check if the invited user exists. If yes, return the user, else create a new user by triggering // createNewUserAndSendInviteEmail. In both the cases, send the appropriate emails Mono> inviteUsersMono = Flux.fromIterable(usernames) - .flatMap(username -> Mono.zip(Mono.just(username), workspaceMono, currentUserMono, permissionGroupMono)) + .flatMap(username -> Mono.zip(Mono.just(username), workspaceMono, currentUserMono, permissionGroupMono, defaultPermissionGroupsMono)) .flatMap(tuple -> { String username = tuple.getT1(); Workspace workspace = tuple.getT2(); eventData.put(FieldName.WORKSPACE, workspace); User currentUser = tuple.getT3(); PermissionGroup permissionGroup = tuple.getT4(); + List defaultPermissionGroups = tuple.getT5(); - return userRepository.findByEmail(username) + Mono getUserFromDbAndCheckIfUserExists = userRepository.findByEmail(username) + .flatMap(user -> { + return throwErrorIfUserAlreadyExistsInWorkspace(user, defaultPermissionGroups) + // If no errors, proceed forward + .thenReturn(user); + }); + + return getUserFromDbAndCheckIfUserExists .flatMap(existingUser -> { // The user already existed, just send an email informing that the user has been added // to a new workspace @@ -170,4 +186,17 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage return bulkAddUserResultMono.then(sendAnalyticsEventMono).then(inviteUsersMono); } + private Mono throwErrorIfUserAlreadyExistsInWorkspace(User user, + List defaultPermissionGroups) { + + return Flux.fromIterable(defaultPermissionGroups) + .map(permissionGroup -> { + if (permissionGroup.getAssignedToUserIds().contains(user.getId())) { + throw new AppsmithException(AppsmithError.USER_ALREADY_EXISTS_IN_WORKSPACE, user.getUsername(), permissionGroup.getName()); + } + return TRUE; + }) + .then(Mono.just(TRUE)); + } + } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java index af62eaa3f9..96bcb81aa4 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java @@ -1253,6 +1253,62 @@ public class WorkspaceServiceTest { .verifyComplete(); } + @Test + @WithUserDetails(value = "api_user") + public void addUserToWorkspaceIfUserAlreadyMember_throwsError() { + Workspace workspace = new Workspace(); + workspace.setName("addUserToWorkspaceIfUserAlreadyMember_throwsError"); + workspace.setDomain("example.com"); + workspace.setWebsite("https://example.com"); + + Mono workspaceMono = workspaceService + .create(workspace) + .cache(); + + Flux permissionGroupFlux = workspaceMono + .flatMapMany(workspace1 -> permissionGroupRepository.findAllById(workspace1.getDefaultPermissionGroups())); + + Mono adminPermissionGroupMono = permissionGroupFlux + .filter(permissionGroup -> permissionGroup.getName().startsWith(ADMINISTRATOR)) + .single(); + + Mono developerPermissionGroupMono = permissionGroupFlux + .filter(permissionGroup -> permissionGroup.getName().startsWith(DEVELOPER)) + .single(); + + InviteUsersDTO inviteUsersDTO = new InviteUsersDTO(); + ArrayList users = new ArrayList<>(); + users.add("usertest@usertest.com"); + inviteUsersDTO.setUsernames(users); + + + Mono> userAddedToWorkspaceTwiceMono = adminPermissionGroupMono + .flatMap(adminPermissionGroup -> { + + // Add user to workspace first as admin + inviteUsersDTO.setPermissionGroupId(adminPermissionGroup.getId()); + + return userAndAccessManagementService.inviteUsers(inviteUsersDTO, origin); + }) + .then(developerPermissionGroupMono) + .flatMap(developerPermissionGroup -> { + + // Now try to add the user to the workspace as developer + inviteUsersDTO.setPermissionGroupId(developerPermissionGroup.getId()); + + return userAndAccessManagementService.inviteUsers(inviteUsersDTO, origin); + }); + + + StepVerifier + .create(userAddedToWorkspaceTwiceMono) + .expectErrorMatches(throwable -> throwable instanceof AppsmithException + && throwable.getMessage() + .equals(AppsmithError.USER_ALREADY_EXISTS_IN_WORKSPACE + .getMessage("usertest@usertest.com", "Administrator - addUserToWorkspaceIfUserAlreadyMember_throwsError"))) + .verify(); + } + @Test @WithUserDetails(value = "api_user") public void inviteRolesGivenAdministrator() { diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ShareWorkspacePermissionTests.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ShareWorkspacePermissionTests.java index 22cd009a64..ede5d42ec7 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ShareWorkspacePermissionTests.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ShareWorkspacePermissionTests.java @@ -7,7 +7,6 @@ import com.appsmith.server.domains.Application; import com.appsmith.server.domains.PermissionGroup; import com.appsmith.server.domains.User; import com.appsmith.server.domains.Workspace; -import com.appsmith.server.dtos.InviteUsersDTO; import com.appsmith.server.dtos.PermissionGroupInfoDTO; import com.appsmith.server.helpers.PluginExecutorHelper; import com.appsmith.server.repositories.ApplicationRepository; @@ -20,6 +19,7 @@ import com.appsmith.server.services.NewActionService; import com.appsmith.server.services.NewPageService; import com.appsmith.server.services.PermissionGroupService; import com.appsmith.server.services.PluginService; +import com.appsmith.server.services.SessionUserService; import com.appsmith.server.services.UserService; import com.appsmith.server.services.UserWorkspaceService; import com.appsmith.server.services.WorkspaceService; @@ -36,7 +36,6 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; -import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -98,6 +97,9 @@ public class ShareWorkspacePermissionTests { @Autowired UserAndAccessManagementService userAndAccessManagementService; + @Autowired + SessionUserService sessionUserService; + Application savedApplication; Workspace savedWorkspace; @@ -105,7 +107,6 @@ public class ShareWorkspacePermissionTests { String workspaceId; @BeforeEach - @WithUserDetails(value = "api_user") public void setup() { User apiUser = userService.findByEmail("api_user").block(); @@ -119,9 +120,6 @@ public class ShareWorkspacePermissionTests { application.setWorkspaceId(workspaceId); savedApplication = applicationPageService.createApplication(application, workspaceId).block(); - InviteUsersDTO inviteUsersDTO = new InviteUsersDTO(); - ArrayList emails = new ArrayList<>(); - PermissionGroup adminPermissionGroup = permissionGroupService.getByDefaultWorkspace(savedWorkspace, AclPermission.READ_PERMISSION_GROUP_MEMBERS) .collectList().block() .stream() @@ -134,28 +132,17 @@ public class ShareWorkspacePermissionTests { .filter(permissionGroupElem -> permissionGroupElem.getName().startsWith(FieldName.DEVELOPER)) .findFirst().get(); - // Invite Admin - emails.add("admin@solutiontest.com"); - inviteUsersDTO.setUsernames(emails); - inviteUsersDTO.setPermissionGroupId(adminPermissionGroup.getId()); - userAndAccessManagementService.inviteUsers(inviteUsersDTO, "http://localhost:8080").block(); - - emails.clear(); - - // Invite Developer - emails.add("developer@solutiontest.com"); - inviteUsersDTO.setUsernames(emails); - inviteUsersDTO.setPermissionGroupId(developerPermissionGroup.getId()); - userAndAccessManagementService.inviteUsers(inviteUsersDTO, "http://localhost:8080").block(); User userAdmin = userService.findByEmail("admin@solutiontest.com").block(); User userDeveloper = userService.findByEmail("developer@solutiontest.com").block(); - // Set the correct ownerships and permissions + // Manually set the admin and developer access for the workspace (instead of going via the service functions) adminPermissionGroup.setAssignedToUserIds(Set.of(apiUser.getId(), userAdmin.getId())); permissionGroupRepository.save(adminPermissionGroup).block(); developerPermissionGroup.setAssignedToUserIds(Set.of(userDeveloper.getId())); permissionGroupRepository.save(developerPermissionGroup).block(); + + permissionGroupService.cleanPermissionGroupCacheForUsers(List.of(userAdmin.getEmail(), userDeveloper.getEmail())).block(); } @Test