From 52e7ed4f02ddf8f8a6af2d834f26aad0d3201973 Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Thu, 13 Aug 2020 18:53:00 +0530 Subject: [PATCH] Bug Fix : When role changes from developer to admin, the user was not being given `make application public` permission for the application (#302) * During add role to an organziation, the application was only inheriting from subset of the organization permissions. Generalized this code to ensure that this doesnt happen again in the future when more permissions are introduced. Refactored some code as well. --- .../appsmith/server/acl/PolicyGenerator.java | 4 +- .../appsmith/server/helpers/PolicyUtils.java | 54 ++++--------------- .../services/ApplicationServiceImpl.java | 4 +- .../services/UserOrganizationServiceImpl.java | 24 ++++----- .../server/services/UserServiceImpl.java | 3 +- 5 files changed, 27 insertions(+), 62 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/acl/PolicyGenerator.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/acl/PolicyGenerator.java index 3903eac04a..dc40bf3b16 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/acl/PolicyGenerator.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/acl/PolicyGenerator.java @@ -176,11 +176,11 @@ public class PolicyGenerator { return childPolicySet; } - public Set getAllChildPolicies(Set policySet, Class inheritingEntity, Class destinationEntity) { + public Set getAllChildPolicies(Set policySet, Class sourceEntity, Class destinationEntity) { Set policies = policySet.stream() .map(policy -> { AclPermission aclPermission = AclPermission - .getPermissionByValue(policy.getPermission(), inheritingEntity); + .getPermissionByValue(policy.getPermission(), sourceEntity); // Get all the child policies for the given policy and aclPermission return getChildPolicies(policy, aclPermission, destinationEntity); }).flatMap(Collection::stream) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PolicyUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PolicyUtils.java index 0658fd185a..e2e691c57e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PolicyUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PolicyUtils.java @@ -7,7 +7,6 @@ import com.appsmith.server.acl.PolicyGenerator; import com.appsmith.server.domains.Action; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.Datasource; -import com.appsmith.server.domains.Organization; import com.appsmith.server.domains.Page; import com.appsmith.server.domains.User; import com.appsmith.server.repositories.ActionRepository; @@ -27,13 +26,6 @@ import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; -import static com.appsmith.server.acl.AclPermission.MANAGE_APPLICATIONS; -import static com.appsmith.server.acl.AclPermission.MANAGE_PAGES; -import static com.appsmith.server.acl.AclPermission.ORGANIZATION_MANAGE_APPLICATIONS; -import static com.appsmith.server.acl.AclPermission.ORGANIZATION_READ_APPLICATIONS; -import static com.appsmith.server.acl.AclPermission.READ_APPLICATIONS; -import static com.appsmith.server.acl.AclPermission.READ_PAGES; - @Component public class PolicyUtils { @@ -148,18 +140,6 @@ public class PolicyUtils { .collect(Collectors.toMap(Policy::getPermission, Function.identity())); } - public Map generateChildrenPoliciesFromOrganizationPolicies(Map orgPolicyMap, Class destinationEntity) { - Set extractedInterestingPolicySet = new HashSet<>(orgPolicyMap.values()) - .stream() - .filter(policy -> policy.getPermission().equals(ORGANIZATION_MANAGE_APPLICATIONS.getValue()) - || policy.getPermission().equals(ORGANIZATION_READ_APPLICATIONS.getValue())) - .collect(Collectors.toSet()); - - return policyGenerator.getAllChildPolicies(extractedInterestingPolicySet, Organization.class, destinationEntity) - .stream() - .collect(Collectors.toMap(Policy::getPermission, Function.identity())); - } - public Flux updateWithNewPoliciesToDatasourcesByOrgId(String orgId, Map newPoliciesMap, boolean addPolicyToObject) { return datasourceRepository @@ -194,18 +174,6 @@ public class PolicyUtils { .flatMapMany(updatedApplications -> applicationRepository.saveAll(updatedApplications)); } - public Map generatePagePoliciesFromApplicationPolicies(Map applicationPolicyMap) { - Set extractedInterestingPolicySet = new HashSet<>(applicationPolicyMap.values()) - .stream() - .filter(policy -> policy.getPermission().equals(MANAGE_APPLICATIONS.getValue()) - || policy.getPermission().equals(READ_APPLICATIONS.getValue())) - .collect(Collectors.toSet()); - - return policyGenerator.getAllChildPolicies(extractedInterestingPolicySet, Application.class, Page.class) - .stream() - .collect(Collectors.toMap(Policy::getPermission, Function.identity())); - } - public Flux updateWithApplicationPermissionsToAllItsPages(String applicationId, Map newPagePoliciesMap, boolean addPolicyToObject) { return pageRepository @@ -222,18 +190,6 @@ public class PolicyUtils { .flatMapMany(updatedPages -> pageRepository.saveAll(updatedPages)); } - public Map generateActionPoliciesFromPagePolicies(Map pagePolicyMap) { - Set extractedInterestingPolicySet = new HashSet<>(pagePolicyMap.values()) - .stream() - .filter(policy -> policy.getPermission().equals(MANAGE_PAGES.getValue()) - || policy.getPermission().equals(READ_PAGES.getValue())) - .collect(Collectors.toSet()); - - return policyGenerator.getAllChildPolicies(extractedInterestingPolicySet, Page.class, Action.class) - .stream() - .collect(Collectors.toMap(Policy::getPermission, Function.identity())); - } - public Flux updateWithPagePermissionsToAllItsActions(String pageId, Map newActionPoliciesMap, boolean addPolicyToObject) { return actionRepository @@ -255,4 +211,14 @@ public class PolicyUtils { .collectList() .flatMapMany(updatedActions -> actionRepository.saveAll(updatedActions)); } + + public Map generateInheritedPoliciesFromSourcePolicies(Map sourcePolicyMap, + Class sourceEntity, + Class destinationEntity) { + Set extractedInterestingPolicySet = new HashSet<>(sourcePolicyMap.values()); + + return policyGenerator.getAllChildPolicies(extractedInterestingPolicySet, sourceEntity, destinationEntity) + .stream() + .collect(Collectors.toMap(Policy::getPermission, Function.identity())); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java index 0625d61916..57121a8626 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationServiceImpl.java @@ -213,8 +213,8 @@ public class ApplicationServiceImpl extends BaseService applicationPolicyMap = policyUtils.generatePolicyFromPermission(Set.of(applicationPermission), user); - Map pagePolicyMap = policyUtils.generatePagePoliciesFromApplicationPolicies(applicationPolicyMap); - Map actionPolicyMap = policyUtils.generateActionPoliciesFromPagePolicies(pagePolicyMap); + Map pagePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(applicationPolicyMap, Application.class, Page.class); + Map actionPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(pagePolicyMap, Page.class, Action.class); Map datasourcePolicyMap = policyUtils.generatePolicyFromPermission(Set.of(datasourcePermission), user); Flux updatedPagesFlux = policyUtils.updateWithApplicationPermissionsToAllItsPages(application.getId(), pagePolicyMap, isPublic); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserOrganizationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserOrganizationServiceImpl.java index 5b40a3dea1..482f485c5f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserOrganizationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserOrganizationServiceImpl.java @@ -152,10 +152,10 @@ public class UserOrganizationServiceImpl implements UserOrganizationService { // Generate all the policies for Organization, Application, Page and Actions for the current user Set rolePermissions = role.getPermissions(); Map orgPolicyMap = policyUtils.generatePolicyFromPermission(rolePermissions, user); - Map applicationPolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Application.class); - Map datasourcePolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Datasource.class); - Map pagePolicyMap = policyUtils.generatePagePoliciesFromApplicationPolicies(applicationPolicyMap); - Map actionPolicyMap = policyUtils.generateActionPoliciesFromPagePolicies(pagePolicyMap); + Map applicationPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Application.class); + Map datasourcePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Datasource.class); + Map pagePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(applicationPolicyMap, Application.class, Page.class); + Map actionPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(pagePolicyMap, Page.class, Action.class); //Now update the organization policies Organization updatedOrganization = policyUtils.addPoliciesToExistingObject(orgPolicyMap, organization); @@ -215,10 +215,10 @@ public class UserOrganizationServiceImpl implements UserOrganizationService { // Generate all the policies for Organization, Application, Page and Actions Set rolePermissions = role.getPermissions(); Map orgPolicyMap = policyUtils.generatePolicyFromPermission(rolePermissions, user); - Map applicationPolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Application.class); - Map datasourcePolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Datasource.class); - Map pagePolicyMap = policyUtils.generatePagePoliciesFromApplicationPolicies(applicationPolicyMap); - Map actionPolicyMap = policyUtils.generateActionPoliciesFromPagePolicies(pagePolicyMap); + Map applicationPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Application.class); + Map datasourcePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Datasource.class); + Map pagePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(applicationPolicyMap, Application.class, Page.class); + Map actionPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(pagePolicyMap, Page.class, Action.class); //Now update the organization policies Organization updatedOrganization = policyUtils.removePoliciesFromExistingObject(orgPolicyMap, organization); @@ -333,10 +333,10 @@ public class UserOrganizationServiceImpl implements UserOrganizationService { // Generate all the policies for Organization, Application, Page and Actions for the current user Set rolePermissions = role.getPermissions(); Map orgPolicyMap = policyUtils.generatePolicyFromPermissionForMultipleUsers(rolePermissions, users); - Map applicationPolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Application.class); - Map datasourcePolicyMap = policyUtils.generateChildrenPoliciesFromOrganizationPolicies(orgPolicyMap, Datasource.class); - Map pagePolicyMap = policyUtils.generatePagePoliciesFromApplicationPolicies(applicationPolicyMap); - Map actionPolicyMap = policyUtils.generateActionPoliciesFromPagePolicies(pagePolicyMap); + Map applicationPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Application.class); + Map datasourcePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(orgPolicyMap, Organization.class, Datasource.class); + Map pagePolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(applicationPolicyMap, Application.class, Page.class); + Map actionPolicyMap = policyUtils.generateInheritedPoliciesFromSourcePolicies(pagePolicyMap, Page.class, Action.class); //Now update the organization policies Organization updatedOrganization = (Organization) policyUtils.addPoliciesToExistingObject(orgPolicyMap, organization); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java index 7e6b7773db..8862b1b9ee 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java @@ -282,8 +282,7 @@ public class UserServiceImpl extends BaseService i .findByEmail(user.getEmail()) .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, "token", token))) .flatMap(passwordResetTokenRepository::delete) - .thenReturn(userFromDb) - .flatMap(repository::save) + .then(repository.save(userFromDb)) .thenReturn(true); }); }