Merge branch 'develop'

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

CromIAM/src/main/resources/application.conf

@@ -24,7 +24,7 @@ swagger_oauth {
 
 akka {
   log-dead-letters = "off"
-  loggers = ["akka.event.slf4j.Slf4jLogger"]
+  loggers = ["cromwell.core.logging.EnhancedSlf4jLogger"]
 
   http {
     server {

CromIAM/src/main/resources/logback.xml

@@ -1,5 +1,15 @@
 <configuration>
 
+    <!-- Enhanced thread and date reporting. -->
+    <conversionRule
+            conversionWord="et"
+            converterClass="cromwell.core.logging.EnhancedThreadConverter"
+    />
+    <conversionRule
+            conversionWord="ed"
+            converterClass="cromwell.core.logging.EnhancedDateConverter"
+    />
+
     <!-- default properties for FILEROLLER need to be set upfront -->
 
     <if condition='property("FILEROLLER_DIR").equals("")'>
@@ -18,7 +28,7 @@
         <then>
             <appender name="STANDARD_APPENDER" class="ch.qos.logback.core.ConsoleAppender">
                 <encoder>
-                    <pattern>%date %X{sourceThread} %-5level - %msg%n</pattern>
+                    <pattern>%ed{yyyy-MM-dd HH:mm:ss,SSS} %et %-5level - %msg%n</pattern>
                 </encoder>
             </appender>
         </then>
@@ -68,7 +78,7 @@
 
                 </rollingPolicy>
                 <encoder>
-                    <pattern>%d{yyyy-MM-dd HH:mm:ss,SSS} [%thread] %-5level %logger{35} - %msg%n</pattern>
+                    <pattern>%ed{yyyy-MM-dd HH:mm:ss,SSS} [%et] %-5level %logger{35} - %msg%n</pattern>
                 </encoder>
             </appender>
         </then>

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")

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)
+
 }

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

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 {

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)
+    }
+  }
 }

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)

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)
         }
       }
     }

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", "[email protected]"))
+    ) ~> 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", "[email protected]"))
+      // "[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 {

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 == "[email protected]" || user.userId.value == MockSamClient.AuthorizedUserCollectionStr)
+      FailureResponseOrT.pure(true)
+    else if (user.userId.value == "[email protected]")
+      FailureResponseOrT.pure(false)
+    else
+      throw new Exception("Misconfigured test")
+  }
+
   override def requestAuth(authorizationRequest: CollectionAuthorizationRequest,
                            cromIamRequest: HttpRequest): FailureResponseOrT[Unit] = {
     authorizationRequest.user.userId.value match {