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

ID-787 Get user/self terms of service history. #1254

Merged
merged 15 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
79 changes: 79 additions & 0 deletions src/main/resources/swagger/api-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3370,6 +3370,58 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/ErrorReport'
/termsOfService/v1/user/self/history:
get:
tags:
- TermsOfService
summary: gets a user's own terms of service acceptance/rejection action history
operationId: userTermsOfServiceSelfGetHistory
parameters:
- name: limit
in: query
description: limit on how many records to return, defaults to 100
required: false
schema:
type: string
responses:
200:
description: user has a history of terms of service actions
content:
application/json:
schema:
$ref: '#/components/schemas/UserTermsOfServiceHistory'
404:
description: user has no history of terms of service actions
content:
application/json:
schema:
$ref: '#/components/schemas/ErrorReport'
/termsOfService/v1/user/{sam_user_id}/history:
get:
tags:
- TermsOfService
summary: gets a user's terms of service acceptance/rejection action history, can only get another user's history if the requester is an admin.
operationId: userTermsOfServiceGetUserHistory
parameters:
- name: sam_user_id
in: path
description: the id of the sam user to get terms of service status for
required: true
schema:
type: string
- name: limit
in: query
description: limit on how many records to return, defaults to 100
required: false
schema:
type: string
responses:
200:
description: user has a history of terms of service actions
content:
application/json:
schema:
$ref: '#/components/schemas/UserTermsOfServiceHistory'
/version:
get:
tags:
Expand Down Expand Up @@ -4091,6 +4143,33 @@ components:
type: boolean
description: based on the user's currently accepted terms of service version and when the terms of service were last changed,
should the user still be permitted to use the system?
UserTermsOfServiceHistory:
required:
- history
type: object
description: a user's terms of service action history
properties:
history:
type: array
items:
$ref: '#/components/schemas/UserTermsOfServiceHistoryRecord'
UserTermsOfServiceHistoryRecord:
required:
- action
- version
- timestamp
type: object
properties:
action:
type: string
description: the terms of service action the user took
version:
type: string
description: the version of the terms of service the user accepted/rejected
timestamp:
type: string
format: date-time
description: the timestamp of the action
ManagedResourceGroupCoordinates:
required:
- tenantId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ trait OldUserRoutes extends SamUserDirectives with SamRequestContextDirectives {
get {
withUserAllowInactive(samRequestContext) { samUser =>
complete {
tosService.getTosComplianceStatus(samUser, samRequestContext).map { tosAcceptanceStatus =>
tosService.getTermsOfServiceComplianceStatus(samUser, samRequestContext).map { tosAcceptanceStatus =>
StatusCodes.OK -> Option(JsBoolean(tosAcceptanceStatus.permitsSystemUsage))
}
}
Expand Down Expand Up @@ -144,7 +144,7 @@ trait OldUserRoutes extends SamUserDirectives with SamRequestContextDirectives {
} ~
path("termsOfServiceComplianceStatus") {
get {
complete(tosService.getTosComplianceStatus(user, samRequestContext))
complete(tosService.getTermsOfServiceComplianceStatus(user, samRequestContext))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,17 @@ trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirec
complete(tosService.rejectCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent))
}
}
} ~
pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
pathEndOrSingleSlash {
get {
parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
complete {
tosService.getTermsOfServiceHistoryForUser(samUser.id, samRequestContext, limit)
}
}
}
}
}
} ~
// The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's
Expand All @@ -113,7 +124,9 @@ trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirec
pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
pathEndOrSingleSlash {
getWithTelemetry(samRequestContext, userIdParam(requestUserId)) {
complete(StatusCodes.NotImplemented)
parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit))
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,14 @@ trait DirectoryDAO {

def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext, action: Option[String] = None): IO[Option[SamUserTos]]
def getUserTermsOfServiceVersion(
userId: WorkbenchUserId,
tosVersion: Option[String],
samRequestContext: SamRequestContext,
action: Option[String] = None
): IO[Option[SamUserTos]]
def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]]
def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity]
def loadPetManagedIdentity(petManagedIdentityId: PetManagedIdentityId, samRequestContext: SamRequestContext): IO[Option[PetManagedIdentity]]
def getUserFromPetManagedIdentity(petManagedIdentityObjectId: ManagedIdentityObjectId, samRequestContext: SamRequestContext): IO[Option[SamUser]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -649,27 +649,51 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
}

// When no tosVersion is specified, return the latest TosRecord for the user
Ghost-in-a-Jar marked this conversation as resolved.
Show resolved Hide resolved
override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
getUserTosVersion(userId, None, samRequestContext)

override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext, action: Option[String] = None): IO[Option[SamUserTos]] =
getUserTermsOfServiceVersion(userId, None, samRequestContext, action)

