diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRoutes.scala index 8daeecdc7..7b73fe7e0 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRoutes.scala +++ b/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) + } } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamUserDirectives.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamUserDirectives.scala index c48253b6f..5bd6855aa 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamUserDirectives.scala +++ b/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" diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala index ae86106ce..465faf3d6 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala +++ b/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 { 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 index 304e73a3e..48986e8a1 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala +++ b/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"))) } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutes.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutes.scala index ca9f20d20..613b420ef 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutes.scala +++ b/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) + } } } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala index feec46361..c0ffadf39 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDaoBuilder.scala +++ b/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 diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala index 76258de06..6bd9654eb 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockTosServiceBuilder.scala +++ b/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 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 index 26c19b5b8..c404b58d4 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala +++ b/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") + } + } } }