Moved isAdmin check into TosService

pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala

@@ -142,7 +142,7 @@ object MockTestSupport extends MockTestSupport {
     )
     val mockManagedGroupService =
       new ManagedGroupService(mockResourceService, policyEvaluatorService, resourceTypes, policyDAO, directoryDAO, googleExt, "example.com")
-    val tosService = new TosService(directoryDAO, tosConfig)
+    val tosService = new TosService(googleExt, directoryDAO, tosConfig)
     val azureService = new AzureService(MockCrlService(), directoryDAO, new MockAzureManagedResourceGroupDAO)
     MockSamDependencies(
       mockResourceService,
@@ -170,7 +170,7 @@ object MockTestSupport extends MockTestSupport {
     samDependencies.userService,
     samDependencies.statusService,
     samDependencies.managedGroupService,
-    samDependencies.tosService.tosConfig,
+    samDependencies.tosService.termsOfServiceConfig,
     samDependencies.directoryDAO,
     samDependencies.policyEvaluatorService,
     samDependencies.tosService,

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -372,7 +372,7 @@ object Boot extends IOApp with LazyLogging {
       config.emailDomain,
       config.adminConfig.allowedEmailDomains
     )
-    val tosService = new TosService(directoryDAO, config.termsOfServiceConfig)
+    val tosService = new TosService(cloudExtensionsInitializer.cloudExtensions, directoryDAO, config.termsOfServiceConfig)
     val userService = new UserService(directoryDAO, cloudExtensionsInitializer.cloudExtensions, config.blockedEmailDomains, tosService)
     val statusService =
       new StatusService(directoryDAO, cloudExtensionsInitializer.cloudExtensions, 10 seconds, 1 minute)

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutes.scala

@@ -8,7 +8,7 @@ import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server.{Directive0, ExceptionHandler}
 import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
-import org.broadinstitute.dsde.workbench.sam.service.UserService
+import org.broadinstitute.dsde.workbench.sam.service.{TosService, UserService}
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import spray.json.JsBoolean
 
@@ -19,6 +19,7 @@ import scala.concurrent.ExecutionContext
 trait OldUserRoutes extends SamUserDirectives with SamRequestContextDirectives {
   implicit val executionContext: ExecutionContext
   val userService: UserService
+  val tosService: TosService
 
   /** Changes a 403 error to a 404 error. Used when `UserInfoDirectives` throws a 403 in the case where a user is not found. In most routes that is appropriate
     * but in the user routes it should be a 404.

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamUserDirectives.scala

@@ -50,13 +50,6 @@ trait SamUserDirectives {
       }
     }
 
-  // Ideally, we would just make this check from inside the *Service.scala code, but not all Services have access to
-  // cloudExtensions and I don't think we should add cloudExtensions just for checking if a user is an admin.  So this
-  // was added so we can do the "isAdmin calculation" in the routes, just like we've always done it, but then pass this
-  // data into the Services to let them make their own authz determination.  If we can change the way we define admins
-  // from _not_ depending on Google, then we may be able to get rid of this directive.
-  def isWorkbenchAdmin(samUser: SamUser): Directive1[Boolean]
-
   def asAdminServiceUser: Directive0
 
   val oldTermsOfServiceUrl = "app.terra.bio/#terms-of-service"

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala

@@ -64,9 +64,6 @@ trait StandardSamUserDirectives extends SamUserDirectives with LazyLogging with
     SamUser(genWorkbenchUserId(System.currentTimeMillis()), googleSubjectId, oidcHeaders.email, azureB2CId, false)
   }
 
-  override def isWorkbenchAdmin(samUser: SamUser): Directive1[Boolean] =
-    onSuccess(cloudExtensions.isWorkbenchAdmin(samUser.email))
-
   /** Utility function that knows how to convert all the various headers into OIDCHeaders
     */
   private def requireOidcHeaders: Directive1[OIDCHeaders] =

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala

@@ -5,10 +5,10 @@ import akka.http.scaladsl.model.StatusCodes
 import akka.http.scaladsl.server
 import akka.http.scaladsl.server.Directives.{pathPrefix, _}
 import org.broadinstitute.dsde.workbench.model.WorkbenchUserId
+import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
 import org.broadinstitute.dsde.workbench.sam.model.api.SamUser
 import org.broadinstitute.dsde.workbench.sam.service.TosService
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
-import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
 
 import scala.concurrent.ExecutionContext
 import scala.util.matching.Regex
@@ -72,7 +72,7 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
             pathEndOrSingleSlash {
               get {
                 complete {
-                  tosService.getTermsOfServiceDetailsForUser(samUser.id, samUser, isAdmin = false, samRequestContext)
+                  tosService.getTermsOfServiceDetailsForUser(samUser.id, samUser, samRequestContext)
                 }
               }
             } ~
@@ -96,11 +96,9 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
             validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") {
               val requestUserId = WorkbenchUserId(userId)
               pathEndOrSingleSlash {
-                isWorkbenchAdmin(samUser) { isAdmin =>
-                  get {
-                    complete {
-                      tosService.getTermsOfServiceDetailsForUser(requestUserId, samUser, isAdmin, samRequestContext)
-                    }
+                get {
+                  complete {
+                    tosService.getTermsOfServiceDetailsForUser(requestUserId, samUser, samRequestContext)
                   }
                 }
               } ~

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala

@@ -24,64 +24,77 @@ import scala.concurrent.duration.Duration
 import scala.concurrent.{Await, ExecutionContext}
 import scala.io.Source
 
-class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceConfig)(
+class TosService(
+    val cloudExtensions: CloudExtensions,
+    directoryDao: DirectoryDAO,
+    val termsOfServiceConfig: TermsOfServiceConfig
+)(
     implicit val executionContext: ExecutionContext,
     implicit val actorSystem: ActorSystem
 ) extends LazyLogging {
 
-  private val termsOfServiceUri = s"${tosConfig.baseUrl}/${tosConfig.version}/termsOfService.md"
-  private val privacyPolicyUri = s"${tosConfig.baseUrl}/${tosConfig.version}/privacyPolicy.md"
+  private val termsOfServiceUri = s"${termsOfServiceConfig.baseUrl}/${termsOfServiceConfig.version}/termsOfService.md"
+  private val privacyPolicyUri = s"${termsOfServiceConfig.baseUrl}/${termsOfServiceConfig.version}/privacyPolicy.md"
 
   val termsOfServiceText: String = TermsOfServiceDocument(termsOfServiceUri)
   val privacyPolicyText: String = TermsOfServiceDocument(privacyPolicyUri)
 
   def acceptTosStatus(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] =
     directoryDao
-      .acceptTermsOfService(userId, tosConfig.version, samRequestContext)
-      .withInfoLogMessage(s"$userId has accepted version ${tosConfig.version} of the Terms of Service")
+      .acceptTermsOfService(userId, termsOfServiceConfig.version, samRequestContext)
+      .withInfoLogMessage(s"$userId has accepted version ${termsOfServiceConfig.version} of the Terms of Service")
 
   def rejectTosStatus(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] =
     directoryDao
-      .rejectTermsOfService(userId, tosConfig.version, samRequestContext)
-      .withInfoLogMessage(s"$userId has rejected version ${tosConfig.version} of the Terms of Service")
+      .rejectTermsOfService(userId, termsOfServiceConfig.version, samRequestContext)
+      .withInfoLogMessage(s"$userId has rejected version ${termsOfServiceConfig.version} of the Terms of Service")
 
   def getTosConfig(): IO[TermsOfServiceConfigResponse] =
     IO.pure(
       TermsOfServiceConfigResponse(
-        enforced = tosConfig.isTosEnabled,
-        currentVersion = tosConfig.version,
-        inGracePeriod = tosConfig.isGracePeriodEnabled,
+        enforced = termsOfServiceConfig.isTosEnabled,
+        currentVersion = termsOfServiceConfig.version,
+        inGracePeriod = termsOfServiceConfig.isGracePeriodEnabled,
         inRollingAcceptanceWindow = isRollingWindowInEffect()
       )
     )
 
-  private def isRollingWindowInEffect() = tosConfig.rollingAcceptanceWindowExpiration.exists(Instant.now().isBefore(_))
+  private def isRollingWindowInEffect() = termsOfServiceConfig.rollingAcceptanceWindowExpiration.exists(Instant.now().isBefore(_))
 
   @Deprecated
   def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[OldTermsOfServiceDetails] =
     directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
-      OldTermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
+      OldTermsOfServiceDetails(isEnabled = true, termsOfServiceConfig.isGracePeriodEnabled, termsOfServiceConfig.version, tos.map(_.version))
     }
 
   def getTermsOfServiceDetailsForUser(
       requestedUserId: WorkbenchUserId,
       requestingUser: SamUser,
-      isAdmin: Boolean,
       samRequestContext: SamRequestContext
-  ): IO[TermsOfServiceDetails] =
-    if (isAdmin || (requestedUserId == requestingUser.id)) {
+  ): IO[TermsOfServiceDetails] = {
+    def loadUserTosDetails =
       for {
-        currentSamUserTos <- loadTosRecordForUser(requestedUserId, Option(tosConfig.version), samRequestContext)
-        previousSamUserTos <- loadTosRecordForUser(requestedUserId, tosConfig.previousVersion, samRequestContext)
+        currentSamUserTos <- loadTosRecordForUser(requestedUserId, Option(termsOfServiceConfig.version), samRequestContext)
+        previousSamUserTos <- loadTosRecordForUser(requestedUserId, termsOfServiceConfig.previousVersion, samRequestContext)
         requestedUser <- loadUser(requestedUserId, samRequestContext)
       } yield TermsOfServiceDetails(
         currentSamUserTos.version,
         currentSamUserTos.createdAt,
         tosAcceptancePermitsSystemUsage(requestedUser, Option(currentSamUserTos), Option(previousSamUserTos))
       )
+
+    if (requestedUserId != requestingUser.id) {
+      IO.fromFuture(IO(cloudExtensions.isWorkbenchAdmin(requestingUser.email))).flatMap { isAdmin =>
+        if (isAdmin) {
+          loadUserTosDetails
+        } else {
+          IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Unauthorized, "You are not allowed to make this request")))
+        }
+      }
     } else {
-      IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Unauthorized, "You are not allowed to make this request")))
+      loadUserTosDetails
     }
