From 39f1d2c25b6808320cad35a8cc236f23bced4394 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Tue, 14 Nov 2023 15:59:45 -0500 Subject: [PATCH 01/10] ID-787 Get user/self terms of service history. --- src/main/resources/swagger/api-docs.yaml | 58 +++++++++++++++ .../sam/api/TermsOfServiceRoutes.scala | 15 +++- .../sam/dataAccess/DirectoryDAO.scala | 1 + .../sam/dataAccess/PostgresDirectoryDAO.scala | 17 ++++- .../dsde/workbench/sam/model/SamModel.scala | 3 + .../sam/model/api/SamJsonSupport.scala | 6 ++ .../workbench/sam/service/TosService.scala | 21 +++++- .../sam/api/MockSamRoutesBuilder.scala | 6 ++ .../sam/api/TermsOfServiceRouteSpec.scala | 66 ++++++++++++++++- .../sam/dataAccess/MockDirectoryDAO.scala | 13 +++- .../dataAccess/MockDirectoryDaoBuilder.scala | 10 ++- .../sam/service/MockTosServiceBuilder.scala | 19 ++++- .../sam/service/TosServiceSpec.scala | 70 ++++++++++++++++++- 13 files changed, 297 insertions(+), 8 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 49b5601fc..2171fa3d1 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -3364,6 +3364,64 @@ 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/UserTermsOfServiceDetails' + 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/UserTermsOfServiceDetails' + 404: + description: user has no history of terms of service actions + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' /version: get: tags: diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala index 45049ecaa..b7652d59f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala @@ -95,6 +95,17 @@ trait TermsOfServiceRoutes extends SamUserDirectives { complete(tosService.rejectTosStatus(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 @@ -111,7 +122,9 @@ trait TermsOfServiceRoutes extends SamUserDirectives { pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history pathEndOrSingleSlash { get { - complete(StatusCodes.NotImplemented) + parameters("limit".as[Integer].withDefault(100)) { (limit: Int) => + complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit)) + } } } } 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 a76306f5b..613ababc5 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 @@ -78,6 +78,7 @@ trait DirectoryDAO { 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 getUserTosHistory(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]] 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 f8bcdb661..4ac16d076 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 @@ -648,7 +648,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } } - // When no tosVersion is specified, return the latest TosRecord for the user override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = readOnlyTransaction("getUserTos", samRequestContext) { implicit session => val tosTable = TosTable.syntax @@ -683,6 +682,22 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } } + override def getUserTosHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = + readOnlyTransaction("getUserTosHistory", 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 { 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 1cab49266..cd5bdba9d 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 @@ -85,6 +85,8 @@ object UserStatusDetails { ) @Lenses final case class TermsOfServiceDetails(latestAcceptedVersion: String, acceptedOn: Instant, permitsSystemUsage: 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() @@ -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 diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala index e8306ae88..7905e2b5c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/SamJsonSupport.scala @@ -29,6 +29,8 @@ import org.broadinstitute.dsde.workbench.sam.model.{ TermsOfServiceAcceptance, TermsOfServiceComplianceStatus, TermsOfServiceDetails, + TermsOfServiceHistory, + TermsOfServiceHistoryRecord, UserIdInfo, UserPolicyResponse, UserResourcesResponse, @@ -73,6 +75,10 @@ object SamJsonSupport { implicit val TermsOfServiceDetailsFormat = jsonFormat3(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) 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 48df75050..48d3c7ddf 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 @@ -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} @@ -112,6 +119,18 @@ class TosService( case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user:${userId}")) } + def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] = + ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) { + directoryDao.getUserTosHistory(userId, samRequestContext, limit).map { + case samUserTosHistory if samUserTosHistory.isEmpty => + throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find any Terms of Service entries for user:${userId}")) + case samUserTosHistory => + TermsOfServiceHistory( + samUserTosHistory.map(historyRecord => TermsOfServiceHistoryRecord(historyRecord.action, historyRecord.version, historyRecord.createdAt)) + ) + } + } + def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for { latestUserTos <- directoryDao.getUserTos(samUser.id, samRequestContext) previousUserTos <- directoryDao.getUserTosVersion(samUser.id, tosConfig.previousVersion, samRequestContext) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala index 3afd1f63c..94a33bd5a 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala @@ -7,6 +7,7 @@ import akka.stream.Materializer import cats.effect.IO import org.broadinstitute.dsde.workbench.model.{ErrorReportSource, WorkbenchGroup} import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics +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 @@ -76,6 +77,11 @@ class MockSamRoutesBuilder(allUsersGroup: WorkbenchGroup)(implicit system: Actor this } + def withTosHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockSamRoutesBuilder = { + mockTosServiceBuilder.withTosHistoryForUser(samUser, tosHistory) + this + } + def withUserAttributes(samUser: SamUser, userAttributes: SamUserAttributes): MockSamRoutesBuilder = { userServiceBuilder.withUserAttributes(samUser, userAttributes) this diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala index 9a469b38c..c77b47fc2 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala @@ -6,14 +6,18 @@ import akka.http.scaladsl.server.Route import akka.http.scaladsl.testkit.ScalatestRouteTest import org.broadinstitute.dsde.workbench.model.WorkbenchEmail import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, databaseEnabledClue} +import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, TermsOfServiceConfigResponse} -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, TermsOfServiceDetails} +import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, TermsOfServiceDetails, TermsOfServiceHistory, TermsOfServiceHistoryRecord} import org.broadinstitute.dsde.workbench.sam.service.CloudExtensions import org.broadinstitute.dsde.workbench.sam.{Generator, TestSupport} import org.scalatest.concurrent.Eventually.eventually import org.scalatest.funspec.AnyFunSpec import org.scalatest.matchers.should.Matchers +import spray.json.DefaultJsonProtocol._ + +import java.time.Instant class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRouteTest with TestSupport { @@ -152,6 +156,66 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } } + describe("GET /api/termsOfService/v1/user/{USER_ID}/history") { + val samRoutes = TestSamRoutes(Map.empty) + it("should return a list of `TermsOfServiceHistoryRecord`") { + val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) + val defaultUser: SamUser = Generator.genWorkbenchUserGoogle.sample.get + val record1 = TermsOfServiceHistoryRecord(TosTable.ACCEPT, "0", Instant.now) + val record2 = TermsOfServiceHistoryRecord(TosTable.REJECT, "0", Instant.now.minusSeconds(5)) + val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) + .withEnabledUser(defaultUser) + .withTosHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) + + Get(s"/api/termsOfService/v1/user/${defaultUser.id}/history") ~> mockSamRoutesBuilder.build.route ~> check { + withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { + responseAs[List[TermsOfServiceHistoryRecord]] shouldBe List(record1, record2) + } + status shouldEqual StatusCodes.OK + } + } + + it("should return 404 when user has no acceptance history") { + val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) + val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) + .withEnabledUser(Generator.genWorkbenchUserGoogle.sample.get) + + Get("/api/termsOfService/v1/user/12345abc/history") ~> mockSamRoutesBuilder.build.route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + } + + describe("GET /api/termsOfService/v1/user/self/history") { + val samRoutes = TestSamRoutes(Map.empty) + it("should return a list of `TermsOfServiceHistoryRecord`") { + val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) + val defaultUser: SamUser = Generator.genWorkbenchUserGoogle.sample.get + val record1 = TermsOfServiceHistoryRecord(TosTable.ACCEPT, "0", Instant.now) + val record2 = TermsOfServiceHistoryRecord(TosTable.REJECT, "0", Instant.now.minusSeconds(5)) + val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) + .withEnabledUser(defaultUser) + .withTosHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) + + Get(s"/api/termsOfService/v1/user/self/history") ~> mockSamRoutesBuilder.build.route ~> check { + withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { + responseAs[List[TermsOfServiceHistoryRecord]] shouldBe List(record1, record2) + } + status shouldEqual StatusCodes.OK + } + } + + it("should return 404 when user has no acceptance history") { + val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) + val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) + .withDisabledUser(Generator.genWorkbenchUserGoogle.sample.get) + + Get("/api/termsOfService/v1/user/self/history") ~> mockSamRoutesBuilder.build.route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + } + it("should return 204 when tos accepted") { val samRoutes = TestSamRoutes(Map.empty) eventually { 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 b34e56bac..72f6025cc 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 @@ -1,6 +1,5 @@ package org.broadinstitute.dsde.workbench.sam.dataAccess -import java.util.Date import akka.http.scaladsl.model.StatusCodes import cats.effect.IO import cats.implicits._ @@ -14,6 +13,7 @@ import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbench import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant +import java.util.Date import scala.collection.concurrent.TrieMap import scala.collection.mutable @@ -23,6 +23,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench 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 userTosHistory: mutable.Map[WorkbenchUserId, List[SamUserTos]] = new TrieMap() private val userAttributes: mutable.Map[WorkbenchUserId, SamUserAttributes] = new TrieMap() private val usersWithEmails: mutable.Map[WorkbenchEmail, WorkbenchUserId] = new TrieMap() @@ -313,6 +314,8 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench case Some(user) => users.put(userId, user) userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) + val userHistory = userTosHistory.getOrElse(userId, List.empty) + userTosHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) true } @@ -322,6 +325,8 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench case Some(user) => users.put(userId, user) userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) + val userHistory = userTosHistory.getOrElse(userId, List.empty) + userTosHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) true } @@ -342,6 +347,12 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench } } + override def getUserTosHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = + loadUser(userId, samRequestContext).map { + case None => List.empty + case Some(_) => userTosHistory.getOrElse(userId, List.empty) + } + override def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] = { if (petManagedIdentitiesByUser.keySet.contains(petManagedIdentity.id)) { IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Conflict, s"pet managed identity ${petManagedIdentity.id} already exists"))) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala index c03d84ccd..9788c62f1 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala @@ -3,8 +3,8 @@ package org.broadinstitute.dsde.workbench.sam.dataAccess import cats.effect.IO import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos} import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, SamUserAttributes} +import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.mockito.ArgumentMatchersSugar.{any, eqTo} import org.mockito.{IdiomaticMockito, Strictness} @@ -129,6 +129,14 @@ case class MockDirectoryDaoBuilder() extends IdiomaticMockito { this } + def withTermsOfServiceHistoryForUser(samUser: SamUser, tosHistory: List[SamUserTos]): MockDirectoryDaoBuilder = { + makeUserExist(samUser) + mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(tosHistory.head)) + mockedDirectoryDAO.getUserTosVersion(eqTo(samUser.id), any[Option[String]], any[SamRequestContext]) returns IO(Option(tosHistory.head)) + mockedDirectoryDAO.getUserTosHistory(eqTo(samUser.id), any[SamRequestContext], any[Integer]) returns IO(tosHistory) + this + } + // Bare minimum for a user to exist: // - has an ID // - has an email 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 53f01727f..2a21272f2 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 @@ -5,7 +5,7 @@ import cats.effect.IO import org.broadinstitute.dsde.workbench.model.{ErrorReport, ErrorReportSource, WorkbenchExceptionWithErrorReport, WorkbenchUserId} import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.model.api.SamUser -import org.broadinstitute.dsde.workbench.sam.model.{TermsOfServiceComplianceStatus, TermsOfServiceDetails} +import org.broadinstitute.dsde.workbench.sam.model.{TermsOfServiceComplianceStatus, TermsOfServiceDetails, TermsOfServiceHistory, TermsOfServiceHistoryRecord} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.mockito.Mockito.{RETURNS_SMART_NULLS, lenient} import org.mockito.invocation.InvocationOnMock @@ -30,6 +30,14 @@ case class MockTosServiceBuilder() extends MockitoSugar { this } + def withTosHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockTosServiceBuilder = { + lenient() + .doReturn(IO.pure(tosHistory)) + .when(tosService) + .getTermsOfServiceHistoryForUser(ArgumentMatchers.eq(samUser.id), any[SamRequestContext], any[Integer]) + this + } + def withAcceptedStateForUser(samUser: SamUser, isAccepted: Boolean, version: String = "v1"): MockTosServiceBuilder = { setAcceptedStateForUserTo(samUser, isAccepted, version) this @@ -45,6 +53,10 @@ case class MockTosServiceBuilder() extends MockitoSugar { .doReturn(IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"")(new ErrorReportSource("MockTosServiceBuilder"))))) .when(tosService) .getTermsOfServiceDetailsForUser(any[WorkbenchUserId], any[SamRequestContext]) + lenient() + .doReturn(IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"")(new ErrorReportSource("MockTosServiceBuilder"))))) + .when(tosService) + .getTermsOfServiceHistoryForUser(any[WorkbenchUserId], any[SamRequestContext], any[Integer]) } private def setAcceptedStateForUserTo(samUser: SamUser, isAccepted: Boolean, version: String) = { @@ -63,6 +75,11 @@ case class MockTosServiceBuilder() extends MockitoSugar { .doReturn(IO.pure(TermsOfServiceDetails(version, rightNow, permitsSystemUsage = isAccepted))) .when(tosService) .getTermsOfServiceDetailsForUser(ArgumentMatchers.eq(samUser.id), any[SamRequestContext]) + + lenient() + .doReturn(IO.pure(TermsOfServiceHistory(List(TermsOfServiceHistoryRecord(action, version, rightNow))))) + .when(tosService) + .getTermsOfServiceHistoryForUser(ArgumentMatchers.eq(samUser.id), any[SamRequestContext], any[Integer]) } def build: TosService = tosService 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 87e07123e..bd9fd2613 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 @@ -11,7 +11,7 @@ import org.broadinstitute.dsde.workbench.sam.dataAccess.{DirectoryDAO, MockDirec import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.matchers.{TermsOfServiceDetailsMatchers, TimeMatchers} import org.broadinstitute.dsde.workbench.sam.model.api.TermsOfServiceConfigResponse -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos, TermsOfServiceDetails} +import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUserTos, TermsOfServiceDetails, TermsOfServiceHistory} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.broadinstitute.dsde.workbench.sam.{Generator, PropertyBasedTesting, TestSupport} import org.mockito.Mockito.RETURNS_SMART_NULLS @@ -559,5 +559,73 @@ class TosServiceSpec(_system: ActorSystem) assert(e.errorReport.statusCode.value == StatusCodes.Unauthorized, "User should not be authorized to see other users' Terms of Service details") } } + + "can retrieve Terms of Service history for a user" - { + "if the requesting user is an admin" in { + // Arrange + val tosVersion = "0" + val adminUser = Generator.genWorkbenchUserBoth.sample.get + val record1 = SamUserTos(adminUser.id, tosVersion, TosTable.ACCEPT, Instant.now()) + val record2 = SamUserTos(adminUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5)) + val directoryDao = new MockDirectoryDaoBuilder() + .withTermsOfServiceHistoryForUser(defaultUser, List(record1, record2)) + .build + + val tosService = new TosService(NoExtensions, directoryDao, TestSupport.tosConfig) + + // Act + val userTosDetails: TermsOfServiceHistory = + runAndWait(tosService.getTermsOfServiceHistoryForUser(defaultUser.id, SamRequestContext(None, None, Some(adminUser)), 5)) + + // Assert + userTosDetails.history.size shouldBe 2 + userTosDetails.history.head shouldBe record1.toHistoryRecord + userTosDetails.history.last shouldBe record2.toHistoryRecord + } + + "if the requesting user is not an admin but is the same as the requested user" in { + // Arrange + val tosVersion = "0" + val userTos1 = SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()) + val userTos2 = SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5)) + val directoryDao = new MockDirectoryDaoBuilder() + .withTermsOfServiceHistoryForUser(defaultUser, List(userTos1, userTos2)) + .build + + val tosService = new TosService(NoExtensions, directoryDao, TestSupport.tosConfig) + + // Act + val userTosDetails: TermsOfServiceHistory = + runAndWait(tosService.getTermsOfServiceHistoryForUser(defaultUser.id, SamRequestContext(None, None, Some(defaultUser)), 5)) + + // Assert + userTosDetails.history.size shouldBe 2 + userTosDetails.history.head shouldBe userTos1.toHistoryRecord + userTosDetails.history.last shouldBe userTos2.toHistoryRecord + } + } + "cannot retrieve Terms of Service history for another user" - { + "if requesting user is not an admin and the requested user is a different user" in { + // Arrange + val tosVersion = "v1" + val nonAdminUser = Generator.genWorkbenchUserBoth.sample.get + val someRandoUser = Generator.genWorkbenchUserBoth.sample.get + val userTos1 = SamUserTos(someRandoUser.id, tosVersion, TosTable.ACCEPT, Instant.now()) + val userTos2 = SamUserTos(someRandoUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5)) + val directoryDao = new MockDirectoryDaoBuilder() + .withTermsOfServiceHistoryForUser(someRandoUser, List(userTos1, userTos2)) + .build + val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build + + val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig) + + // Act and Assert + val e = intercept[WorkbenchExceptionWithErrorReport] { + runAndWait(tosService.getTermsOfServiceHistoryForUser(someRandoUser.id, SamRequestContext(None, None, Some(nonAdminUser)), 5)) + } + + assert(e.errorReport.statusCode.value == StatusCodes.Unauthorized, "User should not be authorized to see other users' Terms of Service details") + } + } } } From d2bbc355e29669a8b54c8407bf700471d4f2ce1f Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Thu, 16 Nov 2023 13:53:49 -0500 Subject: [PATCH 02/10] Add to api-docs.yaml --- src/main/resources/swagger/api-docs.yaml | 31 ++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 2171fa3d1..262b6950b 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -3383,7 +3383,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/UserTermsOfServiceDetails' + $ref: '#/components/schemas/UserTermsOfServiceHistory' 404: description: user has no history of terms of service actions content: @@ -3415,7 +3415,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/UserTermsOfServiceDetails' + $ref: '#/components/schemas/UserTermsOfServiceHistory' 404: description: user has no history of terms of service actions content: @@ -4139,6 +4139,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: + latestAcceptedVersion: + type: array + items: + $ref: '#/components/schemas/UserTermsOfServiceRecord' + UserTermsOfServiceRecord: + 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 From db670cbc17147f31c4933134dfde25f796ceaed7 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Thu, 16 Nov 2023 13:55:49 -0500 Subject: [PATCH 03/10] Refactor Tos -> TermsOfService --- .../workbench/sam/api/OldUserRoutes.scala | 4 +- .../sam/api/TermsOfServiceRoutes.scala | 2 +- .../sam/dataAccess/DirectoryDAO.scala | 6 +- .../sam/dataAccess/PostgresDirectoryDAO.scala | 6 +- .../workbench/sam/service/TosService.scala | 28 ++-- .../workbench/sam/service/UserService.scala | 8 +- .../sam/api/MockSamRoutesBuilder.scala | 4 +- .../sam/api/TermsOfServiceRouteSpec.scala | 4 +- .../sam/dataAccess/MockDirectoryDAO.scala | 6 +- .../dataAccess/MockDirectoryDaoBuilder.scala | 16 +-- .../dataAccess/PostgresDirectoryDAOSpec.scala | 10 +- .../sam/service/MockTosServiceBuilder.scala | 6 +- .../sam/service/TosServiceSpec.scala | 128 +++++++++--------- .../sam/service/UserServiceSpec.scala | 4 +- 14 files changed, 116 insertions(+), 116 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala index 203d2c4c5..bb7b72389 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala @@ -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)) } } @@ -144,7 +144,7 @@ trait OldUserRoutes extends SamUserDirectives with SamRequestContextDirectives { } ~ path("termsOfServiceComplianceStatus") { get { - complete(tosService.getTosComplianceStatus(user, samRequestContext)) + complete(tosService.getTermsOfServiceComplianceStatus(user, samRequestContext)) } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala index 17f5b2db5..64c7518c4 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala @@ -53,7 +53,7 @@ trait TermsOfServiceRoutes extends SamUserDirectives { parameters("doc".as[String].?) { (doc: Option[String]) => val docSet = doc.map(_.split(",").toSet).getOrElse(Set.empty) if (docSet.subsetOf(Set(tosService.termsOfServiceTextKey, tosService.privacyPolicyTextKey))) - complete(tosService.getTosText(docSet)) + complete(tosService.getTermsOfServiceText(docSet)) else complete(StatusCodes.BadRequest) } 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 613ababc5..4532e9ecd 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 @@ -76,9 +76,9 @@ 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 getUserTosHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] + def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] + def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): 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]] 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 4ac16d076..d702dc54b 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 @@ -648,7 +648,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } } - override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = readOnlyTransaction("getUserTos", samRequestContext) { implicit session => val tosTable = TosTable.syntax val column = TosTable.column @@ -664,7 +664,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val userTosRecordOpt.map(TosTable.unmarshalUserRecord) } - override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = { + override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = { if (tosVersion.isEmpty) return IO(None) readOnlyTransaction("getUserTos", samRequestContext) { implicit session => val tosTable = TosTable.syntax @@ -682,7 +682,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } } - override def getUserTosHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = + override def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = readOnlyTransaction("getUserTosHistory", samRequestContext) { implicit session => val tosTable = TosTable.syntax val column = TosTable.column 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 1d929e432..241a85576 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 @@ -70,7 +70,7 @@ class TosService( ) ) - def getTosText(docSet: Set[String]): IO[String] = + def getTermsOfServiceText(docSet: Set[String]): IO[String] = docSet match { case set if set.isEmpty => IO.pure(termsOfServiceText) case set if set.equals(Set(privacyPolicyTextKey)) => IO.pure(privacyPolicyText) @@ -86,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)) } @@ -114,14 +114,14 @@ class TosService( // Note: if version is None, then the query will return the last accepted ToS info for the user private def loadTosRecordForUser(userId: WorkbenchUserId, version: Option[String], samRequestContext: SamRequestContext): IO[SamUserTos] = - directoryDao.getUserTosVersion(userId, version, samRequestContext).map { + directoryDao.getUserTermsOfServiceVersion(userId, version, samRequestContext).map { case Some(samUserTos) => samUserTos case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user:${userId}")) } def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] = ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) { - directoryDao.getUserTosHistory(userId, samRequestContext, limit).map { + directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map { case samUserTosHistory if samUserTosHistory.isEmpty => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find any Terms of Service entries for user:${userId}")) case samUserTosHistory => @@ -131,10 +131,10 @@ class TosService( } } - def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for { - latestUserTos <- directoryDao.getUserTos(samUser.id, samRequestContext) - previousUserTos <- directoryDao.getUserTosVersion(samUser.id, tosConfig.previousVersion, samRequestContext) - userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(latestUserTos) + def getTermsOfServiceComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for { + latestUserTos <- directoryDao.getUserTermsOfService(samUser.id, samRequestContext) + previousUserTos <- directoryDao.getUserTermsOfServiceVersion(samUser.id, tosConfig.previousVersion, samRequestContext) + userHasAcceptedLatestVersion = userHasAcceptedLatestTermsOfServiceVersion(latestUserTos) permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos, previousUserTos) } yield TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage) @@ -149,14 +149,14 @@ class TosService( if (userIsServiceAccount) { return true } - if (userHasRejectedLatestTosVersion(userTos)) { + if (userHasRejectedLatestTermsOfServiceVersion(userTos)) { return false } userTos.exists { tos => - val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(Option(tos)) + val userHasAcceptedLatestVersion = userHasAcceptedLatestTermsOfServiceVersion(Option(tos)) val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT - val userHasAcceptedPreviousVersion = userHasAcceptedPreviousTosVersion(previousUserTos) + val userHasAcceptedPreviousVersion = userHasAcceptedPreviousTermsOfServiceVersion(previousUserTos) val userInsideOfRollingAcceptanceWindow = isRollingWindowInEffect() && userHasAcceptedPreviousVersion userHasAcceptedLatestVersion || userInsideOfRollingAcceptanceWindow || userCanUseSystemUnderGracePeriod @@ -164,17 +164,17 @@ class TosService( } } - private def userHasAcceptedLatestTosVersion(userTos: Option[SamUserTos]): Boolean = + private def userHasAcceptedLatestTermsOfServiceVersion(userTos: Option[SamUserTos]): Boolean = userTos.exists { tos => tos.version.contains(tosConfig.version) && tos.action == TosTable.ACCEPT } - private def userHasRejectedLatestTosVersion(userTos: Option[SamUserTos]): Boolean = + private def userHasRejectedLatestTermsOfServiceVersion(userTos: Option[SamUserTos]): Boolean = userTos.exists { tos => tos.version.contains(tosConfig.version) && tos.action == TosTable.REJECT } - private def userHasAcceptedPreviousTosVersion(previousUserTos: Option[SamUserTos]): Boolean = + private def userHasAcceptedPreviousTermsOfServiceVersion(previousUserTos: Option[SamUserTos]): Boolean = previousUserTos.exists(tos => tos.action == TosTable.ACCEPT) } 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 9c06fe5b2..1c1f4dfee 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 @@ -339,7 +339,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 @@ -382,7 +382,7 @@ 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]] = @@ -390,7 +390,7 @@ class UserService( 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 } @@ -496,7 +496,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, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala index 94a33bd5a..d9be04155 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala @@ -77,8 +77,8 @@ class MockSamRoutesBuilder(allUsersGroup: WorkbenchGroup)(implicit system: Actor this } - def withTosHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockSamRoutesBuilder = { - mockTosServiceBuilder.withTosHistoryForUser(samUser, tosHistory) + def withTermsOfServiceHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockSamRoutesBuilder = { + mockTosServiceBuilder.withTermsOfServiceHistoryForUser(samUser, tosHistory) this } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala index 47405d8cc..9c031a380 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala @@ -164,7 +164,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou val record2 = TermsOfServiceHistoryRecord(TosTable.REJECT, "0", Instant.now.minusSeconds(5)) val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) .withEnabledUser(defaultUser) - .withTosHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) + .withTermsOfServiceHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) Get(s"/api/termsOfService/v1/user/${defaultUser.id}/history") ~> mockSamRoutesBuilder.build.route ~> check { withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { @@ -195,7 +195,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou val record2 = TermsOfServiceHistoryRecord(TosTable.REJECT, "0", Instant.now.minusSeconds(5)) val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup) .withEnabledUser(defaultUser) - .withTosHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) + .withTermsOfServiceHistoryForUser(defaultUser, TermsOfServiceHistory(List(record1, record2))) Get(s"/api/termsOfService/v1/user/self/history") ~> mockSamRoutesBuilder.build.route ~> check { withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { 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 72f6025cc..e32ca5ee2 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 @@ -330,14 +330,14 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench true } - override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => None case Some(_) => userTos.get(userId) } - override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => None case Some(_) => @@ -347,7 +347,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench } } - override def getUserTosHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = + override def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => List.empty case Some(_) => userTosHistory.getOrElse(userId, List.empty) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala index 9788c62f1..6a622e5a3 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala @@ -41,7 +41,7 @@ case class MockDirectoryDaoBuilder() extends IdiomaticMockito { mockedDirectoryDAO.setUserRegisteredAt(any[WorkbenchUserId], any[Instant], any[SamRequestContext]) returns IO.unit mockedDirectoryDAO.setUserAttributes(any[SamUserAttributes], any[SamRequestContext]) returns IO.unit mockedDirectoryDAO.getUserAttributes(any[WorkbenchUserId], any[SamRequestContext]) returns IO(None) - mockedDirectoryDAO.getUserTosVersion(any[WorkbenchUserId], any[Some[String]], any[SamRequestContext]) returns IO(None) + mockedDirectoryDAO.getUserTermsOfServiceVersion(any[WorkbenchUserId], any[Some[String]], any[SamRequestContext]) returns IO(None) def withHealthyDatabase: MockDirectoryDaoBuilder = { mockedDirectoryDAO.checkStatus(any[SamRequestContext]) returns IO(true) @@ -116,24 +116,24 @@ case class MockDirectoryDaoBuilder() extends IdiomaticMockito { def withAcceptedTermsOfServiceForUser(samUser: SamUser, tosVersion: String): MockDirectoryDaoBuilder = { makeUserExist(samUser) val samUserTos = SamUserTos(samUser.id, tosVersion, TosTable.ACCEPT, Instant.now) - mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos)) - mockedDirectoryDAO.getUserTosVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfService(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfServiceVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext]) returns IO(Option(samUserTos)) this } def withRejectedTermsOfServiceForUser(samUser: SamUser, tosVersion: String): MockDirectoryDaoBuilder = { makeUserExist(samUser) val samUserTos = SamUserTos(samUser.id, tosVersion, TosTable.REJECT, Instant.now) - mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos)) - mockedDirectoryDAO.getUserTosVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfService(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfServiceVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext]) returns IO(Option(samUserTos)) this } def withTermsOfServiceHistoryForUser(samUser: SamUser, tosHistory: List[SamUserTos]): MockDirectoryDaoBuilder = { makeUserExist(samUser) - mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(tosHistory.head)) - mockedDirectoryDAO.getUserTosVersion(eqTo(samUser.id), any[Option[String]], any[SamRequestContext]) returns IO(Option(tosHistory.head)) - mockedDirectoryDAO.getUserTosHistory(eqTo(samUser.id), any[SamRequestContext], any[Integer]) returns IO(tosHistory) + mockedDirectoryDAO.getUserTermsOfService(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(tosHistory.head)) + mockedDirectoryDAO.getUserTermsOfServiceVersion(eqTo(samUser.id), any[Option[String]], any[SamRequestContext]) returns IO(Option(tosHistory.head)) + mockedDirectoryDAO.getUserTermsOfServiceHistory(eqTo(samUser.id), any[SamRequestContext], any[Integer]) returns IO(tosHistory) this } 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 9b52f29e5..27624ef36 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 @@ -1601,7 +1601,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.acceptTermsOfService(defaultUser.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true // Assert - val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync() + val userTos = dao.getUserTermsOfService(defaultUser.id, samRequestContext).unsafeRunSync() userTos should not be empty userTos.get.createdAt should beAround(Instant.now()) userTos.get.action shouldBe TosTable.ACCEPT @@ -1615,7 +1615,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.acceptTermsOfService(defaultUser.id, "2", samRequestContext).unsafeRunSync() shouldBe true // Assert - val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync() + val userTos = dao.getUserTermsOfService(defaultUser.id, samRequestContext).unsafeRunSync() userTos should not be empty userTos.get.createdAt should beAround(Instant.now()) userTos.get.action shouldBe TosTable.ACCEPT @@ -1631,7 +1631,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true // Assert - val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + val userTos = dao.getUserTermsOfService(user.id, samRequestContext).unsafeRunSync() userTos should not be empty userTos.get.createdAt should beAround(Instant.now()) userTos.get.action shouldBe TosTable.REJECT @@ -1646,7 +1646,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true // Assert - val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + val userTos = dao.getUserTermsOfService(user.id, samRequestContext).unsafeRunSync() userTos should not be empty userTos.get.createdAt should beAround(Instant.now()) userTos.get.action shouldBe TosTable.REJECT @@ -1661,7 +1661,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B dao.createUser(user, samRequestContext).unsafeRunSync() // Assert - val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync() + val userTos = dao.getUserTermsOfService(user.id, samRequestContext).unsafeRunSync() userTos should be(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 8d79602ea..6dda71e4f 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 @@ -30,7 +30,7 @@ case class MockTosServiceBuilder() extends MockitoSugar { this } - def withTosHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockTosServiceBuilder = { + def withTermsOfServiceHistoryForUser(samUser: SamUser, tosHistory: TermsOfServiceHistory): MockTosServiceBuilder = { lenient() .doReturn(IO.pure(tosHistory)) .when(tosService) @@ -47,7 +47,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], any[SamRequestContext]) + .getTermsOfServiceComplianceStatus(any[SamUser], any[SamRequestContext]) lenient() .doReturn(IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"")(new ErrorReportSource("MockTosServiceBuilder"))))) @@ -67,7 +67,7 @@ case class MockTosServiceBuilder() extends MockitoSugar { lenient() .doReturn(IO.pure(TermsOfServiceComplianceStatus(samUser.id, isAccepted, isAccepted))) .when(tosService) - .getTosComplianceStatus(ArgumentMatchers.argThat(matchesUser), any[SamRequestContext]) + .getTermsOfServiceComplianceStatus(ArgumentMatchers.argThat(matchesUser), any[SamRequestContext]) val action = if (isAccepted) TosTable.ACCEPT else TosTable.REJECT val rightNow = Instant.now 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 0ffc28dc1..514fff9f9 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 @@ -84,28 +84,28 @@ class TosServiceSpec(_system: ActorSystem) "returns terms of service text when no query parameters are passed" in { val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) - val tosText = tosService.getTosText(Set.empty).unsafeRunSync() + val tosText = tosService.getTermsOfServiceText(Set.empty).unsafeRunSync() tosText shouldBe tosService.termsOfServiceText } "returns terms of service text by default" in { val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) - val tosText = tosService.getTosText(Set("termsOfService")).unsafeRunSync() + val tosText = tosService.getTermsOfServiceText(Set("termsOfService")).unsafeRunSync() tosText shouldBe tosService.termsOfServiceText } "returns privacy policy text" in { val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) - val tosText = tosService.getTosText(Set("privacyPolicy")).unsafeRunSync() + val tosText = tosService.getTermsOfServiceText(Set("privacyPolicy")).unsafeRunSync() tosText shouldBe tosService.privacyPolicyText } "returns privacy policy text and terms of service text" in { val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) - val tosText = tosService.getTosText(Set("privacyPolicy", "termsOfService")).unsafeRunSync() + val tosText = tosService.getTermsOfServiceText(Set("privacyPolicy", "termsOfService")).unsafeRunSync() tosText shouldBe s"${tosService.termsOfServiceText}\n\n${tosService.privacyPolicyText}" } @@ -139,11 +139,11 @@ class TosServiceSpec(_system: ActorSystem) val previousTosVersion = Option("1") val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig.copy(version = tosVersion, previousVersion = previousTosVersion)) - when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfService(serviceAccountUser.id, samRequestContext)).thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(serviceAccountUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfServiceVersion(serviceAccountUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) - val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosService.getTermsOfServiceComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } @@ -152,11 +152,11 @@ class TosServiceSpec(_system: ActorSystem) val previousTosVersion = Option("1") val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig.copy(version = tosVersion, previousVersion = previousTosVersion)) - when(dirDAO.getUserTos(uamiUser.id, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfService(uamiUser.id, samRequestContext)).thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(uamiUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfServiceVersion(uamiUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) - val complianceStatus = tosService.getTosComplianceStatus(uamiUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosService.getTermsOfServiceComplianceStatus(uamiUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } @@ -232,13 +232,13 @@ class TosServiceSpec(_system: ActorSystem) ) ) - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(defaultUser.id, previousTosVersion, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousTosVersion, samRequestContext)) .thenReturn(IO.pure(None)) - val complianceStatus = tosService.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosService.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -264,13 +264,13 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withoutRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 1 val complianceStatus = - tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -278,12 +278,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withoutRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 4 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -291,12 +291,12 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 7 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -304,12 +304,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 10 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -319,13 +319,13 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withoutRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 2 val complianceStatus = - tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -333,12 +333,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withoutRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 5 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -346,12 +346,12 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 8 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -359,12 +359,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 11 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -374,13 +374,13 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withoutRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 3 val complianceStatus = - tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -388,12 +388,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withoutRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 6 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -401,12 +401,12 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 9 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -414,12 +414,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 12 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -430,13 +430,13 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withoutRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) val complianceStatus = - tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -444,12 +444,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withoutRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -457,12 +457,12 @@ class TosServiceSpec(_system: ActorSystem) withoutGracePeriod - { withRollingAcceptanceWindow - { canUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -470,12 +470,12 @@ class TosServiceSpec(_system: ActorSystem) withGracePeriod - { withRollingAcceptanceWindow - { cannotUseTheSystem in { - when(dirDAO.getUserTos(defaultUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(defaultUser.id, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now())))) - when(dirDAO.getUserTosVersion(defaultUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -484,12 +484,12 @@ class TosServiceSpec(_system: ActorSystem) "when a service account is using the api" - { "let it use the api regardless of tos status" in { - when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext)) + when(dirDAO.getUserTermsOfService(serviceAccountUser.id, samRequestContext)) .thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(serviceAccountUser.id, previousVersionOpt, samRequestContext)) + when(dirDAO.getUserTermsOfServiceVersion(serviceAccountUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) val complianceStatus = - tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() + tosServiceV2GracePeriodDisabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(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 9d3b0896d..131f636d4 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 @@ -137,7 +137,7 @@ class OldUserServiceMockSpec(_system: ActorSystem) when(googleExtensions.onGroupUpdate(any[Seq[WorkbenchGroupIdentity]], any[SamRequestContext])).thenReturn(IO.unit) mockTosService = mock[TosService](RETURNS_SMART_NULLS) - when(mockTosService.getTosComplianceStatus(any[SamUser], any[SamRequestContext])) + when(mockTosService.getTermsOfServiceComplianceStatus(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)) @@ -168,7 +168,7 @@ class OldUserServiceMockSpec(_system: ActorSystem) } it should "return UserStatusDiagnostics.tosAccepted as false if user's TOS status is false" in { - when(mockTosService.getTosComplianceStatus(enabledUser, samRequestContext)) + when(mockTosService.getTermsOfServiceComplianceStatus(enabledUser, samRequestContext)) .thenReturn(IO.pure(TermsOfServiceComplianceStatus(enabledUser.id, false, false))) val status = service.getUserStatusDiagnostics(enabledUser.id, samRequestContext).unsafeRunSync() status.value.tosAccepted shouldBe false From b9b90d64f11079c306cd72f89f5de48ca56d8ea0 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Thu, 16 Nov 2023 14:17:16 -0500 Subject: [PATCH 04/10] format --- .../sam/dataAccess/PostgresDirectoryDAO.scala | 6 +++- .../sam/api/TermsOfServiceRouteSpec.scala | 8 +++-- .../sam/service/TosServiceSpec.scala | 36 ++++++++++++------- 3 files changed, 35 insertions(+), 15 deletions(-) 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 d702dc54b..f4d18d06b 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 @@ -664,7 +664,11 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val userTosRecordOpt.map(TosTable.unmarshalUserRecord) } - override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = { + override def getUserTermsOfServiceVersion( + userId: WorkbenchUserId, + tosVersion: Option[String], + samRequestContext: SamRequestContext + ): IO[Option[SamUserTos]] = { if (tosVersion.isEmpty) return IO(None) readOnlyTransaction("getUserTos", samRequestContext) { implicit session => val tosTable = TosTable.syntax diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala index 9c031a380..78f93971e 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala @@ -168,7 +168,9 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou Get(s"/api/termsOfService/v1/user/${defaultUser.id}/history") ~> mockSamRoutesBuilder.build.route ~> check { withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { - responseAs[String] shouldBe s"""{"history":[{"action":"${record1.action}","timestamp":"${record1.timestamp}","version":"${record1.version}"},{"action":"${record2.action}","timestamp":"${record2.timestamp}","version":"${record2.version}"}]}""" + responseAs[ + String + ] shouldBe s"""{"history":[{"action":"${record1.action}","timestamp":"${record1.timestamp}","version":"${record1.version}"},{"action":"${record2.action}","timestamp":"${record2.timestamp}","version":"${record2.version}"}]}""" responseAs[TermsOfServiceHistory] shouldBe TermsOfServiceHistory(List(record1, record2)) } status shouldEqual StatusCodes.OK @@ -199,7 +201,9 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou Get(s"/api/termsOfService/v1/user/self/history") ~> mockSamRoutesBuilder.build.route ~> check { withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceHistoryRecord`.") { - responseAs[String] shouldBe s"""{"history":[{"action":"${record1.action}","timestamp":"${record1.timestamp}","version":"${record1.version}"},{"action":"${record2.action}","timestamp":"${record2.timestamp}","version":"${record2.version}"}]}""" + responseAs[ + String + ] shouldBe s"""{"history":[{"action":"${record1.action}","timestamp":"${record1.timestamp}","version":"${record1.version}"},{"action":"${record2.action}","timestamp":"${record2.timestamp}","version":"${record2.version}"}]}""" responseAs[TermsOfServiceHistory] shouldBe TermsOfServiceHistory(List(record1, record2)) } status shouldEqual StatusCodes.OK 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 514fff9f9..85a4e4e60 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 @@ -283,7 +283,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 4 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -296,7 +297,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 7 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -309,7 +311,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(None)) // CASE 10 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -338,7 +341,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 5 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -351,7 +355,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 8 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -364,7 +369,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 11 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -393,7 +399,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 6 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -406,7 +413,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 9 - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -419,7 +427,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) // CASE 12 - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } @@ -449,7 +458,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowDisabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -462,7 +472,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodDisabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } @@ -475,7 +486,8 @@ class TosServiceSpec(_system: ActorSystem) when(dirDAO.getUserTermsOfServiceVersion(defaultUser.id, previousVersionOpt, samRequestContext)) .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, previousVersion, TosTable.ACCEPT, Instant.now())))) - val complianceStatus = tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() + val complianceStatus = + tosServiceV2GracePeriodEnabledAcceptanceWindowEnabled.getTermsOfServiceComplianceStatus(defaultUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe false } } From f23cbaf67cfb52f34bcd116cd81bd8e2477b99ef Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Thu, 16 Nov 2023 14:29:44 -0500 Subject: [PATCH 05/10] fix test --- .../sam/service/TosServiceSpec.scala | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) 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 85a4e4e60..42ab50353 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 @@ -21,7 +21,6 @@ import org.scalatest.{BeforeAndAfter, BeforeAndAfterAll, OptionValues} import java.time.Instant import scala.concurrent.ExecutionContext.Implicits.global -import scala.concurrent.Future class TosServiceSpec(_system: ActorSystem) extends TestKit(_system) @@ -515,13 +514,9 @@ class TosServiceSpec(_system: ActorSystem) .withAcceptedTermsOfServiceForUser(defaultUser, tosVersion) .build - val tosService = new TosService( - new NoExtensions { - override def isWorkbenchAdmin(memberEmail: WorkbenchEmail): Future[Boolean] = Future.successful(memberEmail == adminUser.email) - }, - directoryDao, - TestSupport.tosConfig - ) + val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withAdminUser().build + + val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig) // Act val userTosDetails: TermsOfServiceDetails = @@ -542,7 +537,9 @@ class TosServiceSpec(_system: ActorSystem) .withAcceptedTermsOfServiceForUser(defaultUser, tosVersion) .build - val tosService = new TosService(NoExtensions, directoryDao, TestSupport.tosConfig) + val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build + + val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig) // Act val userTosDetails: TermsOfServiceDetails = @@ -589,8 +586,9 @@ class TosServiceSpec(_system: ActorSystem) val directoryDao = new MockDirectoryDaoBuilder() .withTermsOfServiceHistoryForUser(defaultUser, List(record1, record2)) .build + val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withAdminUser().build - val tosService = new TosService(NoExtensions, directoryDao, TestSupport.tosConfig) + val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig) // Act val userTosDetails: TermsOfServiceHistory = @@ -611,7 +609,9 @@ class TosServiceSpec(_system: ActorSystem) .withTermsOfServiceHistoryForUser(defaultUser, List(userTos1, userTos2)) .build - val tosService = new TosService(NoExtensions, directoryDao, TestSupport.tosConfig) + val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build + + val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig) // Act val userTosDetails: TermsOfServiceHistory = From a4bdc6a42bab5be64d0f4db6e4f9cb8d10c103f3 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Mon, 20 Nov 2023 11:07:18 -0500 Subject: [PATCH 06/10] address kai comment --- .../sam/dataAccess/MockDirectoryDAO.scala | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) 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 e32ca5ee2..82e951611 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 @@ -22,8 +22,8 @@ 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 userTosHistory: mutable.Map[WorkbenchUserId, List[SamUserTos]] = new TrieMap() + private val userTermsOfService: mutable.Map[WorkbenchUserId, SamUserTos] = new TrieMap() + private val userTermsOfServiceHistory: mutable.Map[WorkbenchUserId, List[SamUserTos]] = new TrieMap() private val userAttributes: mutable.Map[WorkbenchUserId, SamUserAttributes] = new TrieMap() private val usersWithEmails: mutable.Map[WorkbenchEmail, WorkbenchUserId] = new TrieMap() @@ -313,9 +313,9 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench case None => false case Some(user) => users.put(userId, user) - userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) - val userHistory = userTosHistory.getOrElse(userId, List.empty) - userTosHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) + userTermsOfService.put(userId, SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) + val userHistory = userTermsOfServiceHistory.getOrElse(userId, List.empty) + userTermsOfServiceHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.ACCEPT, Instant.now())) true } @@ -324,9 +324,9 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench case None => false case Some(user) => users.put(userId, user) - userTos.put(userId, SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) - val userHistory = userTosHistory.getOrElse(userId, List.empty) - userTosHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) + userTermsOfService.put(userId, SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) + val userHistory = userTermsOfServiceHistory.getOrElse(userId, List.empty) + userTermsOfServiceHistory.put(userId, userHistory :+ SamUserTos(userId, tosVersion, TosTable.REJECT, Instant.now())) true } @@ -334,7 +334,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench loadUser(userId, samRequestContext).map { case None => None case Some(_) => - userTos.get(userId) + userTermsOfService.get(userId) } override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = @@ -342,7 +342,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench case None => None case Some(_) => tosVersion match { - case Some(_) => userTos.get(userId) + case Some(_) => userTermsOfService.get(userId) case None => None } } @@ -350,7 +350,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench override def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => List.empty - case Some(_) => userTosHistory.getOrElse(userId, List.empty) + case Some(_) => userTermsOfServiceHistory.getOrElse(userId, List.empty) } override def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] = { From 7bb8b6ce19d44ea82625a9b444c2352b9a7bdd49 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Mon, 27 Nov 2023 11:17:37 -0500 Subject: [PATCH 07/10] last touches --- .../workbench/sam/api/TermsOfServiceRoutes.scala | 9 +++++---- .../sam/dataAccess/PostgresDirectoryDAO.scala | 11 +++++------ .../dsde/workbench/sam/service/TosService.scala | 14 +++----------- .../workbench/sam/service/TosServiceSpec.scala | 12 ++++++------ 4 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala index 97770caa5..2f7f3bcc3 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala @@ -97,8 +97,7 @@ trait TermsOfServiceRoutes extends SamUserDirectives { complete(tosService.rejectCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent)) } } - } - } ~ + } ~ pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history pathEndOrSingleSlash { get { @@ -109,7 +108,8 @@ trait TermsOfServiceRoutes extends SamUserDirectives { } } } - } ~ + } + } ~ // The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's pathPrefix(Segment) { userId => // api/termsOfService/v1/user/{userId} validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") { @@ -126,7 +126,8 @@ trait TermsOfServiceRoutes extends SamUserDirectives { get { parameters("limit".as[Integer].withDefault(100)) { (limit: Int) => complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit)) - } } + } + } } } } 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 bf6aa634c..fd9ea6d00 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 @@ -649,11 +649,11 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } // When no tosVersion is specified, return the latest TosRecord for the user - override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = - getUserTosVersion(userId, None, samRequestContext) + override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + getUserTermsOfServiceVersion(userId, None, samRequestContext) - override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = - readOnlyTransaction("getUserTos", samRequestContext) { implicit session => + override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + readOnlyTransaction("getUserTermsOfService", samRequestContext) { implicit session => val tosTable = TosTable.syntax val column = TosTable.column @@ -669,10 +669,9 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val 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("getUserTosHistory", samRequestContext) { implicit session => + readOnlyTransaction("getUserTermsOfServiceHistory", samRequestContext) { implicit session => val tosTable = TosTable.syntax val column = TosTable.column 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 e39138797..b8fd53c8d 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 @@ -107,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) latestUserTermsOfService <- maybeTermsOfServiceRecord .map(IO.pure) .getOrElse( @@ -123,13 +123,6 @@ class TosService( case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find user:${userId}")) } - // Note: if version is None, then the query will return the last accepted ToS info for the user - private def loadTosRecordForUser(userId: WorkbenchUserId, version: Option[String], samRequestContext: SamRequestContext): IO[SamUserTos] = - directoryDao.getUserTermsOfServiceVersion(userId, version, samRequestContext).map { - case Some(samUserTos) => samUserTos - case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user:${userId}")) - } - def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] = ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) { directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map { @@ -144,9 +137,8 @@ class TosService( def getTermsOfServiceComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for { latestUserTos <- directoryDao.getUserTermsOfService(samUser.id, samRequestContext) - previousUserTos <- directoryDao.getUserTermsOfServiceVersion(samUser.id, tosConfig.previousVersion, samRequestContext) - userHasAcceptedLatestVersion = userHasAcceptedLatestTermsOfServiceVersion(latestUserTos) - permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos, previousUserTos) + userHasAcceptedLatestVersion = userHasAcceptedCurrentTermsOfService(latestUserTos) + permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos) } yield 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 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 291f22d67..a0688a640 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 @@ -151,11 +151,11 @@ class TosServiceSpec(_system: ActorSystem) val previousTosVersion = Option("1") val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig.copy(version = tosVersion, previousVersion = previousTosVersion)) - when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfService(serviceAccountUser.id, samRequestContext)).thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(serviceAccountUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfServiceVersion(serviceAccountUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) - val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosService.getTermsOfServiceComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } @@ -164,11 +164,11 @@ class TosServiceSpec(_system: ActorSystem) val previousTosVersion = Option("1") val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig.copy(version = tosVersion, previousVersion = previousTosVersion)) - when(dirDAO.getUserTos(uamiUser.id, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfService(uamiUser.id, samRequestContext)).thenReturn(IO.pure(None)) - when(dirDAO.getUserTosVersion(uamiUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) + when(dirDAO.getUserTermsOfServiceVersion(uamiUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None)) - val complianceStatus = tosService.getTosComplianceStatus(uamiUser, samRequestContext).unsafeRunSync() + val complianceStatus = tosService.getTermsOfServiceComplianceStatus(uamiUser, samRequestContext).unsafeRunSync() complianceStatus.permitsSystemUsage shouldBe true } } From dba05a708211c79fa60d4b830f172e3ae636c9ab Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Mon, 27 Nov 2023 14:18:38 -0500 Subject: [PATCH 08/10] Only return acceptances from `getTermsOfServiceDetailsForUser` --- .../sam/dataAccess/DirectoryDAO.scala | 9 ++++++-- .../sam/dataAccess/PostgresDirectoryDAO.scala | 22 ++++++++++++++----- .../workbench/sam/service/TosService.scala | 2 +- .../sam/dataAccess/MockDirectoryDAO.scala | 14 +++++++++--- .../dataAccess/MockDirectoryDaoBuilder.scala | 6 +++-- .../dataAccess/PostgresDirectoryDAOSpec.scala | 12 ++++++++++ 6 files changed, 52 insertions(+), 13 deletions(-) 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 4532e9ecd..5664bd166 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 @@ -76,8 +76,13 @@ trait DirectoryDAO { def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] - def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] - def getUserTermsOfServiceVersion(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]] 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 fd9ea6d00..502cd6e6f 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 @@ -649,20 +649,32 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } // When no tosVersion is specified, return the latest TosRecord for the user - override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = - getUserTermsOfServiceVersion(userId, None, samRequestContext) - - override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + 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 actionConstraint = action match { + case Some(a) => samsqls"and ${column.action} = ${a}" + case None => 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""" 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 b8fd53c8d..63171de0d 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 @@ -107,7 +107,7 @@ class TosService( } private def ensureLatestTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[SamUserTos] = for { - maybeTermsOfServiceRecord <- directoryDao.getUserTermsOfService(userId, samRequestContext) + maybeTermsOfServiceRecord <- directoryDao.getUserTermsOfService(userId, samRequestContext, Option(TosTable.ACCEPT)) latestUserTermsOfService <- maybeTermsOfServiceRecord .map(IO.pure) .getOrElse( 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 82e951611..e94a28668 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 @@ -330,14 +330,22 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench true } - override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext, action: Option[String]): IO[Option[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => None case Some(_) => - userTermsOfService.get(userId) + if (action.isDefined) { + userTermsOfService.get(userId).filter(_.action == action.get) + } else + userTermsOfService.get(userId) } - override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] = + override def getUserTermsOfServiceVersion( + userId: WorkbenchUserId, + tosVersion: Option[String], + samRequestContext: SamRequestContext, + action: Option[String] + ): IO[Option[SamUserTos]] = loadUser(userId, samRequestContext).map { case None => None case Some(_) => diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala index 6a622e5a3..ad8a09e40 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala @@ -116,8 +116,10 @@ case class MockDirectoryDaoBuilder() extends IdiomaticMockito { def withAcceptedTermsOfServiceForUser(samUser: SamUser, tosVersion: String): MockDirectoryDaoBuilder = { makeUserExist(samUser) val samUserTos = SamUserTos(samUser.id, tosVersion, TosTable.ACCEPT, Instant.now) - mockedDirectoryDAO.getUserTermsOfService(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos)) - mockedDirectoryDAO.getUserTermsOfServiceVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfService(eqTo(samUser.id), any[SamRequestContext], any[Option[String]]) returns IO(Option(samUserTos)) + mockedDirectoryDAO.getUserTermsOfServiceVersion(eqTo(samUser.id), eqTo(Some(tosVersion)), any[SamRequestContext], any[Option[String]]) returns IO( + Option(samUserTos) + ) this } 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 27624ef36..5c5933625 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 @@ -1664,6 +1664,18 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B val userTos = dao.getUserTermsOfService(user.id, samRequestContext).unsafeRunSync() userTos should be(None) } + "returns acceptances" in { + assume(databaseEnabled, databaseEnabledClue) + 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 + + // Assert + val userTos = dao.getUserTermsOfService(user.id, samRequestContext, action = Option(TosTable.ACCEPT)).unsafeRunSync() + userTos should be(Some(SamUserTos(user.id, tosConfig.version, TosTable.ACCEPT, Instant.now()))) + } } "checkStatus" - { From 2bc1439135967eb5e40161d082cb81804da1695a Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Tue, 28 Nov 2023 13:22:46 -0500 Subject: [PATCH 09/10] Address tlangs comments --- src/main/resources/swagger/api-docs.yaml | 12 +++--------- .../sam/dataAccess/PostgresDirectoryDAO.scala | 8 ++------ .../dsde/workbench/sam/service/TosService.scala | 3 +-- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 80637c446..74af23b20 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -3422,12 +3422,6 @@ paths: 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' /version: get: tags: @@ -4155,11 +4149,11 @@ components: type: object description: a user's terms of service action history properties: - latestAcceptedVersion: + history: type: array items: - $ref: '#/components/schemas/UserTermsOfServiceRecord' - UserTermsOfServiceRecord: + $ref: '#/components/schemas/UserTermsOfServiceHistoryRecord' + UserTermsOfServiceHistoryRecord: required: - action - version 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 502cd6e6f..c14639420 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 @@ -662,12 +662,8 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val val tosTable = TosTable.syntax val column = TosTable.column - val versionConstraint = if (tosVersion.isDefined) samsqls"and ${column.version} = ${tosVersion.get}" else samsqls"" - - val actionConstraint = action match { - case Some(a) => samsqls"and ${column.action} = ${a}" - case None => 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} 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 63171de0d..7459260e8 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 @@ -126,8 +126,7 @@ class TosService( 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 => - throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find any Terms of Service entries for user:${userId}")) + case samUserTosHistory if samUserTosHistory.isEmpty => TermsOfServiceHistory(List.empty) case samUserTosHistory => TermsOfServiceHistory( samUserTosHistory.map(historyRecord => TermsOfServiceHistoryRecord(historyRecord.action, historyRecord.version, historyRecord.createdAt)) From 522d37eb6778c0818358c777b0731fc7d813c01a Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Tue, 28 Nov 2023 14:54:08 -0500 Subject: [PATCH 10/10] post merge last touches --- .../dsde/workbench/sam/api/TermsOfServiceRoutes.scala | 3 ++- .../dsde/workbench/sam/service/UserService.scala | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala index 8146b5787..a6a4cc680 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala @@ -126,7 +126,8 @@ trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirec getWithTelemetry(samRequestContext, userIdParam(requestUserId)) { parameters("limit".as[Integer].withDefault(100)) { (limit: Int) => complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit)) - } } + } + } } } } 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 7c69f84d5..3cce2f6ef 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 @@ -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 @@ -367,7 +367,7 @@ class UserService( 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 }