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 c81f5aa3b..5d504d687 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 @@ -4,12 +4,10 @@ import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ 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 akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ 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 @@ -17,6 +15,7 @@ import scala.util.matching.Regex trait TermsOfServiceRoutes { val tosService: TosService implicit val executionContext: ExecutionContext + private val samUserIdPattern: Regex = "^[a-zA-Z0-9]+$".r @deprecated("Being replaced by REST-ful termsOfService routes") def oldTermsOfServiceRoutes: server.Route = @@ -39,104 +38,72 @@ trait TermsOfServiceRoutes { } } - // Do not know and do not care who the calling user is - unauthenticated requests def publicTermsOfServiceRoutes: server.Route = - pathPrefix("termsOfService" / "v1")(Routes.publicTermsOfServiceV1Routes) - - // Must be able to authenticate and authorize the calling user - authenticated requests - 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 - - // /api/termsOfService/v1 - def publicTermsOfServiceV1Routes: server.Route = - concat( - pathEndOrSingleSlash(Actions.getCurrentTermsOfService), - pathPrefix("docs")(Routes.termsOfServiceDocRoutes) - ) - - // /api/termsOfService/v1/user - 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, isAdmin, samRequestContext)) - ) - - // /api/termsOfService/v1/docs - private def termsOfServiceDocRoutes: server.Route = - concat( - pathEndOrSingleSlash(Actions.getTermsOfServiceDocs), - pathPrefix("redirect")(termsOfServiceDocsRedirectRoutes) - ) - - // /api/termsOfService/v1/docs/redirect - private def termsOfServiceDocsRedirectRoutes: server.Route = - concat( - pathEndOrSingleSlash(Actions.getTermsOfServiceDocsRedirect) - ) - - // /api/termsOfService/v1/user - 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, isAdmin, samRequestContext)), - pathPrefix("history")(Actions.getTermsOfServiceHistoryForUser(requestedUserId, samRequestContext)) - ) - } - } - - private object Actions { - def getCurrentTermsOfService: server.Route = - get { - complete(StatusCodes.NotImplemented) - } - - def getTermsOfServiceDocs: server.Route = - get { - complete(StatusCodes.NotImplemented) - } - - def getTermsOfServiceDocsRedirect: server.Route = - get { - complete(StatusCodes.NotImplemented) - } - - def getUsersTermsOfServiceDetails( - requestedUserId: WorkbenchUserId, - requestingUser: SamUser, - isAdmin: Boolean, - samRequestContext: SamRequestContext - ): server.Route = - get { - complete(StatusCodes.OK, tosService.getTermsOfServiceDetailsForUser(requestedUserId, requestingUser, isAdmin, samRequestContext)) - } - - def getTermsOfServiceDetailsForSelf(samUser: SamUser, samRequestContext: SamRequestContext): server.Route = - getUsersTermsOfServiceDetails(samUser.id, samUser, isAdmin = false, samRequestContext) - - def acceptTermsOfServiceForUser(samUser: SamUser, samRequestContext: SamRequestContext): server.Route = - put { - complete(StatusCodes.NotImplemented) - } - - def rejectTermsOfServiceForUser(samUser: SamUser, samRequestContext: SamRequestContext): server.Route = - put { - complete(StatusCodes.NotImplemented) + pathPrefix("termsOfService") { + pathPrefix("v1") { // api/termsOfService/v1 + pathEndOrSingleSlash { + get { + complete(tosService.getTosConfig()) + } + } ~ + pathPrefix("docs") { // api/termsOfService/v1/docs + pathEndOrSingleSlash { + get { + complete(StatusCodes.NotImplemented) + } + } ~ + pathPrefix("redirect") { // api/termsOfService/v1/docs/redirect + pathEndOrSingleSlash { + get { + complete(StatusCodes.NotImplemented) + } + } + } + } } + } - def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext): server.Route = - get { - complete(StatusCodes.NotImplemented) + def userTermsOfServiceRoutes(samUser: SamUser, isAdmin: Boolean, samRequestContext: SamRequestContext): server.Route = + pathPrefix("termsOfService") { + pathPrefix("user") { // api/termsOfService/v1/user + pathPrefix("self") { // api/termsOfService/v1/user/self + pathEndOrSingleSlash { + get { + complete(StatusCodes.OK, tosService.getTermsOfServiceDetailsForUser(samUser.id, samUser, isAdmin = false, samRequestContext)) + } + } ~ + pathPrefix("accept") { // api/termsOfService/v1/user/accept + pathEndOrSingleSlash { + put { + complete(StatusCodes.NotImplemented) + } + } + } ~ + pathPrefix("reject") { // api/termsOfService/v1/user/reject + pathEndOrSingleSlash { + put { + complete(StatusCodes.NotImplemented) + } + } + } + } ~ + // The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's + pathPrefix(Segment) { userId => // api/termsOfService/v1/user/{userId} + validate(samUserIdPattern.matches(samUser.id.value), "User ID must be alpha numeric") { + pathEndOrSingleSlash { + get { + complete(StatusCodes.OK, tosService.getTermsOfServiceDetailsForUser(samUser.id, samUser, isAdmin, samRequestContext)) + } + } ~ + pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history + pathEndOrSingleSlash { + get { + complete(StatusCodes.NotImplemented) + } + } + } + } + } } - } + } } 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 a1ef85d3b..e595b8b0a 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 @@ -6,25 +6,22 @@ import akka.http.scaladsl.model.{StatusCodes, Uri} import akka.http.scaladsl.unmarshalling.Unmarshal import cats.effect.IO import com.typesafe.scalalogging.LazyLogging -import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.IOWithLogging -import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.FutureWithLogging import org.broadinstitute.dsde.workbench.model.{ErrorReport, WorkbenchExceptionWithErrorReport, WorkbenchUserId} import org.broadinstitute.dsde.workbench.sam.api.StandardSamUserDirectives -import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO -import org.broadinstitute.dsde.workbench.sam.errorReportSource -import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.broadinstitute.dsde.workbench.sam.config.TermsOfServiceConfig +import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable -import org.broadinstitute.dsde.workbench.sam.model.api.SamUser +import org.broadinstitute.dsde.workbench.sam.errorReportSource +import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, TermsOfServiceConfigResponse} import org.broadinstitute.dsde.workbench.sam.model.{OldTermsOfServiceDetails, SamUserTos, TermsOfServiceComplianceStatus, TermsOfServiceDetails} -import org.broadinstitute.dsde.workbench.sam.model.{SamUserTos, TermsOfServiceComplianceStatus, TermsOfServiceDetails} -import org.broadinstitute.dsde.workbench.sam.model.api.TermsOfServiceConfigResponse +import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.{FutureWithLogging, IOWithLogging} +import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.io.{FileNotFoundException, IOException} -import scala.concurrent.{Await, ExecutionContext} +import java.time.Instant import java.util.concurrent.TimeUnit import scala.concurrent.duration.Duration -import java.time.Instant +import scala.concurrent.{Await, ExecutionContext} import scala.io.Source class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceConfig)( diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala index e30add366..4d4984938 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala @@ -6,42 +6,39 @@ import akka.http.scaladsl.server.Route import akka.http.scaladsl.testkit.ScalatestRouteTest import org.broadinstitute.dsde.workbench.model.WorkbenchEmail import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, databaseEnabledClue} -import org.broadinstitute.dsde.workbench.sam.model.api.TermsOfServiceConfigResponse import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ -import org.broadinstitute.dsde.workbench.sam.model.api.SamUser +import org.broadinstitute.dsde.workbench.sam.model.api.{SamUser, TermsOfServiceConfigResponse} import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, TermsOfServiceDetails} import org.broadinstitute.dsde.workbench.sam.service.CloudExtensions import org.broadinstitute.dsde.workbench.sam.{Generator, TestSupport} import org.scalatest.concurrent.Eventually.eventually import org.scalatest.funspec.AnyFunSpec import org.scalatest.matchers.should.Matchers -import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRouteTest with TestSupport { - val samRoutes: TestSamRoutes = TestSamRoutes(Map.empty) - - describe("GET /tos/text") { - it("should return the tos text") { - assume(databaseEnabled, databaseEnabledClue) - "GET /termsOfService/v1" should "return the current tos config" in { - val samRoutes = TestSamRoutes(Map.empty) - eventually { - Get("/termsOfService/v1") ~> samRoutes.route ~> check { - status shouldEqual StatusCodes.OK - responseAs[TermsOfServiceConfigResponse] shouldBe TermsOfServiceConfigResponse( - enforced = true, - currentVersion = "0", - inGracePeriod = false, - inRollingAcceptanceWindow = false - ) + describe("GET /termsOfService/v1") { + it("return the current tos config") { + val samRoutes = TestSamRoutes(Map.empty) + eventually { + Get("/termsOfService/v1") ~> samRoutes.route ~> check { + status shouldEqual StatusCodes.OK + responseAs[TermsOfServiceConfigResponse] shouldBe TermsOfServiceConfigResponse( + enforced = true, + currentVersion = "0", + inGracePeriod = false, + inRollingAcceptanceWindow = false + ) + } } } } - "GET /tos/text" should "return the tos text" in { - assume(databaseEnabled, databaseEnabledClue) + describe("GET /tos/text") { + it("return the tos text") { + assume(databaseEnabled, databaseEnabledClue) + val samRoutes = TestSamRoutes(Map.empty) eventually { Get("/tos/text") ~> samRoutes.route ~> check { status shouldEqual StatusCodes.OK @@ -52,15 +49,19 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /privacy/text") { - it("should return the privacy policy text") { - Get("/privacy/text") ~> samRoutes.route ~> check { - status shouldEqual StatusCodes.OK - responseAs[String].isEmpty shouldBe false + it("return the privacy policy text") { + val samRoutes = TestSamRoutes(Map.empty) + eventually { + Get("/privacy/text") ~> samRoutes.route ~> check { + status shouldEqual StatusCodes.OK + responseAs[String].isEmpty shouldBe false + } } } } describe("GET /api/termsOfService/v1") { + val samRoutes = TestSamRoutes(Map.empty) it("should be a valid route") { Get("/api/termsOfService/v1") ~> samRoutes.route ~> check { status shouldBe StatusCodes.NotImplemented @@ -69,6 +70,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /api/termsOfService/v1/docs") { + val samRoutes = TestSamRoutes(Map.empty) it("should be a valid route") { Get("/api/termsOfService/v1/docs") ~> samRoutes.route ~> check { status shouldBe StatusCodes.NotImplemented @@ -77,6 +79,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /api/termsOfService/v1/docs/redirect") { + val samRoutes = TestSamRoutes(Map.empty) it("should be a valid route") { Get("/api/termsOfService/v1/docs/redirect") ~> samRoutes.route ~> check { status shouldBe StatusCodes.NotImplemented @@ -85,6 +88,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /api/termsOfService/v1/user") { + val samRoutes = TestSamRoutes(Map.empty) it("should not be handled") { Get("/api/termsOfService/v1/user") ~> samRoutes.route ~> check { assert(!handled, "`GET /api/termsOfService/v1/user` should not be a handled route") @@ -93,6 +97,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /api/termsOfService/v1/user/self") { + val samRoutes = TestSamRoutes(Map.empty) it("should return an instance of `TermsOfServiceDetails`") { Get("/api/termsOfService/v1/user/self") ~> samRoutes.route ~> check { withClue(s"${responseAs[String]} is not parsable as an instance of `TermsOfServiceDetails`.") { @@ -104,6 +109,7 @@ class TermsOfServiceRouteSpec extends AnyFunSpec with Matchers with ScalatestRou } describe("GET /api/termsOfService/v1/user/{USER_ID}") { + val samRoutes = TestSamRoutes(Map.empty) it("should return an instance of `TermsOfServiceDetails`") { val allUsersGroup: BasicWorkbenchGroup = BasicWorkbenchGroup(CloudExtensions.allUsersGroupName, Set(), WorkbenchEmail("all_users@fake.com")) val defaultUser: SamUser = Generator.genWorkbenchUserGoogle.sample.get 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 e0f7d6961..a18cb561a 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 @@ -10,9 +10,8 @@ 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 import org.broadinstitute.dsde.workbench.sam.matchers.{TermsOfServiceDetailsMatchers, TimeMatchers} -import org.broadinstitute.dsde.workbench.sam.model.{SamUserTos, TermsOfServiceDetails} -import org.broadinstitute.dsde.workbench.sam.model.SamUserTos import org.broadinstitute.dsde.workbench.sam.model.api.TermsOfServiceConfigResponse +import org.broadinstitute.dsde.workbench.sam.model.{SamUserTos, TermsOfServiceDetails} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.broadinstitute.dsde.workbench.sam.{Generator, PropertyBasedTesting, TestSupport} import org.mockito.Mockito.RETURNS_SMART_NULLS