ID-690 Get User(s) Endpoint v2 #1208

tlangs · 2023-10-13T16:43:33Z

Ticket:

What:

Some new users/v2 endpoints to get users, via oneself and via admin powers, all in the same API!

Why:

We want to get rid of some of the separate /admin endpoints and gate behavior on standard CRUD endpoints.

How:

Added a SamUserResponse object, which replaces enabled with allowed.
Added a SamUserAllowances object, which details the reasons a user might be allowed/unallowed.
Added a /api/users/v2/self endpoint. This just gets the SamUserResponse object of the calling user
Added a /api/users/v2/{sam_user_id}. A regular user can only get themselves via their ID. An Admin user can get any SamUserResponse object.
Added a /api/users/v2/self/allowed endpoint to allow a user to interrogate why they might be allowed/unallowed
Added a /api/users/v2/{sam_user_id}/allowed endpoint to allow an admin to interrogate why another user might be allowed/unallowed
I've also refactored the model code a little bit.

PR checklist

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

sjkobori · 2023-10-16T19:24:15Z

src/main/resources/swagger/api-docs.yaml

+      required:
+        - id
+        - email
+        - enabled
+        - createdAt
+        - updatedAt
+      properties:


I think enabled did not get updated to allowed here

sjkobori · 2023-10-16T19:27:51Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2.scala

