Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Revert "fix: Leave a role should be supported without permission as l… (#27364)

Browse Source 
…ong as the role is directly assigned to the user (#27129)"

This reverts commit cfc91d16d0.

> 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
This commit is contained in:
Trisha Anand 2023-09-15 22:10:42 +05:30 committed by GitHub
parent d34edec1f4
commit ca1706dac8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 22 additions and 213 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
View File

@ -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(),

1
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java
View File

@ -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"),

2
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCE.java
View File

@ -63,6 +63,4 @@ public interface PermissionGroupServiceCE extends CrudService<PermissionGroup, S
Mono<PermissionGroup> unAssignFromUserAndSendEvent(PermissionGroup permissionGroup, User user);
Mono<PermissionGroup> bulkUnAssignFromUserAndSendEvent(PermissionGroup permissionGroup, List<User> users);
Mono<Boolean> leaveExplicitlyAssignedSelfRole(String permissionGroupId);
}

46
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/PermissionGroupServiceCEImpl.java
View File

@ -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<PermissionGroupRep
.map(pg -> {
Set<Permission> 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<String, Policy> policyMap =
policySolution.generatePolicyFromPermissionGroupForObject(pg, pg.getId());
@ -370,45 +373,4 @@ public class PermissionGroupServiceCEImpl extends BaseService<PermissionGroupRep
return bulkUnassignFromUsers(permissionGroup, users)
.flatMap(permissionGroup1 -> sendEventUserRemovedFromRole(permissionGroup, usernames));
}
@Override
public Mono<Boolean> leaveExplicitlyAssignedSelfRole(String permissionGroupId) {
Mono<User> currentUserMono = sessionUserService.getCurrentUser();
Mono<PermissionGroup> 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<String> 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);
}
}

8
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java
View File

@ -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<UpdateResult> updateUserDataMono =
userMono.flatMap(user -> userDataService.removeRecentWorkspaceAndApps(user.getId(), workspaceId));
Mono<Boolean> removeUserFromOldPermissionGroupMono = oldDefaultPermissionGroupsMono.flatMap(
permissionGroup -> permissionGroupService.leaveExplicitlyAssignedSelfRole(permissionGroup.getId()));
Mono<PermissionGroup> removeUserFromOldPermissionGroupMono = oldDefaultPermissionGroupsMono
.zipWith(userMono)
.flatMap(tuple -> permissionGroupService.unassignFromUser(tuple.getT1(), tuple.getT2()));
return removeUserFromOldPermissionGroupMono.then(updateUserDataMono).then(userMono);
}

111
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PermissionGroupServiceTest.java
View File

@ -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<String> defaultPermissionGroupIds = createdWorkspace.getDefaultPermissionGroups();
List<PermissionGroup> defaultPermissionGroups = permissionGroupRepository
.findAllById(defaultPermissionGroupIds)
.collectList()
.block();
PermissionGroup adminPermissionGroup = defaultPermissionGroups.stream()
.filter(permissionGroup -> permissionGroup.getName().startsWith(ADMINISTRATOR))
.findFirst()
.get();
Mono<Boolean> leaveRoleMono = permissionGroupService
.leaveExplicitlyAssignedSelfRole(adminPermissionGroup.getId())
.cache();
Mono<PermissionGroup> 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<Workspace> 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<Boolean> leaveRoleMono = permissionGroupService
.leaveExplicitlyAssignedSelfRole(createdPermissionGroup.getId())
.cache();
Mono<PermissionGroup> permissionGroupMono = permissionGroupService.findById(createdPermissionGroup.getId());
StepVerifier.create(leaveRoleMono.then(permissionGroupMono))
.expectErrorMessage(AppsmithError.USER_NOT_ASSIGNED_TO_ROLE.getMessage(
"api_user", createdPermissionGroup.getName()))
.verify();
}
}

53
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceTest.java
View File

@ -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<MemberInfoDTO> 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();

5
app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java
View File

@ -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();
}