From e1853841c6681f564495a668ca298a0bc58faf96 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Wed, 11 Oct 2023 10:12:37 -0400 Subject: [PATCH] ID-815 New terms of service tracking table. (#1200) * ID-815 Add new TOS table. --- .../dsde/sam/liquibase/changelog.xml | 1 + .../20230929_add_tos_audit_table.xml | 40 ++++++++ .../sam/api/StandardSamUserDirectives.scala | 4 +- .../dsde/workbench/sam/api/UserRoutes.scala | 6 +- .../sam/dataAccess/DirectoryDAO.scala | 5 +- .../sam/dataAccess/PostgresDirectoryDAO.scala | 45 +++++---- .../workbench/sam/db/tables/TosTable.scala | 39 ++++++++ .../workbench/sam/db/tables/UserTable.scala | 3 - .../sam/google/GoogleExtensions.scala | 7 +- .../dsde/workbench/sam/model/SamModel.scala | 23 +++-- .../workbench/sam/service/TosService.scala | 42 +++++---- .../workbench/sam/service/UserService.scala | 8 +- src/test/scala/Generator.scala | 8 +- .../sam/api/ResourceRoutesV2Spec.scala | 6 +- .../api/StandardSamUserDirectivesSpec.scala | 6 +- .../workbench/sam/api/TestSamRoutes.scala | 5 +- .../sam/dataAccess/MockAccessPolicyDAO.scala | 2 +- .../sam/dataAccess/MockDirectoryDAO.scala | 31 ++++--- .../dataAccess/PostgresDirectoryDAOSpec.scala | 73 ++++++++------- .../sam/google/GoogleExtensionSpec.scala | 2 +- .../sam/service/MockTosServiceBuilder.scala | 5 +- .../sam/service/MockUserService.scala | 4 +- .../service/PolicyEvaluatorServiceSpec.scala | 4 +- .../sam/service/TosServiceSpec.scala | 93 +++++++++++++++---- .../sam/service/UserServiceSpec.scala | 17 ++-- 25 files changed, 328 insertions(+), 151 deletions(-) create mode 100644 src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20230929_add_tos_audit_table.xml create mode 100644 src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/TosTable.scala diff --git a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml index a51655dea..01d46c62f 100644 --- a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml +++ b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml @@ -25,4 +25,5 @@ + diff --git a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20230929_add_tos_audit_table.xml b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20230929_add_tos_audit_table.xml new file mode 100644 index 000000000..78f45caad --- /dev/null +++ b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20230929_add_tos_audit_table.xml @@ -0,0 +1,40 @@ + + + + + + + + + + + + + + + + + + + INSERT INTO sam_user_terms_of_service + (sam_user_id, + version, + action, + created_at) + SELECT su.id as sam_user_id, + su.accepted_tos_version as version, + 'ACCEPT' as action, + '1970-01-01 00:00:00-00' as created_at + FROM sam_user su + WHERE su.accepted_tos_version is not null; + + + + + + + diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala index 9eacdf334..4cdd6034d 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala @@ -61,7 +61,7 @@ trait StandardSamUserDirectives extends SamUserDirectives with LazyLogging with val googleSubjectId = (oidcHeaders.externalId.left.toOption ++ oidcHeaders.googleSubjectIdFromAzure).headOption val azureB2CId = oidcHeaders.externalId.toOption // .right is missing (compared to .left above) since Either is Right biased - SamUser(genWorkbenchUserId(System.currentTimeMillis()), googleSubjectId, oidcHeaders.email, azureB2CId, false, None) + SamUser(genWorkbenchUserId(System.currentTimeMillis()), googleSubjectId, oidcHeaders.email, azureB2CId, false) } /** Utility function that knows how to convert all the various headers into OIDCHeaders @@ -122,7 +122,7 @@ object StandardSamUserDirectives { def getActiveSamUser(oidcHeaders: OIDCHeaders, userService: UserService, tosService: TosService, samRequestContext: SamRequestContext): IO[SamUser] = for { user <- getSamUser(oidcHeaders, userService, samRequestContext) - tosComplianceDetails <- tosService.getTosComplianceStatus(user) + tosComplianceDetails <- tosService.getTosComplianceStatus(user, samRequestContext) } yield { if (!tosComplianceDetails.permitsSystemUsage) { throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Unauthorized, "User must accept the latest terms of service.")) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutes.scala index 7ad268385..0a9fbe4cc 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/UserRoutes.scala @@ -66,7 +66,7 @@ trait UserRoutes extends SamUserDirectives with SamRequestContextDirectives { get { withUserAllowInactive(samRequestContext) { samUser => complete { - tosService.getTosComplianceStatus(samUser).map { tosAcceptanceStatus => + tosService.getTosComplianceStatus(samUser, samRequestContext).map { tosAcceptanceStatus => StatusCodes.OK -> Option(JsBoolean(tosAcceptanceStatus.permitsSystemUsage)) } } @@ -139,12 +139,12 @@ trait UserRoutes extends SamUserDirectives with SamRequestContextDirectives { } ~ path("termsOfServiceDetails") { get { - complete(tosService.getTosDetails(user)) + complete(tosService.getTosDetails(user, samRequestContext)) } } ~ path("termsOfServiceComplianceStatus") { get { - complete(tosService.getTosComplianceStatus(user)) + complete(tosService.getTosComplianceStatus(user, samRequestContext)) } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala index 7b6b3c4f1..35a8f5295 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala @@ -4,7 +4,7 @@ import cats.effect.IO import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId import org.broadinstitute.dsde.workbench.sam.azure.{ManagedIdentityObjectId, PetManagedIdentity, PetManagedIdentityId} -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUser} +import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUser, SamUserTos} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -74,7 +74,8 @@ trait DirectoryDAO { def setGoogleSubjectId(userId: WorkbenchUserId, googleSubjectId: GoogleSubjectId, samRequestContext: SamRequestContext): IO[Unit] def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] - def rejectTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] + def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] + def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] def loadPetManagedIdentity(petManagedIdentityId: PetManagedIdentityId, samRequestContext: SamRequestContext): IO[Option[PetManagedIdentity]] diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala index 77702f607..876b6dd74 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala @@ -353,7 +353,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val ${userColumn.googleSubjectId}, ${userColumn.enabled}, ${userColumn.azureB2cId}, - ${userColumn.acceptedTosVersion}, ${userColumn.createdAt}, ${userColumn.registeredAt}, ${userColumn.updatedAt}) @@ -363,7 +362,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val ${newUser.googleSubjectId}, ${newUser.enabled}, ${newUser.azureB2CId}, - ${newUser.acceptedTosVersion}, ${newUser.createdAt}, ${newUser.registeredAt}, ${newUser.updatedAt})""" @@ -631,25 +629,38 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } } - override def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = + override def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = { + val tosTable = TosTable.syntax + val tosColumns = TosTable.column serializableWriteTransaction("acceptTermsOfService", samRequestContext) { implicit session => - val u = UserTable.column - samsql"""update ${UserTable.table} - set (${u.acceptedTosVersion}, ${u.updatedAt}) = - (${tosVersion}, ${Instant.now()}) - where ${u.id} = ${userId} - and (${u.acceptedTosVersion} is null - or ${u.acceptedTosVersion} != ${tosVersion})""".update().apply() > 0 + samsql"""insert into ${TosTable as tosTable} (${tosColumns.samUserId}, ${tosColumns.version}, ${tosColumns.action}, ${tosColumns.createdAt}) + values ($userId, $tosVersion, ${TosTable.ACCEPT}, ${Instant.now()})""".update().apply() > 0 } + } - override def rejectTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] = + override def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = { + val tosTable = TosTable.syntax + val tosColumns = TosTable.column serializableWriteTransaction("rejectTermsOfService", samRequestContext) { implicit session => - val u = UserTable.column - samsql"""update ${UserTable.table} - set (${u.acceptedTosVersion}, ${u.updatedAt}) = - (null, ${Instant.now()}) - where ${u.id} = ${userId} - and ${u.acceptedTosVersion} is not null""".update().apply() > 0 + samsql"""insert into ${TosTable as tosTable} (${tosColumns.samUserId}, ${tosColumns.version}, ${tosColumns.action}, ${tosColumns.createdAt}) + values ($userId, $tosVersion, ${TosTable.REJECT}, ${Instant.now()})""".update().apply() > 0 + } + } + + override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + readOnlyTransaction("getUserTos", 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 1""" + + val userTosRecordOpt: Option[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).first().apply() + userTosRecordOpt.map(TosTable.unmarshalUserRecord) } override def isEnabled(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Boolean] = diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/TosTable.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/TosTable.scala new file mode 100644 index 000000000..a9592330e --- /dev/null +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/TosTable.scala @@ -0,0 +1,39 @@ +package org.broadinstitute.dsde.workbench.sam.db.tables + +import org.broadinstitute.dsde.workbench.model._ +import org.broadinstitute.dsde.workbench.sam.db.SamTypeBinders +import org.broadinstitute.dsde.workbench.sam.model.SamUserTos +import scalikejdbc._ + +import java.time.Instant + +final case class TosRecord( + samUserId: WorkbenchUserId, + version: String, + action: String, + createdAt: Instant +) + +object TosTable extends SQLSyntaxSupportWithDefaultSamDB[TosRecord] { + val ACCEPT = "ACCEPT" + val REJECT = "REJECT" + override def tableName: String = "SAM_USER_TERMS_OF_SERVICE" + + import SamTypeBinders._ + def apply(e: ResultName[TosRecord])(rs: WrappedResultSet): TosRecord = TosRecord( + rs.get(e.samUserId), + rs.get(e.version), + rs.get(e.action), + rs.get(e.createdAt) + ) + + def apply(o: SyntaxProvider[TosRecord])(rs: WrappedResultSet): TosRecord = apply(o.resultName)(rs) + + def unmarshalUserRecord(tosRecord: TosRecord): SamUserTos = + SamUserTos( + tosRecord.samUserId, + tosRecord.version, + tosRecord.action, + tosRecord.createdAt + ) +} diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/UserTable.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/UserTable.scala index 652b86cca..5e30bd33e 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/UserTable.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/UserTable.scala @@ -13,7 +13,6 @@ final case class UserRecord( googleSubjectId: Option[GoogleSubjectId], enabled: Boolean, azureB2cId: Option[AzureB2CId], - acceptedTosVersion: Option[String], createdAt: Instant, registeredAt: Option[Instant], updatedAt: Instant @@ -29,7 +28,6 @@ object UserTable extends SQLSyntaxSupportWithDefaultSamDB[UserRecord] { rs.stringOpt(e.googleSubjectId).map(GoogleSubjectId), rs.get(e.enabled), rs.stringOpt(e.azureB2cId).map(AzureB2CId), - rs.stringOpt(e.acceptedTosVersion), rs.get(e.createdAt), rs.timestampOpt(e.registeredAt).map(_.toInstant), rs.get(e.updatedAt) @@ -44,7 +42,6 @@ object UserTable extends SQLSyntaxSupportWithDefaultSamDB[UserRecord] { userRecord.email, userRecord.azureB2cId, userRecord.enabled, - userRecord.acceptedTosVersion, userRecord.createdAt, userRecord.registeredAt, userRecord.updatedAt diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index d2a98d750..775039eba 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -140,8 +140,7 @@ class GoogleExtensions( Option(googleSubjectId), googleServicesConfig.serviceAccountClientEmail, None, - false, - None + false ) samApplication.userService.createUser(newUser, samRequestContext).map(_ => newUser) } @@ -424,7 +423,7 @@ class GoogleExtensions( subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext) key <- subject match { case Some(userId: WorkbenchUserId) => - getPetServiceAccountKey(SamUser(userId, None, userEmail, None, false, None), project, samRequestContext).map(Option(_)) + getPetServiceAccountKey(SamUser(userId, None, userEmail, None, false), project, samRequestContext).map(Option(_)) case _ => IO.pure(None) } } yield key @@ -445,7 +444,7 @@ class GoogleExtensions( subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext) key <- subject match { case Some(userId: WorkbenchUserId) => - IO.fromFuture(IO(getArbitraryPetServiceAccountKey(SamUser(userId, None, userEmail, None, false, None), samRequestContext))).map(Option(_)) + IO.fromFuture(IO(getArbitraryPetServiceAccountKey(SamUser(userId, None, userEmail, None, false), samRequestContext))).map(Option(_)) case _ => IO.none } } yield key diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala index 59abf7ec4..1a2b5c373 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala @@ -3,8 +3,8 @@ package org.broadinstitute.dsde.workbench.sam.model import monocle.macros.Lenses import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.model.google.GoogleModelJsonSupport.InstantFormat -import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AccessPolicyMembershipResponse, AdminUpdateUserRequest} import org.broadinstitute.dsde.workbench.sam.model.api.SamApiJsonProtocol.PolicyInfoResponseBodyJsonFormat +import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AccessPolicyMembershipResponse, AdminUpdateUserRequest} import org.broadinstitute.dsde.workbench.sam.service.ManagedGroupService.MangedGroupRoleName import spray.json.{DefaultJsonProtocol, JsValue, RootJsonFormat} @@ -28,7 +28,7 @@ object SamJsonSupport { implicit val ResourceTypeFormat = jsonFormat6(ResourceType.apply) - implicit val SamUserFormat = jsonFormat9(SamUser.apply) + implicit val SamUserFormat = jsonFormat8(SamUser.apply) implicit val UserStatusDetailsFormat = jsonFormat2(UserStatusDetails.apply) @@ -322,10 +322,9 @@ object SamUser { googleSubjectId: Option[GoogleSubjectId], email: WorkbenchEmail, azureB2CId: Option[AzureB2CId], - enabled: Boolean, - acceptedTosVersion: Option[String] + enabled: Boolean ): SamUser = - SamUser(id, googleSubjectId, email, azureB2CId, enabled, acceptedTosVersion, Instant.EPOCH, None, Instant.EPOCH) + SamUser(id, googleSubjectId, email, azureB2CId, enabled, Instant.EPOCH, None, Instant.EPOCH) } final case class SamUser( @@ -334,7 +333,6 @@ final case class SamUser( email: WorkbenchEmail, azureB2CId: Option[AzureB2CId], enabled: Boolean, - acceptedTosVersion: Option[String], createdAt: Instant, registeredAt: Option[Instant], updatedAt: Instant @@ -347,8 +345,17 @@ final case class SamUser( this.googleSubjectId == user.googleSubjectId && this.email == user.email && this.azureB2CId == user.azureB2CId && - this.enabled == user.enabled && - this.acceptedTosVersion == user.acceptedTosVersion + this.enabled == user.enabled + case _ => false + } +} + +final case class SamUserTos(id: WorkbenchUserId, version: String, action: String, createdAt: Instant) { + override def equals(other: Any): Boolean = other match { + case userTos: SamUserTos => + this.id == userTos.id && + this.version == userTos.version && + this.action == userTos.action case _ => false } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala index 567c3aaa9..946ebbd1f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala @@ -10,7 +10,8 @@ import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO import org.broadinstitute.dsde.workbench.sam.errorReportSource import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.broadinstitute.dsde.workbench.sam.config.TermsOfServiceConfig -import org.broadinstitute.dsde.workbench.sam.model.{SamUser, TermsOfServiceComplianceStatus, TermsOfServiceDetails} +import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable +import org.broadinstitute.dsde.workbench.sam.model.{SamUser, SamUserTos, TermsOfServiceComplianceStatus, TermsOfServiceDetails} import scala.concurrent.ExecutionContext import java.io.{FileNotFoundException, IOException} @@ -27,32 +28,41 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo def rejectTosStatus(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] = directoryDao - .rejectTermsOfService(userId, samRequestContext) + .rejectTermsOfService(userId, tosConfig.version, samRequestContext) .withInfoLogMessage(s"$userId has rejected version ${tosConfig.version} of the Terms of Service") @Deprecated - def getTosDetails(samUser: SamUser): IO[TermsOfServiceDetails] = - IO.pure(TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, samUser.acceptedTosVersion)) + def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceDetails] = + directoryDao.getUserTos(samUser.id, samRequestContext).map { tos => + TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version)) + } - def getTosComplianceStatus(samUser: SamUser): IO[TermsOfServiceComplianceStatus] = { - val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(samUser) - val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser) - IO.pure(TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)) - } + def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = + directoryDao.getUserTos(samUser.id, samRequestContext).map { tos => + val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(tos) + val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, tos) + TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage) + } /** If grace period enabled, don't check ToS, return true If ToS disabled, return true Otherwise return true if user has accepted ToS, or is a service account */ - private def tosAcceptancePermitsSystemUsage(user: SamUser): Boolean = { - val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(user) - val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && user.acceptedTosVersion.isDefined + private def tosAcceptancePermitsSystemUsage(user: SamUser, userTos: Option[SamUserTos]): Boolean = { val userIsServiceAccount = StandardSamUserDirectives.SAdomain.matches(user.email.value) // Service Account users do not need to accept ToS - val tosDisabled = !tosConfig.isTosEnabled + val userIsPermitted = userTos.exists { tos => + val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(Option(tos)) + val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT + val tosDisabled = !tosConfig.isTosEnabled + + userHasAcceptedLatestVersion || userCanUseSystemUnderGracePeriod || tosDisabled - userHasAcceptedLatestVersion || userCanUseSystemUnderGracePeriod || userIsServiceAccount || tosDisabled + } + userIsPermitted || userIsServiceAccount } - private def userHasAcceptedLatestTosVersion(samUser: SamUser): Boolean = - samUser.acceptedTosVersion.contains(tosConfig.version) + private def userHasAcceptedLatestTosVersion(userTos: Option[SamUserTos]): Boolean = + userTos.exists { tos => + tos.version.contains(tosConfig.version) && tos.action == TosTable.ACCEPT + } /** Get the terms of service text and send it to the caller * @return diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala index 3fd303488..8d192cdb0 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala @@ -213,7 +213,7 @@ class UserService(val directoryDAO: DirectoryDAO, val cloudExtensions: CloudExte _ <- validateEmailAddress(inviteeEmail, blockedEmailDomains) existingSubject <- directoryDAO.loadSubjectFromEmail(inviteeEmail, samRequestContext) createdUser <- existingSubject match { - case None => createUserInternal(SamUser(genWorkbenchUserId(System.currentTimeMillis()), None, inviteeEmail, None, false, None), samRequestContext) + case None => createUserInternal(SamUser(genWorkbenchUserId(System.currentTimeMillis()), None, inviteeEmail, None, false), samRequestContext) case Some(_) => IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Conflict, s"email ${inviteeEmail} already exists"))) } @@ -273,7 +273,7 @@ class UserService(val directoryDAO: DirectoryDAO, val cloudExtensions: CloudExte allUsersStatus <- directoryDAO.isGroupMember(allUsersGroup.id, user.id, samRequestContext) recover { case _: NameNotFoundException => false } - tosComplianceStatus <- tosService.getTosComplianceStatus(user) + tosComplianceStatus <- tosService.getTosComplianceStatus(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 @@ -316,7 +316,7 @@ class UserService(val directoryDAO: DirectoryDAO, val cloudExtensions: CloudExte // 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) + tosAcceptanceDetails <- tosService.getTosComplianceStatus(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]] = @@ -324,7 +324,7 @@ class UserService(val directoryDAO: DirectoryDAO, val cloudExtensions: CloudExte directoryDAO.loadUser(userId, samRequestContext).flatMap { case Some(user) => // pulled out of for comprehension to allow concurrent execution - val tosAcceptanceStatus = tosService.getTosComplianceStatus(user) + val tosAcceptanceStatus = tosService.getTosComplianceStatus(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 } diff --git a/src/test/scala/Generator.scala b/src/test/scala/Generator.scala index 694a1fc03..65b8953c1 100644 --- a/src/test/scala/Generator.scala +++ b/src/test/scala/Generator.scala @@ -61,7 +61,7 @@ object Generator { email <- genNonPetEmail googleSubjectId <- genGoogleSubjectId userId <- genWorkbenchUserId - } yield SamUser(userId, Some(googleSubjectId), email, None, false, None) + } yield SamUser(userId, Some(googleSubjectId), email, None, false) val genFirecloudUser = for { email <- genFirecloudEmail @@ -77,20 +77,20 @@ object Generator { email <- genServiceAccountEmail googleSubjectId <- genGoogleSubjectId userId <- genWorkbenchUserId - } yield SamUser(userId, Option(googleSubjectId), email, None, false, None) + } yield SamUser(userId, Option(googleSubjectId), email, None, false) val genWorkbenchUserAzure = for { email <- genNonPetEmail azureB2CId <- genAzureB2CId userId <- genWorkbenchUserId - } yield SamUser(userId, None, email, Option(azureB2CId), false, None) + } yield SamUser(userId, None, email, Option(azureB2CId), false) val genWorkbenchUserBoth = for { email <- genNonPetEmail googleSubjectId <- genGoogleSubjectId azureB2CId <- genAzureB2CId userId <- genWorkbenchUserId - } yield SamUser(userId, Option(googleSubjectId), email, Option(azureB2CId), false, None) + } yield SamUser(userId, Option(googleSubjectId), email, Option(azureB2CId), false) val genPetServiceAccountId = for { userId <- genWorkbenchUserId diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala index c55f2c2b1..eff039bf9 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala @@ -447,7 +447,7 @@ class ResourceRoutesV2Spec extends RetryableAnyFlatSpec with Matchers with TestS ) val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType)) - val secondUser = SamUser(WorkbenchUserId("11112"), Some(GoogleSubjectId("11112")), WorkbenchEmail("some-other-user@example.com"), None, true, None) + val secondUser = SamUser(WorkbenchUserId("11112"), Some(GoogleSubjectId("11112")), WorkbenchEmail("some-other-user@example.com"), None, true) runAndWait(samRoutes.userService.createUser(secondUser, samRequestContext)) val resourceId = ResourceId("foo") @@ -560,7 +560,7 @@ class ResourceRoutesV2Spec extends RetryableAnyFlatSpec with Matchers with TestS val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType)) // create a second owner so our test user can leave the owner policy without orphaning the resource - val secondOwner = SamUser(WorkbenchUserId("1111112"), Some(GoogleSubjectId("1111112")), WorkbenchEmail("seconduser@gmail.com"), None, true, None) + val secondOwner = SamUser(WorkbenchUserId("1111112"), Some(GoogleSubjectId("1111112")), WorkbenchEmail("seconduser@gmail.com"), None, true) runAndWait(samRoutes.userService.createUser(secondOwner, samRequestContext)) val resourceId = ResourceId("foo") @@ -668,7 +668,7 @@ class ResourceRoutesV2Spec extends RetryableAnyFlatSpec with Matchers with TestS ) val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType)) - val secondUser = SamUser(WorkbenchUserId("11112"), Some(GoogleSubjectId("11112")), WorkbenchEmail("some-other-user@example.com"), None, true, None) + val secondUser = SamUser(WorkbenchUserId("11112"), Some(GoogleSubjectId("11112")), WorkbenchEmail("some-other-user@example.com"), None, true) runAndWait(samRoutes.userService.createUser(secondUser, samRequestContext)) val resourceId = ResourceId("foo") diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectivesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectivesSpec.scala index 7160744ec..d773e69b6 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectivesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectivesSpec.scala @@ -141,7 +141,7 @@ class StandardSamUserDirectivesSpec extends AnyFlatSpec with PropertyBasedTestin val headers = createRequiredHeaders(externalId, email, accessToken) val user = TestSupport.newUserWithAcceptedTos( services, - SamUser(userId, externalId.left.toOption, email = email, azureB2CId = externalId.toOption, true, None), + SamUser(userId, externalId.left.toOption, email = email, azureB2CId = externalId.toOption, true), samRequestContext ) Get("/").withHeaders(headers) ~> @@ -255,7 +255,7 @@ class StandardSamUserDirectivesSpec extends AnyFlatSpec with PropertyBasedTestin services.withNewUser(samRequestContext)(user => complete(user.copy(id = WorkbenchUserId("")).toString)) } ~> check { status shouldBe StatusCodes.OK - responseAs[String] shouldEqual SamUser(WorkbenchUserId(""), externalId.left.toOption, email, externalId.toOption, false, None).toString + responseAs[String] shouldEqual SamUser(WorkbenchUserId(""), externalId.left.toOption, email, externalId.toOption, false).toString } } @@ -268,7 +268,7 @@ class StandardSamUserDirectivesSpec extends AnyFlatSpec with PropertyBasedTestin services.withNewUser(samRequestContext)(x => complete(x.copy(id = WorkbenchUserId("")).toString)) } ~> check { status shouldBe StatusCodes.OK - responseAs[String] shouldEqual SamUser(WorkbenchUserId(""), Option(googleSubjectId), email, Option(azureB2CId), false, None).toString + responseAs[String] shouldEqual SamUser(WorkbenchUserId(""), Option(googleSubjectId), email, Option(azureB2CId), false).toString } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala index fd3a62881..9ad9e0af7 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala @@ -12,7 +12,7 @@ import org.broadinstitute.dsde.workbench.google.mock.MockGoogleDirectoryDAO import org.broadinstitute.dsde.workbench.model.WorkbenchEmail import org.broadinstitute.dsde.workbench.oauth2.mock.FakeOpenIDConnectConfiguration import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics -import org.broadinstitute.dsde.workbench.sam.TestSupport.{samRequestContext, tosConfig} +import org.broadinstitute.dsde.workbench.sam.TestSupport.samRequestContext import org.broadinstitute.dsde.workbench.sam.azure.{AzureService, CrlService, MockCrlService} import org.broadinstitute.dsde.workbench.sam.config.AppConfig.AdminConfig import org.broadinstitute.dsde.workbench.sam.config.{LiquibaseConfig, TermsOfServiceConfig} @@ -206,14 +206,13 @@ object TestSamRoutes { val mockStatusService = new StatusService(directoryDAO, cloudXtns) val azureService = new AzureService(crlService.getOrElse(MockCrlService(Option(user))), directoryDAO, new MockAzureManagedResourceGroupDAO) - val userToTestWith = if (acceptTermsOfService) user.copy(acceptedTosVersion = Some(tosConfig.version)) else user new TestSamRoutes( mockResourceService, policyEvaluatorService, mockUserService, mockStatusService, mockManagedGroupService, - userToTestWith, + user, tosService = mockTosService, cloudExtensions = cloudXtns, azureService = Some(azureService) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 76bdea26b..c4ed639c0 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -214,7 +214,7 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policy.members } members.flatten.collect { case u: WorkbenchUserId => - SamUser(u, Some(GoogleSubjectId(u.value)), WorkbenchEmail("dummy"), None, false, None) + SamUser(u, Some(GoogleSubjectId(u.value)), WorkbenchEmail("dummy"), None, false) }.toSet } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala index efdc34b17..689df11cb 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala @@ -8,7 +8,8 @@ import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId import org.broadinstitute.dsde.workbench.sam._ import org.broadinstitute.dsde.workbench.sam.azure.{ManagedIdentityObjectId, PetManagedIdentity, PetManagedIdentityId} -import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, SamUser} +import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable +import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, SamUser, SamUserTos} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -20,6 +21,7 @@ import scala.collection.mutable class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, WorkbenchGroup] = new TrieMap(), passStatusCheck: Boolean = true) extends DirectoryDAO { private val groupSynchronizedDates: mutable.Map[WorkbenchGroupIdentity, Date] = new TrieMap() private val users: mutable.Map[WorkbenchUserId, SamUser] = new TrieMap() + private val userTos: mutable.Map[WorkbenchUserId, SamUserTos] = new TrieMap() private val userAttributes: mutable.Map[WorkbenchUserId, mutable.Map[String, Any]] = new TrieMap() private val usersWithEmails: mutable.Map[WorkbenchEmail, WorkbenchUserId] = new TrieMap() @@ -324,24 +326,25 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench loadUser(userId, samRequestContext).map { case None => false case Some(user) => - if (user.acceptedTosVersion.contains(tosVersion)) { - false - } else { - users.put(userId, user.copy(acceptedTosVersion = Option(tosVersion))) - true - } + users.put(userId, user) + userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) + true } - override def rejectTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] = + override def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = loadUser(userId, samRequestContext).map { case None => false case Some(user) => - if (user.acceptedTosVersion.isEmpty) { - false - } else { - users.put(userId, user.copy(acceptedTosVersion = None)) - true - } + users.put(userId, user) + userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) + true + } + + override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + loadUser(userId, samRequestContext).map { + case None => None + case Some(_) => + userTos.get(userId) } override def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] = { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala index 757aad91a..123b12003 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala @@ -8,6 +8,7 @@ import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, datab import org.broadinstitute.dsde.workbench.sam.azure._ import org.broadinstitute.dsde.workbench.sam.db.SamParameterBinderFactory._ import org.broadinstitute.dsde.workbench.sam.db.TestDbReference +import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.matchers.TimeMatchers import org.broadinstitute.dsde.workbench.sam.model._ import org.broadinstitute.dsde.workbench.sam.{Generator, RetryableAnyFreeSpec, TestSupport} @@ -423,7 +424,6 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B user.email should equal(expectedUser.email) user.azureB2CId should equal(expectedUser.azureB2CId) user.enabled should equal(expectedUser.enabled) - user.acceptedTosVersion should equal(expectedUser.acceptedTosVersion) user.registeredAt should equal(expectedUser.registeredAt) } } @@ -503,7 +503,6 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B user.email should equal(expectedUser.email) user.azureB2CId should equal(expectedUser.azureB2CId) user.enabled should equal(expectedUser.enabled) - user.acceptedTosVersion should equal(expectedUser.acceptedTosVersion) user.createdAt should equal(expectedUser.createdAt) user.registeredAt should equal(expectedUser.registeredAt) user.updatedAt should equal(expectedUser.updatedAt) @@ -1485,6 +1484,13 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B "accept the terms of service for a new user" in { dao.createUser(defaultUser, samRequestContext).unsafeRunSync() dao.acceptTermsOfService(defaultUser.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true + + // Assert + val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync() + userTos should not be empty + userTos.get.createdAt should beAround(Instant.now()) + userTos.get.action shouldBe TosTable.ACCEPT + userTos.get.version shouldBe tosConfig.version } "accept the terms of service for a user who has already accepted a previous version of the terms of service" in { @@ -1493,50 +1499,53 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.createUser(defaultUser, samRequestContext).unsafeRunSync() dao.acceptTermsOfService(defaultUser.id, "0", samRequestContext).unsafeRunSync() shouldBe true dao.acceptTermsOfService(defaultUser.id, "2", samRequestContext).unsafeRunSync() shouldBe true + + // Assert + val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync() + userTos should not be empty + userTos.get.createdAt should beAround(Instant.now()) + userTos.get.action shouldBe TosTable.ACCEPT + userTos.get.version shouldBe "2" } + } - "sets the updatedAt datetime to the current datetime" in { - // Arrange - val user = Generator.genWorkbenchUserGoogle.sample.get.copy( - updatedAt = Instant.parse("2010-10-10T10:10:10Z") - ) + "rejectTermsOfService" - { + "reject the terms of service for an new user" in { + val user = Generator.genWorkbenchUserGoogle.sample.get dao.createUser(user, samRequestContext).unsafeRunSync() - - // Act - dao.acceptTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() + dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true // Assert - val loadedUser = dao.loadUser(user.id, samRequestContext).unsafeRunSync() - loadedUser.value.updatedAt should beAround(Instant.now()) + val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + userTos should not be empty + userTos.get.createdAt should beAround(Instant.now()) + userTos.get.action shouldBe TosTable.REJECT + userTos.get.version shouldBe tosConfig.version } - } - "rejectTermsOfService" - { "reject the terms of service for an existing user" in { - dao.createUser(defaultUser, samRequestContext).unsafeRunSync() - dao.acceptTermsOfService(defaultUser.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true - dao.rejectTermsOfService(defaultUser.id, samRequestContext).unsafeRunSync() shouldBe true - } + val user = Generator.genWorkbenchUserGoogle.sample.get + dao.createUser(user, samRequestContext).unsafeRunSync() + dao.acceptTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true + dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true - "cannot reject the terms of service for a user who has not accepted terms of service previously" in { - dao.createUser(defaultUser, samRequestContext).unsafeRunSync() - dao.rejectTermsOfService(defaultUser.id, samRequestContext).unsafeRunSync() shouldBe false + // Assert + val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + userTos should not be empty + userTos.get.createdAt should beAround(Instant.now()) + userTos.get.action shouldBe TosTable.REJECT + userTos.get.version shouldBe tosConfig.version } + } - "sets the updatedAt datetime to the current datetime" in { - // Arrange - val user = Generator.genWorkbenchUserGoogle.sample.get.copy( - updatedAt = Instant.parse("2010-10-10T10:10:10Z"), - acceptedTosVersion = Option(tosConfig.version) - ) + "load terms of service" - { + "returns none if no record" in { + val user = Generator.genWorkbenchUserGoogle.sample.get dao.createUser(user, samRequestContext).unsafeRunSync() - // Act - dao.rejectTermsOfService(user.id, samRequestContext).unsafeRunSync() - // Assert - val loadedUser = dao.loadUser(user.id, samRequestContext).unsafeRunSync() - loadedUser.value.updatedAt should beAround(Instant.now()) + val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + userTos should be(None) } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala index d5bbdaed9..94a3daa6b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala @@ -423,7 +423,7 @@ class GoogleExtensionSpec(_system: ActorSystem) googleExtensions.deleteUserPetServiceAccount(newUser.userInfo.userSubjectId, googleProject, samRequestContext).unsafeRunSync() shouldBe true // the user should still exist in DB - dirDAO.loadUser(defaultUser.id, samRequestContext).unsafeRunSync() shouldBe Some(defaultUser.copy(enabled = true, acceptedTosVersion = Some("0"))) + dirDAO.loadUser(defaultUser.id, samRequestContext).unsafeRunSync() shouldBe Some(defaultUser.copy(enabled = true)) // the pet should not exist in DB dirDAO.loadPetServiceAccount(PetServiceAccountId(defaultUser.id, googleProject), samRequestContext).unsafeRunSync() shouldBe None diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala index fc754613c..e54d1c767 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala @@ -2,6 +2,7 @@ package org.broadinstitute.dsde.workbench.sam.service import cats.effect.IO import org.broadinstitute.dsde.workbench.sam.model.{SamUser, TermsOfServiceComplianceStatus} +import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.mockito.Mockito.{RETURNS_SMART_NULLS, lenient} import org.mockito.invocation.InvocationOnMock import org.mockito.scalatest.MockitoSugar @@ -32,7 +33,7 @@ case class MockTosServiceBuilder() extends MockitoSugar { lenient() .doAnswer((i: InvocationOnMock) => IO.pure(TermsOfServiceComplianceStatus(i.getArgument[SamUser](0).id, isAccepted, isAccepted))) .when(tosService) - .getTosComplianceStatus(any[SamUser]) + .getTosComplianceStatus(any[SamUser], any[SamRequestContext]) private def setAcceptedStateForUserTo(samUser: SamUser, isAccepted: Boolean) = { val matchesUser = new ArgumentMatcher[SamUser] { @@ -42,7 +43,7 @@ case class MockTosServiceBuilder() extends MockitoSugar { lenient() .doReturn(IO.pure(TermsOfServiceComplianceStatus(samUser.id, isAccepted, isAccepted))) .when(tosService) - .getTosComplianceStatus(ArgumentMatchers.argThat(matchesUser)) + .getTosComplianceStatus(ArgumentMatchers.argThat(matchesUser), any[SamRequestContext]) } def build: TosService = tosService diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserService.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserService.scala index 8cc019c79..eb949f7c4 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserService.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockUserService.scala @@ -144,8 +144,8 @@ class MockUserService( def acceptTermsOfServiceDAO(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = directoryDAO.acceptTermsOfService(userId, tosVersion, samRequestContext) - def rejectTermsOfServiceDAO(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] = - directoryDAO.rejectTermsOfService(userId, samRequestContext) + def rejectTermsOfServiceDAO(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = + directoryDAO.rejectTermsOfService(userId, tosVersion, samRequestContext) def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] = directoryDAO.createPetManagedIdentity(petManagedIdentity, samRequestContext) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala index 6d7112758..121e87942 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala @@ -127,8 +127,8 @@ class PolicyEvaluatorServiceSpec extends RetryableAnyFlatSpec with Matchers with private[service] def savePolicyMembers(policy: AccessPolicy) = policy.members.toList.traverse { case u: WorkbenchUserId => - dirDAO.createUser(SamUser(u, None, WorkbenchEmail(u.value + "@foo.bar"), None, false, None), samRequestContext).recoverWith { - case _: WorkbenchException => IO.pure(SamUser(u, None, WorkbenchEmail(u.value + "@foo.bar"), None, false, None)) + dirDAO.createUser(SamUser(u, None, WorkbenchEmail(u.value + "@foo.bar"), None, false), samRequestContext).recoverWith { case _: WorkbenchException => + IO.pure(SamUser(u, None, WorkbenchEmail(u.value + "@foo.bar"), None, false)) } case g: WorkbenchGroupName => managedGroupService.createManagedGroup(ResourceId(g.value), dummyUser, samRequestContext = samRequestContext).recoverWith { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala index f58e8ca0e..313b8c065 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala @@ -5,6 +5,8 @@ import cats.effect.unsafe.implicits.{global => globalEc} import org.broadinstitute.dsde.workbench.model.WorkbenchUserId import org.broadinstitute.dsde.workbench.sam.TestSupport.tosConfig import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO +import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable +import org.broadinstitute.dsde.workbench.sam.model.SamUserTos import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.broadinstitute.dsde.workbench.sam.{Generator, PropertyBasedTesting, TestSupport} import org.mockito.Mockito.RETURNS_SMART_NULLS @@ -12,6 +14,7 @@ import org.mockito.scalatest.MockitoSugar import org.scalatest.freespec.AnyFreeSpec import org.scalatest.{BeforeAndAfter, BeforeAndAfterAll} +import java.time.Instant import scala.concurrent.ExecutionContext.Implicits.global class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll with BeforeAndAfter with PropertyBasedTesting with MockitoSugar { @@ -29,6 +32,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll before { clearDatabase() TestSupport.truncateAll + reset(dirDAO) } protected def clearDatabase(): Unit = @@ -53,7 +57,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll } "rejects the ToS for a user" in { - when(dirDAO.rejectTermsOfService(any[WorkbenchUserId], any[SamRequestContext])) + when(dirDAO.rejectTermsOfService(any[WorkbenchUserId], any[String], any[SamRequestContext])) .thenReturn(IO.pure(true)) // reject and get ToS status @@ -61,23 +65,27 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll val rejectTosStatusResult = tosService.rejectTosStatus(defaultUser.id, samRequestContext).unsafeRunSync() rejectTosStatusResult shouldBe true - verify(dirDAO).rejectTermsOfService(defaultUser.id, samRequestContext) + verify(dirDAO).rejectTermsOfService(defaultUser.id, tosConfig.version, samRequestContext) } "always allows service account users to use the system" in { + val tosVersion = "2" val tosService = - new TosService(dirDAO, TestSupport.tosConfig.copy(version = "2")) - val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser).unsafeRunSync() + new TosService(dirDAO, TestSupport.tosConfig.copy(version = tosVersion)) + when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext)) + .thenReturn(IO.pure(Some(SamUserTos(serviceAccountUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) + val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } + val tosVersion = "2" val withoutGracePeriod = "without the grace period enabled" val withGracePeriod = " with the grace period enabled" val cannotUseTheSystem = "says the user cannot use the system" val canUseTheSystem = "says the user can use the system" - val tosServiceV2 = new TosService(dirDAO, TestSupport.tosConfig.copy(version = "2")) + val tosServiceV2 = new TosService(dirDAO, TestSupport.tosConfig.copy(version = tosVersion)) val tosServiceV2GracePeriodEnabled = - new TosService(dirDAO, TestSupport.tosConfig.copy(version = "2", isGracePeriodEnabled = true)) + new TosService(dirDAO, TestSupport.tosConfig.copy(version = tosVersion, isGracePeriodEnabled = true)) /** | Case | Grace Period Enabled | Accepted Version | Current Version | User accepted latest | Permits system usage | * |:-----|:---------------------|:-----------------|:----------------|:---------------------|:---------------------| @@ -91,65 +99,116 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll "when the user has not accepted any ToS version" - { "says the user has not accepted the latest version" in { - val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser).unsafeRunSync() + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.userHasAcceptedLatestTos shouldBe false } withoutGracePeriod - { cannotUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) // CASE 1 - val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser).unsafeRunSync() + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } withGracePeriod - { cannotUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) // CASE 4 - val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } } "when the user has accepted a non-current ToS version" - { - val userAcceptedV0 = defaultUser.copy(acceptedTosVersion = Option("0")) "says the user has not accepted the latest version" in { - val complianceStatus = tosServiceV2.getTosComplianceStatus(userAcceptedV0).unsafeRunSync() + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.userHasAcceptedLatestTos shouldBe false } withoutGracePeriod - { cannotUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) // CASE 2 - val complianceStatus = tosServiceV2.getTosComplianceStatus(userAcceptedV0).unsafeRunSync() + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } withGracePeriod - { canUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) // CASE 5 - val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(userAcceptedV0).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } } "when the user has accepted the current ToS version" - { - val userAcceptedCurrentVersion = defaultUser.copy(acceptedTosVersion = Option("2")) "says the user has accepted the latest version" in { - val complianceStatus = tosServiceV2.getTosComplianceStatus(userAcceptedCurrentVersion).unsafeRunSync() + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.userHasAcceptedLatestTos shouldBe true } withoutGracePeriod - { canUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) // CASE 3 - val complianceStatus = tosServiceV2.getTosComplianceStatus(userAcceptedCurrentVersion).unsafeRunSync() + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } withGracePeriod - { canUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) // CASE 6 - val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(userAcceptedCurrentVersion).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } } + + "when the user has rejected the latest ToS version" - { + "says the user has rejected the latest version" in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + complianceStatus.userHasAcceptedLatestTos shouldBe false + } + withoutGracePeriod - { + cannotUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) + // CASE 1 + val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + complianceStatus.permitsSystemUsage shouldBe false + } + } + withGracePeriod - { + cannotUseTheSystem in { + when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) + // CASE 4 + val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + complianceStatus.permitsSystemUsage shouldBe false + } + } + } + "when a service account is using the api" - { + "let it use the api regardless of tos status" in { + when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext)) + .thenReturn(IO.pure(None)) + val complianceStatus = tosServiceV2.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() + complianceStatus.permitsSystemUsage shouldBe true + } + } } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala index d46a2cc81..66690e4e7 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala @@ -112,7 +112,7 @@ class OldUserServiceMockSpec when(googleExtensions.onGroupUpdate(any[Seq[WorkbenchGroupIdentity]], any[SamRequestContext])).thenReturn(IO.unit) mockTosService = mock[TosService](RETURNS_SMART_NULLS) - when(mockTosService.getTosComplianceStatus(any[SamUser])) + when(mockTosService.getTosComplianceStatus(any[SamUser], any[SamRequestContext])) .thenAnswer((i: InvocationOnMock) => IO.pure(TermsOfServiceComplianceStatus(i.getArgument[SamUser](0).id, true, true))) service = Mockito.spy(new UserService(dirDAO, googleExtensions, Seq(blockedDomain), mockTosService)) @@ -143,7 +143,8 @@ class OldUserServiceMockSpec } it should "return UserStatusDiagnostics.tosAccepted as false if user's TOS status is false" in { - when(mockTosService.getTosComplianceStatus(enabledUser)).thenReturn(IO.pure(TermsOfServiceComplianceStatus(enabledUser.id, false, false))) + when(mockTosService.getTosComplianceStatus(enabledUser, samRequestContext)) + .thenReturn(IO.pure(TermsOfServiceComplianceStatus(enabledUser.id, false, false))) val status = service.getUserStatusDiagnostics(enabledUser.id, samRequestContext).unsafeRunSync() status.value.tosAccepted shouldBe false } @@ -506,7 +507,7 @@ class OldUserServiceSpec // Lookup the invited user and their ID val invitedUserId = dirDAO.loadSubjectFromEmail(emailToInvite, samRequestContext).unsafeRunSync().value.asInstanceOf[WorkbenchUserId] val invitedUser = dirDAO.loadUser(invitedUserId, samRequestContext).unsafeRunSync().getOrElse(fail("Failed to load invited user after inviting them")) - invitedUser shouldBe SamUser(invitedUserId, None, emailToInvite, None, false, None) + invitedUser shouldBe SamUser(invitedUserId, None, emailToInvite, None, false) // Give them a fake GoogleSubjectId and a new WorkbenchUserId and use that to register them. // The real code in org/broadinstitute/dsde/workbench/sam/api/UserRoutes.scala calls @@ -514,7 +515,7 @@ class OldUserServiceSpec // WorkbenchUserId for the SamUser on the request. val googleSubjectId = Option(GoogleSubjectId("123456789")) val newRegisteringUserId = WorkbenchUserId("11111111111111111") - val registeringUser = SamUser(newRegisteringUserId, googleSubjectId, emailToInvite, None, false, None) + val registeringUser = SamUser(newRegisteringUserId, googleSubjectId, emailToInvite, None, false) val registeredUser = service.createUser(registeringUser, samRequestContext).unsafeRunSync() registeredUser.userInfo.userSubjectId should { equal(invitedUser.id) and @@ -548,14 +549,14 @@ class OldUserServiceSpec val userInPostgres = dirDAO.loadUser(invitedUserId, samRequestContext).unsafeRunSync() userInPostgres.value should { - equal(SamUser(invitedUserId, None, inviteeEmail, None, false, None)) + equal(SamUser(invitedUserId, None, inviteeEmail, None, false)) } val registeringUser = genWorkbenchUserGoogle.sample.get.copy(email = inviteeEmail) runAndWait(service.createUser(registeringUser, samRequestContext)) val updatedUserInPostgres = dirDAO.loadUser(invitedUserId, samRequestContext).unsafeRunSync() - updatedUserInPostgres.value shouldBe SamUser(invitedUserId, registeringUser.googleSubjectId, inviteeEmail, None, true, None) + updatedUserInPostgres.value shouldBe SamUser(invitedUserId, registeringUser.googleSubjectId, inviteeEmail, None, true) } it should "update azureB2CId for this user" in { @@ -566,13 +567,13 @@ class OldUserServiceSpec val invitedUserId = dirDAO.loadSubjectFromEmail(inviteeEmail, samRequestContext).unsafeRunSync().value.asInstanceOf[WorkbenchUserId] val userInPostgres = dirDAO.loadUser(invitedUserId, samRequestContext).unsafeRunSync() - userInPostgres.value should equal(SamUser(invitedUserId, None, inviteeEmail, None, false, None)) + userInPostgres.value should equal(SamUser(invitedUserId, None, inviteeEmail, None, false)) val registeringUser = genWorkbenchUserAzure.sample.get.copy(email = inviteeEmail) runAndWait(service.createUser(registeringUser, samRequestContext)) val updatedUserInPostgres = dirDAO.loadUser(invitedUserId, samRequestContext).unsafeRunSync() - updatedUserInPostgres.value shouldBe SamUser(invitedUserId, None, inviteeEmail, registeringUser.azureB2CId, true, None) + updatedUserInPostgres.value shouldBe SamUser(invitedUserId, None, inviteeEmail, registeringUser.azureB2CId, true) } "UserService getUserIdInfoFromEmail" should "return the email along with the userSubjectId and googleSubjectId" in {