From 12d1ea74d426ca777c59c29eae9a793a5d7de4de Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Mon, 8 Aug 2022 10:32:20 -0700 Subject: [PATCH 01/10] [PF-1681] Add discoverer policy when creating SAM workspace --- openapi/src/parts/workspace.yaml | 2 +- .../bio/terra/workspace/service/iam/model/WsmIamRole.java | 7 +++++-- .../service/workspace/flight/SyncSamGroupsStep.java | 3 +++ 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/openapi/src/parts/workspace.yaml b/openapi/src/parts/workspace.yaml index c5f1e40d70..e4712b9b62 100644 --- a/openapi/src/parts/workspace.yaml +++ b/openapi/src/parts/workspace.yaml @@ -435,7 +435,7 @@ components: IamRole: description: Enum containing all valid IAM roles on a Workspace type: string - enum: ['READER', 'WRITER', 'APPLICATION', 'OWNER'] + enum: ['DISCOVERER', 'READER', 'WRITER', 'APPLICATION', 'OWNER'] Properties: description: Optional list of key-value pairs of strings diff --git a/service/src/main/java/bio/terra/workspace/service/iam/model/WsmIamRole.java b/service/src/main/java/bio/terra/workspace/service/iam/model/WsmIamRole.java index 94e8e31aca..5bfc87c466 100644 --- a/service/src/main/java/bio/terra/workspace/service/iam/model/WsmIamRole.java +++ b/service/src/main/java/bio/terra/workspace/service/iam/model/WsmIamRole.java @@ -11,6 +11,7 @@ /** Internal representation of IAM roles. */ public enum WsmIamRole { + DISCOVERER("discoverer", ApiIamRole.DISCOVERER), READER("reader", ApiIamRole.READER), WRITER("writer", ApiIamRole.WRITER), APPLICATION("application", ApiIamRole.APPLICATION), @@ -40,8 +41,8 @@ public static WsmIamRole fromApiModel(ApiIamRole apiModel) { /** * Return the WsmIamRole corresponding to the provided Sam role, or null if the Sam role does not - * match a Wsm role. There are valid roles on workspaces in Sam which do not map to WsmIam roles, - * in general WSM should ignore these roles. + * match a Wsm role. There are roles on workspaces in Sam that are used by Rawls and not WSM -- in + * general WSM should ignore these roles. */ public static WsmIamRole fromSam(String samRole) { return Arrays.stream(WsmIamRole.values()) @@ -65,6 +66,8 @@ public static Optional getHighestRole(UUID workspaceId, List Date: Mon, 8 Aug 2022 13:45:36 -0700 Subject: [PATCH 02/10] Tests, don't return discoverer workspace for ListWorkspaces --- .../workspace/service/iam/SamService.java | 8 +- .../WorkspaceApiControllerConnectedTest.java | 123 ++++++++++++++++++ .../workspace/common/utils/MockMvcUtils.java | 1 + 3 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java diff --git a/service/src/main/java/bio/terra/workspace/service/iam/SamService.java b/service/src/main/java/bio/terra/workspace/service/iam/SamService.java index d94b2b18dc..4190e30906 100644 --- a/service/src/main/java/bio/terra/workspace/service/iam/SamService.java +++ b/service/src/main/java/bio/terra/workspace/service/iam/SamService.java @@ -300,8 +300,14 @@ public Map listWorkspaceIdsAndHighestRoles(AuthenticatedUserRe .filter(Objects::nonNull) .collect(Collectors.toList()); Optional highestRole = WsmIamRole.getHighestRole(workspaceId, roles); - // Skip workspaces with no roles. (That means there's a role this WSM doesn't know about.) + // Skip workspaces with no roles. (That means there's a role this WSM doesn't know + // about.) if (highestRole.isPresent()) { + // TODO(PF-1875): Support requesting discoverer workspaces in ListWorkspaces. + // For now, don't return discoverer workspaces. + if (highestRole.get().equals(WsmIamRole.DISCOVERER)) { + continue; + } workspacesAndRoles.put(workspaceId, highestRole.get()); } } catch (IllegalArgumentException e) { diff --git a/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java new file mode 100644 index 0000000000..31b81fc84f --- /dev/null +++ b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java @@ -0,0 +1,123 @@ +package bio.terra.workspace.app.configuration.external.controller; + +import static bio.terra.workspace.common.utils.MockMvcUtils.GRANT_ROLE_PATH_FORMAT; +import static bio.terra.workspace.common.utils.MockMvcUtils.WORKSPACES_V1_BY_UUID_PATH_FORMAT; +import static bio.terra.workspace.common.utils.MockMvcUtils.WORKSPACES_V1_PATH; +import static bio.terra.workspace.common.utils.MockMvcUtils.addAuth; +import static bio.terra.workspace.common.utils.MockMvcUtils.addJsonContentType; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import bio.terra.workspace.common.BaseConnectedTest; +import bio.terra.workspace.common.fixtures.WorkspaceFixtures; +import bio.terra.workspace.connected.UserAccessUtils; +import bio.terra.workspace.generated.model.ApiCreatedWorkspace; +import bio.terra.workspace.generated.model.ApiGrantRoleRequestBody; +import bio.terra.workspace.generated.model.ApiWorkspaceDescription; +import bio.terra.workspace.generated.model.ApiWorkspaceDescriptionList; +import bio.terra.workspace.service.iam.AuthenticatedUserRequest; +import bio.terra.workspace.service.iam.model.WsmIamRole; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.List; +import java.util.UUID; +import org.apache.http.HttpStatus; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; + +/** + * Connected tests for WorkspaceApiController. + * + *

In general, we would like to move towards testing new endpoints via controller instead of + * calling services directly like we have in the past. Although this test duplicates coverage + * currently in WorkspaceServiceTest, it's intended as a proof-of-concept for future mockMvc-based + * tests. + * + *

Use this instead of WorkspaceApiControllerTest, if you want to use real + * bio.terra.workspace.service.iam.SamService. + */ +@AutoConfigureMockMvc +public class WorkspaceApiControllerConnectedTest extends BaseConnectedTest { + + @Autowired private MockMvc mockMvc; + @Autowired private ObjectMapper objectMapper; + // @Autowired private SamService samService; + @Autowired private UserAccessUtils userAccessUtils; + + @Test + public void getWorkspace_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception { + ApiCreatedWorkspace workspace = createWorkspace(); + + grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail()); + + getWorkspaceDescriptionExpectingError( + userAccessUtils.secondUserAuthRequest(), workspace.getId(), HttpStatus.SC_FORBIDDEN); + } + + @Test + public void listWorkspaces_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception { + ApiCreatedWorkspace workspace = createWorkspace(); + + grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail()); + + List listedWorkspaces = + listWorkspaces(userAccessUtils.secondUserAuthRequest()); + assertTrue(listedWorkspaces.isEmpty()); + } + + private ApiCreatedWorkspace createWorkspace() throws Exception { + var createRequest = WorkspaceFixtures.createWorkspaceRequestBody(); + String serializedResponse = + mockMvc + .perform( + addJsonContentType( + addAuth( + post(WORKSPACES_V1_PATH) + .content(objectMapper.writeValueAsString(createRequest)), + userAccessUtils.defaultUserAuthRequest()))) + .andExpect(status().is(HttpStatus.SC_OK)) + .andReturn() + .getResponse() + .getContentAsString(); + return objectMapper.readValue(serializedResponse, ApiCreatedWorkspace.class); + } + + private void getWorkspaceDescriptionExpectingError( + AuthenticatedUserRequest userRequest, UUID id, int statusCode) throws Exception { + mockMvc + .perform(addAuth(get(String.format(WORKSPACES_V1_BY_UUID_PATH_FORMAT, id)), userRequest)) + .andExpect(status().is(statusCode)); + } + + private List listWorkspaces(AuthenticatedUserRequest request) + throws Exception { + String serializedResponse = + mockMvc + .perform(addAuth(get(WORKSPACES_V1_PATH), request)) + .andExpect(status().is(HttpStatus.SC_OK)) + .andReturn() + .getResponse() + .getContentAsString(); + return objectMapper + .readValue(serializedResponse, ApiWorkspaceDescriptionList.class) + .getWorkspaces(); + } + + private void grantRole(UUID workspaceId, WsmIamRole role, String memberEmail) throws Exception { + var requestBody = new ApiGrantRoleRequestBody().memberEmail(memberEmail); + mockMvc + .perform( + addJsonContentType( + addAuth( + post(String.format(GRANT_ROLE_PATH_FORMAT, workspaceId, role.name())) + .content(objectMapper.writeValueAsString(requestBody)), + userAccessUtils.defaultUserAuthRequest()))) + .andExpect(status().is(HttpStatus.SC_NO_CONTENT)) + .andReturn() + .getResponse() + .getContentAsString(); + } +} diff --git a/service/src/test/java/bio/terra/workspace/common/utils/MockMvcUtils.java b/service/src/test/java/bio/terra/workspace/common/utils/MockMvcUtils.java index 1067c319f9..7d67289810 100644 --- a/service/src/test/java/bio/terra/workspace/common/utils/MockMvcUtils.java +++ b/service/src/test/java/bio/terra/workspace/common/utils/MockMvcUtils.java @@ -24,6 +24,7 @@ public class MockMvcUtils { public static final String CLONE_WORKSPACE_PATH_FORMAT = "/api/workspaces/v1/%s/clone"; public static final String UPDATE_WORKSPACES_V1_PROPERTIES_PATH_FORMAT = "/api/workspaces/v1/%s/properties"; + public static final String GRANT_ROLE_PATH_FORMAT = "/api/workspaces/v1/%s/roles/%s/members"; public static final String CREATE_SNAPSHOT_PATH_FORMAT = "/api/workspaces/v1/%s/resources/referenced/datarepo/snapshots"; public static final String CREATE_CLOUD_CONTEXT_PATH_FORMAT = From e6a3a1bd620914612131498b7095113462bb54cb Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Mon, 8 Aug 2022 15:27:44 -0700 Subject: [PATCH 03/10] fix CreateCustomGcpRolesStep --- integration/README.md | 4 ++-- .../workspace/CloudSyncRoleMapping.java | 22 +++++++++++++------ .../flight/CreateCustomGcpRolesStep.java | 3 +++ .../workspace/flight/GcpCloudSyncStep.java | 14 ++++++++---- .../workspace/CloudSyncRoleMappingTest.java | 6 +++++ .../flight/CreateGcpContextFlightV2Test.java | 1 + 6 files changed, 37 insertions(+), 13 deletions(-) diff --git a/integration/README.md b/integration/README.md index 67bd3119ba..1f9a8bedd0 100644 --- a/integration/README.md +++ b/integration/README.md @@ -30,12 +30,12 @@ Perf Test #### Run a test suite To run a test suite, specify the path of the test configuration under the `suites` directory as shown in the following example. -NB: The `BasicIntegration.json` and `BasicNightlyPerf.json` are sample test suite configurations for Integration and Perf tests. +NB: The `FullIntegration.json` and `BasicNightlyPerf.json` are sample test suite configurations for Integration and Perf tests. See `test-runner-integration.yml` and `test-runner-nightly-perf.yml` in `.github/workflows` for use cases. Integration Test ``` -./gradlew runTest --args="suites/BasicIntegration.json /tmp/TR" +./gradlew runTest --args="suites/FullIntegration.json /tmp/TR" ``` Perf Test diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java b/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java index f2094260e9..db8fcbe73a 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java @@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.List; +import java.util.Optional; /** * This mapping describes the project-level GCP roles granted to members of a workspace. @@ -111,11 +112,18 @@ public class CloudSyncRoleMapping { CustomGcpIamRole.of("PROJECT_OWNER", PROJECT_OWNER_PERMISSIONS); // Currently, workspace editors, applications and owners have the same cloud permissions as // writers. If that changes, create a new CustomGcpIamRole and modify the map below. - public static final ImmutableMap CUSTOM_GCP_PROJECT_IAM_ROLES = - ImmutableMap.of( - WsmIamRole.OWNER, PROJECT_OWNER, - // TODO: this should map to PROJECT_APPLICATION if that's created. - WsmIamRole.APPLICATION, PROJECT_WRITER, - WsmIamRole.WRITER, PROJECT_WRITER, - WsmIamRole.READER, PROJECT_READER); + public static final ImmutableMap> + CUSTOM_GCP_PROJECT_IAM_ROLES = + ImmutableMap.of( + WsmIamRole.OWNER, + Optional.of(PROJECT_OWNER), + // TODO: this should map to PROJECT_APPLICATION if that's created. + WsmIamRole.APPLICATION, + Optional.of(PROJECT_WRITER), + WsmIamRole.WRITER, + Optional.of(PROJECT_WRITER), + WsmIamRole.READER, + Optional.of(PROJECT_READER), + WsmIamRole.DISCOVERER, + Optional.empty()); } diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java index c65f10b2f1..95b9472f9a 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java @@ -15,6 +15,7 @@ import com.google.api.services.iam.v1.model.Role; import com.google.common.collect.ImmutableSet; import java.io.IOException; +import java.util.Optional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; @@ -42,6 +43,8 @@ public StepResult doStep(FlightContext flightContext) // which would lead to unnecessary CONFLICT responses from GCP. ImmutableSet customProjectRoles = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES.values().stream() + .filter(roleOptional -> roleOptional.isPresent()) + .map(Optional::get) .collect(ImmutableSet.toImmutableSet()); for (CustomGcpIamRole customProjectRole : customProjectRoles) { createCustomRole(customProjectRole, projectId); diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java index c93135de37..364da65dce 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java @@ -1,5 +1,6 @@ package bio.terra.workspace.service.workspace.flight; +import static bio.terra.workspace.service.workspace.CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES; import static bio.terra.workspace.service.workspace.flight.WorkspaceFlightMapKeys.GCP_PROJECT_ID; import bio.terra.cloudres.google.cloudresourcemanager.CloudResourceManagerCow; @@ -22,6 +23,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Optional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -71,7 +73,12 @@ public StepResult doStep(FlightContext flightContext) newBindings.addAll(currentPolicy.getBindings()); // Add appropriate project-level roles for each WSM IAM role. workspaceRoleGroupsMap.forEach( - (role, email) -> newBindings.add(bindingForRole(role, email, gcpProjectId))); + (role, email) -> { + Optional customRoleOptional = CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); + if (customRoleOptional.isPresent()) { + newBindings.add(bindingForRole(customRoleOptional.get(), email, gcpProjectId)); + } + }); Policy newPolicy = new Policy() @@ -97,12 +104,11 @@ private String toMemberIdentifier(String samEmail) { /** * Build the project-level role binding for a given group, using CloudSyncRoleMapping. * - * @param role The role granted to this user. Translated to GCP roles using CloudSyncRoleMapping. + * @param customRole GCP custom role * @param email The email of the Google group being granted a role. * @param gcpProjectId The ID of the project the custom role is defined in. */ - private Binding bindingForRole(WsmIamRole role, String email, String gcpProjectId) { - CustomGcpIamRole customRole = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); + private Binding bindingForRole(CustomGcpIamRole customRole, String email, String gcpProjectId) { return new Binding() .setRole(customRole.getFullyQualifiedRoleName(gcpProjectId)) .setMembers(Collections.singletonList(toMemberIdentifier(email))); diff --git a/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java b/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java index a63f126a07..667035236d 100644 --- a/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java +++ b/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java @@ -18,11 +18,13 @@ void writerPermissionsContainReaderPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.READER) + .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) + .get() .getIncludedPermissions())))); } @@ -31,11 +33,13 @@ void applicationPermissionsContainWriterPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) + .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.APPLICATION) + .get() .getIncludedPermissions())))); } @@ -44,11 +48,13 @@ void ownerPermissionsContainWriterPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) + .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.OWNER) + .get() .getIncludedPermissions())))); } } diff --git a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java index 67341b0367..4dbde2ce3b 100644 --- a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java +++ b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java @@ -353,6 +353,7 @@ private void assertRoleBindingInPolicy( String expectedGcpRoleName = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(role) + .get() .getFullyQualifiedRoleName(projectId); List actualGcpBindingList = gcpPolicy.getBindings(); List actualGcpRoleList = From 8268fa9f0387025f0674250cb3db73cc952055d3 Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Mon, 8 Aug 2022 16:23:18 -0700 Subject: [PATCH 04/10] fix more tests --- .../bio/terra/workspace/service/iam/SamServiceTest.java | 7 +++++-- .../workspace/flight/CreateGcpContextFlightV2Test.java | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/service/src/test/java/bio/terra/workspace/service/iam/SamServiceTest.java b/service/src/test/java/bio/terra/workspace/service/iam/SamServiceTest.java index 8aef489aea..1af4172276 100644 --- a/service/src/test/java/bio/terra/workspace/service/iam/SamServiceTest.java +++ b/service/src/test/java/bio/terra/workspace/service/iam/SamServiceTest.java @@ -279,13 +279,15 @@ void listPermissionsIncludesAddedUsers() throws Exception { .role(WsmIamRole.OWNER) .users(Collections.singletonList(userAccessUtils.getDefaultUserEmail())) .build(); + RoleBinding expectedWriterBinding = + RoleBinding.builder().role(WsmIamRole.WRITER).users(Collections.emptyList()).build(); RoleBinding expectedReaderBinding = RoleBinding.builder() .role(WsmIamRole.READER) .users(Collections.singletonList(userAccessUtils.getSecondUserEmail())) .build(); - RoleBinding expectedWriterBinding = - RoleBinding.builder().role(WsmIamRole.WRITER).users(Collections.emptyList()).build(); + RoleBinding expectedDiscovererBinding = + RoleBinding.builder().role(WsmIamRole.DISCOVERER).users(Collections.emptyList()).build(); RoleBinding expectedApplicationBinding = RoleBinding.builder().role(WsmIamRole.APPLICATION).users(Collections.emptyList()).build(); assertThat( @@ -294,6 +296,7 @@ void listPermissionsIncludesAddedUsers() throws Exception { equalTo(expectedOwnerBinding), equalTo(expectedWriterBinding), equalTo(expectedReaderBinding), + equalTo(expectedDiscovererBinding), equalTo(expectedApplicationBinding))); } diff --git a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java index 4dbde2ce3b..7ca69ef46a 100644 --- a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java +++ b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java @@ -331,8 +331,8 @@ private void assertPolicyGroupsSynced(UUID workspaceUuid, Project project) throw .getIamPolicy(project.getProjectId(), new GetIamPolicyRequest()) .execute(); for (WsmIamRole role : WsmIamRole.values()) { - // Don't check MANAGER role, which isn't synced to GCP. - if (role.equals(WsmIamRole.MANAGER)) { + // Don't check roles which aren't synced to GCP. + if (role.equals(WsmIamRole.MANAGER) || role.equals(WsmIamRole.DISCOVERER)) { continue; } assertRoleBindingInPolicy( From 8de451d74f39a7d61b19f6365f606613e715be6b Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Mon, 8 Aug 2022 16:37:08 -0700 Subject: [PATCH 05/10] go back to original CUSTOM_GCP_PROJECT_IAM_ROLES --- .../workspace/CloudSyncRoleMapping.java | 22 ++++++------------- .../flight/CreateCustomGcpRolesStep.java | 3 --- .../workspace/flight/GcpCloudSyncStep.java | 13 +++++------ .../WorkspaceApiControllerConnectedTest.java | 1 - .../workspace/CloudSyncRoleMappingTest.java | 6 ----- .../flight/CreateGcpContextFlightV2Test.java | 1 - 6 files changed, 13 insertions(+), 33 deletions(-) diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java b/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java index db8fcbe73a..f2094260e9 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/CloudSyncRoleMapping.java @@ -5,7 +5,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.List; -import java.util.Optional; /** * This mapping describes the project-level GCP roles granted to members of a workspace. @@ -112,18 +111,11 @@ public class CloudSyncRoleMapping { CustomGcpIamRole.of("PROJECT_OWNER", PROJECT_OWNER_PERMISSIONS); // Currently, workspace editors, applications and owners have the same cloud permissions as // writers. If that changes, create a new CustomGcpIamRole and modify the map below. - public static final ImmutableMap> - CUSTOM_GCP_PROJECT_IAM_ROLES = - ImmutableMap.of( - WsmIamRole.OWNER, - Optional.of(PROJECT_OWNER), - // TODO: this should map to PROJECT_APPLICATION if that's created. - WsmIamRole.APPLICATION, - Optional.of(PROJECT_WRITER), - WsmIamRole.WRITER, - Optional.of(PROJECT_WRITER), - WsmIamRole.READER, - Optional.of(PROJECT_READER), - WsmIamRole.DISCOVERER, - Optional.empty()); + public static final ImmutableMap CUSTOM_GCP_PROJECT_IAM_ROLES = + ImmutableMap.of( + WsmIamRole.OWNER, PROJECT_OWNER, + // TODO: this should map to PROJECT_APPLICATION if that's created. + WsmIamRole.APPLICATION, PROJECT_WRITER, + WsmIamRole.WRITER, PROJECT_WRITER, + WsmIamRole.READER, PROJECT_READER); } diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java index 95b9472f9a..c65f10b2f1 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/CreateCustomGcpRolesStep.java @@ -15,7 +15,6 @@ import com.google.api.services.iam.v1.model.Role; import com.google.common.collect.ImmutableSet; import java.io.IOException; -import java.util.Optional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; @@ -43,8 +42,6 @@ public StepResult doStep(FlightContext flightContext) // which would lead to unnecessary CONFLICT responses from GCP. ImmutableSet customProjectRoles = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES.values().stream() - .filter(roleOptional -> roleOptional.isPresent()) - .map(Optional::get) .collect(ImmutableSet.toImmutableSet()); for (CustomGcpIamRole customProjectRole : customProjectRoles) { createCustomRole(customProjectRole, projectId); diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java index 364da65dce..5a13da0d36 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java @@ -23,7 +23,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Optional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -73,10 +72,9 @@ public StepResult doStep(FlightContext flightContext) newBindings.addAll(currentPolicy.getBindings()); // Add appropriate project-level roles for each WSM IAM role. workspaceRoleGroupsMap.forEach( - (role, email) -> { - Optional customRoleOptional = CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); - if (customRoleOptional.isPresent()) { - newBindings.add(bindingForRole(customRoleOptional.get(), email, gcpProjectId)); + (wsmRole, email) -> { + if (CUSTOM_GCP_PROJECT_IAM_ROLES.containsKey(wsmRole)) { + newBindings.add(bindingForRole(wsmRole, email, gcpProjectId)); } }); @@ -104,11 +102,12 @@ private String toMemberIdentifier(String samEmail) { /** * Build the project-level role binding for a given group, using CloudSyncRoleMapping. * - * @param customRole GCP custom role + * @param role The role granted to this user. Translated to GCP roles using CloudSyncRoleMapping. * @param email The email of the Google group being granted a role. * @param gcpProjectId The ID of the project the custom role is defined in. */ - private Binding bindingForRole(CustomGcpIamRole customRole, String email, String gcpProjectId) { + private Binding bindingForRole(WsmIamRole role, String email, String gcpProjectId) { + CustomGcpIamRole customRole = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); return new Binding() .setRole(customRole.getFullyQualifiedRoleName(gcpProjectId)) .setMembers(Collections.singletonList(toMemberIdentifier(email))); diff --git a/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java index 31b81fc84f..8e534f68aa 100644 --- a/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java +++ b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java @@ -44,7 +44,6 @@ public class WorkspaceApiControllerConnectedTest extends BaseConnectedTest { @Autowired private MockMvc mockMvc; @Autowired private ObjectMapper objectMapper; - // @Autowired private SamService samService; @Autowired private UserAccessUtils userAccessUtils; @Test diff --git a/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java b/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java index 667035236d..a63f126a07 100644 --- a/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java +++ b/service/src/test/java/bio/terra/workspace/service/workspace/CloudSyncRoleMappingTest.java @@ -18,13 +18,11 @@ void writerPermissionsContainReaderPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.READER) - .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) - .get() .getIncludedPermissions())))); } @@ -33,13 +31,11 @@ void applicationPermissionsContainWriterPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) - .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.APPLICATION) - .get() .getIncludedPermissions())))); } @@ -48,13 +44,11 @@ void ownerPermissionsContainWriterPermissions() { assertThat( CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.WRITER) - .get() .getIncludedPermissions(), everyItem( in( (CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(WsmIamRole.OWNER) - .get() .getIncludedPermissions())))); } } diff --git a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java index 7ca69ef46a..089a7ac128 100644 --- a/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java +++ b/service/src/test/java/bio/terra/workspace/service/workspace/flight/CreateGcpContextFlightV2Test.java @@ -353,7 +353,6 @@ private void assertRoleBindingInPolicy( String expectedGcpRoleName = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES .get(role) - .get() .getFullyQualifiedRoleName(projectId); List actualGcpBindingList = gcpPolicy.getBindings(); List actualGcpRoleList = From f64dab82e88346bb1da9cda7812c4802588360f6 Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:20:54 -0700 Subject: [PATCH 06/10] review fixes --- README.md | 8 +++++ .../workspace/flight/GcpCloudSyncStep.java | 2 +- .../WorkspaceApiControllerConnectedTest.java | 29 ++++++++++++++++--- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index b18b9b8beb..f8325bb3b7 100644 --- a/README.md +++ b/README.md @@ -316,3 +316,11 @@ The integration tests live in the `integration` project. Consult the integration In the early days of the project, there were JUnit-based integration tests. We are in process of migrating them to Test Runner. + +#### Cleaning up workspaces in tests + +For connected tests that use real SamService/Broad dev SAM, delete workspaces at +the end of test, so Broad dev SAM isn't cluttered with test workspaces. + +For integration tests, don't need to delete workspace because it's taken care of +by janitor. diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java index 5a13da0d36..ee414c7a68 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java @@ -107,7 +107,7 @@ private String toMemberIdentifier(String samEmail) { * @param gcpProjectId The ID of the project the custom role is defined in. */ private Binding bindingForRole(WsmIamRole role, String email, String gcpProjectId) { - CustomGcpIamRole customRole = CloudSyncRoleMapping.CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); + CustomGcpIamRole customRole = CUSTOM_GCP_PROJECT_IAM_ROLES.get(role); return new Binding() .setRole(customRole.getFullyQualifiedRoleName(gcpProjectId)) .setMembers(Collections.singletonList(toMemberIdentifier(email))); diff --git a/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java index 8e534f68aa..6023f79071 100644 --- a/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java +++ b/service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java @@ -6,6 +6,7 @@ import static bio.terra.workspace.common.utils.MockMvcUtils.addAuth; import static bio.terra.workspace.common.utils.MockMvcUtils.addJsonContentType; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -23,6 +24,8 @@ import java.util.List; import java.util.UUID; import org.apache.http.HttpStatus; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; @@ -46,10 +49,21 @@ public class WorkspaceApiControllerConnectedTest extends BaseConnectedTest { @Autowired private ObjectMapper objectMapper; @Autowired private UserAccessUtils userAccessUtils; + private ApiCreatedWorkspace workspace; + + @BeforeEach + public void setup() throws Exception { + workspace = createWorkspace(); + } + + /** Clean up workspaces from Broad dev SAM. */ + @AfterEach + public void cleanup() throws Exception { + deleteWorkspace(workspace.getId()); + } + @Test public void getWorkspace_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception { - ApiCreatedWorkspace workspace = createWorkspace(); - grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail()); getWorkspaceDescriptionExpectingError( @@ -58,8 +72,6 @@ public void getWorkspace_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws E @Test public void listWorkspaces_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception { - ApiCreatedWorkspace workspace = createWorkspace(); - grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail()); List listedWorkspaces = @@ -119,4 +131,13 @@ private void grantRole(UUID workspaceId, WsmIamRole role, String memberEmail) th .getResponse() .getContentAsString(); } + + private void deleteWorkspace(UUID workspaceId) throws Exception { + mockMvc + .perform( + addAuth( + delete(String.format(WORKSPACES_V1_BY_UUID_PATH_FORMAT, workspaceId)), + userAccessUtils.defaultUserAuthRequest())) + .andExpect(status().is(HttpStatus.SC_NO_CONTENT)); + } } From faac7d05247e279f484ef5ecad16bde016a1f9b9 Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:41:06 -0700 Subject: [PATCH 07/10] minor edit --- README.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f8325bb3b7..46fbb25104 100644 --- a/README.md +++ b/README.md @@ -319,8 +319,11 @@ process of migrating them to Test Runner. #### Cleaning up workspaces in tests +For integration tests, don't need to delete workspaces because it's taken care of +by janitor. + For connected tests that use real SamService/Broad dev SAM, delete workspaces at the end of test, so Broad dev SAM isn't cluttered with test workspaces. +(Connected tests can't use janitor because the WSM is ephemeral.) + -For integration tests, don't need to delete workspace because it's taken care of -by janitor. From 99f9a657624ece7861485337720fb68c38c0809a Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Wed, 10 Aug 2022 09:54:45 -0700 Subject: [PATCH 08/10] don't sync SAM group --- .../service/workspace/flight/SyncSamGroupsStep.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java b/service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java index 84b0f7a5e4..d5142c0bac 100644 --- a/service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java +++ b/service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java @@ -47,9 +47,9 @@ public StepResult doStep(FlightContext flightContext) workspaceRoleGroupMap.put( WsmIamRole.READER, samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.READER, userRequest)); - workspaceRoleGroupMap.put( - WsmIamRole.DISCOVERER, - samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.DISCOVERER, userRequest)); + + // Do not sync discoverer policy. Discoverer roles are not attached to GCP resources; they are + // only used by WSM internally. FlightMap workingMap = flightContext.getWorkingMap(); workingMap.put(WorkspaceFlightMapKeys.IAM_GROUP_EMAIL_MAP, workspaceRoleGroupMap); From 285fc2bab9584591788a5db27a14bc26457caa07 Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Wed, 10 Aug 2022 11:33:12 -0700 Subject: [PATCH 09/10] update "Cleaning up workspaces in tests" --- README.md | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 46fbb25104..ae3a475f47 100644 --- a/README.md +++ b/README.md @@ -319,11 +319,20 @@ process of migrating them to Test Runner. #### Cleaning up workspaces in tests -For integration tests, don't need to delete workspaces because it's taken care of -by janitor. +We have 2 ways of cleaning up resources (WSM workspace, SAM workspace, GCP project): -For connected tests that use real SamService/Broad dev SAM, delete workspaces at -the end of test, so Broad dev SAM isn't cluttered with test workspaces. -(Connected tests can't use janitor because the WSM is ephemeral.) +1. Connected tests use Janitor. Janitor deletes GCP project and not SAM + workspace (see + [here](https://github.com/DataBiosphere/terra-workspace-manager/pull/755#discussion_r942717257) for details). +2. Tests call WSM `deleteWorkspace()`. This deletes WSM workspace + SAM workspace + + GCP project. +Connected tests that use mock SamService: Tests don't need to call +`deleteWorkspace()` because there is no SAM workspace to clean up. +Connected tests that use real SamService: Tests should call `deleteWorkspace()` +to clean up SAM workspaces. Why not just call `deleteWorkspace()` and not use +janitor? Janitor is useful in case test fails (or `deleteWorkspace()` fails). + +Integration tests: Tests should call `deleteWorkspace()` because integration +tests don't use janitor. From 17d64b6d6cf4b891977230672b6f4cc9cd592105 Mon Sep 17 00:00:00 2001 From: melissachang <10929390+melissachang@users.noreply.github.com> Date: Wed, 10 Aug 2022 11:38:44 -0700 Subject: [PATCH 10/10] add note about WorkspaceAllocateTestScriptBase --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ae3a475f47..be5e10bb54 100644 --- a/README.md +++ b/README.md @@ -335,4 +335,5 @@ to clean up SAM workspaces. Why not just call `deleteWorkspace()` and not use janitor? Janitor is useful in case test fails (or `deleteWorkspace()` fails). Integration tests: Tests should call `deleteWorkspace()` because integration -tests don't use janitor. +tests don't use janitor. Most tests don't need to worry about this because +`WorkspaceAllocateTestScriptBase.java` deletes the workspace it creates.