Skip to content

Commit

Permalink
move TOS admin enforcement to route level (#1622)
Browse files Browse the repository at this point in the history
  • Loading branch information
@davidangb
davidangb authored Jan 10, 2025
1 parent 217768e commit 294c19d
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 92 deletions.
15 changes: 11 additions & 4 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@ 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.{TermsOfServiceDetails, TermsOfServiceHistory}
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.util.{SamRequestContext, SupportsAdmin}

import scala.concurrent.ExecutionContext
import scala.util.matching.Regex

trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirectives {
trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirectives with SupportsAdmin {
val tosService: TosService
implicit val executionContext: ExecutionContext
private val samUserIdPattern: Regex = "^[a-zA-Z0-9]+$".r
Expand Down Expand Up @@ -123,7 +124,9 @@ trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirec
getWithTelemetry(samRequestContext, userIdParam(requestUserId)) {
rejectEmptyResponse {
complete {
tosService.getTermsOfServiceDetailsForUser(requestUserId, samRequestContext)
ensureAdminIfNeeded[Option[TermsOfServiceDetails]](requestUserId, samRequestContext) {
tosService.getTermsOfServiceDetailsForUser(requestUserId, samRequestContext)
}
}
}
}
Expand All @@ -132,7 +135,11 @@ trait TermsOfServiceRoutes extends SamUserDirectives with SamRequestContextDirec
pathEndOrSingleSlash {
getWithTelemetry(samRequestContext, userIdParam(requestUserId)) {
parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit))
complete {
ensureAdminIfNeeded[TermsOfServiceHistory](requestUserId, samRequestContext) {
tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit)
}
}
}
}
}
Expand Down
43 changes: 19 additions & 24 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import org.broadinstitute.dsde.workbench.sam.model.{
TermsOfServiceHistoryRecord
}
import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.{FutureWithLogging, IOWithLogging}
import org.broadinstitute.dsde.workbench.sam.util.{SamRequestContext, SupportsAdmin}
import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext

import java.io.{FileNotFoundException, IOException}
import java.time.Instant
Expand All @@ -38,8 +38,7 @@ class TosService(
)(
implicit val executionContext: ExecutionContext,
implicit val actorSystem: ActorSystem
) extends LazyLogging
with SupportsAdmin {
) extends LazyLogging {

val termsOfServiceTextKey = "termsOfService"
val privacyPolicyTextKey = "privacyPolicy"
Expand Down Expand Up @@ -94,20 +93,18 @@ class TosService(
userId: WorkbenchUserId,
samRequestContext: SamRequestContext
): IO[Option[TermsOfServiceDetails]] =
ensureAdminIfNeeded[Option[TermsOfServiceDetails]](userId, samRequestContext) {
for {
latestTermsOfServiceAcceptance <- directoryDao.getUserTermsOfService(userId, samRequestContext, Option(TosTable.ACCEPT))
latestTermsOfServiceAction <- directoryDao.getUserTermsOfService(userId, samRequestContext)
requestedUser <- loadUser(userId, samRequestContext)
} yield latestTermsOfServiceAcceptance.map(tosDetails =>
TermsOfServiceDetails(
Option(tosDetails.version),
Option(tosDetails.createdAt),
tosAcceptancePermitsSystemUsage(requestedUser, latestTermsOfServiceAction),
latestTermsOfServiceAcceptance.exists(_.version.equals(tosConfig.version))
)
for {
latestTermsOfServiceAcceptance <- directoryDao.getUserTermsOfService(userId, samRequestContext, Option(TosTable.ACCEPT))
latestTermsOfServiceAction <- directoryDao.getUserTermsOfService(userId, samRequestContext)
requestedUser <- loadUser(userId, samRequestContext)
} yield latestTermsOfServiceAcceptance.map(tosDetails =>
TermsOfServiceDetails(
Option(tosDetails.version),
Option(tosDetails.createdAt),
tosAcceptancePermitsSystemUsage(requestedUser, latestTermsOfServiceAction),
latestTermsOfServiceAcceptance.exists(_.version.equals(tosConfig.version))
)
}
)

private def loadUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[SamUser] =
directoryDao.loadUser(userId, samRequestContext).map {
Expand All @@ -116,14 +113,12 @@ class TosService(
}

def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] =
ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) {
directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map {
case samUserTosHistory if samUserTosHistory.isEmpty => TermsOfServiceHistory(List.empty)
case samUserTosHistory =>
TermsOfServiceHistory(
samUserTosHistory.map(historyRecord => TermsOfServiceHistoryRecord(historyRecord.action, historyRecord.version, historyRecord.createdAt))
)
}
directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map {
case samUserTosHistory if samUserTosHistory.isEmpty => TermsOfServiceHistory(List.empty)
case samUserTosHistory =>
TermsOfServiceHistory(
samUserTosHistory.map(historyRecord => TermsOfServiceHistoryRecord(historyRecord.action, historyRecord.version, historyRecord.createdAt))
)
}

def getTermsOfServiceComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
Expand Down
30 changes: 28 additions & 2 deletions src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,26 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou
}
}

