ID-807 some validation rules

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

@@ -74,14 +74,16 @@ abstract class SamRoutes(
           serviceAdminRoutes(samRequestContext) ~
           userRoutesV2(samRequestContext) ~
           withActiveUser(samRequestContext) { samUser =>
-            val samRequestContextWithUser = samRequestContext.copy(samUser = Option(samUser))
-            resourceRoutes(samUser, samRequestContextWithUser) ~
-            adminRoutes(samUser, samRequestContextWithUser) ~
-            extensionRoutes(samUser, samRequestContextWithUser) ~
-            groupRoutes(samUser, samRequestContextWithUser) ~
-            azureRoutes(samUser, samRequestContextWithUser) ~
-            userRoutesV1(samUser, samRequestContextWithUser) ~
-            userTermsOfServiceRoutes(samUser, samRequestContextWithUser)
+            isWorkbenchAdmin(samUser) { isAdmin =>
+              val samRequestContextWithUser = samRequestContext.copy(samUser = Option(samUser))
+              resourceRoutes(samUser, samRequestContextWithUser) ~
+              adminRoutes(samUser, samRequestContextWithUser) ~
+              extensionRoutes(samUser, samRequestContextWithUser) ~
+              groupRoutes(samUser, samRequestContextWithUser) ~
+              azureRoutes(samUser, samRequestContextWithUser) ~
+              userRoutesV1(samUser, samRequestContextWithUser) ~
+              userTermsOfServiceRoutes(samUser, isAdmin, samRequestContextWithUser)
+            }
           }
         }
       }

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

@@ -50,6 +50,9 @@ trait SamUserDirectives {
       }
     }
 
+  def isWorkbenchAdmin(samUser: SamUser): Directive1[Boolean] =
+    onSuccess(cloudExtensions.isWorkbenchAdmin(samUser.email))
+
   def asAdminServiceUser: Directive0
 
   val oldTermsOfServiceUrl = "app.terra.bio/#terms-of-service"

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

