From ca1706dac8bf324e0c8deea99ee73173e96e9b7a Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Fri, 15 Sep 2023 22:10:42 +0530 Subject: [PATCH] =?UTF-8?q?Revert=20"fix:=20Leave=20a=20role=20should=20be?= =?UTF-8?q?=20supported=20without=20permission=20as=20l=E2=80=A6=20(#27364?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …ong as the role is directly assigned to the user (#27129)" This reverts commit cfc91d16d070235f72b392bc1f70e3f95cb08d9a. > Pull Request Template > > Use this template to quickly create a well written pull request. Delete all quotes before creating the pull request. > ## Description > Add a TL;DR when description is extra long (helps content team) > > Please include a summary of the changes and which issue has been fixed. Please also include relevant motivation > and context. List any dependencies that are required for this change > > Links to Notion, Figma or any other documents that might be relevant to the PR > > #### PR fixes following issue(s) Fixes # (issue number) > if no issue exists, please create an issue and ask the maintainers about this first > > #### Media > A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video > > #### Type of change > Please delete options that are not relevant. - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Chore (housekeeping or task changes that don't impact user perception) - This change requires a documentation update > > > ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [ ] Manual - [ ] JUnit - [ ] Jest - [ ] Cypress > > #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../server/exceptions/AppsmithError.java | 9 -- .../server/exceptions/AppsmithErrorCode.java | 1 - .../services/ce/PermissionGroupServiceCE.java | 2 - .../ce/PermissionGroupServiceCEImpl.java | 46 +------- .../ce/UserWorkspaceServiceCEImpl.java | 8 +- .../services/PermissionGroupServiceTest.java | 111 ------------------ .../services/UserWorkspaceServiceTest.java | 53 ++------- .../server/services/WorkspaceServiceTest.java | 5 +- 8 files changed, 22 insertions(+), 213 deletions(-) delete mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index 9e61c37da6..9d6ea8ff3e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -108,15 +108,6 @@ public enum AppsmithError { "User doesn''t belong to this workspace", ErrorType.INTERNAL_ERROR, null), - - USER_NOT_ASSIGNED_TO_ROLE( - 400, - AppsmithErrorCode.USER_NOT_ASSIGNED_TO_ROLE.getCode(), - "User {0} has not been assigned role {1}", - AppsmithErrorAction.DEFAULT, - "User has not been assigned to this role", - ErrorType.ARGUMENT_ERROR, - null), NO_CONFIGURATION_FOUND_IN_DATASOURCE( 400, AppsmithErrorCode.NO_CONFIGURATION_FOUND_IN_DATASOURCE.getCode(), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java index 4cdfb89a44..d9e7d6a809 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java @@ -15,7 +15,6 @@ public enum AppsmithErrorCode { DEPRECATED_API("AE-APP-4008", "Deprecated api"), USER_DOESNT_BELONG_ANY_WORKSPACE("AE-APP-4009", "User doesn't belong any workspace"), USER_DOESNT_BELONG_TO_WORKSPACE("AE-APP-4010", "User doesn't belong to workspace"), - USER_NOT_ASSIGNED_TO_ROLE("AE-APP-4011", "User is not assigned to role"), INVALID_ACTION("AE-APP-4012", "Invalid action"), PAYLOAD_TOO_LARGE("AE-APP-4013", "Payload too large"), INVALID_ACTION_NAME("AE-APP-4014", "Invalid action name"), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCE.java index 5583d5ccbd..289054cc6b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCE.java @@ -63,6 +63,4 @@ public interface PermissionGroupServiceCE extends CrudService unAssignFromUserAndSendEvent(PermissionGroup permissionGroup, User user); Mono bulkUnAssignFromUserAndSendEvent(PermissionGroup permissionGroup, List users); - - Mono leaveExplicitlyAssignedSelfRole(String permissionGroupId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java index b3361826be..c261582bc4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java @@ -26,7 +26,6 @@ import org.apache.commons.collections.CollectionUtils; import org.springframework.data.mongodb.core.ReactiveMongoTemplate; import org.springframework.data.mongodb.core.convert.MongoConverter; import org.springframework.data.mongodb.core.query.Update; -import org.springframework.util.StringUtils; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; @@ -38,6 +37,7 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import static com.appsmith.server.acl.AclPermission.UNASSIGN_PERMISSION_GROUPS; import static com.appsmith.server.constants.FieldName.PERMISSION_GROUP_ID; import static com.appsmith.server.constants.FieldName.PUBLIC_PERMISSION_GROUP; import static com.appsmith.server.repositories.ce.BaseAppsmithRepositoryCEImpl.fieldName; @@ -86,6 +86,9 @@ public class PermissionGroupServiceCEImpl extends BaseService { Set permissions = new HashSet<>( Optional.ofNullable(pg.getPermissions()).orElse(Set.of())); + // Permission to unassign self is always given + // so user can unassign himself from permission group + permissions.add(new Permission(pg.getId(), UNASSIGN_PERMISSION_GROUPS)); pg.setPermissions(permissions); Map policyMap = policySolution.generatePolicyFromPermissionGroupForObject(pg, pg.getId()); @@ -370,45 +373,4 @@ public class PermissionGroupServiceCEImpl extends BaseService sendEventUserRemovedFromRole(permissionGroup, usernames)); } - - @Override - public Mono leaveExplicitlyAssignedSelfRole(String permissionGroupId) { - - Mono currentUserMono = sessionUserService.getCurrentUser(); - - Mono permissionGroupMono = repository.findById(permissionGroupId); - - return Mono.zip(currentUserMono, permissionGroupMono) - .flatMap(tuple -> { - User currentUser = tuple.getT1(); - PermissionGroup permissionGroup = tuple.getT2(); - - String userId = currentUser.getId(); - - if (!StringUtils.hasLength(userId)) { - return Mono.error(new AppsmithException(AppsmithError.SESSION_BAD_STATE)); - } - - Set assignedToUserIds = permissionGroup.getAssignedToUserIds(); - - if (!assignedToUserIds.contains(userId)) { - return Mono.error(new AppsmithException( - AppsmithError.USER_NOT_ASSIGNED_TO_ROLE, - currentUser.getUsername(), - permissionGroup.getName())); - } - - assignedToUserIds.remove(userId); - - Update updateObj = new Update(); - String path = fieldName(QPermissionGroup.permissionGroup.assignedToUserIds); - - updateObj.set(path, assignedToUserIds); - - return Mono.zip( - repository.updateById(permissionGroupId, updateObj), - cleanPermissionGroupCacheForUsers(List.of(userId))); - }) - .map(tuple -> TRUE); - } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java index ae717c2071..fe358e0bdb 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java @@ -97,7 +97,7 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE { Workspace workspace = tuple.getT1(); User user = tuple.getT2(); return permissionGroupService.getAllByAssignedToUserAndDefaultWorkspace( - user, workspace, permissionGroupPermission.getAssignPermission()); + user, workspace, permissionGroupPermission.getUnAssignPermission()); }) /* * The below switchIfEmpty will be invoked in 2 cases. @@ -106,7 +106,6 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE { */ .switchIfEmpty(Mono.error(new AppsmithException( AppsmithError.ACTION_IS_NOT_AUTHORIZED, "Workspace is not assigned to the user."))) - // User must be assigned to a single default role of the workspace. We can safely use single() here. .single() .flatMap(permissionGroup -> { if (permissionGroup.getName().startsWith(FieldName.ADMINISTRATOR) @@ -124,8 +123,9 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE { Mono updateUserDataMono = userMono.flatMap(user -> userDataService.removeRecentWorkspaceAndApps(user.getId(), workspaceId)); - Mono removeUserFromOldPermissionGroupMono = oldDefaultPermissionGroupsMono.flatMap( - permissionGroup -> permissionGroupService.leaveExplicitlyAssignedSelfRole(permissionGroup.getId())); + Mono removeUserFromOldPermissionGroupMono = oldDefaultPermissionGroupsMono + .zipWith(userMono) + .flatMap(tuple -> permissionGroupService.unassignFromUser(tuple.getT1(), tuple.getT2())); return removeUserFromOldPermissionGroupMono.then(updateUserDataMono).then(userMono); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java deleted file mode 100644 index 7d7183228a..0000000000 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java +++ /dev/null @@ -1,111 +0,0 @@ -package com.appsmith.server.services; - -import com.appsmith.server.domains.PermissionGroup; -import com.appsmith.server.domains.Workspace; -import com.appsmith.server.exceptions.AppsmithError; -import com.appsmith.server.exceptions.AppsmithException; -import com.appsmith.server.repositories.PermissionGroupRepository; -import com.appsmith.server.repositories.UserRepository; -import lombok.extern.slf4j.Slf4j; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.security.test.context.support.WithUserDetails; -import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit.jupiter.SpringExtension; -import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; - -import java.util.List; -import java.util.Set; - -import static com.appsmith.server.acl.AclPermission.READ_WORKSPACES; -import static com.appsmith.server.constants.ce.FieldNameCE.ADMINISTRATOR; -import static org.assertj.core.api.AssertionsForClassTypes.assertThat; - -@Slf4j -@ExtendWith(SpringExtension.class) -@SpringBootTest -@DirtiesContext -public class PermissionGroupServiceTest { - - @Autowired - PermissionGroupService permissionGroupService; - - @Autowired - PermissionGroupRepository permissionGroupRepository; - - @Autowired - WorkspaceService workspaceService; - - @Autowired - UserRepository userRepository; - - @Test - @WithUserDetails(value = "api_user") - public void valid_leaveRole() { - - Workspace workspace = new Workspace(); - workspace.setName("valid_leaveRole"); - - Workspace createdWorkspace = workspaceService.create(workspace).block(); - - Set defaultPermissionGroupIds = createdWorkspace.getDefaultPermissionGroups(); - List defaultPermissionGroups = permissionGroupRepository - .findAllById(defaultPermissionGroupIds) - .collectList() - .block(); - - PermissionGroup adminPermissionGroup = defaultPermissionGroups.stream() - .filter(permissionGroup -> permissionGroup.getName().startsWith(ADMINISTRATOR)) - .findFirst() - .get(); - - Mono leaveRoleMono = permissionGroupService - .leaveExplicitlyAssignedSelfRole(adminPermissionGroup.getId()) - .cache(); - - Mono permissionGroupMono = permissionGroupService.findById(adminPermissionGroup.getId()); - - String api_user_id = userRepository.findByEmail("api_user").block().getId(); - - StepVerifier.create(leaveRoleMono.then(permissionGroupMono)) - .assertNext(permissionGroup -> { - assertThat(permissionGroup.getAssignedToUserIds().contains(api_user_id)) - .isFalse(); - }) - .verifyComplete(); - - // Assert that the api_user does not have access to the workspace - Mono workspaceMono = leaveRoleMono - .then(workspaceService.findById(createdWorkspace.getId(), READ_WORKSPACES)) - .switchIfEmpty(Mono.error( - new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, "workspace", createdWorkspace.getId()))); - - StepVerifier.create(workspaceMono) - .expectErrorMessage(AppsmithError.NO_RESOURCE_FOUND.getMessage("workspace", createdWorkspace.getId())) - .verify(); - } - - @Test - @WithUserDetails(value = "api_user") - public void invalid_leaveRole() { - - PermissionGroup testPermissionGroup = new PermissionGroup(); - testPermissionGroup.setName("invalid_leaveRole"); - PermissionGroup createdPermissionGroup = - permissionGroupService.create(testPermissionGroup).block(); - - Mono leaveRoleMono = permissionGroupService - .leaveExplicitlyAssignedSelfRole(createdPermissionGroup.getId()) - .cache(); - - Mono permissionGroupMono = permissionGroupService.findById(createdPermissionGroup.getId()); - - StepVerifier.create(leaveRoleMono.then(permissionGroupMono)) - .expectErrorMessage(AppsmithError.USER_NOT_ASSIGNED_TO_ROLE.getMessage( - "api_user", createdPermissionGroup.getName())) - .verify(); - } -} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceTest.java index f744e5f192..4bfd8db13d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceTest.java @@ -3,6 +3,7 @@ package com.appsmith.server.services; import com.appsmith.external.models.Policy; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.acl.AppsmithRole; +import com.appsmith.server.acl.PolicyGenerator; import com.appsmith.server.constants.FieldName; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.PermissionGroup; @@ -12,7 +13,7 @@ import com.appsmith.server.domains.Workspace; import com.appsmith.server.dtos.MemberInfoDTO; import com.appsmith.server.dtos.UpdatePermissionGroupDTO; import com.appsmith.server.exceptions.AppsmithError; -import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.repositories.ApplicationRepository; import com.appsmith.server.repositories.PermissionGroupRepository; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.repositories.WorkspaceRepository; @@ -73,19 +74,20 @@ public class UserWorkspaceServiceTest { @Autowired private PolicySolution policySolution; + @Autowired + private ApplicationRepository applicationRepository; + + @Autowired + private PolicyGenerator policyGenerator; + @Autowired private UserDataService userDataService; @Autowired private ApplicationPageService applicationPageService; - @Autowired - PermissionGroupService permissionGroupService; - private Workspace workspace; - PermissionGroup developerPermissionGroup; - User api_user; - User developer_user; + private User user; @BeforeEach @WithUserDetails(value = "api_user") @@ -102,7 +104,7 @@ public class UserWorkspaceServiceTest { .collectList() .block(); - developerPermissionGroup = permissionGroups.stream() + PermissionGroup developerPermissionGroup = permissionGroups.stream() .filter(permissionGroup -> permissionGroup.getName().startsWith(DEVELOPER)) .findFirst() .get(); @@ -112,9 +114,8 @@ public class UserWorkspaceServiceTest { .findFirst() .get(); - api_user = userService.findByEmail("api_user").block(); + User api_user = userService.findByEmail("api_user").block(); User usertest = userService.findByEmail("usertest@usertest.com").block(); - developer_user = userService.findByEmail("developer@solutiontest.com").block(); // Make api_user a developer and not an administrator // Make user_test an administrator @@ -344,38 +345,6 @@ public class UserWorkspaceServiceTest { .verifyComplete(); } - @Test - @WithUserDetails(value = "api_user") - public void invalid_removeAnotherDeveloperAsDeveloperInWorkspace() { - developerPermissionGroup.setAssignedToUserIds(Set.of(api_user.getId(), developer_user.getId())); - permissionGroupRepository.save(developerPermissionGroup).block(); - permissionGroupService - .cleanPermissionGroupCacheForUsers(List.of(api_user.getId(), developer_user.getId())) - .block(); - - UpdatePermissionGroupDTO updatePermissionGroupDTO = new UpdatePermissionGroupDTO(); - updatePermissionGroupDTO.setNewPermissionGroupId(null); - updatePermissionGroupDTO.setUsername(developer_user.getUsername()); - - Mono removeDeveloperMono = userWorkspaceService.updatePermissionGroupForMember( - workspace.getId(), updatePermissionGroupDTO, "origin"); - - StepVerifier.create(removeDeveloperMono) - .expectErrorMatches(throwable -> throwable instanceof AppsmithException - && throwable - .getMessage() - .equals(AppsmithError.ACTION_IS_NOT_AUTHORIZED.getMessage( - "Change permissionGroup of a member"))) - .verify(); - - // Cleanup the special state created for this test case - developerPermissionGroup.setAssignedToUserIds(Set.of(api_user.getId())); - permissionGroupRepository.save(developerPermissionGroup).block(); - permissionGroupService - .cleanPermissionGroupCacheForUsers(List.of(api_user.getId(), developer_user.getId())) - .block(); - } - @AfterEach public void clear() { User currentUser = userRepository.findByEmail("api_user").block(); 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 eac969a106..e86c4fa15d 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 @@ -399,7 +399,8 @@ public class WorkspaceServiceTest { .filter(policy -> policy.getPermission().equals(UNASSIGN_PERMISSION_GROUPS.getValue())) .findFirst() .ifPresent(policy -> assertThat(policy.getPermissionGroups()) - .containsAll(Set.of(adminPermissionGroup.getId()))); + .containsAll( + Set.of(adminPermissionGroup.getId(), developerPermissionGroup.getId()))); // Assert viewer permission group policies viewerPermissionGroup.getPolicies().stream() @@ -422,7 +423,7 @@ public class WorkspaceServiceTest { .filter(policy -> policy.getPermission().equals(UNASSIGN_PERMISSION_GROUPS.getValue())) .findFirst() .ifPresent(policy -> assertThat(policy.getPermissionGroups()) - .containsAll(Set.of(adminPermissionGroup.getId()))); + .containsAll(Set.of(adminPermissionGroup.getId(), viewerPermissionGroup.getId()))); }) .verifyComplete(); }