it("should return 404 when USER_ID is does not exist") {
it("should return 401 when called as non-admin to inspect another user") {
val defaultUser = Generator.genWorkbenchUserGoogle.sample.get
val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("[email protected]"))
val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup)
.withEnabledUser(defaultUser)
.withAllowedUser(defaultUser)

Get("/api/termsOfService/v1/user/12345abc") ~> Route.seal(mockSamRoutesBuilder.build.route) ~> check {
status shouldEqual StatusCodes.Unauthorized
}
}

it("should return 404 when called as admin and USER_ID does not exist") {
val defaultUser = Generator.genWorkbenchUserGoogle.sample.get
val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("[email protected]"))
val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup)
.callAsAdminUser()
.withEnabledUser(defaultUser)
.withAllowedUser(defaultUser)

Get("/api/termsOfService/v1/user/12345abc") ~> Route.seal(mockSamRoutesBuilder.build.route) ~> check {
status shouldEqual StatusCodes.NotFound
}
Expand Down Expand Up @@ -181,10 +194,23 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou
}
}

it("should return 200 with empty list when user has no acceptance history") {
it("should return 401 when called as non-admin to inspect another user") {
val defaultUser = Generator.genWorkbenchUserGoogle.sample.get
val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("[email protected]"))
val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup)
.withEnabledUser(defaultUser)
.withAllowedUser(defaultUser)

Get("/api/termsOfService/v1/user/12345abc/history") ~> Route.seal(mockSamRoutesBuilder.build.route) ~> check {
status shouldEqual StatusCodes.Unauthorized
}
}

it("should return 200 with empty list when called as admin and user has no acceptance history") {
val defaultUser = Generator.genWorkbenchUserGoogle.sample.get
val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("[email protected]"))
val mockSamRoutesBuilder = new MockSamRoutesBuilder(allUsersGroup)
.callAsAdminUser()
.withEnabledUser(defaultUser)
.withAllowedUser(defaultUser)

Expand Down
78 changes: 16 additions & 62 deletions src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
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.{WorkbenchEmail, WorkbenchExceptionWithErrorReport, WorkbenchUserId}
import org.broadinstitute.dsde.workbench.model.{WorkbenchEmail, WorkbenchUserId}
import org.broadinstitute.dsde.workbench.sam.TestSupport.tosConfig
import org.broadinstitute.dsde.workbench.sam.dataAccess.{DirectoryDAO, MockDirectoryDaoBuilder}
import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
Expand Down Expand Up @@ -764,22 +763,22 @@ class TosServiceSpec(_system: ActorSystem)
}
}