+  }
 
   private def loadUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[SamUser] =
     directoryDao.loadUser(userId, samRequestContext).map {
@@ -98,15 +111,15 @@ 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.getUserTosVersion(samUser.id, tosConfig.previousVersion, samRequestContext)
+    previousUserTos <- directoryDao.getUserTosVersion(samUser.id, termsOfServiceConfig.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 = {
-    if (!tosConfig.isTosEnabled) {
+    if (!termsOfServiceConfig.isTosEnabled) {
       return true
     }
     // Service Account users do not need to accept ToS
@@ -119,7 +132,7 @@ 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 userCanUseSystemUnderGracePeriod = termsOfServiceConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT
 
       val userHasAcceptedPreviousVersion = userHasAcceptedPreviousTosVersion(previousUserTos)
       val userInsideOfRollingAcceptanceWindow = isRollingWindowInEffect() && userHasAcceptedPreviousVersion
@@ -131,12 +144,12 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
 
   private def userHasAcceptedLatestTosVersion(userTos: Option[SamUserTos]): Boolean =
     userTos.exists { tos =>
-      tos.version.contains(tosConfig.version) && tos.action == TosTable.ACCEPT
+      tos.version.contains(termsOfServiceConfig.version) && tos.action == TosTable.ACCEPT
     }
 
   private def userHasRejectedLatestTosVersion(userTos: Option[SamUserTos]): Boolean =
     userTos.exists { tos =>
-      tos.version.contains(tosConfig.version) && tos.action == TosTable.REJECT
+      tos.version.contains(termsOfServiceConfig.version) && tos.action == TosTable.REJECT
     }
 
   private def userHasAcceptedPreviousTosVersion(previousUserTos: Option[SamUserTos]): Boolean =

src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala

@@ -150,7 +150,7 @@ object TestSupport extends TestSupport {
     )
     val mockManagedGroupService =
       new ManagedGroupService(mockResourceService, policyEvaluatorService, resourceTypes, policyDAO, directoryDAO, googleExt, "example.com")
-    val tosService = new TosService(directoryDAO, tosConfig)
+    val tosService = new TosService(googleExt, directoryDAO, tosConfig)
     val azureService = new AzureService(MockCrlService(), directoryDAO, new MockAzureManagedResourceGroupDAO)
     SamDependencies(
       mockResourceService,
@@ -179,7 +179,7 @@ object TestSupport extends TestSupport {
     samDependencies.userService,
     samDependencies.statusService,
     samDependencies.managedGroupService,
-    samDependencies.tosService.tosConfig,
+    samDependencies.tosService.termsOfServiceConfig,
     samDependencies.policyEvaluatorService,
     samDependencies.tosService,
     LiquibaseConfig("", false),

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/AdminResourceTypesRoutesSpec.scala

@@ -69,7 +69,7 @@ class AdminResourceTypesRoutesSpec extends AnyFlatSpec with Matchers with TestSu
 
     val cloudExtensions = SamSuperAdminExtensions(isSamSuperAdmin)
 
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(cloudExtensions, directoryDAO, TestSupport.tosConfig)
     val mockUserService = new UserService(directoryDAO, cloudExtensions, Seq.empty, tosService)
     val mockStatusService = new StatusService(directoryDAO, cloudExtensions)
     val mockManagedGroupService =

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutesBuilder.scala

@@ -1,7 +1,7 @@
 package org.broadinstitute.dsde.workbench.sam.api
 
 import akka.actor.ActorSystem
-import akka.http.scaladsl.server.Directives.{onSuccess, provide, reject}
+import akka.http.scaladsl.server.Directives.{onSuccess, reject}
 import akka.http.scaladsl.server._
 import akka.stream.Materializer
 import cats.effect.IO
@@ -158,7 +158,6 @@ class MockSamRoutesBuilder(allUsersGroup: WorkbenchGroup)(implicit system: Actor
       override def asAdminServiceUser: Directive0 =
         if (asServiceAdminUser) Directive.Empty else reject(AuthorizationFailedRejection)
 
-      override def isWorkbenchAdmin(samUser: SamUser): Directive1[Boolean] = provide(asServiceAdminUser)
     }
   }
 }

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamUserDirectives.scala

@@ -30,5 +30,4 @@ trait MockSamUserDirectives extends SamUserDirectives {
     case Some(u) => provide(u)
   }
 
-  override def isWorkbenchAdmin(samUser: SamUser): Directive1[Boolean] = provide(false)
 }

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutesSpec.scala

@@ -86,7 +86,7 @@ trait OldUserRoutesSpecHelper extends AnyFlatSpec with Matchers with ScalatestRo
   def withDefaultRoutes[T](testCode: TestSamRoutes => T): T = {
     val directoryDAO = new MockDirectoryDAO()
 
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val samRoutes = new TestSamRoutes(
       null,
       null,

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutesV1Spec.scala

@@ -19,7 +19,7 @@ class OldUserRoutesV1Spec extends OldUserRoutesSpecHelper {
   def withSARoutes[T](testCode: (TestSamRoutes, TestSamRoutes) => T): T = {
     val directoryDAO = new MockDirectoryDAO()
 
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val samRoutes = new TestSamRoutes(
       null,
       null,

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/OldUserRoutesV2Spec.scala

@@ -16,7 +16,7 @@ class OldUserRoutesV2Spec extends OldUserRoutesSpecHelper {
   def withSARoutes[T](testCode: (TestSamRoutes, TestSamRoutes) => T): T = {
     val directoryDAO = new MockDirectoryDAO()
 
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val samRoutes = new TestSamRoutes(
       null,
       null,

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesSpec.scala

@@ -48,7 +48,7 @@ class ResourceRoutesSpec extends RetryableAnyFlatSpec with Matchers with Scalate
     val emailDomain = "example.com"
     val policyEvaluatorService = PolicyEvaluatorService(emailDomain, resourceTypes, accessPolicyDAO, directoryDAO)
     val mockResourceService = new ResourceService(resourceTypes, policyEvaluatorService, accessPolicyDAO, directoryDAO, NoExtensions, emailDomain, Set.empty)
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val mockUserService = new UserService(directoryDAO, NoExtensions, Seq.empty, tosService)
     val mockStatusService = new StatusService(directoryDAO, NoExtensions)
     val mockManagedGroupService =

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala

@@ -53,7 +53,7 @@ class ResourceRoutesV2Spec extends RetryableAnyFlatSpec with Matchers with TestS
     resourceTypes.map { case (resourceTypeName, resourceType) =>
       when(mockResourceService.getResourceType(resourceTypeName)).thenReturn(IO(Option(resourceType)))
     }
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val mockUserService = new UserService(directoryDAO, NoExtensions, Seq.empty, tosService)
     val mockStatusService = new StatusService(directoryDAO, NoExtensions)
     val mockManagedGroupService =

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectivesSpec.scala

@@ -35,7 +35,7 @@ class StandardSamUserDirectivesSpec extends AnyFlatSpec with PropertyBasedTestin
       override implicit val executionContext: ExecutionContext = null
       override val cloudExtensions: CloudExtensions = null
       override val termsOfServiceConfig: TermsOfServiceConfig = null
-      override val tosService: TosService = new TosService(dirDAO, tosConfig)
+      override val tosService: TosService = new TosService(cloudExtensions, dirDAO, tosConfig)
       override val userService: UserService = new MockUserService(directoryDAO = dirDAO, tosService = tosService)
       override val adminConfig: AppConfig.AdminConfig = testAdminConfig
 

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StatusRouteSpec.scala

@@ -45,7 +45,7 @@ class StatusRouteSpec extends RetryableAnyFlatSpec with Matchers with ScalatestR
 
     val emailDomain = "example.com"
     val mockResourceService = new ResourceService(Map.empty, null, policyDAO, directoryDAO, NoExtensions, emailDomain, Set.empty)
-    val tosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, TestSupport.tosConfig)
     val mockUserService = new UserService(directoryDAO, NoExtensions, Seq.empty, tosService)
     val mockStatusService = new StatusService(directoryDAO, NoExtensions)
     val mockManagedGroupService = new ManagedGroupService(mockResourceService, null, Map.empty, policyDAO, directoryDAO, NoExtensions, emailDomain)