CORE-232: user supportSummary API #1617

davidangb · 2025-01-03T17:23:52Z

Ticket: https://broadworkbench.atlassian.net/browse/CORE-232

What:

New admin API for information about a user, including direct, indirect, and unsynchronized group membership counts.

This is implemented by adding the group membership counts to the existing logic for the getSamUserCombinedState API, and then creating a new admin API which reuses that logic but allows an admin to execute that logic for any user.

Review Questions:

What permissions should this API have, and how should we implement them? The new API has the same permissions as GET /api/admin/v1/user/email/{email} - specifically, it requires super-admin. This probably won't work to expose as a support API. I'd like to follow the pattern set by the group supportSummary, which uses a resource type admin action … but Sam has no resource type for "user". Should we make one, even if we don't create user resources?

Example

This new API returns a payload that looks like:

{
  "additionalDetails": {
    "enterpriseFeatures": {
      "format": "flat",
      "resources": []
    }
  },
  "allowances": {
    "allowed": true,
    "details": {
      "enabled": true,
      "termsOfService": true
    }
  },
  "attributes": {
    "marketingConsent": true,
    "userId": "2736276474111094565b5"
  },
  "favoriteResources": [],
  "groupMembershipCounts": {
    "directSynchronized": 2,
    "totalSynchronized": 4,
    "unsynchronized": 1
  },
  "samUser": {
    "azureB2CId": "9ec01dd3-ddec-4273-8e34-39fdd2a480ab",
    "createdAt": "2025-01-07T19:01:14.166491Z",
    "email": "davidan.dev@gmail.com",
    "enabled": true,
    "googleSubjectId": "111137720113699110757",
    "id": "2736276474111094565b5",
    "registeredAt": "2025-01-07T19:01:14.164172Z",
    "updatedAt": "2025-01-07T19:01:18.917368Z"
  },
  "termsOfServiceDetails": {
    "acceptedOn": "2025-01-07T19:01:18.889191Z",
    "isCurrentVersion": true,
    "latestAcceptedVersion": "1",
    "permitsSystemUsage": true
  }
}

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

davidangb · 2025-01-03T20:01:42Z

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

+      Map("enterpriseFeatures" -> enterpriseFeatures.toJson),
+      directGroupMembershipCount = directGroupMemberships,
+      indirectGroupMembershipCount = indirectGroupMemberships
+    )


This follows the pattern set by getSamUserCombinedState in UserRoutesV2, in which business logic that requires multiple services is implemented in the route class. Is there a better place to put this logic?

I don't love having the business logic in the route class, but alternatives are probably not worth the effort and the pattern already exists in Sam so seems fine to me.

I would put this in userService (which already knows about tosService).

Unfortunately UserService doesn't know about ResourceService, which we need to get the enterprise features. Whaddya think - is it worth giving UserService that knowledge, or do we not actually care about the enterprise features at this point?

Moved to UserService. This is kinda hacky in that the method requires the caller to pass in a ResourceService. I am hesitant to change the UserService constructor just for this one API.

davidangb · 2025-01-03T20:03:49Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

+        .apply()
+        .map(UserTable.unmarshalUserRecord)
+    }
+


modeled after loadUserByAzureB2CId

davidangb · 2025-01-03T20:04:39Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

+
+      query.map(rs => rs.int(1)).single().apply().getOrElse(0)
+    }
+


I based these SQL queries off previous Slack discussion. I believe they are correct … but would love confirmation/oversight!

They look correct to me but I haven't done any manual verification

They also look correct to me. One thing to note is that this does not include public resources but we probably want to deal with that separately.

davidangb · 2025-01-03T20:05:40Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala

+    directoryDAO.countDirectGroupMemberships(samUser, samRequestContext)
+
+  def countIndirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] =
+    directoryDAO.countIndirectGroupMemberships(samUser, samRequestContext)


I did not create unit tests for the three new methods here in UserService, since they don't add any logic above/beyond the PostgresDirectoryDAO methods.

marctalbott

My understanding is that we can't expand access to the Sam super admin group to everyone who will need to call this API, so creating a user resource_type_admin resource seems reasonable to me even though it's definitely a departure from other resource_type_admin resources

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

marctalbott · 2025-01-03T21:24:56Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

+
+      query.map(rs => rs.int(1)).single().apply().getOrElse(0)
+    }
+


They look correct to me but I haven't done any manual verification

marctalbott · 2025-01-03T21:27:40Z

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

@@ -146,6 +146,32 @@ paths:
            application/json:
              schema:
                $ref: '#/components/schemas/UserStatus'
+  /api/admin/v1/user/email/{email}/supportSummary:


I generally favor the usage of Sam user ids over emails when identifying users, but I expect emails will usually be more convenient for callers of this API. It might be helpful to expose this information via both email and user id in this case.

marctalbott · 2025-01-03T21:30:20Z

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

+      Map("enterpriseFeatures" -> enterpriseFeatures.toJson),
+      directGroupMembershipCount = directGroupMemberships,
+      indirectGroupMembershipCount = indirectGroupMemberships
+    )


I don't love having the business logic in the route class, but alternatives are probably not worth the effort and the pattern already exists in Sam so seems fine to me.

marctalbott · 2025-01-03T21:31:54Z

...in/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamUserSupportSummaryResponse.scala

