[WM-2500][WM-2502] Fetch Github token from ECM for importing and running private workflows

services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala

@@ -7,10 +7,14 @@ object GithubAuthVending {
     override def serviceName: String = "GithubAuthVending"
   }
 
-  case class GithubAuthRequest(terraToken: String) extends GithubAuthVendingMessage
+  // types of tokens
+  case class TerraToken(value: String)
+  case class GithubToken(value: String)
+
+  case class GithubAuthRequest(terraToken: TerraToken) extends GithubAuthVendingMessage
 
   sealed trait GithubAuthVendingResponse extends GithubAuthVendingMessage
-  case class GithubAuthTokenResponse(accessToken: String) extends GithubAuthVendingResponse
+  case class GithubAuthTokenResponse(githubAccessToken: GithubToken) extends GithubAuthVendingResponse
   case object NoGithubAuthResponse extends GithubAuthVendingResponse
   case class GithubAuthVendingFailure(errorMsg: String) extends GithubAuthVendingResponse
 

services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala

@@ -15,10 +15,11 @@
   GithubAuthTokenResponse,
   GithubAuthVendingFailure,
   GithubAuthVendingResponse,
-  NoGithubAuthResponse
+  NoGithubAuthResponse,
+  TerraToken
 }
-
 import net.ceedubs.ficus.Ficus._
+
 import scala.concurrent.{ExecutionContext, Future}
 
 trait GithubAuthVendingSupport extends AskSupport with StrictLogging {
@@ -30,7 +31,7 @@
   def importAuthProvider(token: String)(implicit timeout: Timeout): ImportAuthProvider = new GithubImportAuthProvider {
     override def authHeader(): Future[Map[String, String]] =
       serviceRegistryActor
-        .ask(GithubAuthRequest(token))
+        .ask(GithubAuthRequest(TerraToken(token)))
         .mapTo[GithubAuthVendingResponse]
         .recoverWith {
           case e: AskTimeoutException =>
@@ -41,10 +42,11 @@
             Future.failed(new Exception("Failed to resolve github auth token", e))
         }
         .flatMap {
-          case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}"))
+          case GithubAuthTokenResponse(githubToken) =>
+            Future.successful(Map("Authorization" -> s"Bearer ${githubToken.value}"))
           case NoGithubAuthResponse => Future.successful(Map.empty)
           case GithubAuthVendingFailure(error) =>
             Future.failed(new Exception(s"Failed to resolve GitHub auth token. Error: $error"))
         }
   }
 

services/src/main/scala/cromwell/services/auth/ecm/EcmService.scala