@@ -41,8 +41,8 @@ trait TermsOfServiceRoutes {
   def publicTermsOfServiceRoutes: server.Route =
     pathPrefix("termsOfService" / "v1")(Routes.publicTermsOfServiceV1Routes)
 
-  def userTermsOfServiceRoutes(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
-    pathPrefix("termsOfService" / "v1" / "user")(Routes.userTermsOfServiceV1Routes(samUser, samRequestContext))
+  def userTermsOfServiceRoutes(samUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): server.Route =
+    pathPrefix("termsOfService" / "v1" / "user")(Routes.userTermsOfServiceV1Routes(samUser, isAdmin, samRequestContext))
 
   private object Routes {
     private val samUserIdPattern: Regex = "^[a-zA-Z0-9]+$".r
@@ -55,12 +55,12 @@ trait TermsOfServiceRoutes {
       )
 
     // /api/termsOfService/v1/user
-    def userTermsOfServiceV1Routes(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
+    def userTermsOfServiceV1Routes(samUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): server.Route =
       concat(
         pathPrefix("self")(Actions.getTermsOfServiceDetailsForSelf(samUser, samRequestContext)),
         pathPrefix("accept")(pathEndOrSingleSlash(Actions.acceptTermsOfServiceForUser(samUser, samRequestContext))),
         pathPrefix("reject")(pathEndOrSingleSlash(Actions.rejectTermsOfServiceForUser(samUser, samRequestContext))),
-        pathPrefix(Segment)(userId => Routes.termsOfServiceForUserRoutes(userId, samUser, samRequestContext))
+        pathPrefix(Segment)(userId => Routes.termsOfServiceForUserRoutes(userId, samUser, isAdmin, samRequestContext))
       )
 
     // /termsOfService/v1/docs
@@ -77,11 +77,11 @@ trait TermsOfServiceRoutes {
       )
 
     // /api/termsOfService/v1/user
-    private def termsOfServiceForUserRoutes(rawUserId: String, requestingSamUser: SamUser, samRequestContext: SamRequestContext): server.Route =
+    private def termsOfServiceForUserRoutes(rawUserId: String, requestingSamUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): server.Route =
       validate(samUserIdPattern.matches(rawUserId), "User ID must be alpha numeric") {
         val requestedUserId = WorkbenchUserId(rawUserId)
         concat(
-          pathEndOrSingleSlash(Actions.getUsersTermsOfServiceDetails(requestedUserId, requestingSamUser, samRequestContext)),
+          pathEndOrSingleSlash(Actions.getUsersTermsOfServiceDetails(requestedUserId, requestingSamUser, isAdmin, samRequestContext)),
           pathPrefix("history")(Actions.getTermsOfServiceHistoryForUser(requestedUserId, samRequestContext))
         )
       }
@@ -103,13 +103,13 @@ trait TermsOfServiceRoutes {
         complete(StatusCodes.NotImplemented)
       }
 
-    def getUsersTermsOfServiceDetails(requestedUserId: WorkbenchUserId, requestingUser: SamUser, samRequestContext: SamRequestContext): server.Route =
+    def getUsersTermsOfServiceDetails(requestedUserId: WorkbenchUserId, requestingUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): server.Route =
       get {
-        complete(StatusCodes.OK, tosService.getTermsOfServiceDetails(requestedUserId, requestingUser, samRequestContext))
+        complete(StatusCodes.OK, tosService.getTermsOfServiceDetails(requestedUserId, requestingUser, isAdmin, samRequestContext))
       }
 
     def getTermsOfServiceDetailsForSelf(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
-      getUsersTermsOfServiceDetails(samUser.id, samUser, samRequestContext)
+      getUsersTermsOfServiceDetails(samUser.id, samUser, isAdmin = false, samRequestContext)
 
     def acceptTermsOfServiceForUser(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
       put {

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

@@ -52,10 +52,14 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
       OldTermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
     }
 
-  def getTermsOfServiceDetails(requestedUserId: WorkbenchUserId, requestingUser: SamUser, samRequestContext: SamRequestContext): IO[SamUserTos] = {
-    directoryDao.getUserTos(requestedUserId, samRequestContext).map {
-      case Some(samUserTos) => samUserTos
-      case None => return IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user ${requestedUserId}")))
+  def getTermsOfServiceDetails(requestedUserId: WorkbenchUserId, requestingUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): IO[SamUserTos] = {
+    if (isAdmin || (requestedUserId == requestingUser.id)) {
+      directoryDao.getUserTos(requestedUserId, samRequestContext).map {
+        case Some(samUserTos) => samUserTos
+        case None => return IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user ${requestedUserId}")))
+      }
+    } else {
+      IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Unauthorized, "You are not allowed to make this request")))
     }
   }
 

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

@@ -68,14 +68,16 @@ abstract class MockSamRoutes(
             pathPrefix("api") {
               // IMPORTANT - all routes under /api must have an active user
               withActiveUser(samRequestContext) { samUser =>
-                val samRequestContextWithUser = samRequestContext.copy(samUser = Option(samUser))
-                resourceRoutes(samUser, samRequestContextWithUser) ~
-                  adminRoutes(samUser, samRequestContextWithUser) ~
-                  extensionRoutes(samUser, samRequestContextWithUser) ~
-                  groupRoutes(samUser, samRequestContextWithUser) ~
-                  userRoutesV1(samUser, samRequestContextWithUser) ~
-                  azureRoutes(samUser, samRequestContextWithUser) ~
-                  userTermsOfServiceRoutes(samUser, samRequestContextWithUser)
+                isWorkbenchAdmin(samUser) { isAdmin =>
+                  val samRequestContextWithUser = samRequestContext.copy(samUser = Option(samUser))
+                  resourceRoutes(samUser, samRequestContextWithUser) ~
+                    adminRoutes(samUser, samRequestContextWithUser) ~
+                    extensionRoutes(samUser, samRequestContextWithUser) ~
+                    groupRoutes(samUser, samRequestContextWithUser) ~
+                    userRoutesV1(samUser, samRequestContextWithUser) ~
+                    azureRoutes(samUser, samRequestContextWithUser) ~
+                    userTermsOfServiceRoutes(samUser, isAdmin, samRequestContextWithUser)
+                }
               }
             }
         }

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

@@ -2,7 +2,7 @@ package org.broadinstitute.dsde.workbench.sam.dataAccess
 
 import cats.effect.IO
 import org.broadinstitute.dsde.workbench.model._
-import org.broadinstitute.dsde.workbench.sam.model.BasicWorkbenchGroup
+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.util.SamRequestContext
 import org.mockito.ArgumentMatchersSugar.{any, eqTo}
@@ -111,6 +111,18 @@ case class MockDirectoryDaoBuilder() extends IdiomaticMockito {
     this
   }
 
+  def withAcceptedTermsOfServiceForUser(samUser: SamUser, tosVersion: String): MockDirectoryDaoBuilder = {
+    val samUserTos = SamUserTos(samUser.id, tosVersion, "accepted", Instant.now)
+    mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos))
+    this
+  }
+
+  def withRejectedTermsOfServiceForUser(samUser: SamUser, tosVersion: String): MockDirectoryDaoBuilder = {
+    val samUserTos = SamUserTos(samUser.id, tosVersion, "rejected", Instant.now)
+    mockedDirectoryDAO.getUserTos(eqTo(samUser.id), any[SamRequestContext]) returns IO(Option(samUserTos))
+    this
+  }
+
   // Bare minimum for a user to exist:
   // - has an ID
   // - has an email

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

@@ -29,8 +29,8 @@ case class MockTosServiceBuilder() extends MockitoSugar {
     this
   }
 
-  def withAcceptedStateForUser(samUser: SamUser, isAccepted: Boolean): MockTosServiceBuilder = {
-    setAcceptedStateForUserTo(samUser, isAccepted)
+  def withAcceptedStateForUser(samUser: SamUser, isAccepted: Boolean, version: String = "v1"): MockTosServiceBuilder = {
+    setAcceptedStateForUserTo(samUser, isAccepted, version)
     this
   }
 
@@ -43,10 +43,10 @@ case class MockTosServiceBuilder() extends MockitoSugar {
     lenient()
       .doReturn(IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"")(new ErrorReportSource("MockTosServiceBuilder")))))
       .when(tosService)
-      .getTermsOfServiceDetails(any[WorkbenchUserId], any[SamUser], any[SamRequestContext])
+      .getTermsOfServiceDetails(any[WorkbenchUserId], any[SamUser], any[Boolean], any[SamRequestContext])
   }
 
-  private def setAcceptedStateForUserTo(samUser: SamUser, isAccepted: Boolean, version: String = "v1") = {
+  private def setAcceptedStateForUserTo(samUser: SamUser, isAccepted: Boolean, version: String) = {
     val matchesUser = new ArgumentMatcher[SamUser] {
       override def matches(argument: SamUser): Boolean =
         argument.id.equals(samUser.id)
@@ -61,7 +61,7 @@ case class MockTosServiceBuilder() extends MockitoSugar {
     lenient()
       .doReturn(IO.pure(SamUserTos(samUser.id, version, action, rightNow)))
       .when(tosService)
-      .getTermsOfServiceDetails(ArgumentMatchers.eq(samUser.id), any[SamUser], any[SamRequestContext])
+      .getTermsOfServiceDetails(ArgumentMatchers.eq(samUser.id), any[SamUser], any[Boolean], any[SamRequestContext])
   }
 
   def build: TosService = tosService

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

@@ -1,20 +1,23 @@
 package org.broadinstitute.dsde.workbench.sam.service
 
 import akka.actor.ActorSystem
+import akka.http.scaladsl.model.StatusCodes
 import akka.testkit.TestKit
 import cats.effect.IO
 import cats.effect.unsafe.implicits.{global => globalEc}
-import org.broadinstitute.dsde.workbench.model.WorkbenchUserId
+import org.broadinstitute.dsde.workbench.model.{WorkbenchExceptionWithErrorReport, WorkbenchUserId}
 import org.broadinstitute.dsde.workbench.sam.TestSupport.tosConfig
-import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO
+import org.broadinstitute.dsde.workbench.sam.dataAccess.{DirectoryDAO, MockDirectoryDaoBuilder}
 import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
+import org.broadinstitute.dsde.workbench.sam.matchers.TimeMatchers
 import org.broadinstitute.dsde.workbench.sam.model.SamUserTos
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import org.broadinstitute.dsde.workbench.sam.{Generator, PropertyBasedTesting, TestSupport}
 import org.mockito.Mockito.RETURNS_SMART_NULLS
 import org.mockito.scalatest.MockitoSugar
+import org.scalatest.Inside.inside
 import org.scalatest.freespec.AnyFreeSpecLike
-import org.scalatest.{BeforeAndAfter, BeforeAndAfterAll}
+import org.scalatest.{BeforeAndAfter, BeforeAndAfterAll, OptionValues}
 
 import java.time.Instant
 import scala.concurrent.ExecutionContext.Implicits.global
@@ -26,7 +29,9 @@ class TosServiceSpec(_system: ActorSystem)
     with BeforeAndAfterAll
     with BeforeAndAfter
     with PropertyBasedTesting
-    with MockitoSugar {
+    with MockitoSugar
+    with TimeMatchers
+    with OptionValues {
 
   def this() = this(ActorSystem("TosServiceSpec"))
 
@@ -39,10 +44,10 @@ class TosServiceSpec(_system: ActorSystem)
     super.afterAll()
   }
 
-  lazy val dirDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS)
+  private lazy val dirDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS)
 
-  val defaultUser = Generator.genWorkbenchUserBoth.sample.get
-  val serviceAccountUser = Generator.genWorkbenchUserServiceAccount.sample.get
+  private val defaultUser = Generator.genWorkbenchUserBoth.sample.get
+  private val serviceAccountUser = Generator.genWorkbenchUserServiceAccount.sample.get
 
   before {
     clearDatabase()
@@ -414,6 +419,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))
@@ -425,5 +431,70 @@ class TosServiceSpec(_system: ActorSystem)
         complianceStatus.permitsSystemUsage shouldBe true
       }
     }
