-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PF-1681] Add discoverer policy when creating SAM workspace resource #755
Changes from 5 commits
12d1ea7
571d23f
e6a3a1b
8268fa9
8de451d
f64dab8
faac7d0
99f9a65
285fc2b
17d64b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: now that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
newBindings.add(bindingForRole(wsmRole, email, gcpProjectId)); | ||
} | ||
}); | ||
|
||
Policy newPolicy = | ||
new Policy() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,9 @@ public StepResult doStep(FlightContext flightContext) | |
workspaceRoleGroupMap.put( | ||
WsmIamRole.READER, | ||
samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.READER, userRequest)); | ||
workspaceRoleGroupMap.put( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to sync the discoverer role. That will create Google groups for discoverers. We only need that when we are going to apply that group to a cloud resource ACL; like granting a READER actual read access to a GCS bucket. I think we only ever enforce discoverer by checking with Sam - never the cloud. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, good point. |
||
WsmIamRole.DISCOVERER, | ||
samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.DISCOVERER, userRequest)); | ||
|
||
FlightMap workingMap = flightContext.getWorkingMap(); | ||
workingMap.put(WorkspaceFlightMapKeys.IAM_GROUP_EMAIL_MAP, workspaceRoleGroupMap); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
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. | ||
* | ||
* <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; | ||
|
||
@Test | ||
public void getWorkspace_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception { | ||
ApiCreatedWorkspace workspace = createWorkspace(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since these are talking to real Sam, these tests will leave workspaces. could you delete them from these tests as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks. I always forget when to have tests delete workspaces, so I added a section to README, PTAL. |
||
|
||
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<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(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just filter in ListWorkspaces, so you don't have to remember to come back to this code?
You need the filtering there anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next PR, I updated both here and ListWorkspaces.