@@ -14,24 +14,22 @@
   private val getGithubAccessTokenApiPath = "api/oauth/v1/github/access-token"
 
   /*
-     ECM doesn't have a standard error response format. Some of the responses contains HTML tags in it. This helper
-     method returns custom error message for 401 and 403 errors as they contain HTML tags. For 400, 404 and 500 the
-     Swagger suggests that the response format is of ErrorReport schema and this method tries to extract the
-     actual message from the JSON object and returns it. In case of other status codes or if it fails to parse JSON it
-     returns the original error response.
-     ErrorReport schema: {"message":<actual_error_msg>, "statusCode":<code>}
+     ECM does generally return standard JSON error response, but for 401 status code it seems some other layer in
+     between (like the apache proxies, etc) returns HTML pages. This helper method returns custom error message for 401
+     status code as it contains HTML tags. For all other status code, the response format is generally of ErrorReport
+     schema and this method tries to extract the actual message from the JSON object and return it. In case it fails
+     to parse JSON, it returns the original error response body.
+     ErrorReport schema: {"message":"<actual_error_msg>", "statusCode":<code>}
    */
   def extractErrorMessage(errorCode: StatusCode, responseBodyAsStr: String): String =
     errorCode match {
       case StatusCodes.Unauthorized => "Invalid or missing authentication credentials."
-      case StatusCodes.Forbidden => "User doesn't have the right permission(s) to fetch Github token."
-      case StatusCodes.BadRequest | StatusCodes.NotFound | StatusCodes.InternalServerError =>
+      case _ =>
         Try(responseBodyAsStr.parseJson) match {
           case Success(JsObject(fields)) =>
             fields.get("message").map(_.toString().replaceAll("\"", "")).getOrElse(responseBodyAsStr)
           case _ => responseBodyAsStr
         }
-      case _ => responseBodyAsStr
     }
 
   def getGithubAccessToken(
@@ -39,15 +37,15 @@
   )(implicit actorSystem: ActorSystem, ec: ExecutionContext): Future[String] = {
 
     def responseEntityToFutureStr(responseEntity: ResponseEntity): Future[String] =
       responseEntity.dataBytes.runFold(ByteString(""))(_ ++ _).map(_.utf8String)
 
     val headers: HttpHeader = RawHeader("Authorization", s"Bearer $userToken")
     val httpRequest =
       HttpRequest(method = HttpMethods.GET, uri = s"$baseEcmUrl/$getGithubAccessTokenApiPath").withHeaders(headers)
 
     Http()
       .singleRequest(httpRequest)
       .flatMap((response: HttpResponse) =>
         if (response.status.isFailure()) {
           responseEntityToFutureStr(response.entity) flatMap { errorBody =>
             val errorMessage = extractErrorMessage(response.status, errorBody)

services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala

@@ -3,11 +3,13 @@
 import akka.actor.{Actor, ActorRef, ActorSystem, Props}
 import com.typesafe.config.Config
 import com.typesafe.scalalogging.LazyLogging
+import common.util.StringUtil.EnhancedToStringable
 import cromwell.core.Dispatcher.ServiceDispatcher
 import cromwell.services.auth.GithubAuthVending.{
   GithubAuthRequest,
   GithubAuthTokenResponse,
   GithubAuthVendingFailure,
+  GithubToken,
   NoGithubAuthResponse
 }
 import cromwell.services.auth.ecm.{EcmConfig, EcmService}
@@ -25,27 +27,32 @@
 
   lazy val enabled: Boolean = serviceConfig.getBoolean("enabled")
 
-  private lazy val ecmConfigOpt: Option[EcmConfig] = EcmConfig.apply(serviceConfig)
+  lazy val ecmConfigOpt: Option[EcmConfig] = EcmConfig.apply(serviceConfig)
   lazy val ecmServiceOpt: Option[EcmService] = ecmConfigOpt.map(ecmConfig => new EcmService(ecmConfig.baseUrl))
 
   override def receive: Receive = {
-    case GithubAuthRequest(userToken) if enabled =>
+    case GithubAuthRequest(terraToken) if enabled =>
       val respondTo = sender()
       ecmServiceOpt match {
         case Some(ecmService) =>
-          ecmService.getGithubAccessToken(userToken) onComplete {
-            case Success(githubToken) => respondTo ! GithubAuthTokenResponse(githubToken)
+          ecmService.getGithubAccessToken(terraToken.value) onComplete {
+            case Success(token) => respondTo ! GithubAuthTokenResponse(GithubToken(token))
             case Failure(e) => respondTo ! GithubAuthVendingFailure(e.getMessage)
           }
         case None =>
           respondTo ! GithubAuthVendingFailure(
             "Invalid configuration for service 'GithubAuthVending': missing 'ecm.base-url' value."
           )
       }
+    case GithubAuthRequest(_) => sender() ! NoGithubAuthResponse
     // This service currently doesn't do any work on shutdown but the service registry pattern requires it
     // (see https://github.com/broadinstitute/cromwell/issues/2575)
     case ShutdownCommand => context stop self
-    case _ => sender() ! NoGithubAuthResponse
+    case other =>
+      logger.error(
+        s"Programmer Error: Unexpected message ${other.toPrettyElidedString(1000)} received by ${this.self.path.name}."
+      )
+      sender() ! GithubAuthVendingFailure(s"Received unexpected message ${other.toPrettyElidedString(1000)}.")
   }
 }
 

services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala

@@ -8,7 +8,7 @@ import com.typesafe.config.ConfigFactory
 import cromwell.core.TestKitSuite
 import cromwell.languages.util.ImportResolver.GithubImportAuthProvider
 import cromwell.services.ServiceRegistryActor.ServiceRegistryFailure
-import cromwell.services.auth.GithubAuthVending.GithubAuthRequest
+import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubToken, TerraToken}
 import cromwell.services.auth.GithubAuthVendingSupportSpec.TestGithubAuthVendingSupport
 import org.scalatest.concurrent.Eventually
 import org.scalatest.flatspec.AnyFlatSpecLike
@@ -42,8 +42,8 @@ class GithubAuthVendingSupportSpec extends TestKitSuite with AnyFlatSpecLike wit
     val provider = testSupport.importAuthProvider("user-token")
     val authHeader: Future[Map[String, String]] = provider.authHeader()
 
-    serviceRegistryActor.expectMsg(GithubAuthRequest("user-token"))
-    serviceRegistryActor.reply(GithubAuthVending.GithubAuthTokenResponse("github-token"))
+    serviceRegistryActor.expectMsg(GithubAuthRequest(TerraToken("user-token")))
+    serviceRegistryActor.reply(GithubAuthVending.GithubAuthTokenResponse(GithubToken("github-token")))
 
     Await.result(authHeader, 10.seconds) should be(Map("Authorization" -> "Bearer github-token"))
   }
@@ -54,7 +54,7 @@ class GithubAuthVendingSupportSpec extends TestKitSuite with AnyFlatSpecLike wit
     val provider = testSupport.importAuthProvider("user-token")
     val authHeader: Future[Map[String, String]] = provider.authHeader()
 
-    serviceRegistryActor.expectMsg(GithubAuthRequest("user-token"))
+    serviceRegistryActor.expectMsg(GithubAuthRequest(TerraToken("user-token")))
     serviceRegistryActor.reply(GithubAuthVending.NoGithubAuthResponse)
 
     Await.result(authHeader, 10.seconds) should be(Map.empty)
@@ -66,7 +66,7 @@ class GithubAuthVendingSupportSpec extends TestKitSuite with AnyFlatSpecLike wit
     val provider = testSupport.importAuthProvider("user-token")
     val authHeader: Future[Map[String, String]] = provider.authHeader()
 
-    serviceRegistryActor.expectMsg(GithubAuthRequest("user-token"))
+    serviceRegistryActor.expectMsg(GithubAuthRequest(TerraToken("user-token")))
     serviceRegistryActor.reply(GithubAuthVending.GithubAuthVendingFailure("BOOM"))
 
     eventually {
@@ -94,7 +94,7 @@ class GithubAuthVendingSupportSpec extends TestKitSuite with AnyFlatSpecLike wit
     val provider = testSupport.importAuthProvider("user-token")
     val authHeader: Future[Map[String, String]] = provider.authHeader()
 
-    serviceRegistryActor.expectMsg(GithubAuthRequest("user-token"))
+    serviceRegistryActor.expectMsg(GithubAuthRequest(TerraToken("user-token")))
     serviceRegistryActor.reply(ServiceRegistryFailure("GithubAuthVending"))
 
     eventually {

services/src/test/scala/cromwell/services/auth/ecm/EcmServiceSpec.scala

@@ -9,7 +9,7 @@ class EcmServiceSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyC
 
   private val ecmService = new EcmService("https://mock-ecm-url.org")
 
-  private val ecm400ErrorMsg = "No enum constant bio.terra.externalcreds.generated.model.Provider.githubb"
+  private val ecm400ErrorMsg = "No enum constant bio.terra.externalcreds.generated.model.Provider.MyOwnProvider"
   private val ecm404ErrorMsg =
     "No linked account found for user ID: 123 and provider: github. Please go to the Terra Profile page External Identities tab to link your account for this provider"
 
@@ -20,11 +20,6 @@ class EcmServiceSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyC
      "<h2>could be anything</h2>",
      "Invalid or missing authentication credentials."
     ),
-    ("return custom 403 error when status code is 403",
-     StatusCodes.Forbidden,
-     "<h2>could be anything</h2>",
-     "User doesn't have the right permission(s) to fetch Github token."
-    ),
     ("extract message from valid ErrorReport JSON if status code is 400",
      StatusCodes.BadRequest,
      s"""{ "message" : "$ecm400ErrorMsg", "statusCode" : 400}""",
@@ -45,10 +40,10 @@ class EcmServiceSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyC
      "Response error - not a JSON",
      "Response error - not a JSON"
     ),
-    ("return response error body for other non-success status codes",
-     StatusCodes.BadGateway,
-     "Response error - Bad Gateway",
-     "Response error - Bad Gateway"
+    ("return response error body if JSON doesn't contain 'message' key",
+     StatusCodes.BadRequest,
+     """{"non-message-key" : "error message"}""",
+     """{"non-message-key" : "error message"}"""
     )
   )
 

services/src/test/scala/cromwell/services/auth/impl/GithubAuthVendingActorSpec.scala

@@ -8,94 +8,81 @@ import cromwell.services.auth.GithubAuthVending.{
   GithubAuthRequest,
   GithubAuthTokenResponse,
   GithubAuthVendingFailure,
-  NoGithubAuthResponse
+  GithubToken,
+  NoGithubAuthResponse,
+  TerraToken
 }
 import cromwell.services.auth.ecm.EcmService
 import cromwell.services.auth.impl.GithubAuthVendingActorSpec.TestGithubAuthVendingActor
 import org.scalatest.concurrent.Eventually
 import org.scalatest.flatspec.AnyFlatSpecLike
 import org.scalatest.matchers.should.Matchers
+import org.scalatest.prop.TableDrivenPropertyChecks
 
 import scala.concurrent.{ExecutionContext, Future}
 
-class GithubAuthVendingActorSpec extends TestKitSuite with AnyFlatSpecLike with Matchers with Eventually {
+class GithubAuthVendingActorSpec
+    extends TestKitSuite
+    with AnyFlatSpecLike
+    with Matchers
+    with Eventually
+    with TableDrivenPropertyChecks {
 
+  final private val validUserToken = "valid_user_token"
   final private val githubAuthEnabledServiceConfig =
     ConfigFactory.parseString(s"""
                                  |enabled = true
                                  |auth.azure = true
                                  |ecm.base-url = "https://mock-ecm-url.org"
         """.stripMargin)
 
-  behavior of "GithubAuthVendingActor"
-
-  it should "return NoGithubAuthResponse if GithubAuthVending is disabled" in {
-    val serviceConfig = ConfigFactory.parseString(s"""
-                                                     |enabled = false
-                                                     |auth.azure = false
-      """.stripMargin)
-
-    val serviceRegistryActor = TestProbe()
-    val actor = system.actorOf(
-      Props(new GithubAuthVendingActor(serviceConfig, ConfigFactory.parseString(""), serviceRegistryActor.ref))
-    )
-
-    eventually {
-      serviceRegistryActor.send(actor, GithubAuthRequest("valid_user_token"))
-      serviceRegistryActor.expectMsg(NoGithubAuthResponse)
-    }
-  }
-
-  it should "return invalid configuration error if no ECM base url is found" in {
-    val serviceConfig = ConfigFactory.parseString(s"""
-                                                     |enabled = true
-                                                     |auth.azure = true
-      """.stripMargin)
-
-    val serviceRegistryActor = TestProbe()
-    val actor = system.actorOf(
-      Props(new GithubAuthVendingActor(serviceConfig, ConfigFactory.parseString(""), serviceRegistryActor.ref))
-    )
-
-    eventually {
-      serviceRegistryActor.send(actor, GithubAuthRequest("valid_user_token"))
-      serviceRegistryActor.expectMsg(
-        GithubAuthVendingFailure("Invalid configuration for service 'GithubAuthVending': missing 'ecm.base-url' value.")
-      )
-    }
-  }
+  private def githubAuthConfigWithoutEcmUrl(flag: Boolean): Config =
+    ConfigFactory.parseString(s"""
+                                 |enabled = $flag
+                                 |auth.azure = $flag
+        """.stripMargin)
 
-  it should "return Github token if found" in {
-    val serviceRegistryActor = TestProbe()
-    val actor = system.actorOf(
-      Props(
-        new TestGithubAuthVendingActor(githubAuthEnabledServiceConfig,
-                                       ConfigFactory.parseString(""),
-                                       serviceRegistryActor.ref
-        )
-      )
+  private val testCases = Table(
+    ("test name", "service config", "use TestEcmService class", "terra token", "response"),
+    ("return NoGithubAuthResponse if GithubAuthVending is disabled",
+     githubAuthConfigWithoutEcmUrl(false),
+     false,
+     validUserToken,
+     NoGithubAuthResponse
+    ),
+    ("return invalid configuration error if no ECM base url is found",
+     githubAuthConfigWithoutEcmUrl(true),
+     false,
+     validUserToken,
+     GithubAuthVendingFailure("Invalid configuration for service 'GithubAuthVending': missing 'ecm.base-url' value.")
+    ),
+    ("return Github token if found",
+     githubAuthEnabledServiceConfig,
+     true,
+     validUserToken,
+     GithubAuthTokenResponse(GithubToken("gha_token"))
+    ),
+    ("return failure message if ECM service returns non-successful response",
+     githubAuthEnabledServiceConfig,
+     true,
+     "invalid_user_token",
+     GithubAuthVendingFailure("Exception thrown for testing purposes")
     )
+  )
 
-    eventually {
-      serviceRegistryActor.send(actor, GithubAuthRequest("valid_user_token"))
-      serviceRegistryActor.expectMsg(GithubAuthTokenResponse("gha_token"))
-    }
-  }
+  behavior of "GithubAuthVendingActor"
 
-  it should "return failure message if ECM service returns non-successful response" in {
-    val serviceRegistryActor = TestProbe()
-    val actor = system.actorOf(
-      Props(
-        new TestGithubAuthVendingActor(githubAuthEnabledServiceConfig,
-                                       ConfigFactory.parseString(""),
-                                       serviceRegistryActor.ref
-        )
+  forAll(testCases) { (testName, serviceConfig, useTestEcmService, userTerraToken, expectedResponseMsg) =>
+    it should testName in {
+      val serviceRegistryActor = TestProbe()
+      val actor = system.actorOf(
+        Props(new TestGithubAuthVendingActor(serviceConfig, serviceRegistryActor.ref, useTestEcmService))
       )
-    )
 
-    eventually {
-      serviceRegistryActor.send(actor, GithubAuthRequest("invalid_user_token"))
-      serviceRegistryActor.expectMsg(GithubAuthVendingFailure("Exception thrown for testing purposes"))
+      eventually {
+        serviceRegistryActor.send(actor, GithubAuthRequest(TerraToken(userTerraToken)))
+        serviceRegistryActor.expectMsg(expectedResponseMsg)
+      }
     }
   }
 }
@@ -113,8 +100,13 @@ class TestEcmService(baseUrl: String) extends EcmService(baseUrl) {
 
 object GithubAuthVendingActorSpec {
 
-  class TestGithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef)
-      extends GithubAuthVendingActor(serviceConfig, globalConfig, serviceRegistryActor) {
-    override lazy val ecmServiceOpt: Option[EcmService] = Some(new TestEcmService("https://mock-ecm-url.org"))
+  class TestGithubAuthVendingActor(serviceConfig: Config,
+                                   serviceRegistryActor: ActorRef,
+                                   useTestEcmServiceClass: Boolean
+  ) extends GithubAuthVendingActor(serviceConfig, ConfigFactory.parseString(""), serviceRegistryActor) {
+
+    override lazy val ecmServiceOpt: Option[EcmService] =
+      if (useTestEcmServiceClass) Some(new TestEcmService("https://mock-ecm-url.org"))
+      else ecmConfigOpt.map(ecmConfig => new EcmService(ecmConfig.baseUrl))
   }
 }