override def getUserTermsOfServiceVersion(
userId: WorkbenchUserId,
tosVersion: Option[String],
samRequestContext: SamRequestContext,
action: Option[String] = None
): IO[Option[SamUserTos]] =
readOnlyTransaction("getUserTermsOfService", samRequestContext) { implicit session =>
val tosTable = TosTable.syntax
val column = TosTable.column

val versionConstraint = if (tosVersion.isDefined) samsqls"and ${column.version} = ${tosVersion.get}" else samsqls""
val versionConstraint = tosVersion.map(v => samsqls"and ${column.version} = $v").getOrElse(samsqls"")
val actionConstraint = action.map(a => samsqls"and ${column.action} = $a").getOrElse(samsqls"")

val loadUserTosQuery =
samsql"""select ${tosTable.resultAll}
from ${TosTable as tosTable}
where ${column.samUserId} = $userId $versionConstraint
where ${column.samUserId} = $userId
$versionConstraint
$actionConstraint
order by ${column.createdAt} desc
limit 1"""

val userTosRecordOpt: Option[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).first().apply()
userTosRecordOpt.map(TosTable.unmarshalUserRecord)
}

override def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] =
readOnlyTransaction("getUserTermsOfServiceHistory", samRequestContext) { implicit session =>
val tosTable = TosTable.syntax
val column = TosTable.column

val loadUserTosQuery =
samsql"""select ${tosTable.resultAll}
from ${TosTable as tosTable}
where ${column.samUserId} = ${userId}
order by ${column.createdAt} desc
limit ${limit}"""

val userTosRecordOpt: List[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).list().apply()
userTosRecordOpt.map(TosTable.unmarshalUserRecord)
}

