Skip to content
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

CORE-232: user supportSummary API #1617

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/main/resources/swagger/api-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,32 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/UserStatus'
/api/admin/v1/user/email/{email}/supportSummary:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am expecting that support will be dealing with email addresses instead of userids - but it would be trivial to add a supportSummary API that accepts an id if we needed one

get:
tags:
- Admin
summary: Get information about a user useful for Terra support
operationId: getAdminUserSummaryInfo
parameters:
- name: email
in: path
description: Email address of user to check
required: true
schema:
type: string
responses:
200:
description: user summary information
content:
application/json:
schema:
$ref: '#/components/schemas/SamUserSupportSummary'
500:
description: Internal Server Error
content:
application/json:
schema:
$ref: '#/components/schemas/ErrorReport'
/api/admin/v2/users:
get:
tags:
Expand Down Expand Up @@ -4277,6 +4303,27 @@ components:
items:
type: string
description: The parent groups of the group
SamUserSupportSummary:
type: object
properties:
additionalDetails:
type: object
description: >
additional details about the user, such as enterprise features they have access to example: '{ "enterpriseFeatures": ["feature1", "feature2"] }'
allowances:
$ref: '#/components/schemas/SamUserAllowances'
attributes:
$ref: '#/components/schemas/SamUserAttributes'
directGroupMembershipCount:
type: integer
description: number of groups to which this user is a direct member
indirectGroupMembershipCount:
type: integer
description: number of groups to which this user is a direct or indirect member
samUser:
$ref: '#/components/schemas/SamUserResponse'
termsOfServiceDetails:
$ref: '#/components/schemas/UserTermsOfServiceDetails'
PolicyIdentifiers:
required:
- policyName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
import org.broadinstitute.dsde.workbench.sam.model.SamResourceActions.{adminAddMember, adminReadPolicies, adminRemoveMember}
import org.broadinstitute.dsde.workbench.sam.model.SamResourceTypes.resourceTypeAdminName
import org.broadinstitute.dsde.workbench.sam.model._
import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AdminUpdateUserRequest, SamUser}
import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AdminUpdateUserRequest, SamUser, SamUserSupportSummaryResponse}
import org.broadinstitute.dsde.workbench.sam.service.{ManagedGroupService, ResourceService}
import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
import spray.json.DefaultJsonProtocol._
import spray.json.enrichAny
import spray.json.JsBoolean
import org.broadinstitute.dsde.workbench.sam.model.api.ManagedGroupModelJsonSupport._