"can retrieve Terms of Service details for a user" - {
"if the requesting user is an admin" in {
"can retrieve Terms of Service details" - {
"for a different user" in {
// Arrange
val tosVersion = "0"
val adminUser = Generator.genWorkbenchUserBoth.sample.get
val callingUser = Generator.genWorkbenchUserBoth.sample.get
val directoryDao = new MockDirectoryDaoBuilder()
.withAcceptedTermsOfServiceForUser(defaultUser, tosVersion)
.build

val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withAdminUser().build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

// Act
val userTosDetails: TermsOfServiceDetails =
runAndWait(tosService.getTermsOfServiceDetailsForUser(defaultUser.id, SamRequestContext(None, None, Some(adminUser)))).get
runAndWait(tosService.getTermsOfServiceDetailsForUser(defaultUser.id, SamRequestContext(None, None, Some(callingUser)))).get

// Assert
userTosDetails should have {
Expand All @@ -789,14 +788,14 @@ class TosServiceSpec(_system: ActorSystem)
}
}

"if the requesting user is not an admin but is the same as the requested user" in {
"when the calling user is the same as the requesting user" in {
// Arrange
val tosVersion = "0"
val directoryDao = new MockDirectoryDaoBuilder()
.withAcceptedTermsOfServiceForUser(defaultUser, tosVersion)
.build

val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

Expand All @@ -813,53 +812,31 @@ class TosServiceSpec(_system: ActorSystem)
}
}

"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()
.withAcceptedTermsOfServiceForUser(someRandoUser, tosVersion)
.build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

// Act and Assert
val e = intercept[WorkbenchExceptionWithErrorReport] {
runAndWait(tosService.getTermsOfServiceDetailsForUser(someRandoUser.id, SamRequestContext(None, None, Some(nonAdminUser))))
}

assert(e.errorReport.statusCode.value == StatusCodes.Unauthorized, "User should not be authorized to see other users' Terms of Service details")
}
}

"can retrieve Terms of Service history for a user" - {
"if the requesting user is an admin" in {
"for a different user" in {
// Arrange
val tosVersion = "0"
val adminUser = Generator.genWorkbenchUserBoth.sample.get
val record1 = SamUserTos(adminUser.id, tosVersion, TosTable.ACCEPT, Instant.now())
val record2 = SamUserTos(adminUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5))
val callingUser = Generator.genWorkbenchUserBoth.sample.get
val record1 = SamUserTos(callingUser.id, tosVersion, TosTable.ACCEPT, Instant.now())
val record2 = SamUserTos(callingUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5))
val directoryDao = new MockDirectoryDaoBuilder()
.withTermsOfServiceHistoryForUser(defaultUser, List(record1, record2))
.build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withAdminUser().build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

// Act
val userTosDetails: TermsOfServiceHistory =
runAndWait(tosService.getTermsOfServiceHistoryForUser(defaultUser.id, SamRequestContext(None, None, Some(adminUser)), 5))
runAndWait(tosService.getTermsOfServiceHistoryForUser(defaultUser.id, SamRequestContext(None, None, Some(callingUser)), 5))

// Assert
userTosDetails.history.size shouldBe 2
userTosDetails.history.head shouldBe record1.toHistoryRecord
userTosDetails.history.last shouldBe record2.toHistoryRecord
}

"if the requesting user is not an admin but is the same as the requested user" in {
"when the calling user is the same as the requesting user" in {
// Arrange
val tosVersion = "0"
val userTos1 = SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now())
Expand All @@ -868,7 +845,7 @@ class TosServiceSpec(_system: ActorSystem)
.withTermsOfServiceHistoryForUser(defaultUser, List(userTos1, userTos2))
.build

val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

Expand All @@ -882,28 +859,5 @@ class TosServiceSpec(_system: ActorSystem)
userTosDetails.history.last shouldBe userTos2.toHistoryRecord
}
}
"cannot retrieve Terms of Service history 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 userTos1 = SamUserTos(someRandoUser.id, tosVersion, TosTable.ACCEPT, Instant.now())
val userTos2 = SamUserTos(someRandoUser.id, tosVersion, TosTable.REJECT, Instant.now().minusSeconds(5))
val directoryDao = new MockDirectoryDaoBuilder()
.withTermsOfServiceHistoryForUser(someRandoUser, List(userTos1, userTos2))
.build
val cloudExt = MockCloudExtensionsBuilder(allUsersGroup).withNonAdminUser().build

val tosService = new TosService(cloudExt, directoryDao, TestSupport.tosConfig)

// Act and Assert
val e = intercept[WorkbenchExceptionWithErrorReport] {
runAndWait(tosService.getTermsOfServiceHistoryForUser(someRandoUser.id, SamRequestContext(None, None, Some(nonAdminUser)), 5))
}

assert(e.errorReport.statusCode.value == StatusCodes.Unauthorized, "User should not be authorized to see other users' Terms of Service details")
}
}
}
}

0 comments on commit 294c19d

Please sign in to comment.