+          // api/users/v2/self
+          pathEndOrSingleSlash {
+            getSamUserResponse(samUser, samRequestContext)
+          } ~
+            // api/users/v2/self/allowed
+            pathPrefix("allowed") {


I appreciate these comments, akka formatting is not easy to read...

sjkobori · 2023-10-16T19:36:20Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2Spec.scala

+      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
+      .withAllowedUser(defaultUser) // "allowed" user we will check the status of


Since these tests are on the API routing layer, they should not have a distinction between and allowed and enabled user. I think we should defer to using withAllowedUser

If not possible otherwise, this may be a place where we need to mock out the only routes for backwards compatibility.

I wasn't sure what to do here. I think it is valuable, because a user being enabled doesn't necessarily mean they can use Terra. I think there's value in being able to test and respond accordingly. I do agree that this is clunky, having to specify both things. Maybe all usages of withEnabledUser(s) should be migrated to a new withReadyUser(s) call, which would do both enabling and allowing, leaving the other two methods there for testing the specifics if need be.

I think in general, the api mocking builder should only care about mocking things from the api layer's perspective. So the distinction between allowed and enabled isnt a detail which the api is concerned about, so I dont think the mock should have any logic on it in this layer. In the service layer, it is important to make that distinction, but I dont believe so in the api layer.

Good point. I'll change things.

Actually, I'm going to take it back. As I was rewriting, I realized that the difference between enabled and allowed is significant to the api layer. They mean different things. allowed for the v2 endpoints, means the user is able to use the system, which means they're both enabled in the db AND have accepted the Terms of Service. These things are spelled out in the API responses, so we need to be able to test them. The test code as written doesn't actually mandate that the test "know" that allowed means enabled and ToS accepted, but the responses should look right.

It looks like this has been modified, but IIRC I was trying to have these things call each other so that if I wanted to say .withEnabledUser it would take care of creating the user automatically. So carrying that logic forward, I would expect .withAllowedUser to make the user enabled as well, since for a user to be Allowed it must also be enabled.

sjkobori · 2023-10-16T19:40:25Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2Spec.scala

+  "GET /api/users/v2/{sam_user_id}" should "return the regular user if they're getting themselves" in {
+    // Arrange
+    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
+      .withEnabledUsers(Seq(defaultUser, otherUser))
+      .withAllowedUsers(Seq(defaultUser, otherUser))
+      .callAsNonAdminUser()
+      .build


I think now that we have logic dependant on if the calling user's idenitity is an admin or not, we are going to need to pass args into callAsNonAdminUser since it would not be true for any arbitrary non admin, it is only true of the nonAdmin who is calling is in fact the user that is being gotten

Since we're testing the logic based on the calling user, not the requested user, I think the functionality is still valid. It is kinda un-intuitive though, making all calls to the routes be as an Admin user. I think it could be refactored, but I don't think its necessary to do in this PR

I guess what I am trying to get as is: how can we know if the calling non admin user cannot make requests for any other enabled/allowed user? It seems like there would not be any distinction in this test

Added the samUser arg to callAs(Non)AdminUser methods

@tlangs you're 100% right that needing to construct the routes WITH the one and only 1 user that we will be calling those routes with is indeed gross. It's a function of how the routes are written and where we do the permissions checks (pro-tip: don't sprinkle business logic like authorization checks in your routing logic). This is definitely a refactoring we should do at some point, and for now, the callAsFooUser was the best solution I could come up with that read somewhat nicely and got us around the yucky.

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV1.scala

…te/sam into tl_ID-690_get_user_endpoint

Ghost-in-a-Jar

This looks good to me, but just in case we should do some manual testing in dev/staging before the prod deploy of this stuff. We def don't want non-admin users to be able to grab each-other's info.

tlangs · 2023-10-18T16:00:43Z

@Ghost-in-a-Jar Absolutely. I'll test this on a BEE before merging.

tlangs · 2023-10-18T17:21:01Z

@Ghost-in-a-Jar

# Get another user as an admin
$ curl -X 'GET'   'https://sam.tlangs-user-endpoints.bee.envs-terra.bio/api/users/v2/26976461068667bfb9222' \
   -H 'accept: application/json' -H "Authorization: Bearer $(gcloud auth print-access-token tlangs@test.firecloud.org)"
> {"allowed":true,"azureB2CId":"d7d55ae5-07c7-41c0-8349-79305b3fbc52","createdAt":"2023-10-18T16:21:46.873051Z","email":"tlangs@test.firecloud.org","googleSubjectId":"112547541637891094041","id":"26976461068667bfb9222","registeredAt":"2023-10-18T16:21:46.872268Z","updatedAt":"2023-10-18T16:21:49.929332Z"}

# Get another user as a non-admin
$ curl -X 'GET'   'https://sam.tlangs-user-endpoints.bee.envs-terra.bio/api/users/v2/2697646592417bef9cd21'   -H 'accept: application/json' -H "Authorization: Bearer $(gcloud auth print-access-token tlangs@test.firecloud.org)"
> {"causes":[],"message":"You must be an admin.","source":"sam","stackTrace":[],"statusCode":404}

gpolumbo-broad · 2023-10-18T21:30:35Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala

@@ -33,7 +32,7 @@ trait UserRoutes extends SamUserDirectives with SamRequestContextDirectives {
    })
  }

-  def userRoutes(samRequestContext: SamRequestContext): server.Route =
+  def oldUserRoutes(samRequestContext: SamRequestContext): server.Route =


We should mark this as deprecated

I think we should only mark as deprecated once all the functionality is implemented in the V2 routes

gpolumbo-broad · 2023-10-18T21:31:53Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV1.scala

+trait UserRoutesV1 extends SamUserDirectives with SamRequestContextDirectives {
+  val userService: UserService
+
+  def userRoutesV1(samUser: SamUser, samRequestContext: SamRequestContext): server.Route = pathPrefix("users") {


Should have happened before, but we should mark this as deprecated

gpolumbo-broad · 2023-10-18T21:33:01Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2.scala

+      pathPrefix("users") {
+        pathPrefix("v2") {
+          pathPrefix("self") {
+            // api/users/v2/self


Hahaha ❤️ I was literally just doing this 5 minutes ago in my own PR so I could understand what the hell the routes are. This syntax is brutal.

Its sooooo bad

gpolumbo-broad · 2023-10-18T21:35:55Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamUserAllowances.scala

+  def apply(allowed: Boolean, enabled: Boolean, termsOfService: Boolean): SamUserAllowances =
+    SamUserAllowances(allowed, Map("enabled" -> enabled, "termsOfService" -> termsOfService))
+}
+final case class SamUserAllowances(


or maybe SamUserAllowance?

I like it being plural, since it details out all the reasons someone might not be allowed to use the system.

gpolumbo-broad · 2023-10-18T21:48:43Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2Spec.scala

+  "GET /api/users/v2/{sam_user_id}" should "return the regular user if they're getting themselves" in {
+    // Arrange
+    val samRoutes = new MockSamRoutesBuilder(allUsersGroup)
+      .withEnabledUsers(Seq(defaultUser, otherUser))
+      .withAllowedUsers(Seq(defaultUser, otherUser))
+      .callAsNonAdminUser()
+      .build


@tlangs you're 100% right that needing to construct the routes WITH the one and only 1 user that we will be calling those routes with is indeed gross. It's a function of how the routes are written and where we do the permissions checks (pro-tip: don't sprinkle business logic like authorization checks in your routing logic). This is definitely a refactoring we should do at some point, and for now, the callAsFooUser was the best solution I could come up with that read somewhat nicely and got us around the yucky.

gpolumbo-broad · 2023-10-18T21:55:19Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV2Spec.scala

+      .withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
+      .withAllowedUser(defaultUser) // "allowed" user we will check the status of


It looks like this has been modified, but IIRC I was trying to have these things call each other so that if I wanted to say .withEnabledUser it would take care of creating the user automatically. So carrying that logic forward, I would expect .withAllowedUser to make the user enabled as well, since for a user to be Allowed it must also be enabled.

sonarqubecloud · 2023-10-19T14:09:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
5.5% Duplication

tlangs added 4 commits October 12, 2023 16:46

refactor SamUser into its own file

0a36476

endpoints

8da07d8

a wee bit of a refactor

5682628

tests

6cd8837

tlangs changed the title ~~ID-690 get user endpoint~~ ID-690 Get User(s) Endpoint v2 Oct 13, 2023

tlangs added 2 commits October 16, 2023 14:09

tests for endpoints

6b3a257

user allowed endpoint

bfa676d

sjkobori reviewed Oct 16, 2023

View reviewed changes

Ghost-in-a-Jar reviewed Oct 16, 2023

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala Show resolved Hide resolved

Ghost-in-a-Jar reviewed Oct 16, 2023

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutesV1.scala Show resolved Hide resolved

tlangs added 3 commits October 18, 2023 09:21

Merge branch 'develop' into tl_ID-690_get_user_endpoint

6624bd2

update api doc

c04a52f

Merge branch 'tl_ID-690_get_user_endpoint' of github.com:broadinstitu…

52e47c5

…te/sam into tl_ID-690_get_user_endpoint

Ghost-in-a-Jar approved these changes Oct 18, 2023

View reviewed changes

PR comments and cleanup

4c5d9c5

gpolumbo-broad approved these changes Oct 18, 2023

View reviewed changes

sjkobori approved these changes Oct 19, 2023

View reviewed changes

tlangs added 2 commits October 19, 2023 10:07

Merge branch 'develop' into tl_ID-690_get_user_endpoint

d0b446b

Merge branch 'develop' into tl_ID-690_get_user_endpoint

c380e7d

tlangs merged commit 5a91c3f into develop Oct 19, 2023
15 checks passed

tlangs deleted the tl_ID-690_get_user_endpoint branch October 19, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ID-690 Get User(s) Endpoint v2 #1208

ID-690 Get User(s) Endpoint v2 #1208

tlangs commented Oct 13, 2023 •

edited

Loading

sjkobori Oct 16, 2023 •

edited

Loading

tlangs Oct 18, 2023

sjkobori Oct 16, 2023

sjkobori Oct 16, 2023

sjkobori Oct 16, 2023

tlangs Oct 16, 2023

sjkobori Oct 18, 2023

tlangs Oct 18, 2023

tlangs Oct 18, 2023

gpolumbo-broad Oct 18, 2023

sjkobori Oct 16, 2023

tlangs Oct 16, 2023

sjkobori Oct 18, 2023

tlangs Oct 18, 2023

gpolumbo-broad Oct 18, 2023

Ghost-in-a-Jar left a comment

tlangs commented Oct 18, 2023

tlangs commented Oct 18, 2023

gpolumbo-broad Oct 18, 2023

tlangs Oct 19, 2023

gpolumbo-broad Oct 18, 2023

gpolumbo-broad Oct 18, 2023

tlangs Oct 19, 2023

gpolumbo-broad Oct 18, 2023

tlangs Oct 19, 2023

gpolumbo-broad Oct 18, 2023

gpolumbo-broad Oct 18, 2023

sonarqubecloud bot commented Oct 19, 2023

		.withEnabledUser(defaultUser) // "persisted/enabled" user we will check the status of
		.withAllowedUser(defaultUser) // "allowed" user we will check the status of

ID-690 Get User(s) Endpoint v2 #1208

ID-690 Get User(s) Endpoint v2 #1208

Conversation

tlangs commented Oct 13, 2023 • edited Loading

sjkobori Oct 16, 2023 • edited Loading

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

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

Ghost-in-a-Jar left a comment

Choose a reason for hiding this comment

tlangs commented Oct 18, 2023

tlangs commented Oct 18, 2023

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

sonarqubecloud bot commented Oct 19, 2023

tlangs commented Oct 13, 2023 •

edited

Loading

sjkobori Oct 16, 2023 •

edited

Loading