Expand Down Expand Up @@ -48,13 +49,24 @@ trait AdminRoutes extends SecurityDirectives with SamRequestContextDirectives wi
def adminUserRoutes(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
pathPrefix("user") {
asWorkbenchAdmin(samUser) {
path("email" / Segment) { email =>
pathPrefix("email" / Segment) { email =>
val workbenchEmail = WorkbenchEmail(email)
getWithTelemetry(samRequestContext, emailParam(workbenchEmail)) {
complete {
userService
.getUserStatusFromEmail(workbenchEmail, samRequestContext)
.map(status => (if (status.isDefined) OK else NotFound) -> status)
pathEndOrSingleSlash {
getWithTelemetry(samRequestContext, emailParam(workbenchEmail)) {
complete {
userService
.getUserStatusFromEmail(workbenchEmail, samRequestContext)
.map(status => (if (status.isDefined) OK else NotFound) -> status)
}
}
} ~
pathPrefix("supportSummary") {
pathEndOrSingleSlash {
getWithTelemetry(samRequestContext, emailParam(workbenchEmail)) {
complete {
getSamUserSupportSummary(workbenchEmail, samRequestContext)
}
}
}
}
} ~
Expand Down Expand Up @@ -284,4 +296,33 @@ trait AdminRoutes extends SecurityDirectives with SamRequestContextDirectives wi
user.id,
samRequestContext
)

private def getSamUserSupportSummary(workbenchEmail: WorkbenchEmail, samRequestContext: SamRequestContext) =
for {
samUserOption <- userService.getUserFromEmail(workbenchEmail, samRequestContext)
samUser = samUserOption.getOrElse(throw new RuntimeException("user not found"))
allowances <- userService.getUserAllowances(samUser, samRequestContext)
maybeAttributes <- userService.getUserAttributes(samUser.id, samRequestContext)
termsOfServiceDetails <- tosService.getTermsOfServiceDetailsForUser(samUser.id, samRequestContext)
enterpriseFeatures <- resourceService
.listResourcesFlat(
samUser.id,
Set(ResourceTypeName("enterprise-feature")),
Set.empty,
Set(ResourceRoleName("user")),
Set.empty,
includePublic = false,
samRequestContext
)
directGroupMemberships <- userService.countDirectGroupMemberships(samUser, samRequestContext)
indirectGroupMemberships <- userService.countIndirectGroupMemberships(samUser, samRequestContext)
} yield SamUserSupportSummaryResponse(
samUser,
allowances,
maybeAttributes,
termsOfServiceDetails.getOrElse(TermsOfServiceDetails(None, None, permitsSystemUsage = false, isCurrentVersion = false)),
Map("enterpriseFeatures" -> enterpriseFeatures.toJson),
directGroupMembershipCount = directGroupMemberships,
indirectGroupMembershipCount = indirectGroupMemberships
)
Copy link
Contributor Author

@davidangb davidangb Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ trait DirectoryDAO {

def setUserAzureB2CId(userId: WorkbenchUserId, b2cId: AzureB2CId, samRequestContext: SamRequestContext): IO[Unit]

def loadUserByEmail(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[SamUser]]

def updateUserEmail(userId: WorkbenchUserId, email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Unit]

def deleteUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Unit]
Expand All @@ -98,6 +100,10 @@ trait DirectoryDAO {

def listFlattenedGroupMembers(groupName: WorkbenchGroupName, samRequestContext: SamRequestContext): IO[Set[WorkbenchUserId]]

def countDirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int]

def countIndirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int]

def enableIdentity(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Unit]

def disableIdentity(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Unit]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,20 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
}
}

override def loadUserByEmail(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
readOnlyTransaction("loadUserByEmail", samRequestContext) { implicit session =>
val userTable = UserTable.syntax

val loadUserQuery = samsql"""select ${userTable.resultAll}
from ${UserTable as userTable}
where ${userTable.email} = ${email}"""
loadUserQuery
.map(UserTable(userTable))
.single()
.apply()
.map(UserTable.unmarshalUserRecord)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modeled after loadUserByAzureB2CId

override def updateUserEmail(userId: WorkbenchUserId, email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Unit] = IO.unit

override def updateUser(samUser: SamUser, userUpdate: AdminUpdateUserRequest, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
Expand Down Expand Up @@ -718,6 +732,28 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
}
}

override def countDirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] =
davidangb marked this conversation as resolved.
Show resolved Hide resolved
readOnlyTransaction("countDirectGroupMemberships", samRequestContext) { implicit session =>
val query = samsql"""select count(distinct g.id) directMembershipCount
from sam_group g
join sam_group_member gm on g.id = gm.group_id
where g.synchronized_date is not null
and gm.member_user_id = ${samUser.id}"""

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

override def countIndirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] =
davidangb marked this conversation as resolved.
Show resolved Hide resolved
readOnlyTransaction("countDirectGroupMemberships", samRequestContext) { implicit session =>
davidangb marked this conversation as resolved.
Show resolved Hide resolved
val query = samsql"""select count(distinct g.id) indirectMembershipCount
from sam_group g
join sam_group_member_flat gmf on g.id = gmf.group_id
where g.synchronized_date is not null
and gmf.member_user_id = ${samUser.id}"""

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

davidangb marked this conversation as resolved.
Show resolved Hide resolved
override def enableIdentity(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Unit] =
subject match {
case userId: WorkbenchUserId =>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.broadinstitute.dsde.workbench.sam.model.api

import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
import org.broadinstitute.dsde.workbench.sam.model.api.SamUserAllowances.SamUserAllowedResponseFormat
import org.broadinstitute.dsde.workbench.sam.model.api.SamUserAttributes.SamUserAttributesFormat
import org.broadinstitute.dsde.workbench.sam.model.TermsOfServiceDetails
import spray.json.DefaultJsonProtocol._
import spray.json._

object SamUserSupportSummaryResponse {
implicit val SamUserSupportSummaryResponseFormat: RootJsonFormat[SamUserSupportSummaryResponse] = jsonFormat7(SamUserSupportSummaryResponse.apply)
}
final case class SamUserSupportSummaryResponse(
samUser: SamUser,
allowances: SamUserAllowances,
attributes: Option[SamUserAttributes],
termsOfServiceDetails: TermsOfServiceDetails,
additionalDetails: Map[String, JsValue],
directGroupMembershipCount: Int,
indirectGroupMembershipCount: Int // TODO: for clarity, should this be "directAndIndirectGroup..."?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davidangb davidangb Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,9 @@ class UserService(
def getUserFromPetManagedIdentity(petManagedIdentityObjectId: ManagedIdentityObjectId, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
directoryDAO.getUserFromPetManagedIdentity(petManagedIdentityObjectId, samRequestContext)

def getUserFromEmail(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
directoryDAO.loadUserByEmail(email, samRequestContext)

def getSubjectFromGoogleSubjectId(googleSubjectId: GoogleSubjectId, samRequestContext: SamRequestContext): IO[Option[WorkbenchSubject]] =
directoryDAO.loadSubjectFromGoogleSubjectId(googleSubjectId, samRequestContext)
def getSubjectFromEmail(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[WorkbenchSubject]] =
Expand Down Expand Up @@ -505,6 +508,12 @@ class UserService(
case None => IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"User $workbenchUserId not found")))
}
}

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's kinda smelly to require ResourceService as an argument to this method. But I also didn't want to update all 67 calls to the UserService constructor just to support this one method.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I agree. An alternative would be to create another Service class that depends on both but 🤷 . I leave it up to you.

def countIndirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] =
directoryDAO.countIndirectGroupMemberships(samUser, samRequestContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

object UserService {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,10 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
override def listFlattenedGroupMembers(groupName: WorkbenchGroupName, samRequestContext: SamRequestContext): IO[Set[WorkbenchUserId]] =
IO(listGroupUsers(groupName, Set.empty))

override def countDirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] = ???

override def countIndirectGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] = ???

override def loadUserByAzureB2CId(userId: AzureB2CId, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
IO.pure(users.values.find(_.azureB2CId.contains(userId)))

Expand All @@ -347,6 +351,9 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
} yield users += user.id -> user.copy(azureB2CId = Option(b2CId))
}

override def loadUserByEmail(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
IO.pure(users.values.find(_.email.equals(email)))

override def checkStatus(samRequestContext: SamRequestContext): IO[Boolean] = IO(passStatusCheck)

override def loadUserByGoogleSubjectId(userId: GoogleSubjectId, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,52 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
}
}

"countDirectGroupMemberships" - {
"calculate the direct count" in {
assume(databaseEnabled, databaseEnabledClue)

val subGroupId = WorkbenchGroupName("subGroup")
val subGroup = BasicWorkbenchGroup(subGroupId, Set(defaultUser.id), WorkbenchEmail("subGroup@foo.com"))
val parentGroupId = WorkbenchGroupName("parentGroup")
val parentGroup = BasicWorkbenchGroup(parentGroupId, Set(subGroupId), WorkbenchEmail("parentGroup@foo.com"))

val createdUser = dao.createUser(defaultUser, samRequestContext).unsafeRunSync()
dao.createGroup(subGroup, samRequestContext = samRequestContext).unsafeRunSync()
dao.createGroup(parentGroup, samRequestContext = samRequestContext).unsafeRunSync()

// countDirectGroupMemberships() only counts synchronized groups;
// update the sync date for the groups we just created
dao.updateSynchronizedDateAndVersion(subGroup, samRequestContext).unsafeRunSync()
dao.updateSynchronizedDateAndVersion(parentGroup, samRequestContext).unsafeRunSync()

val directCount = dao.countDirectGroupMemberships(defaultUser, samRequestContext).unsafeRunSync()
directCount shouldBe 1
}
}

"countIndirectGroupMemberships" - {
"calculate the indirect count" in {
assume(databaseEnabled, databaseEnabledClue)

val subGroupId = WorkbenchGroupName("subGroup")
val subGroup = BasicWorkbenchGroup(subGroupId, Set(defaultUser.id), WorkbenchEmail("subGroup@foo.com"))
val parentGroupId = WorkbenchGroupName("parentGroup")
val parentGroup = BasicWorkbenchGroup(parentGroupId, Set(subGroupId), WorkbenchEmail("parentGroup@foo.com"))

dao.createUser(defaultUser, samRequestContext).unsafeRunSync()
dao.createGroup(subGroup, samRequestContext = samRequestContext).unsafeRunSync()
dao.createGroup(parentGroup, samRequestContext = samRequestContext).unsafeRunSync()

// countIndirectGroupMemberships() only counts synchronized groups;
// update the sync date for the groups we just created
dao.updateSynchronizedDateAndVersion(subGroup, samRequestContext).unsafeRunSync()
dao.updateSynchronizedDateAndVersion(parentGroup, samRequestContext).unsafeRunSync()

val indirectCount = dao.countIndirectGroupMemberships(defaultUser, samRequestContext).unsafeRunSync()
indirectCount shouldBe 2
}
}

"createPetServiceAccount" - {
"create pet service accounts" in {
assume(databaseEnabled, databaseEnabledClue)
Expand Down Expand Up @@ -1710,6 +1756,15 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
}
}

"loadUserByEmail" - {
"load a user from their email" in {
assume(databaseEnabled, databaseEnabledClue)
dao.createUser(defaultUser, samRequestContext).unsafeRunSync()

dao.loadUserByEmail(defaultUser.email, samRequestContext).unsafeRunSync() shouldBe Some(defaultUser)
}
}

"createPetManagedIdentity" - {
"create pet managed identity" in {
assume(databaseEnabled, databaseEnabledClue)
Expand Down
Loading