From 6282932c897fbe2e811291d147481e50335ce7bc Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Mon, 20 Feb 2023 20:37:38 +0530 Subject: [PATCH] chore: refactoring workspace creation and archive methods to facilitate environment creation in EE. (#20494) ## Description > In Enterprise edition, the environments will be created by default when we create a new workspace. However to facilitate those changes, and to keep code maintainable, these snippets are neccesary. > This PR has refactored workspace creation flow into separate methods to - allow addition of resourceIds in `permissionGroup` permissions in EE. - added static method to reduce code repeatability > This PR has changed some methods in workspaceServiceTest, those methods were using same workspace object to create new workspaces when called in a loop, however the workspace object used to create was assigned id after first `workspaceService.create` call, the subsequent create calls returned same workspace object, furthermore the linked dependents were new each time causing problem. As a fix, creating a new workspace object each time for create calls in loop. ## TL;DR > Refactor to facilitate the default environment creation PR in the EE Fixes https://github.com/appsmithorg/appsmith/issues/20790 > This issue is a facilitator PR for [default environments with workspace PR](https://github.com/appsmithorg/appsmith-ee/pull/1027). ## Type of change - Chore (housekeeping or task changes that don't impact user perception) ## How Has This Been Tested? - Manual ## Checklist: ### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] 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 - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag --- .../server/helpers/PermissionUtils.java | 16 ++++ .../services/ce/WorkspaceServiceCEImpl.java | 96 ++++++++++--------- .../server/services/WorkspaceServiceTest.java | 52 +++++----- 3 files changed, 93 insertions(+), 71 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PermissionUtils.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PermissionUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PermissionUtils.java new file mode 100644 index 0000000000..99f2c4f7e7 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/PermissionUtils.java @@ -0,0 +1,16 @@ +package com.appsmith.server.helpers; + +import com.appsmith.server.dtos.Permission; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +public class PermissionUtils { + + public static Set collateAllPermissions(Set... permissionArgs) { + Set permissions = new HashSet<>(); + Arrays.stream(permissionArgs).peek(permissions::addAll).toList(); + return permissions; + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java index 04b92b9135..7f8dccfdf0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java @@ -46,6 +46,7 @@ import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; import jakarta.validation.Validator; + import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -67,6 +68,7 @@ import static com.appsmith.server.constants.FieldName.WORKSPACE_DEVELOPER_DESCRI import static com.appsmith.server.constants.FieldName.WORKSPACE_VIEWER_DESCRIPTION; import static com.appsmith.server.constants.PatternConstants.EMAIL_PATTERN; import static com.appsmith.server.constants.PatternConstants.WEBSITE_PATTERN; +import static com.appsmith.server.helpers.PermissionUtils.collateAllPermissions; import static java.lang.Boolean.TRUE; @Slf4j @@ -81,7 +83,7 @@ public class WorkspaceServiceCEImpl extends BaseService { - return Mono.zip(generateDefaultPermissionGroups(createdWorkspace1, user), Mono.just(createdWorkspace1)) - .flatMap(tuple -> { - - Set permissionGroups = tuple.getT1(); - Workspace createdWorkspace = tuple.getT2(); - - createdWorkspace.setDefaultPermissionGroups( - permissionGroups.stream() - .map(PermissionGroup::getId) - .collect(Collectors.toSet()) - ); - - // Apply the permissions to the workspace - for (PermissionGroup permissionGroup : permissionGroups) { - Map policyMap = policyUtils.generatePolicyFromPermissionGroupForObject(permissionGroup, createdWorkspace.getId()); - - createdWorkspace = policyUtils.addPoliciesToExistingObject(policyMap, createdWorkspace); - } - - return repository.save(createdWorkspace); - }); - }) + .flatMap(createdWorkspace -> enrichWorkspaceWithDependents(createdWorkspace, user)) .flatMap(analyticsService::sendCreateEvent); } + protected Mono enrichWorkspaceWithDependents(Workspace createdWorkspace, User user) { + // Generate the default permission groups & policy for the current user + return generateDefaultPermissionGroups(createdWorkspace, user) + .flatMap(permissionGroups -> enrichDependents(permissionGroups,createdWorkspace) + .then(addPoliciesAndSaveWorkspace(permissionGroups, createdWorkspace))); + } + + /** + * returns the created workspace without any Operations + * See EE overrides for complete usage + * @param permissionGroups + * @param createdWorkspace + * @return Mono of the createdWorkspace + */ + protected Mono enrichDependents(Set permissionGroups, Workspace createdWorkspace) { + return Mono.just(createdWorkspace); + } + + protected Mono addPoliciesAndSaveWorkspace(Set permissionGroups, Workspace createdWorkspace) { + createdWorkspace.setDefaultPermissionGroups( + permissionGroups.stream() + .map(PermissionGroup::getId) + .collect(Collectors.toSet())); + // Apply the permissions to the workspace + for (PermissionGroup permissionGroup : permissionGroups) { + Map policyMap = policyUtils.generatePolicyFromPermissionGroupForObject(permissionGroup, createdWorkspace.getId()); + createdWorkspace = policyUtils.addPoliciesToExistingObject(policyMap, createdWorkspace); + } + return repository.save(createdWorkspace); + } + protected Mono isCreateWorkspaceAllowed(Boolean isDefaultWorkspace) { return Mono.just(TRUE); } @@ -285,7 +295,8 @@ public class WorkspaceServiceCEImpl extends BaseService> generatePermissionsForDefaultPermissionGroups(Set permissionGroups, Workspace workspace, User user) { + Mono> generatePermissionsForDefaultPermissionGroups(Set permissionGroups, + Workspace workspace, User user) { PermissionGroup adminPermissionGroup = permissionGroups.stream() .filter(permissionGroup -> permissionGroup.getName().startsWith(ADMINISTRATOR)) .findFirst().get(); @@ -316,11 +327,10 @@ public class WorkspaceServiceCEImpl extends BaseService permissions = new HashSet<>(); - permissions.addAll(workspacePermissions); - permissions.addAll(assignPermissionGroupPermissions); - permissions.addAll(readPermissionGroupPermissions); - permissions.addAll(unassignPermissionGroupPermissions); + Set permissions = collateAllPermissions(workspacePermissions, + assignPermissionGroupPermissions, + readPermissionGroupPermissions, + unassignPermissionGroupPermissions); adminPermissionGroup.setPermissions(permissions); // Assign the user creating the permission group to this permission group @@ -337,10 +347,9 @@ public class WorkspaceServiceCEImpl extends BaseService new Permission(permissionGroup.getId(), ASSIGN_PERMISSION_GROUPS)) .collect(Collectors.toSet()); - permissions = new HashSet<>(); - permissions.addAll(workspacePermissions); - permissions.addAll(assignPermissionGroupPermissions); - permissions.addAll(readPermissionGroupPermissions); + permissions = collateAllPermissions(workspacePermissions, + assignPermissionGroupPermissions, + readPermissionGroupPermissions); developerPermissionGroup.setPermissions(permissions); // App Viewer Permissions @@ -354,10 +363,10 @@ public class WorkspaceServiceCEImpl extends BaseService new Permission(permissionGroup.getId(), ASSIGN_PERMISSION_GROUPS)) .collect(Collectors.toSet()); - permissions = new HashSet<>(); - permissions.addAll(workspacePermissions); - permissions.addAll(assignPermissionGroupPermissions); - permissions.addAll(readPermissionGroupPermissions); + + permissions = collateAllPermissions(workspacePermissions, + assignPermissionGroupPermissions, + readPermissionGroupPermissions); viewerPermissionGroup.setPermissions(permissions); Mono> savedPermissionGroupsMono = Flux.fromIterable(permissionGroups) @@ -385,15 +394,16 @@ public class WorkspaceServiceCEImpl extends BaseService tuple.getT1()); } - private Mono> generateDefaultPermissionGroups(Workspace workspace, User user) { + protected Mono> generateDefaultPermissionGroups(Workspace workspace, User user) { return generateDefaultPermissionGroupsWithoutPermissions(workspace) // Generate the permissions per permission group - .flatMap(permissionGroups -> generatePermissionsForDefaultPermissionGroups(permissionGroups, workspace, user)); + .flatMap(permissionGroups -> generatePermissionsForDefaultPermissionGroups(permissionGroups, workspace, + user)); } /** @@ -531,7 +541,7 @@ public class WorkspaceServiceCEImpl extends BaseService createWorkspace = workspaceService.create(workspace); String[] validEmails = {"valid@email.com", "valid@email.co.in", "valid@email-assoc.co.in"}; for (String validEmail: validEmails) { - Mono updateWorkspace = createWorkspace + Workspace workspace = new Workspace(); + workspace.setName("Test Update Name"); + workspace.setDomain("example.com"); + workspace.setWebsite("https://example.com"); + workspace.setSlug("test-update-name"); + Mono updateWorkspace = workspaceService.create(workspace) .flatMap(t -> { Workspace newWorkspace = new Workspace(); newWorkspace.setEmail(validEmail); @@ -563,15 +562,14 @@ public class WorkspaceServiceTest { .users(Set.of("api_user")) .build(); - Workspace workspace = new Workspace(); - workspace.setName("Test Update Name"); - workspace.setDomain("example.com"); - workspace.setWebsite("https://example.com"); - workspace.setSlug("test-update-name"); - Mono createWorkspace = workspaceService.create(workspace); String[] invalidEmails = {"invalid@.com", "@invalid.com"}; for (String invalidEmail : invalidEmails) { - Mono updateWorkspace = createWorkspace + Workspace workspace = new Workspace(); + workspace.setName("Test Update Name"); + workspace.setDomain("example.com"); + workspace.setWebsite("https://example.com"); + workspace.setSlug("test-update-name"); + Mono updateWorkspace = workspaceService.create(workspace) .flatMap(t -> { Workspace newWorkspace = new Workspace(); newWorkspace.setEmail(invalidEmail); @@ -595,19 +593,18 @@ public class WorkspaceServiceTest { .users(Set.of("api_user")) .build(); - Workspace workspace = new Workspace(); - workspace.setName("Test Update Name"); - workspace.setDomain("example.com"); - workspace.setWebsite("https://example.com"); - workspace.setSlug("test-update-name"); - Mono createWorkspace = workspaceService.create(workspace); String[] validWebsites = {"https://www.valid.website.com", "http://www.valid.website.com", "https://valid.website.com", "http://valid.website.com", "www.valid.website.com", "valid.website.com", "valid-website.com", "valid.12345.com", "12345.com", "https://www.valid.website.com/", "http://www.valid.website.com/", "https://valid.website.complete/", "http://valid.website.com/", "www.valid.website.com/", "valid.website.com/", "valid-website.com/", "valid.12345.com/", "12345.com/"}; for (String validWebsite: validWebsites) { - Mono updateWorkspace = createWorkspace + Workspace workspace = new Workspace(); + workspace.setName("Test Update Name"); + workspace.setDomain("example.com"); + workspace.setWebsite("https://example.com"); + workspace.setSlug("test-update-name"); + Mono updateWorkspace = workspaceService.create(workspace) .flatMap(t -> { Workspace newWorkspace = new Workspace(); newWorkspace.setWebsite(validWebsite); @@ -633,16 +630,15 @@ public class WorkspaceServiceTest { .users(Set.of("api_user")) .build(); - Workspace workspace = new Workspace(); - workspace.setName("Test Update Name"); - workspace.setDomain("example.com"); - workspace.setWebsite("https://example.com"); - workspace.setSlug("test-update-name"); - Mono createWorkspace = workspaceService.create(workspace); String[] invalidWebsites = {"htp://www.invalid.website.com", "htp://invalid.website.com", "htp://www", "www", "www."}; for (String invalidWebsite : invalidWebsites) { - Mono updateWorkspace = createWorkspace + Workspace workspace = new Workspace(); + workspace.setName("Test Update Name"); + workspace.setDomain("example.com"); + workspace.setWebsite("https://example.com"); + workspace.setSlug("test-update-name"); + Mono updateWorkspace = workspaceService.create(workspace) .flatMap(t -> { Workspace newWorkspace = new Workspace(); newWorkspace.setWebsite(invalidWebsite);