Skip to content

Commit

Permalink
Fixed comment missing issue when user role is changed (appsmithorg#5898)
Browse files Browse the repository at this point in the history
* -fixed comment missing issue when user role is changed

* WIP: add test for policy utils

* -added test for policy utils comment permission when users are added or removed

* -removed unused code

* -removed public access modifier for an internal function

* -add test to verify comment thread policy updated when user role changed in organization

* -add tests for add user and remove user from organization to test comment thread policies
  • Loading branch information
nayan-rafiq authored Jul 21, 2021
1 parent dea9f26 commit 5261f78
Show file tree
Hide file tree
Showing 5 changed files with 354 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ public <T extends BaseDomain> T addPoliciesToExistingObject(Map<String, Policy>
// TODO: Investigate a solution without using deep-copy.
final Map<String, Policy> policyMap1 = new HashMap<>();
for (Map.Entry<String, Policy> entry : policyMap.entrySet()) {
policyMap1.put(entry.getKey(), entry.getValue());
Policy entryValue = entry.getValue();
Policy policy = Policy.builder()
.users(new HashSet<>(entryValue.getUsers()))
.permission(entryValue.getPermission())
.groups(new HashSet<>(entryValue.getGroups()))
.build();
policyMap1.put(entry.getKey(), policy);
}

// Append the user to the existing permission policy if it already exists.
Expand Down Expand Up @@ -221,7 +227,7 @@ public Flux<NewPage> updateWithApplicationPermissionsToAllItsPages(String applic
.saveAll(updatedPages));
}

public Flux<CommentThread> updateWithApplicationPermissionsToAllItsCommentThreads(
public Flux<CommentThread> updateCommentThreadPermissions(
String applicationId, Map<String, Policy> commentThreadPolicyMap, String username, boolean addPolicyToObject) {

return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,12 @@
public interface UserOrganizationService {
Mono<User> addUserToOrganization(String orgId, User user);

Mono<User> saveUser(User user);

Mono<Organization> addUserRoleToOrganization(String orgId, UserRole userRole);

Mono<Organization> addUserToOrganizationGivenUserObject(Organization organization, User user, UserRole userRole);

Mono<User> leaveOrganization(String orgId);

Mono<Organization> removeUserRoleFromOrganizationGivenUserObject(Organization organization, User user);

Mono<UserRole> updateRoleForMember(String orgId, UserRole userRole, String originHeader);

Mono<Organization> bulkAddUsersToOrganization(Organization organization, List<User> users, String roleName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,6 @@ public Mono<User> addUserToOrganization(String orgId, User user) {
.flatMap(userRepository::save);
}

@Override
public Mono<User> saveUser(User user) {
return userRepository.save(user);
}

@Override
public Mono<Organization> addUserRoleToOrganization(String orgId, UserRole userRole) {
Mono<Organization> organizationMono = organizationRepository.findById(orgId, MANAGE_ORGANIZATIONS)
Expand Down Expand Up @@ -169,7 +164,9 @@ public Mono<Organization> addUserToOrganizationGivenUserObject(Organization orga
Map<String, Policy> datasourcePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Datasource.class);
Map<String, Policy> pagePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(applicationPolicyMap, Application.class, Page.class);
Map<String, Policy> actionPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(pagePolicyMap, Page.class, Action.class);

Map<String, Policy> commentThreadPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(
applicationPolicyMap, Application.class, CommentThread.class
);
//Now update the organization policies
Organization updatedOrganization = policyUtils.addPoliciesToExistingObject(orgPolicyMap, organization);
updatedOrganization.setUserRoles(userRoles);
Expand All @@ -182,13 +179,21 @@ public Mono<Organization> addUserToOrganizationGivenUserObject(Organization orga
.flatMap(application -> policyUtils.updateWithApplicationPermissionsToAllItsPages(application.getId(), pagePolicyMap, true));
Flux<NewAction> updatedActionsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateWithPagePermissionsToAllItsActions(application.getId(), actionPolicyMap, true));
Flux<CommentThread> updatedThreadsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateCommentThreadPermissions(application.getId(), commentThreadPolicyMap, user.getUsername(), true));

return Mono.zip(updatedDatasourcesFlux.collectList(), updatedPagesFlux.collectList(), updatedActionsFlux.collectList(), Mono.just(updatedOrganization))
.flatMap(tuple -> {
//By now all the datasources/applications/pages/actions have been updated. Just save the organization now
Organization updatedOrgBeforeSave = tuple.getT4();
return organizationRepository.save(updatedOrgBeforeSave);
});
return Mono.zip(
updatedDatasourcesFlux.collectList(),
updatedPagesFlux.collectList(),
updatedActionsFlux.collectList(),
Mono.just(updatedOrganization),
updatedThreadsFlux.collectList()
)
.flatMap(tuple -> {
//By now all the datasources/applications/pages/actions have been updated. Just save the organization now
Organization updatedOrgBeforeSave = tuple.getT4();
return organizationRepository.save(updatedOrgBeforeSave);
});
}

@Override
Expand All @@ -212,8 +217,7 @@ public Mono<User> leaveOrganization(String orgId) {
});
}

@Override
public Mono<Organization> removeUserRoleFromOrganizationGivenUserObject(Organization organization, User user) {
private Mono<Organization> removeUserRoleFromOrganizationGivenUserObject(Organization organization, User user) {
List<UserRole> userRoles = organization.getUserRoles();
if (userRoles == null) {
return Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.USER + " in organization", organization.getName()));
Expand Down Expand Up @@ -258,7 +262,7 @@ public Mono<Organization> removeUserRoleFromOrganizationGivenUserObject(Organiza
Flux<NewAction> updatedActionsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateWithPagePermissionsToAllItsActions(application.getId(), actionPolicyMap, false));
Flux<CommentThread> updatedThreadsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateWithApplicationPermissionsToAllItsCommentThreads(
.flatMap(application -> policyUtils.updateCommentThreadPermissions(
application.getId(), commentThreadPolicyMap, user.getUsername(), false
));

Expand Down Expand Up @@ -434,7 +438,7 @@ public Mono<Organization> bulkAddUsersToOrganization(Organization organization,
Flux<NewAction> updatedActionsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateWithPagePermissionsToAllItsActions(application.getId(), actionPolicyMap, true));
Flux<CommentThread> updatedThreadsFlux = updatedApplicationsFlux
.flatMap(application -> policyUtils.updateWithApplicationPermissionsToAllItsCommentThreads(
.flatMap(application -> policyUtils.updateCommentThreadPermissions(
application.getId(), commentThreadPolicyMap, null, true
));

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package com.appsmith.server.helpers;

import com.appsmith.external.models.Policy;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.domains.CommentThread;
import com.appsmith.server.domains.User;
import com.appsmith.server.repositories.CommentThreadRepository;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
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.context.junit4.SpringRunner;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.test.StepVerifier;

import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.assertj.core.api.Assertions.assertThat;

@SpringBootTest
@RunWith(SpringRunner.class)
public class PolicyUtilsTest {

@Autowired
private PolicyUtils policyUtils;

@Autowired
private CommentThreadRepository commentThreadRepository;

@Before
public void cleanUp() {
commentThreadRepository.deleteAll().block();
}

@Test
@WithUserDetails("api_user")
public void updateWithApplicationPermissionsToAllItsCommentThreads_AddPermissions_PermissionsAdded() {
// create a thread
String testApplicationId = "test-application-id";
CommentThread commentThread = new CommentThread();
commentThread.setApplicationId(testApplicationId);
Map<String, Policy> commentThreadPolicies = policyUtils.generatePolicyFromPermission(
Set.of(AclPermission.MANAGE_THREAD, AclPermission.COMMENT_ON_THREAD), "api_user"
);
commentThread.setPolicies(Set.copyOf(commentThreadPolicies.values()));
Mono<CommentThread> saveThreadMono = commentThreadRepository.save(commentThread);

// add a new user and update the policies of the new user
String newUserName = "new_test_user";
Map<String, Policy> commentThreadPoliciesForNewUser = policyUtils.generatePolicyFromPermission(
Set.of(AclPermission.COMMENT_ON_THREAD), newUserName
);
Flux<CommentThread> updateCommentThreads = policyUtils.updateCommentThreadPermissions(
testApplicationId, commentThreadPoliciesForNewUser, newUserName, true
);

// check if new policies updated
Mono<List<CommentThread>> applicationCommentList = saveThreadMono
.thenMany(updateCommentThreads)
.collectList()
.thenMany(commentThreadRepository.findByApplicationId(testApplicationId, AclPermission.READ_THREAD))
.collectList();

StepVerifier.create(applicationCommentList)
.assertNext(commentThreads -> {
assertThat(commentThreads.size()).isEqualTo(1);
CommentThread commentThread1 = commentThreads.get(0);
Set<Policy> policies = commentThread1.getPolicies();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.MANAGE_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.MANAGE_THREAD.getValue(), newUserName)).isFalse();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.READ_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.READ_THREAD.getValue(), newUserName)).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.COMMENT_ON_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.COMMENT_ON_THREAD.getValue(), newUserName)).isTrue();
})
.verifyComplete();
}

@Test
@WithUserDetails("api_user")
public void updateWithApplicationPermissionsToAllItsCommentThreads_RemovePermissions_PermissionsRemoved() {
String newUserName = "new_test_user";

// create a thread with two users having permission on it
String testApplicationId = "test-application-id";
CommentThread commentThread = new CommentThread();
commentThread.setApplicationId(testApplicationId);
commentThread.setPolicies(new HashSet<>());

User user1 = new User();
user1.setEmail("api_user");

User user2 = new User();
user2.setEmail(newUserName);

Map<String, Policy> commentThreadPolicies = policyUtils.generatePolicyFromPermissionForMultipleUsers(
Set.of(AclPermission.MANAGE_THREAD, AclPermission.COMMENT_ON_THREAD), List.of(user1, user2)
);

commentThread.setPolicies(Set.copyOf(commentThreadPolicies.values()));
Mono<CommentThread> saveThreadMono = commentThreadRepository.save(commentThread);

// remove an user and update the policies of the user
Map<String, Policy> commentThreadPoliciesForNewUser = policyUtils.generatePolicyFromPermission(
Set.of(AclPermission.MANAGE_THREAD, AclPermission.COMMENT_ON_THREAD), newUserName
);
Flux<CommentThread> updateCommentThreads = policyUtils.updateCommentThreadPermissions(
testApplicationId, commentThreadPoliciesForNewUser, newUserName, false
);

// check if new policies updated
Mono<List<CommentThread>> applicationCommentList = saveThreadMono
.thenMany(updateCommentThreads)
.collectList()
.thenMany(commentThreadRepository.findByApplicationId(testApplicationId, AclPermission.READ_THREAD))
.collectList();

StepVerifier.create(applicationCommentList)
.assertNext(commentThreads -> {
assertThat(commentThreads.size()).isEqualTo(1);
CommentThread commentThread1 = commentThreads.get(0);
Set<Policy> policies = commentThread1.getPolicies();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.MANAGE_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.MANAGE_THREAD.getValue(), newUserName)).isFalse();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.READ_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.READ_THREAD.getValue(), newUserName)).isFalse();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.COMMENT_ON_THREAD.getValue(), "api_user")).isTrue();
assertThat(policyUtils.isPermissionPresentForUser(policies, AclPermission.COMMENT_ON_THREAD.getValue(), newUserName)).isFalse();
})
.verifyComplete();
}
}
Loading

0 comments on commit 5261f78

Please sign in to comment.