override def isEnabled(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Boolean] =
readOnlyTransaction("isEnabled", samRequestContext) { implicit session =>
val userIdOpt = subject match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ object UserStatusDetails {
)

@Lenses final case class TermsOfServiceDetails(latestAcceptedVersion: String, acceptedOn: Instant, permitsSystemUsage: Boolean, isCurrentVersion: Boolean)
@Lenses final case class TermsOfServiceHistory(history: List[TermsOfServiceHistoryRecord])
@Lenses final case class TermsOfServiceHistoryRecord(action: String, version: String, timestamp: Instant)

@Lenses final case class ResourceActionPattern(value: String, description: String, authDomainConstrainable: Boolean) {
def matches(other: ResourceAction) = value.r.pattern.matcher(other.value).matches()
Expand Down Expand Up @@ -254,6 +256,7 @@ final case class SamUserTos(id: WorkbenchUserId, version: String, action: String
this.action == userTos.action
case _ => false
}
def toHistoryRecord: TermsOfServiceHistoryRecord = TermsOfServiceHistoryRecord(action, version, createdAt)
}
object SamLenses {
val resourceIdentityAccessPolicy = AccessPolicy.id composeLens FullyQualifiedPolicyId.resource
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import org.broadinstitute.dsde.workbench.sam.model.{
TermsOfServiceAcceptance,
TermsOfServiceComplianceStatus,
TermsOfServiceDetails,
TermsOfServiceHistory,
TermsOfServiceHistoryRecord,
UserIdInfo,
UserPolicyResponse,
UserResourcesResponse,
Expand Down Expand Up @@ -73,6 +75,10 @@ object SamJsonSupport {

implicit val TermsOfServiceDetailsFormat = jsonFormat4(TermsOfServiceDetails.apply)

implicit val termsOfServiceHistoryRecordFormat = jsonFormat3(TermsOfServiceHistoryRecord.apply)

implicit val termsOfServiceHistory = jsonFormat1(TermsOfServiceHistory.apply)

implicit val SamUserTosFormat = jsonFormat4(SamUserTos.apply)

implicit val termsOfAcceptanceStatusFormat = jsonFormat3(TermsOfServiceComplianceStatus.apply)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@ import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO
import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
import org.broadinstitute.dsde.workbench.sam.errorReportSource
import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, TermsOfServiceConfigResponse}
import org.broadinstitute.dsde.workbench.sam.model.{OldTermsOfServiceDetails, SamUserTos, TermsOfServiceComplianceStatus, TermsOfServiceDetails}
import org.broadinstitute.dsde.workbench.sam.model.{
OldTermsOfServiceDetails,
SamUserTos,
TermsOfServiceComplianceStatus,
TermsOfServiceDetails,
TermsOfServiceHistory,
TermsOfServiceHistoryRecord
}
import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.{FutureWithLogging, IOWithLogging}
import org.broadinstitute.dsde.workbench.sam.util.{SamRequestContext, SupportsAdmin}

Expand Down Expand Up @@ -79,7 +86,7 @@ class TosService(

@Deprecated
def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[OldTermsOfServiceDetails] =
directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
directoryDao.getUserTermsOfService(samUser.id, samRequestContext).map { tos =>
OldTermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
}

Expand All @@ -100,7 +107,7 @@ class TosService(
}

private def ensureLatestTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[SamUserTos] = for {
maybeTermsOfServiceRecord <- directoryDao.getUserTos(userId, samRequestContext)
maybeTermsOfServiceRecord <- directoryDao.getUserTermsOfService(userId, samRequestContext, Option(TosTable.ACCEPT))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

more context on this change https://broadinstitute.slack.com/archives/C0DSD41QT/p1701111037789719?thread_ts=1701103991.553039&cid=C0DSD41QT

this endpoint will always return data on the latest acceptance even if the user's latest action was to reject.

latestUserTermsOfService <- maybeTermsOfServiceRecord
.map(IO.pure)
.getOrElse(
Expand All @@ -115,8 +122,20 @@ class TosService(
case Some(samUser) => samUser
case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find user:${userId}"))
}
def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
latestUserTos <- directoryDao.getUserTos(samUser.id, samRequestContext)

def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] =
ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) {
directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map {
case samUserTosHistory if samUserTosHistory.isEmpty => TermsOfServiceHistory(List.empty)
case samUserTosHistory =>
TermsOfServiceHistory(
samUserTosHistory.map(historyRecord => TermsOfServiceHistoryRecord(historyRecord.action, historyRecord.version, historyRecord.createdAt))
)
}
}

def getTermsOfServiceComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
latestUserTos <- directoryDao.getUserTermsOfService(samUser.id, samRequestContext)
userHasAcceptedLatestVersion = userHasAcceptedCurrentTermsOfService(latestUserTos)
permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos)
} yield TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ class UserService(
allUsersStatus <- directoryDAO.isGroupMember(allUsersGroup.id, user.id, samRequestContext) recover { case _: NameNotFoundException =>
false
}
tosComplianceStatus <- tosService.getTosComplianceStatus(user, samRequestContext)
tosComplianceStatus <- tosService.getTermsOfServiceComplianceStatus(user, samRequestContext)
adminEnabled <- directoryDAO.isEnabled(user.id, samRequestContext)
} yield {
// We are removing references to LDAP but this will require an API version change here, so we are leaving
Expand Down Expand Up @@ -360,14 +360,14 @@ class UserService(
// Mixing up the endpoint to return user info AND status information is only causing problems and confusion
def getUserStatusInfo(user: SamUser, samRequestContext: SamRequestContext): IO[UserStatusInfo] =
for {
tosAcceptanceDetails <- tosService.getTosComplianceStatus(user, samRequestContext)
tosAcceptanceDetails <- tosService.getTermsOfServiceComplianceStatus(user, samRequestContext)
} yield UserStatusInfo(user.id.value, user.email.value, tosAcceptanceDetails.permitsSystemUsage && user.enabled, user.enabled)

def getUserStatusDiagnostics(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[UserStatusDiagnostics]] =
directoryDAO.loadUser(userId, samRequestContext).flatMap {
case Some(user) =>
// pulled out of for comprehension to allow concurrent execution
val tosAcceptanceStatus = tosService.getTosComplianceStatus(user, samRequestContext)
val tosAcceptanceStatus = tosService.getTermsOfServiceComplianceStatus(user, samRequestContext)
val adminEnabledStatus = directoryDAO.isEnabled(user.id, samRequestContext)
val allUsersStatus = cloudExtensions.getOrCreateAllUsersGroup(directoryDAO, samRequestContext).flatMap { allUsersGroup =>
directoryDAO.isGroupMember(allUsersGroup.id, user.id, samRequestContext) recover { case e: NameNotFoundException => false }
Expand Down Expand Up @@ -457,7 +457,7 @@ class UserService(

def getUserAllowances(samUser: SamUser, samRequestContext: SamRequestContext): IO[SamUserAllowances] =
for {
tosStatus <- tosService.getTosComplianceStatus(samUser, samRequestContext)
tosStatus <- tosService.getTermsOfServiceComplianceStatus(samUser, samRequestContext)
} yield SamUserAllowances(
allowed = samUser.enabled && tosStatus.permitsSystemUsage,
enabled = samUser.enabled,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import akka.http.scaladsl.server.Directives.{onSuccess, reject}
import akka.http.scaladsl.server._
import akka.stream.Materializer
import org.broadinstitute.dsde.workbench.model.{ErrorReportSource, WorkbenchGroup}
import org.broadinstitute.dsde.workbench.sam.model.TermsOfServiceHistory
import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes}
import org.broadinstitute.dsde.workbench.sam.service._
import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
Expand Down Expand Up @@ -74,6 +75,11 @@ class MockSamRoutesBuilder(allUsersGroup: WorkbenchGroup)(implicit system: Actor
this
}

def withTermsOfServiceHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockSamRoutesBuilder = {
mockTosServiceBuilder.withTermsOfServiceHistoryForUser(samUser, tosHistory)
this
}

def withUserAttributes(samUser: SamUser, userAttributes: SamUserAttributes): MockSamRoutesBuilder = {
userServiceBuilder.withUserAttributes(samUser, userAttributes)
this
Expand Down
Loading
Loading