+
+    "can retrieve Terms of Service details for a user" - {
+      "if the requesting user is an admin" in {
+        // Arrange
+        val tosVersion = "v1"
+        val adminUser = Generator.genWorkbenchUserBoth.sample.get
+        val directoryDao = new MockDirectoryDaoBuilder()
+          .withAcceptedTermsOfServiceForUser(defaultUser, tosVersion)
+          .build
+
+        val tosService = new TosService(directoryDao, TestSupport.tosConfig)
+
+        // Act
+        val samUserTos = runAndWait(tosService.getTermsOfServiceDetails(defaultUser.id, adminUser, isAdmin = true, samRequestContext))
+
+        // Assert
+        inside(samUserTos) { case SamUserTos(id, version, action, createdAt) =>
+          id should be(defaultUser.id)
+          version should be(tosVersion)
+          action should be("accepted")
+          createdAt should beAround(Instant.now)
+        }
+      }
+
+      "if the requesting user is not an admin but is the same as the requested user" in {
+        // Arrange
+        val tosVersion = "v1"
+        val tosService = MockTosServiceBuilder()
+          .withAcceptedStateForUser(defaultUser, isAccepted = true, tosVersion)
+          .build
+
+        // Act
+        val samUserTos = runAndWait(tosService.getTermsOfServiceDetails(defaultUser.id, defaultUser, isAdmin = false, samRequestContext))
+
+        // Assert
+        inside(samUserTos) { case SamUserTos(id, version, action, createdAt) =>
+          id should be(defaultUser.id)
+          version should be(tosVersion)
+          action should be("accepted")
+          createdAt should beAround(Instant.now)
+        }
+      }
+    }
+
+    "cannot retrieve Terms of Service details 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 directoryDao = new MockDirectoryDaoBuilder()
+          .withExistingUser(someRandoUser)
+          .withAcceptedTermsOfServiceForUser(someRandoUser, tosVersion)
+          .build
+
+        val tosService = new TosService(directoryDao, TestSupport.tosConfig)
+
+        // Act and Assert
+        val e = intercept[WorkbenchExceptionWithErrorReport] {
+          runAndWait(tosService.getTermsOfServiceDetails(someRandoUser.id, nonAdminUser, isAdmin = false, samRequestContext))
+        }
+
+        assert(e.errorReport.statusCode.value == StatusCodes.Unauthorized, "User should not be authorized to see other users' Terms of Service details")
+      }
+    }
   }
 }