[DC-1206] Add endpoints to list, add, and delete members from Sam group associated with Snapshots byRequestId #1810

cinyecai · 2024-09-11T18:45:35Z

Jira ticket: https://broadworkbench.atlassian.net/browse/DC-1206

Summary of changes

[ROUGH DRAFT]
Added get, post, and delete endpoints for group members on snapshot access requests

I used examples I found around TDR to help me write all the code, so I don't have the greatest understanding of everything and I'm not sure if there are things that need adjusting to this specific ticket. I'm looking forward to everyone's feedback!

Testing Strategy

I'm not sure the appropriate way to test these changes. Guidance on that would be appreciated!

rjohanek · 2024-09-12T13:42:23Z

src/main/java/bio/terra/app/controller/SnapshotAccessRequestApiController.java

+      deleteSnapshotAccessRequestGroupMember(UUID id, String memberEmail) {
+    AuthenticatedUserRequest userRequest = getAuthenticatedInfo();
+    iamService.verifyAuthorization(
+        userRequest, IamResourceType.SNAPSHOT_BUILDER_REQUEST, id.toString(), IamAction.APPROVE);


These look good! I wonder if we need to make a new IAmAction for these actions or if its okay to extend the use of APPROVE for now.

I agree, maybe create another ticket to add this action? Something like manage_collaborators to handle both add and delete?

rjohanek · 2024-09-12T13:46:08Z

src/main/java/bio/terra/service/auth/iam/IamProviderInterface.java

+  List<String> removeGroupPolicyEmail(
+      String accessToken, String groupName, String policyName, String memberEmail)
+      throws InterruptedException;
+


I think some documentation on these would be great. I like that the add and remove email return the new list of emails in the group, I think that mirrors the other policy member methods, but some documentation explaining what is returned would be helpful.

rjohanek · 2024-09-12T15:54:52Z

src/main/java/bio/terra/service/auth/iam/IamService.java

+  public List<String> removeEmailFromGroup(String groupName, String policyName, String email) {
+    String tdrSaAccessToken = googleCredentialsService.getApplicationDefaultAccessToken(SCOPES);
+    return callProvider(
+        () -> iamProvider.removeGroupPolicyEmail(tdrSaAccessToken, groupName, policyName, email));


maybe some javadoc for these as well

rjohanek · 2024-09-12T15:58:01Z

src/main/java/bio/terra/service/auth/iam/sam/SamIam.java

+            groupName,
+            policyName,
+            memberEmail,
+            new Object()); // Not sure what should be here for object


It looks like addPolicyMemberInner passes in a null instead of a new Object maybe we could just follow that pattern

rjohanek · 2024-09-12T15:58:53Z

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java

+  public SnapshotAccessRequestMembersResponse deleteGroupMember(UUID id, String memberEmail) {
+    SnapshotAccessRequestModel model = snapshotRequestDao.getById(id);
+    return new SnapshotAccessRequestMembersResponse()
+        .members(iamService.removeEmailFromGroup(model.samGroupName(), "member", memberEmail));


For all of these, do we only want people with the member role? Is there a way to get people of any role?

We should only ever add users as members to the sam group. Only the TDR service account will be an admin.

rjohanek · 2024-09-12T16:04:12Z

src/main/resources/api/data-repository-openapi.yaml

+      description: Get the members in the DAC group
+      operationId: getSnapshotAccessRequestGroupMembers
+      parameters:
+        - $ref: '#/components/parameters/Id'


I think the reuse of an existing object is a good idea, but Id specifies that the format of the string is a UUID and this will not match that format because it will be an email. So if that type doesn't exist yet maybe adding a new email type would be good

I think this is the snapshot request ID, so isn't that a UUID?

Yes, I think for the GET, it's passing in the UUID of the snapshot access request whose group members you want to get

rjohanek · 2024-09-12T16:05:18Z

src/main/resources/api/data-repository-openapi.yaml

+              schema:
+                $ref: '#/components/schemas/ErrorModel'
+        404:
+          description: No dataset found that meets the request parameters.


for all error code descriptions, the language needs to be updated slightly so it matches the use case

rjohanek

great work! I would love to talk to you about unit testing these changes and finding examples!

fboulnois

I agree with @rjohanek 's suggestions. I suppose the unit tests are a separate ticket or upcoming?

pshapiro4broad

I have a few comments, also I'd expect to see unit tests of this code and possibly an integration or connected test too.

pshapiro4broad · 2024-09-16T19:47:14Z

src/main/java/bio/terra/app/controller/SnapshotAccessRequestApiController.java

+      deleteSnapshotAccessRequestGroupMember(UUID id, String memberEmail) {
+    AuthenticatedUserRequest userRequest = getAuthenticatedInfo();
+    iamService.verifyAuthorization(
+        userRequest, IamResourceType.SNAPSHOT_BUILDER_REQUEST, id.toString(), IamAction.APPROVE);


I agree, maybe create another ticket to add this action? Something like manage_collaborators to handle both add and delete?

pshapiro4broad · 2024-09-17T13:33:58Z

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java

+  public SnapshotAccessRequestMembersResponse getGroupMembers(UUID id) {
+    SnapshotAccessRequestModel model = snapshotRequestDao.getById(id);
+    return new SnapshotAccessRequestMembersResponse()
+        .members(iamService.getGroupPolicyEmails(model.samGroupName(), "member"));


Instead of "member" this should use a constant. I see we have IamRole.MEMBER, could that work here?

pshapiro4broad · 2024-09-17T13:35:03Z

src/main/resources/api/data-repository-openapi.yaml

+      description: Get the members in the DAC group
+      operationId: getSnapshotAccessRequestGroupMembers
+      parameters:
+        - $ref: '#/components/parameters/Id'


I think this is the snapshot request ID, so isn't that a UUID?

pshapiro4broad · 2024-09-17T13:37:32Z

src/main/resources/api/data-repository-openapi.yaml

+        - name: memberEmail
+          in: query
+          description: The email of the user to add
+          required: true
+          schema:
+            type: string


It's confusing to me that this API takes both memberEmail and PolicyMemberRequest, I would expect the API to only accept one or the other. Is there a need for both in this API? If you only need one, I would use requestBody, as that is more typical for POST than query.

pshapiro4broad · 2024-09-17T13:43:00Z

src/main/resources/api/data-repository-openapi.yaml

+      operationId: deleteSnapshotAccessRequestGroupMember
+      parameters:
+        - $ref: '#/components/parameters/Id'
+        - name: memberEmail


Since we don't have an openapi type for email (e.g. we could define a type with a regex to match only emails) I think we need to validate the request. One way to do that is to use ValidationUtils.isValidEmail().

sonarcloud · 2024-09-20T16:17:01Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Collaborators API endpoints rough draft

f0fed46

cinyecai requested review from a team as code owners September 11, 2024 18:45

cinyecai requested review from rushtong and fboulnois and removed request for a team September 11, 2024 18:45

Fixing formatting

f2d3570

rjohanek reviewed Sep 12, 2024

View reviewed changes

rushtong removed their request for review September 13, 2024 15:13

fboulnois approved these changes Sep 16, 2024

View reviewed changes

pshapiro4broad reviewed Sep 17, 2024

View reviewed changes

cinyecai added 2 commits September 20, 2024 12:05

Addressing some PR feedback

13e230b

Fix spotless failure

c032983

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DC-1206] Add endpoints to list, add, and delete members from Sam group associated with Snapshots byRequestId #1810

[DC-1206] Add endpoints to list, add, and delete members from Sam group associated with Snapshots byRequestId #1810

cinyecai commented Sep 11, 2024

rjohanek Sep 12, 2024

pshapiro4broad Sep 16, 2024

rjohanek Sep 12, 2024

rjohanek Sep 12, 2024

rjohanek Sep 12, 2024

rjohanek Sep 12, 2024

snf2ye Sep 20, 2024

rjohanek Sep 12, 2024

pshapiro4broad Sep 17, 2024

cinyecai Sep 20, 2024

rjohanek Sep 12, 2024

rjohanek left a comment

fboulnois left a comment

pshapiro4broad left a comment

pshapiro4broad Sep 16, 2024

pshapiro4broad Sep 17, 2024

pshapiro4broad Sep 17, 2024

pshapiro4broad Sep 17, 2024

pshapiro4broad Sep 17, 2024

sonarcloud bot commented Sep 20, 2024

[DC-1206] Add endpoints to list, add, and delete members from Sam group associated with Snapshots byRequestId #1810

Are you sure you want to change the base?

[DC-1206] Add endpoints to list, add, and delete members from Sam group associated with Snapshots byRequestId #1810

Conversation

cinyecai commented Sep 11, 2024

Summary of changes

Testing Strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjohanek left a comment

Choose a reason for hiding this comment

fboulnois left a comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 20, 2024

Quality Gate passed