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,

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,7 +24,11 @@ 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 tosConfig: TermsOfServiceConfig
+)(
     implicit val executionContext: ExecutionContext,
     implicit val actorSystem: ActorSystem
 ) extends LazyLogging {
@@ -66,10 +70,9 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
   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)
@@ -79,9 +82,19 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
         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 {

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,

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)

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

@@ -192,7 +192,7 @@ object TestSamRoutes {
       emailDomain,
       allowedAdminEmailDomains = adminEmailDomains.getOrElse(Set.empty)
     )
-    val mockTosService = new TosService(directoryDAO, TestSupport.tosConfig)
+    val mockTosService = new TosService(cloudXtns, directoryDAO, TestSupport.tosConfig)
     val mockUserService = new UserService(directoryDAO, cloudXtns, Seq.empty, mockTosService)
     val mockManagedGroupService =
       new ManagedGroupService(mockResourceService, policyEvaluatorService, resourceTypesWithAdmin, policyDAO, directoryDAO, cloudXtns, emailDomain)

src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala

@@ -41,7 +41,7 @@ class AzureServiceSpec(_system: ActorSystem) extends TestKit(_system) with AnyFl
     // create dependencies
     val directoryDAO = new PostgresDirectoryDAO(dbRef, dbRef)
     val crlService = new CrlService(azureServicesConfig.get)
-    val tosService = new TosService(directoryDAO, tosConfig)
+    val tosService = new TosService(NoExtensions, directoryDAO, tosConfig)
     val userService = new UserService(directoryDAO, NoExtensions, Seq.empty, tosService)
     val azureTestConfig = config.getConfig("testStuff.azure")
     val azureService = new AzureService(crlService, directoryDAO, new MockAzureManagedResourceGroupDAO)

src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAOSpec.scala

@@ -65,7 +65,7 @@ class MockAccessPolicyDAOSpec(_system: ActorSystem)
     val policyEvaluatorService = PolicyEvaluatorService(shared.emailDomain, shared.resourceTypes, ldapPolicyDao, ldapDirDao)
     val resourceService =
       new ResourceService(shared.resourceTypes, policyEvaluatorService, ldapPolicyDao, ldapDirDao, NoExtensions, shared.emailDomain, Set.empty)
-    val userService = new UserService(ldapDirDao, NoExtensions, Seq.empty, new TosService(ldapDirDao, TestSupport.tosConfig))
+    val userService = new UserService(ldapDirDao, NoExtensions, Seq.empty, new TosService(NoExtensions, ldapDirDao, TestSupport.tosConfig))
     val managedGroupService =
       new ManagedGroupService(resourceService, policyEvaluatorService, shared.resourceTypes, ldapPolicyDao, ldapDirDao, NoExtensions, shared.emailDomain)
     shared.resourceTypes foreach { case (_, resourceType) => resourceService.createResourceType(resourceType, samRequestContext).unsafeRunSync() }
@@ -81,7 +81,7 @@ class MockAccessPolicyDAOSpec(_system: ActorSystem)
     val resourceService =
       new ResourceService(shared.resourceTypes, policyEvaluatorService, mockPolicyDAO, mockDirectoryDAO, NoExtensions, shared.emailDomain, Set.empty)
     val userService =
-      new UserService(mockDirectoryDAO, NoExtensions, Seq.empty, new TosService(mockDirectoryDAO, TestSupport.tosConfig))
+      new UserService(mockDirectoryDAO, NoExtensions, Seq.empty, new TosService(NoExtensions, mockDirectoryDAO, TestSupport.tosConfig))
     val managedGroupService =
       new ManagedGroupService(resourceService, policyEvaluatorService, shared.resourceTypes, mockPolicyDAO, mockDirectoryDAO, NoExtensions, shared.emailDomain)
   }

src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala

@@ -490,7 +490,7 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca
           samDeps.userService,
           samDeps.resourceService,
           samDeps.statusService,
-          new TosService(samDeps.directoryDAO, TestSupport.tosConfig)
+          new TosService(samDeps.cloudExtensions, samDeps.directoryDAO, TestSupport.tosConfig)
         )
       )
       .unsafeRunSync()
@@ -514,7 +514,7 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca
           samDeps.userService,
           samDeps.resourceService,
           samDeps.statusService,
-          new TosService(samDeps.directoryDAO, TestSupport.tosConfig)
+          new TosService(samDeps.cloudExtensions, samDeps.directoryDAO, TestSupport.tosConfig)
         )
       )
       .unsafeRunSync()

src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala

@@ -462,7 +462,7 @@ class GoogleExtensionSpec(_system: ActorSystem)
       configResourceTypes,
       superAdminsGroup
     )
-    val tosService = new TosService(dirDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig)
     val service = new UserService(dirDAO, googleExtensions, Seq.empty, tosService)
 
     val defaultUser = Generator.genWorkbenchUserGoogle.sample.get
@@ -865,10 +865,10 @@ class GoogleExtensionSpec(_system: ActorSystem)
     )
 
     val app = SamApplication(
-      new UserService(mockDirectoryDAO, ge, Seq.empty, new TosService(mockDirectoryDAO, TestSupport.tosConfig)),
+      new UserService(mockDirectoryDAO, ge, Seq.empty, new TosService(NoExtensions, mockDirectoryDAO, TestSupport.tosConfig)),
       new ResourceService(configResourceTypes, null, mockAccessPolicyDAO, mockDirectoryDAO, ge, "example.com", Set.empty),
       null,
-      new TosService(mockDirectoryDAO, TestSupport.tosConfig)
+      new TosService(NoExtensions, mockDirectoryDAO, TestSupport.tosConfig)
     )
     val resourceAndPolicyName =
       FullyQualifiedPolicyId(FullyQualifiedResourceId(CloudExtensions.resourceTypeName, GoogleExtensions.resourceId), AccessPolicyName("owner"))
@@ -1237,7 +1237,7 @@ class GoogleExtensionSpec(_system: ActorSystem)
       configResourceTypes,
       superAdminsGroup
     )
-    val tosService = new TosService(dirDAO, TestSupport.tosConfig)
+    val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig)
     val service = new UserService(dirDAO, googleExtensions, Seq.empty, tosService)
 
     (googleExtensions, service, tosService)

src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala

@@ -44,7 +44,7 @@ case class MockTosServiceBuilder() extends MockitoSugar {
     lenient()
       .doReturn(IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"")(new ErrorReportSource("MockTosServiceBuilder")))))
       .when(tosService)
-      .getTermsOfServiceDetailsForUser(any[WorkbenchUserId], any[SamUser], any[Boolean], any[SamRequestContext])
+      .getTermsOfServiceDetailsForUser(any[WorkbenchUserId], any[SamUser], any[SamRequestContext])
   }
 
   private def setAcceptedStateForUserTo(samUser: SamUser, isAccepted: Boolean, version: String) = {
@@ -62,7 +62,7 @@ case class MockTosServiceBuilder() extends MockitoSugar {
     lenient()
       .doReturn(IO.pure(TermsOfServiceDetails(version, rightNow, permitsSystemUsage = isAccepted)))
       .when(tosService)
-      .getTermsOfServiceDetailsForUser(ArgumentMatchers.eq(samUser.id), any[SamUser], any[Boolean], any[SamRequestContext])
+      .getTermsOfServiceDetailsForUser(ArgumentMatchers.eq(samUser.id), any[SamUser], any[SamRequestContext])
   }
 
   def build: TosService = tosService