diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c40f47b651..e1158be3ca4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Cromwell Change Log +## 84 Release Notes + +### CromIAM enabled user checks + +For Cromwell instances utilizing the optional CromIAM identity and access management component, the following endpoints now verify that the calling user is enabled before forwarding the request. +* `/api/workflows/v1/backends` +* `/api/womtool/v1/describe` + +This change makes the above endpoints consistent with the existing behavior of all the other endpoints in the `/api/` path of CromIAM. + +## 83 Release Notes + +* Changes the type of several primary key columns in call caching tables from int to bigint. The database migration may be lengthy if your database contains a large amount of call caching data. + ## 82 Release Notes * Restored missing example configuration file diff --git a/CromIAM/src/main/scala/cromiam/instrumentation/CromIamInstrumentation.scala b/CromIAM/src/main/scala/cromiam/instrumentation/CromIamInstrumentation.scala index 63f48073146..65b164f00f6 100644 --- a/CromIAM/src/main/scala/cromiam/instrumentation/CromIamInstrumentation.scala +++ b/CromIAM/src/main/scala/cromiam/instrumentation/CromIamInstrumentation.scala @@ -20,6 +20,7 @@ trait CromIamInstrumentation extends CromwellInstrumentation { val samPrefix: NonEmptyList[String] = NonEmptyList.one("sam") val getWhitelistPrefix = NonEmptyList.one("get-whitelist") + val getUserEnabledPrefix = NonEmptyList.one("get-user-enabled") val userCollectionPrefix = NonEmptyList.one("user-collection") val authCollectionPrefix = NonEmptyList.one("auth-collection") val registerCollectionPrefix = NonEmptyList.one("register-collection") diff --git a/CromIAM/src/main/scala/cromiam/sam/SamClient.scala b/CromIAM/src/main/scala/cromiam/sam/SamClient.scala index f289251a2fb..d6a315f8241 100644 --- a/CromIAM/src/main/scala/cromiam/sam/SamClient.scala +++ b/CromIAM/src/main/scala/cromiam/sam/SamClient.scala @@ -18,6 +18,7 @@ import cromiam.sam.SamResourceJsonSupport._ import cromiam.server.status.StatusCheckedSubsystem import cromwell.api.model._ import mouse.boolean._ +import spray.json.RootJsonFormat import scala.concurrent.ExecutionContextExecutor @@ -73,6 +74,33 @@ class SamClient(scheme: String, } yield whitelisted } + def isUserEnabledSam(user: User, cromIamRequest: HttpRequest): FailureResponseOrT[Boolean] = { + val request = HttpRequest( + method = HttpMethods.GET, + uri = samUserStatusUri, + headers = List[HttpHeader](user.authorization) + ) + + for { + response <- instrumentRequest( + () => Http().singleRequest(request).asFailureResponseOrT, + cromIamRequest, + instrumentationPrefixForSam(getUserEnabledPrefix) + ) + userEnabled <- response.status match { + case StatusCodes.OK => + val unmarshal: IO[UserStatusInfo] = IO.fromFuture(IO(Unmarshal(response.entity).to[UserStatusInfo])) + FailureResponseOrT.right[HttpResponse](unmarshal).map { userInfo => + if (!userInfo.enabled) log.info("Access denied for user {}", user.userId) + userInfo.enabled + } + case _ => + log.error("Could not verify access with Sam for user {}, error was {} {}", user.userId, response.status, response.toString().take(100)) + FailureResponseOrT.pure[IO, HttpResponse](false) + } + } yield userEnabled + } + def collectionsForUser(user: User, cromIamRequest: HttpRequest): FailureResponseOrT[List[Collection]] = { val request = HttpRequest(method = HttpMethods.GET, uri = samBaseCollectionUri, headers = List[HttpHeader](user.authorization)) @@ -170,6 +198,7 @@ class SamClient(scheme: String, private lazy val samBaseResourceUri = s"$samBaseUri/api/resource" private lazy val samBaseCollectionUri = s"$samBaseResourceUri/workflow-collection" private lazy val samSubmitWhitelistUri = s"$samBaseResourceUri/caas/submit/action/get_whitelist" + private lazy val samUserStatusUri = s"$samBaseUri/register/user/v2/self/info" } @@ -188,4 +217,8 @@ object SamClient { def SamRegisterCollectionExceptionResp(statusCode: StatusCode) = HttpResponse(status = statusCode, entity = SamRegisterCollectionException(statusCode).getMessage) + case class UserStatusInfo(adminEnabled: Boolean, enabled: Boolean, userEmail: String, userSubjectId: String) + + implicit val UserStatusInfoFormat: RootJsonFormat[UserStatusInfo] = jsonFormat4(UserStatusInfo) + } diff --git a/CromIAM/src/main/scala/cromiam/webservice/CromIamApiService.scala b/CromIAM/src/main/scala/cromiam/webservice/CromIamApiService.scala index 63694599476..7a16e5ea797 100644 --- a/CromIAM/src/main/scala/cromiam/webservice/CromIamApiService.scala +++ b/CromIAM/src/main/scala/cromiam/webservice/CromIamApiService.scala @@ -81,7 +81,7 @@ trait CromIamApiService extends RequestSupport def abortRoute: Route = path("api" / "workflows" / Segment / Segment / Abort) { (_, workflowId) => post { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => logUserWorkflowAction(user, workflowId, Abort) complete { authorizeAbortThenForwardToCromwell(user, workflowId, req).asHttpResponse @@ -93,7 +93,7 @@ trait CromIamApiService extends RequestSupport //noinspection MutatorLikeMethodIsParameterless def releaseHoldRoute: Route = path("api" / "workflows" / Segment / Segment / ReleaseHold) { (_, workflowId) => post { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => logUserWorkflowAction(user, workflowId, ReleaseHold) complete { authorizeUpdateThenForwardToCromwell(user, workflowId, req).asHttpResponse @@ -112,7 +112,7 @@ trait CromIamApiService extends RequestSupport def labelPatchRoute: Route = { path("api" / "workflows" / Segment / Segment / Labels) { (_, workflowId) => patch { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => entity(as[String]) { labels => logUserWorkflowAction(user, workflowId, Labels) validateLabels(Option(labels)) { _ => // Not using the labels, just using this to verify they didn't specify labels we don't want them to @@ -130,7 +130,7 @@ trait CromIamApiService extends RequestSupport def callCacheDiffRoute: Route = path("api" / "workflows" / Segment / "callcaching" / "diff") { _ => get { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => logUserAction(user, "call caching diff") parameterSeq { parameters => val paramMap = parameters.toMap @@ -150,11 +150,9 @@ trait CromIamApiService extends RequestSupport */ private def workflowGetRoute(urlSuffix: String): Route = path("api" / "workflows" / Segment / urlSuffix) { _ => get { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => logUserAction(user, urlSuffix) - complete { - cromwellClient.forwardToCromwell(req).asHttpResponse - } + forwardIfUserEnabled(user, req, cromwellClient, samClient) } } } @@ -166,7 +164,7 @@ trait CromIamApiService extends RequestSupport private def workflowRoute(urlSuffix: String, method: Directive0): Route = path("api" / "workflows" / Segment / Segment / urlSuffix) { (_, workflowId) => method { - extractUserAndRequest { (user, req) => + extractUserAndStrictRequest { (user, req) => logUserWorkflowAction(user, workflowId, urlSuffix) complete { authorizeReadThenForwardToCromwell(user, List(workflowId), req).asHttpResponse diff --git a/CromIAM/src/main/scala/cromiam/webservice/QuerySupport.scala b/CromIAM/src/main/scala/cromiam/webservice/QuerySupport.scala index cddabe74a57..e9397605c6a 100644 --- a/CromIAM/src/main/scala/cromiam/webservice/QuerySupport.scala +++ b/CromIAM/src/main/scala/cromiam/webservice/QuerySupport.scala @@ -55,7 +55,7 @@ trait QuerySupport extends RequestSupport { * directive */ private def preprocessQuery: Directive[(User, List[Collection], HttpRequest)] = { - extractUserAndRequest tflatMap { case (user, cromIamRequest) => + extractUserAndStrictRequest tflatMap { case (user, cromIamRequest) => log.info("Received query " + cromIamRequest.method.value + " request for user " + user.userId) onComplete(samClient.collectionsForUser(user, cromIamRequest).value.unsafeToFuture()) flatMap { diff --git a/CromIAM/src/main/scala/cromiam/webservice/RequestSupport.scala b/CromIAM/src/main/scala/cromiam/webservice/RequestSupport.scala index 12b231485f7..c9b6a196368 100644 --- a/CromIAM/src/main/scala/cromiam/webservice/RequestSupport.scala +++ b/CromIAM/src/main/scala/cromiam/webservice/RequestSupport.scala @@ -6,6 +6,13 @@ import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server._ import cromiam.auth.User import org.broadinstitute.dsde.workbench.model.WorkbenchUserId +import akka.http.scaladsl.model.HttpResponse +import akka.http.scaladsl.server.Directives.{authorize, complete, onComplete} +import akka.http.scaladsl.server.Route +import cromiam.cromwell.CromwellClient +import cromiam.sam.SamClient + +import scala.util.{Failure, Success} trait RequestSupport { def extractStrictRequest: Directive1[HttpRequest] = { @@ -26,10 +33,27 @@ trait RequestSupport { } } - def extractUserAndRequest: Directive[(User, HttpRequest)] = { + def extractUserAndStrictRequest: Directive[(User, HttpRequest)] = { for { user <- extractUser request <- extractStrictRequest } yield (user, request) } + + def forwardIfUserEnabled(user: User, req: HttpRequest, cromwellClient: CromwellClient, samClient: SamClient): Route = { + import cromwell.api.model.EnhancedFailureResponseOrHttpResponseT + + onComplete(samClient.isUserEnabledSam(user, req).value.unsafeToFuture()) { + case Success(Left(httpResponse: HttpResponse)) => complete(httpResponse) + case Success(Right(isEnabled: Boolean)) => + authorize(isEnabled) { + complete { + cromwellClient.forwardToCromwell(req).asHttpResponse + } + } + case Failure(e) => + val message = s"Unable to look up enablement status for user ${user.userId}: ${e.getMessage}. Please try again later." + throw new RuntimeException(message, e) + } + } } diff --git a/CromIAM/src/main/scala/cromiam/webservice/SubmissionSupport.scala b/CromIAM/src/main/scala/cromiam/webservice/SubmissionSupport.scala index c1f5477475b..52a05d1cdc7 100644 --- a/CromIAM/src/main/scala/cromiam/webservice/SubmissionSupport.scala +++ b/CromIAM/src/main/scala/cromiam/webservice/SubmissionSupport.scala @@ -31,7 +31,7 @@ trait SubmissionSupport extends RequestSupport { // FIXME - getting pathPrefix to shrink this keeps hosing up, there's gotta be some way to do this def submitRoute: Route = (path("api" / "workflows" / Segment) | path("api" / "workflows" / Segment / "batch")) { _ => post { - extractUserAndRequest { (user, request) => + extractUserAndStrictRequest { (user, request) => log.info("Received submission request from user " + user.userId) onComplete(samClient.isSubmitWhitelisted(user, request).value.unsafeToFuture()) { case Success(Left(httpResponse)) => complete(httpResponse) diff --git a/CromIAM/src/main/scala/cromiam/webservice/WomtoolRouteSupport.scala b/CromIAM/src/main/scala/cromiam/webservice/WomtoolRouteSupport.scala index 671d4a76543..a6098b1fae0 100644 --- a/CromIAM/src/main/scala/cromiam/webservice/WomtoolRouteSupport.scala +++ b/CromIAM/src/main/scala/cromiam/webservice/WomtoolRouteSupport.scala @@ -2,20 +2,18 @@ package cromiam.webservice import akka.http.scaladsl.server.Directives._ import cromiam.cromwell.CromwellClient -import cromwell.api.model._ +import cromiam.sam.SamClient trait WomtoolRouteSupport extends RequestSupport { // When this trait is mixed into `CromIamApiService` the value of `cromwellClient` is the reader (non-abort) address val cromwellClient: CromwellClient + val samClient: SamClient val womtoolRoutes = path("api" / "womtool" / Segment / "describe") { _ => post { - extractStrictRequest { req => - complete { - // This endpoint requires authn which it gets for free from the proxy, does not care about authz - cromwellClient.forwardToCromwell(req).asHttpResponse - } + extractUserAndStrictRequest { (user, req) => + forwardIfUserEnabled(user, req, cromwellClient, samClient) } } } diff --git a/CromIAM/src/test/scala/cromiam/webservice/CromIamApiServiceSpec.scala b/CromIAM/src/test/scala/cromiam/webservice/CromIamApiServiceSpec.scala index 5ad6a976204..89945c5bfcb 100644 --- a/CromIAM/src/test/scala/cromiam/webservice/CromIamApiServiceSpec.scala +++ b/CromIAM/src/test/scala/cromiam/webservice/CromIamApiServiceSpec.scala @@ -4,7 +4,8 @@ import akka.event.NoLogging import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.model.headers.{Authorization, OAuth2BearerToken, RawHeader} import akka.http.scaladsl.model.{ContentTypes, HttpEntity, HttpHeader} -import akka.http.scaladsl.server.MissingHeaderRejection +import akka.http.scaladsl.server.Route.seal +import akka.http.scaladsl.server.{AuthorizationFailedRejection, MissingHeaderRejection} import akka.http.scaladsl.testkit.ScalatestRouteTest import com.typesafe.config.Config import common.assertion.CromwellTimeoutSpec @@ -303,12 +304,45 @@ class CromIamApiServiceSpec extends AnyFlatSpec with CromwellTimeoutSpec with Ma } } - it should "reject request if it doesn't contain OIDC_CLAIM_user_id in header" in { + it should "reject request if it doesn't contain OIDC_CLAIM_user_id or token" in { Get(s"/api/workflows/$version/backends") ~> allRoutes ~> check { rejection shouldEqual MissingHeaderRejection("OIDC_CLAIM_user_id") } } + it should "return 403 when we request with a disabled user" in { + Get( + s"/api/workflows/$version/backends" + ).withHeaders( + List(Authorization(OAuth2BearerToken("my-token")), RawHeader("OIDC_CLAIM_user_id", "disabled@example.com")) + ) ~> allRoutes ~> check { + rejection shouldEqual AuthorizationFailedRejection + } + } + + it should "reject request if it contains a token and no OIDC_CLAIM_user_id in header" in { + Get( + s"/api/workflows/$version/backends" + ).withHeaders( + List(Authorization(OAuth2BearerToken("my-token"))) + ) ~> allRoutes ~> check { + rejection shouldEqual MissingHeaderRejection("OIDC_CLAIM_user_id") + } + } + + it should "return 404 when no auth token provided" in { + Get( + s"/api/workflows/$version/backends" + ).withHeaders( + List(RawHeader("OIDC_CLAIM_user_id", "enabled@example.com")) + // "[An] explicit call on the Route.seal method is needed in test code, but in your application code it is not necessary." + // https://doc.akka.io/docs/akka-http/current/routing-dsl/testkit.html#testing-sealed-routes + // https://doc.akka.io/docs/akka-http/current/routing-dsl/routes.html#sealing-a-route + ) ~> seal(allRoutes) ~> check { + responseAs[String] shouldEqual "The requested resource could not be found." + status shouldBe NotFound + } + } behavior of "ReleaseHold endpoint" it should "return 200 for authorized user who has collection associated with root workflow" in { diff --git a/CromIAM/src/test/scala/cromiam/webservice/MockClients.scala b/CromIAM/src/test/scala/cromiam/webservice/MockClients.scala index 3fb9689e5cc..59e75347159 100644 --- a/CromIAM/src/test/scala/cromiam/webservice/MockClients.scala +++ b/CromIAM/src/test/scala/cromiam/webservice/MockClients.scala @@ -141,6 +141,15 @@ class MockSamClient(checkSubmitWhitelist: Boolean = true) FailureResponseOrT.pure(!user.userId.value.equalsIgnoreCase(NotWhitelistedUser)) } + override def isUserEnabledSam(user: User, cromIamRequest: HttpRequest): FailureResponseOrT[Boolean] = { + if (user.userId.value == "enabled@example.com" || user.userId.value == MockSamClient.AuthorizedUserCollectionStr) + FailureResponseOrT.pure(true) + else if (user.userId.value == "disabled@example.com") + FailureResponseOrT.pure(false) + else + throw new Exception("Misconfigured test") + } + override def requestAuth(authorizationRequest: CollectionAuthorizationRequest, cromIamRequest: HttpRequest): FailureResponseOrT[Unit] = { authorizationRequest.user.userId.value match { diff --git a/CromIAM/src/test/scala/cromiam/webservice/WomtoolRouteSupportSpec.scala b/CromIAM/src/test/scala/cromiam/webservice/WomtoolRouteSupportSpec.scala index 7ba1c495f23..785c887c374 100644 --- a/CromIAM/src/test/scala/cromiam/webservice/WomtoolRouteSupportSpec.scala +++ b/CromIAM/src/test/scala/cromiam/webservice/WomtoolRouteSupportSpec.scala @@ -2,6 +2,9 @@ package cromiam.webservice import akka.http.scaladsl.model.ContentTypes import akka.http.scaladsl.model.StatusCodes._ +import akka.http.scaladsl.model.headers.{Authorization, OAuth2BearerToken, RawHeader} +import akka.http.scaladsl.server.Route.seal +import akka.http.scaladsl.server.{AuthorizationFailedRejection, MissingHeaderRejection} import akka.http.scaladsl.testkit.ScalatestRouteTest import common.assertion.CromwellTimeoutSpec import org.scalatest.flatspec.AnyFlatSpec @@ -11,15 +14,64 @@ import org.scalatest.matchers.should.Matchers class WomtoolRouteSupportSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers with WomtoolRouteSupport with ScalatestRouteTest { override lazy val cromwellClient = new MockCromwellClient() + override lazy val samClient = new MockSamClient() behavior of "Womtool endpoint routes" it should "return 200 when we request to the right path" in { - Post(s"/api/womtool/v1/describe") ~> womtoolRoutes ~> check { + Post( + s"/api/womtool/v1/describe" + ).withHeaders( + List(Authorization(OAuth2BearerToken("my-token")), RawHeader("OIDC_CLAIM_user_id", "enabled@example.com")) + ) ~> womtoolRoutes ~> check { status shouldBe OK responseAs[String] shouldBe "Hey there, workflow describer" contentType should be(ContentTypes.`text/plain(UTF-8)`) } } + it should "return 403 when we request with a disabled user" in { + Post( + s"/api/womtool/v1/describe" + ).withHeaders( + List(Authorization(OAuth2BearerToken("my-token")), RawHeader("OIDC_CLAIM_user_id", "disabled@example.com")) + ) ~> womtoolRoutes ~> check { + rejection shouldEqual AuthorizationFailedRejection + } + } + + it should "bail out with no user ID" in { + Post( + s"/api/womtool/v1/describe" + ).withHeaders( + List(Authorization(OAuth2BearerToken("my-token"))) + ) ~> womtoolRoutes ~> check { + rejection shouldEqual MissingHeaderRejection("OIDC_CLAIM_user_id") + } + } + + it should "return 404 when no auth token provided" in { + Post( + s"/api/womtool/v1/describe" + ).withHeaders( + List(RawHeader("OIDC_CLAIM_user_id", "enabled@example.com")) + // "[An] explicit call on the Route.seal method is needed in test code, but in your application code it is not necessary." + // https://doc.akka.io/docs/akka-http/current/routing-dsl/testkit.html#testing-sealed-routes + // https://doc.akka.io/docs/akka-http/current/routing-dsl/routes.html#sealing-a-route + ) ~> seal(womtoolRoutes) ~> check { + responseAs[String] shouldEqual "The requested resource could not be found." + status shouldBe NotFound + } + } + + it should "bail out with no headers" in { + Post( + s"/api/womtool/v1/describe" + ).withHeaders( + List.empty + ) ~> womtoolRoutes ~> check { + rejection shouldEqual MissingHeaderRejection("OIDC_CLAIM_user_id") + } + } + }