ID-813 Add TOS rolling acceptance window.

broadinstitute · Oct 13, 2023 · 7273a5a · 7273a5a
1 parent 64f8223
commit 7273a5a
Show file tree

Hide file tree

Showing 8 changed files with 61 additions and 34 deletions.
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala
@@ -122,7 +122,8 @@ object AppConfig {
       config.getString("version"),
       config.getString("baseUrl"),
       // Must be a valid UTC datetime string in ISO 8601 format ex: 2007-12-03T10:15:30.00Z
-      Instant.parse(config.getString("rollingAcceptanceWindowExpirationDatetime"))
+      Instant.parse(config.getString("rollingAcceptanceWindowExpirationDatetime")),
+      config.getString("rollingAcceptanceWindowPreviousTosVersion")
     )
   }
 

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/TermsOfServiceConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/TermsOfServiceConfig.scala
@@ -13,6 +13,10 @@ import java.time.Instant
   *  The expiration time for the rolling acceptance window. If the user has not accepted the new ToS by this time,
   *  they will be denied access to the system. Must be a valid UTC datetime string in ISO 8601 format
   *  example: 2007-12-03T10:15:30.00Z
+  *
+  *  @param rollingAcceptanceWindowPreviousTosVersion
+  *  The previous version of the ToS that the user must have accepted in order to be granted access to the system under
+  *  the rolling window.
   */
 
-case class TermsOfServiceConfig(isTosEnabled: Boolean, isGracePeriodEnabled: Boolean, version: String, baseUrl: String, rollingAcceptanceWindowExpiration: Instant)
+case class TermsOfServiceConfig(isTosEnabled: Boolean, isGracePeriodEnabled: Boolean, version: String, baseUrl: String, rollingAcceptanceWindowExpiration: Instant, rollingAcceptanceWindowPreviousTosVersion: String)
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
@@ -75,7 +75,8 @@ 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 getLatestUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
+  def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
 
   def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity]
   def loadPetManagedIdentity(petManagedIdentityId: PetManagedIdentityId, samRequestContext: SamRequestContext): IO[Option[PetManagedIdentity]]

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala
@@ -647,8 +647,8 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
     }
   }
 
-  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
-    readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
+  override def getLatestUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+    readOnlyTransaction("getLatestUserTos", samRequestContext) { implicit session =>
       val tosTable = TosTable.syntax
       val column = TosTable.column
 
@@ -663,6 +663,22 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       userTosRecordOpt.map(TosTable.unmarshalUserRecord)
     }
 
+  override def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+    readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
+      val tosTable = TosTable.syntax
+      val column = TosTable.column
+
+      val loadUserTosQuery =
+        samsql"""select ${tosTable.resultAll}
+              from ${TosTable as tosTable}
+              where ${column.samUserId} = ${userId} and ${column.version} = ${tosVersion}
+              order by ${column.createdAt} desc
+              limit 1"""
+
+      val userTosRecordOpt: Option[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).first().apply()
+      userTosRecordOpt.map(TosTable.unmarshalUserRecord)
+    }
+
   override def isEnabled(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Boolean] =
     readOnlyTransaction("isEnabled", samRequestContext) { implicit session =>
       val userIdOpt = subject match {

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
@@ -47,27 +47,29 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
 
   @Deprecated
   def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceDetails] =
-    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
+    directoryDao.getLatestUserTos(samUser.id, samRequestContext).map { tos =>
       TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
     }
 
-  def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] =
-    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
-      val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(tos)
-      val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, tos)
-      TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)
-    }
+  def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
+    latestUserTos <-  directoryDao.getLatestUserTos(samUser.id, samRequestContext)
+    previousUserTos <-  directoryDao.getUserTos(samUser.id, tosConfig.rollingAcceptanceWindowPreviousTosVersion, samRequestContext)
+    userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(latestUserTos)
+    permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos, previousUserTos)
+  } 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
     */