+    termsOfServiceDetails: TermsOfServiceDetails,
+    additionalDetails: Map[String, JsValue],
+    directGroupMembershipCount: Int,
+    indirectGroupMembershipCount: Int // TODO: for clarity, should this be "directAndIndirectGroup..."?


Yeah, I might think to add the two counts together to get total otherwise. Maybe totalGroupMembershipCount?

I also wonder if they should indicate that they are counts of synchronized groups and not total group membership in Sam

I refactored this; now we have counts for directSynchronized, totalSynchronized, and unsynchronized.

dvoet

Sam currently has 2 admin mechanisms, the newer backed by resource type admin and the older backed by the fc-admins google group. It looks like the older is only used to gate access to user specific apis (should verify this). It would be great to consolidate to the newer. I think that would mean creating a user resource type admin resource even though user is not a resource type. I could see that being a follow on ticket. All of our support staff need to be suitable though, so they may already be members of fc-admins.

dvoet · 2025-01-06T21:37:34Z

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

+      Map("enterpriseFeatures" -> enterpriseFeatures.toJson),
+      directGroupMembershipCount = directGroupMemberships,
+      indirectGroupMembershipCount = indirectGroupMemberships
+    )


I would put this in userService (which already knows about tosService).

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

dvoet · 2025-01-06T21:44:51Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

+
+      query.map(rs => rs.int(1)).single().apply().getOrElse(0)
+    }
+


They also look correct to me. One thing to note is that this does not include public resources but we probably want to deal with that separately.

dvoet · 2025-01-06T21:57:16Z

This new API is very similar to the getSamUserCombinedState API. Should they be consolidated?

I think it would be nice if they were the same api under hood. Exposing the counts to the user may be useful

sonarqubecloud · 2025-01-07T21:48:46Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

davidangb added 9 commits January 2, 2025 17:10

first draft, user support summary

7994f6d

group membership counts

80aa8d4

unbreak unit tests; remove obsolete comment

8c9af9c

refactor getUserFromEmail into SQL

7f38a14

loadUserByEmail unit test

b81101d

direct/indirect count unit tests

78cf618

Merge branch 'develop' into da_CORE-232_userSupportApis

1279a3b

simplify count sql

d159d67

update openapi spec

026ce34

davidangb commented Jan 3, 2025

View reviewed changes

MockDirectoryDAO.loadUserByEmail implementation

af9404a

davidangb marked this pull request as ready for review January 3, 2025 20:24

davidangb requested a review from a team as a code owner January 3, 2025 20:24

davidangb requested review from samanehsan and marctalbott January 3, 2025 20:24

Merge branch 'develop' into da_CORE-232_userSupportApis

33a7224

marctalbott reviewed Jan 6, 2025

View reviewed changes

dvoet reviewed Jan 6, 2025

View reviewed changes

davidangb added 4 commits January 6, 2025 17:17

rename methods

4cff3e4

fix compile errors

8b8c6f9

Merge branch 'develop' into da_CORE-232_userSupportApis

a3bd5ef

fix compile errors, again

a668d3e

davidangb mentioned this pull request Jan 7, 2025

CORE-232: create 'user' resource type #1620

Open

2 tasks

samanehsan approved these changes Jan 7, 2025

View reviewed changes

davidangb added 2 commits January 7, 2025 15:56

consolidate support summary and combined state APIs

a78b1d9

scalafmt and missed files

b81e35b

davidangb marked this pull request as draft January 7, 2025 21:03

davidangb added 2 commits January 7, 2025 16:37

add unsynchronized count

6b5a816

scalafmt

2460032

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-232: user supportSummary API #1617

CORE-232: user supportSummary API #1617

davidangb commented Jan 3, 2025 •

edited

Loading

davidangb Jan 3, 2025 •

edited

Loading

marctalbott Jan 3, 2025

dvoet Jan 6, 2025

davidangb Jan 6, 2025

davidangb Jan 7, 2025

davidangb Jan 3, 2025

davidangb Jan 3, 2025

marctalbott Jan 3, 2025

dvoet Jan 6, 2025

davidangb Jan 3, 2025

marctalbott left a comment

marctalbott Jan 3, 2025

marctalbott Jan 3, 2025

marctalbott Jan 3, 2025

marctalbott Jan 3, 2025

marctalbott Jan 3, 2025

davidangb Jan 7, 2025 •

edited

Loading

dvoet left a comment

dvoet Jan 6, 2025

dvoet Jan 6, 2025

dvoet commented Jan 6, 2025 •

edited

Loading

sonarqubecloud bot commented Jan 7, 2025

CORE-232: user supportSummary API #1617

Are you sure you want to change the base?

CORE-232: user supportSummary API #1617

Conversation

davidangb commented Jan 3, 2025 • edited Loading

What:

Review Questions:

Example

davidangb Jan 3, 2025 • 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

marctalbott 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

davidangb Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

dvoet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoet commented Jan 6, 2025 • edited Loading

sonarqubecloud bot commented Jan 7, 2025

Quality Gate passed

davidangb commented Jan 3, 2025 •

edited

Loading

davidangb Jan 3, 2025 •

edited

Loading

davidangb Jan 7, 2025 •

edited

Loading

dvoet commented Jan 6, 2025 •

edited

Loading