Responding to trevyn's comments. Make tos config params nullable.

broadinstitute · Oct 18, 2023 · fec9f91 · fec9f91
1 parent 349d5d2
commit fec9f91
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 61 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,8 +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")),
-      config.getString("rollingAcceptanceWindowPreviousTosVersion")
+      config.as[Option[String]]("rollingAcceptanceWindowExpirationDatetime").map(Instant.parse),
+      config.as[Option[String]]("previousVersion")
     )
   }
 

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,7 +13,7 @@ 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
+  * @param previousVersion
   *   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.
   */
 
@@ -22,6 +22,6 @@ case class TermsOfServiceConfig(
     isGracePeriodEnabled: Boolean,
     version: String,
     baseUrl: String,
-    rollingAcceptanceWindowExpiration: Instant,
-    rollingAcceptanceWindowPreviousTosVersion: String
+    rollingAcceptanceWindowExpiration: Option[Instant],
+    previousVersion: Option[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
@@ -76,7 +76,7 @@ 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 getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
+  def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[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
@@ -664,7 +664,8 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       userTosRecordOpt.map(TosTable.unmarshalUserRecord)
     }
 
-  override def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getUserTosVersion(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
       val column = TosTable.column
@@ -679,6 +680,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       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 =>

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
@@ -53,17 +53,19 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
 
   def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
     latestUserTos <- directoryDao.getUserTos(samUser.id, samRequestContext)
-    previousUserTos <- directoryDao.getUserTos(samUser.id, tosConfig.rollingAcceptanceWindowPreviousTosVersion, samRequestContext)
+    previousUserTos <- directoryDao.getUserTosVersion(samUser.id, tosConfig.previousVersion, 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], 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
-
+    if (!tosConfig.isTosEnabled) {
+      return true
+    }
+    // Service Account users do not need to accept ToS
+    val userIsServiceAccount = StandardSamUserDirectives.SAdomain.matches(user.email.value)
     if (userIsServiceAccount) {
       return true
     }
@@ -73,12 +75,16 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
     userTos.exists { tos =>
       val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(Option(tos))
       val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT
-      val tosDisabled = !tosConfig.isTosEnabled
 
       val userHasAcceptedPreviousVersion = userHasAcceptedPreviousTosVersion(previousUserTos)
-      val userInsideOfRollingAcceptanceWindow = tosConfig.rollingAcceptanceWindowExpiration.isAfter(now) && userHasAcceptedPreviousVersion
+      val now = Instant.now()
+      val userInsideOfRollingAcceptanceWindow = tosConfig.rollingAcceptanceWindowExpiration match {
+        case Some(expiration) =>
+          expiration.isAfter(now) && userHasAcceptedPreviousVersion
+        case None => false
+      }
 
-      userHasAcceptedLatestVersion || userInsideOfRollingAcceptanceWindow || userCanUseSystemUnderGracePeriod || tosDisabled
+      userHasAcceptedLatestVersion || userInsideOfRollingAcceptanceWindow || userCanUseSystemUnderGracePeriod
 
     }
   }

diff --git a/src/test/resources/reference.conf b/src/test/resources/reference.conf
@@ -63,7 +63,7 @@ termsOfService {
   version = 1
   url = "app.terra.bio/#terms-of-service"
   rollingAcceptanceWindowExpirationDatetime = "2019-01-01T00:00:00Z"
-  rollingAcceptanceWindowPreviousTosVersion = 0
+  previousVersion = 0
 }
 
 petServiceAccount {

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala
@@ -50,7 +50,7 @@ class TestSamRoutes(
       userService,
       statusService,
       managedGroupService,
-      TermsOfServiceConfig(true, false, "1", "app.terra.bio/#terms-of-service", Instant.now(), "0"),
+      TermsOfServiceConfig(true, false, "1", "app.terra.bio/#terms-of-service", Option(Instant.now()), Option("0")),
       policyEvaluatorService,
       tosService,
       LiquibaseConfig("", false),
@@ -92,7 +92,7 @@ class TestSamTosEnabledRoutes(
       userService,
       statusService,
       managedGroupService,
-      TermsOfServiceConfig(true, false, "1", "app.terra.bio/#terms-of-service", Instant.now(), "0"),
+      TermsOfServiceConfig(true, false, "1", "app.terra.bio/#terms-of-service", Option(Instant.now()), Option("0")),
       policyEvaluatorService,
       tosService,
       LiquibaseConfig("", false),

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
@@ -347,11 +347,14 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
         userTos.get(userId)
     }
 
-  override def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
     loadUser(userId, samRequestContext).map {
       case None => None
       case Some(_) =>
-        userTos.get(userId)
+        tosVersion match {
+          case Some(_) => userTos.get(userId)
+          case None => None
+        }
     }
 
   override def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity] = {