-  private def tosAcceptancePermitsSystemUsage(user: SamUser, userTos: Option[SamUserTos]): Boolean = {
+  private def tosAcceptancePermitsSystemUsage(user: SamUser, userTos: Option[SamUserTos], previousUserTos: Option[SamUserTos]): Boolean = {
     val now = Instant.now()
     val userIsServiceAccount = StandardSamUserDirectives.SAdomain.matches(user.email.value) // Service Account users do not need to accept ToS
     val userIsPermitted = userTos.exists { tos =>
       val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(Option(tos))
       val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT
       val tosDisabled = !tosConfig.isTosEnabled
-      val userInsideOfRollingAcceptanceWindow = tosConfig.rollingAcceptanceWindowExpiration.isAfter(now)
+
+      val userHasAcceptedPreviousVersion = userHasAcceptedPreviousTosVersion(previousUserTos)
+      val userInsideOfRollingAcceptanceWindow = tosConfig.rollingAcceptanceWindowExpiration.isAfter(now) && userHasAcceptedPreviousVersion
 
       userHasAcceptedLatestVersion || userInsideOfRollingAcceptanceWindow || userCanUseSystemUnderGracePeriod || tosDisabled
 
@@ -79,6 +81,9 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
     userTos.exists { tos =>
       tos.version.contains(tosConfig.version) && tos.action == TosTable.ACCEPT
     }
+
+  private def userHasAcceptedPreviousTosVersion(previousUserTos: Option[SamUserTos]): Boolean =
+    previousUserTos.exists(tos => tos.action == TosTable.ACCEPT)
 }
 
 trait TermsOfServiceDocument {

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
@@ -340,7 +340,7 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
         true
     }
 
-  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getLatestUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
     loadUser(userId, samRequestContext).map {
       case None => None
       case Some(_) =>

diff --git a/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala
@@ -1486,7 +1486,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.getLatestUserTos(defaultUser.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.ACCEPT
@@ -1501,7 +1501,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.getLatestUserTos(defaultUser.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.ACCEPT
@@ -1516,7 +1516,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.getLatestUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.REJECT
@@ -1530,7 +1530,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.getLatestUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.REJECT
@@ -1544,7 +1544,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.getLatestUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should be(None)
       }
     }

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
@@ -87,7 +87,7 @@ class TosServiceSpec(_system: ActorSystem)
       val tosVersion = "2"
       val tosService =
         new TosService(dirDAO, TestSupport.tosConfig.copy(version = tosVersion))
-      when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext))
+      when(dirDAO.getLatestUserTos(serviceAccountUser.id, samRequestContext))
         .thenReturn(IO.pure(Some(SamUserTos(serviceAccountUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
       val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
       complianceStatus.permitsSystemUsage shouldBe true
@@ -120,14 +120,14 @@ class TosServiceSpec(_system: ActorSystem)
 
     "when the user has not accepted any ToS version" - {
       "says the user has not accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+        when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 1
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -136,7 +136,7 @@ class TosServiceSpec(_system: ActorSystem)
       }
       withGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 4
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -146,14 +146,14 @@ class TosServiceSpec(_system: ActorSystem)
     }
     "when the user has accepted a non-current ToS version" - {
       "says the user has not accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+        when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 2
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -162,7 +162,7 @@ class TosServiceSpec(_system: ActorSystem)
       }
       withGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 5
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -172,14 +172,14 @@ class TosServiceSpec(_system: ActorSystem)
     }
     "when the user has accepted the current ToS version" - {
       "says the user has accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+        when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe true
       }
       withoutGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 3
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -188,7 +188,7 @@ class TosServiceSpec(_system: ActorSystem)
       }
       withGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 6
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -199,14 +199,14 @@ class TosServiceSpec(_system: ActorSystem)
 
     "when the user has rejected the latest ToS version" - {
       "says the user has rejected the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+        when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
           // CASE 1
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -215,7 +215,7 @@ class TosServiceSpec(_system: ActorSystem)
       }
       withGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
+          when(dirDAO.getLatestUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
           // CASE 4
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -225,7 +225,7 @@ 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.getLatestUserTos(serviceAccountUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
         complianceStatus.permitsSystemUsage shouldBe true