-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: develop
Are you sure you want to change the base?
Changes from 11 commits
7994f6d
80aa8d4
8c9af9c
7f38a14
b81101d
78cf618
1279a3b
d159d67
026ce34
af9404a
33a7224
4cff3e4
8b8c6f9
a3bd5ef
a668d3e
a78b1d9
b81e35b
6b5a816
2460032
fdcf6dd
19aaa1c
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 |
---|---|---|
|
@@ -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._ | ||
|
||
|
@@ -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) | ||
} | ||
} | ||
} | ||
} | ||
} ~ | ||
|
@@ -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 | ||
) | ||
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. 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? 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 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. 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 would put this in 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. Unfortunately 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. Moved to |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
|
||
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. modeled after |
||
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]] = | ||
|
@@ -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 => | ||
|
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..."? | ||
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. Yeah, I might think to add the two counts together to get total otherwise. Maybe 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 also wonder if they should indicate that they are counts of synchronized groups and not total group membership in Sam 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 refactored this; now we have counts for |
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]] = | ||
|
@@ -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) | ||
|
||
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 think it's kinda smelly to require 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 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) | ||
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 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 { | ||
|
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.
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.
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.
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