[PF-1681] Add discoverer policy when creating SAM workspace resource

README.md

@@ -316,3 +316,24 @@ 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
+
+We have 2 ways of cleaning up resources (WSM workspace, SAM workspace, GCP project):
+
+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. Most tests don't need to worry about this because
+`WorkspaceAllocateTestScriptBase.java` deletes the workspace it creates.

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

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

service/src/main/java/bio/terra/workspace/service/iam/SamService.java

@@ -300,8 +300,14 @@ public Map<UUID, WsmIamRole> listWorkspaceIdsAndHighestRoles(AuthenticatedUserRe
                   .filter(Objects::nonNull)
                   .collect(Collectors.toList());
           Optional<WsmIamRole> 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) {

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<WsmIamRole> getHighestRole(UUID workspaceId, List<WsmIamR
       return Optional.of(WsmIamRole.WRITER);
     } else if (roles.contains(WsmIamRole.READER)) {
       return Optional.of(WsmIamRole.READER);
+    } else if (roles.contains(WsmIamRole.DISCOVERER)) {
+      return Optional.of(WsmIamRole.DISCOVERER);
     }
     throw new InternalServerErrorException(
         String.format("Workspace %s has unexpected roles: %s", workspaceId.toString(), roles));

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;
@@ -71,7 +72,11 @@ 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)));
+          (wsmRole, email) -> {
+            if (CUSTOM_GCP_PROJECT_IAM_ROLES.containsKey(wsmRole)) {
+              newBindings.add(bindingForRole(wsmRole, email, gcpProjectId));
+            }
+          });
 
       Policy newPolicy =
           new Policy()
@@ -102,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)));

service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java

@@ -48,6 +48,9 @@ public StepResult doStep(FlightContext flightContext)
         WsmIamRole.READER,
         samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.READER, 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);
 

service/src/test/java/bio/terra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java

@@ -0,0 +1,143 @@
+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.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;
+
+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.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;
+import org.springframework.test.web.servlet.MockMvc;
+
+/**
+ * Connected tests for WorkspaceApiController.
+ *
+ * <p>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.
+ *
+ * <p>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 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 {
+    grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail());
+
+    getWorkspaceDescriptionExpectingError(
+        userAccessUtils.secondUserAuthRequest(), workspace.getId(), HttpStatus.SC_FORBIDDEN);
+  }
+
+  @Test
+  public void listWorkspaces_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception {
+    grantRole(workspace.getId(), WsmIamRole.DISCOVERER, userAccessUtils.getSecondUserEmail());
+
+    List<ApiWorkspaceDescription> 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<ApiWorkspaceDescription> 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();
+  }
+
+  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));
+  }
+}

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 =

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

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(