diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 672d82c8c80..25fd0e6ac87 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -578,10 +578,8 @@ services { config { enabled = false auth.azure = false - # Notes: - # - don't include the 'Bearer' before the token - # - this config value should be removed when support for fetching tokens from ECM has been added to Cromwell - access-token = "dummy-token" + # Set this to the service that Cromwell should retrieve Github access token associated with user's token. + # ecm.base-url = "" } } } diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 30396519016..8a12d0e0cc2 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -593,7 +593,7 @@ object Dependencies { val servicesDependencies: List[ModuleID] = List( "com.google.api" % "gax-grpc" % googleGaxGrpcV, "org.apache.commons" % "commons-csv" % commonsCsvV, - ) ++ testDatabaseDependencies + ) ++ testDatabaseDependencies ++ akkaHttpDependencies val serverDependencies: List[ModuleID] = slf4jBindingDependencies diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala index c74df8c947f..1d82125c1e1 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala @@ -7,11 +7,15 @@ 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(error: Exception) extends GithubAuthVendingResponse + case class GithubAuthVendingFailure(errorMsg: String) extends GithubAuthVendingResponse } diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala index 63fc9404e07..4697a741909 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -15,10 +15,11 @@ import cromwell.services.auth.GithubAuthVending.{ 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 @@ trait GithubAuthVendingSupport extends AskSupport with StrictLogging { 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 @@ trait GithubAuthVendingSupport extends AskSupport with StrictLogging { 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("Failed to resolve github auth token", error)) + Future.failed(new Exception(s"Failed to resolve GitHub auth token. Error: $error")) } } diff --git a/services/src/main/scala/cromwell/services/auth/ecm/EcmConfig.scala b/services/src/main/scala/cromwell/services/auth/ecm/EcmConfig.scala new file mode 100644 index 00000000000..c1f8900d22d --- /dev/null +++ b/services/src/main/scala/cromwell/services/auth/ecm/EcmConfig.scala @@ -0,0 +1,10 @@ +package cromwell.services.auth.ecm + +import com.typesafe.config.Config +import net.ceedubs.ficus.Ficus._ + +final case class EcmConfig(baseUrl: Option[String]) + +object EcmConfig { + def apply(config: Config): EcmConfig = EcmConfig(config.as[Option[String]]("ecm.base-url")) +} diff --git a/services/src/main/scala/cromwell/services/auth/ecm/EcmService.scala b/services/src/main/scala/cromwell/services/auth/ecm/EcmService.scala new file mode 100644 index 00000000000..b65181d5bda --- /dev/null +++ b/services/src/main/scala/cromwell/services/auth/ecm/EcmService.scala @@ -0,0 +1,58 @@ +package cromwell.services.auth.ecm + +import akka.actor.ActorSystem +import akka.http.scaladsl.Http +import akka.http.scaladsl.model._ +import akka.http.scaladsl.model.headers.RawHeader +import akka.util.ByteString +import cromwell.services.auth.GithubAuthVending.{GithubToken, TerraToken} +import spray.json._ + +import scala.concurrent.{ExecutionContext, Future} +import scala.util.{Success, Try} + +class EcmService(baseEcmUrl: String) { + private val getGithubAccessTokenApiPath = "api/oauth/v1/github/access-token" + + /* + 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":"", "statusCode":} + */ + def extractErrorMessage(errorCode: StatusCode, responseBodyAsStr: String): String = + errorCode match { + case StatusCodes.Unauthorized => "Invalid or missing authentication credentials." + case _ => + Try(responseBodyAsStr.parseJson) match { + case Success(JsObject(fields)) => + fields.get("message").map(_.toString().replaceAll("\"", "")).getOrElse(responseBodyAsStr) + case _ => responseBodyAsStr + } + } + + def getGithubAccessToken( + userToken: TerraToken + )(implicit actorSystem: ActorSystem, ec: ExecutionContext): Future[GithubToken] = { + + def responseEntityToFutureStr(responseEntity: ResponseEntity): Future[String] = + responseEntity.dataBytes.runFold(ByteString(""))(_ ++ _).map(_.utf8String) + + val headers: HttpHeader = RawHeader("Authorization", s"Bearer ${userToken.value}") + 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) + Future.failed(new RuntimeException(s"HTTP ${response.status.value}. $errorMessage")) + } + } else responseEntityToFutureStr(response.entity).map(GithubToken) + ) + } +} diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index e0f4c9756a0..2cfe51f344b 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -1,26 +1,57 @@ package cromwell.services.auth.impl -import akka.actor.{Actor, ActorRef, Props} +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, NoGithubAuthResponse} +import cromwell.services.auth.GithubAuthVending.{ + GithubAuthRequest, + GithubAuthTokenResponse, + GithubAuthVendingFailure, + NoGithubAuthResponse +} +import cromwell.services.auth.ecm.{EcmConfig, EcmService} import cromwell.util.GracefulShutdownHelper.ShutdownCommand +import scala.concurrent.ExecutionContext +import scala.util.{Failure, Success} + class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { - lazy val enabled = serviceConfig.getBoolean("enabled") + implicit val system: ActorSystem = context.system + implicit val ec: ExecutionContext = context.dispatcher + + lazy val enabled: Boolean = serviceConfig.getBoolean("enabled") + + lazy val ecmConfigOpt: EcmConfig = EcmConfig(serviceConfig) + lazy val ecmServiceOpt: Option[EcmService] = ecmConfigOpt.baseUrl.map(url => new EcmService(url)) override def receive: Receive = { - case GithubAuthRequest(_) if enabled => - sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) + case GithubAuthRequest(terraToken) if enabled => + val respondTo = sender() + ecmServiceOpt match { + case Some(ecmService) => + ecmService.getGithubAccessToken(terraToken) onComplete { + case Success(githubToken) => respondTo ! GithubAuthTokenResponse(githubToken) + 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)}.") } } diff --git a/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala b/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala index 9a20851ebdd..5e62d001e60 100644 --- a/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala +++ b/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,13 +66,12 @@ 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.GithubAuthVendingFailure(new Exception("BOOM"))) + serviceRegistryActor.expectMsg(GithubAuthRequest(TerraToken("user-token"))) + serviceRegistryActor.reply(GithubAuthVending.GithubAuthVendingFailure("BOOM")) eventually { authHeader.isCompleted should be(true) - authHeader.value.get.failed.get.getMessage should be("Failed to resolve github auth token") - authHeader.value.get.failed.get.getCause.getMessage should be("BOOM") + authHeader.value.get.failed.get.getMessage should be("Failed to resolve GitHub auth token. Error: BOOM") } } @@ -95,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 { diff --git a/services/src/test/scala/cromwell/services/auth/ecm/EcmConfigSpec.scala b/services/src/test/scala/cromwell/services/auth/ecm/EcmConfigSpec.scala new file mode 100644 index 00000000000..a666eac9af1 --- /dev/null +++ b/services/src/test/scala/cromwell/services/auth/ecm/EcmConfigSpec.scala @@ -0,0 +1,30 @@ +package cromwell.services.auth.ecm + +import com.typesafe.config.ConfigFactory +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class EcmConfigSpec extends AnyFlatSpec with Matchers { + + it should "parse ECM base url when present" in { + val config = ConfigFactory.parseString(s""" + |enabled = true + |auth.azure = true + |ecm.base-url = "https://mock-ecm-url.org" + """.stripMargin) + + val actualEcmConfig = EcmConfig(config) + + actualEcmConfig.baseUrl shouldBe defined + actualEcmConfig.baseUrl.get shouldBe "https://mock-ecm-url.org" + } + + it should "return None when ECM base url is absent" in { + val config = ConfigFactory.parseString(s""" + |enabled = true + |auth.azure = true + """.stripMargin) + + EcmConfig(config).baseUrl shouldBe None + } +} diff --git a/services/src/test/scala/cromwell/services/auth/ecm/EcmServiceSpec.scala b/services/src/test/scala/cromwell/services/auth/ecm/EcmServiceSpec.scala new file mode 100644 index 00000000000..b1a2442cad9 --- /dev/null +++ b/services/src/test/scala/cromwell/services/auth/ecm/EcmServiceSpec.scala @@ -0,0 +1,57 @@ +package cromwell.services.auth.ecm + +import akka.http.scaladsl.model.StatusCodes +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.prop.TableDrivenPropertyChecks + +class EcmServiceSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks { + + private val ecmService = new EcmService("https://mock-ecm-url.org") + + 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" + + private val testCases = Table( + ("test name", "response status code", "response body string", "expected error message"), + ("return custom 401 error when status code is 401", + StatusCodes.Unauthorized, + "

could be anything

", + "Invalid or missing authentication credentials." + ), + ("extract message from valid ErrorReport JSON if status code is 400", + StatusCodes.BadRequest, + s"""{ "message" : "$ecm400ErrorMsg", "statusCode" : 400}""", + ecm400ErrorMsg + ), + ("extract message from valid ErrorReport JSON if status code is 404", + StatusCodes.NotFound, + s"""{ "message" : "$ecm404ErrorMsg", "statusCode" : 404}""", + ecm404ErrorMsg + ), + ("extract message from valid ErrorReport JSON if status code is 500", + StatusCodes.InternalServerError, + """{ "message" : "Internal error", "statusCode" : 500}""", + "Internal error" + ), + ("return response error body if it fails to parse JSON", + StatusCodes.InternalServerError, + "Response error - not a JSON", + "Response error - not a JSON" + ), + ("return response error body if JSON doesn't contain 'message' key", + StatusCodes.BadRequest, + """{"non-message-key" : "error message"}""", + """{"non-message-key" : "error message"}""" + ) + ) + + behavior of "extractErrorMessage in EcmService" + + forAll(testCases) { (testName, statusCode, responseBodyAsStr, expectedErrorMsg) => + it should testName in { + assert(ecmService.extractErrorMessage(statusCode, responseBodyAsStr) == expectedErrorMsg) + } + } +} diff --git a/services/src/test/scala/cromwell/services/auth/impl/GithubAuthVendingActorSpec.scala b/services/src/test/scala/cromwell/services/auth/impl/GithubAuthVendingActorSpec.scala new file mode 100644 index 00000000000..f45c701fa22 --- /dev/null +++ b/services/src/test/scala/cromwell/services/auth/impl/GithubAuthVendingActorSpec.scala @@ -0,0 +1,112 @@ +package cromwell.services.auth.impl + +import akka.actor.{ActorRef, ActorSystem, Props} +import akka.testkit.TestProbe +import com.typesafe.config.{Config, ConfigFactory} +import cromwell.core.TestKitSuite +import cromwell.services.auth.GithubAuthVending.{ + GithubAuthRequest, + GithubAuthTokenResponse, + GithubAuthVendingFailure, + 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 + 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) + + private def githubAuthConfigWithoutEcmUrl(flag: Boolean): Config = + ConfigFactory.parseString(s""" + |enabled = $flag + |auth.azure = $flag + """.stripMargin) + + 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") + ) + ) + + behavior of "GithubAuthVendingActor" + + 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)) + ) + + serviceRegistryActor.send(actor, GithubAuthRequest(TerraToken(userTerraToken))) + eventually { + serviceRegistryActor.expectMsg(expectedResponseMsg) + } + } + } +} + +class TestEcmService(baseUrl: String) extends EcmService(baseUrl) { + + override def getGithubAccessToken( + userToken: TerraToken + )(implicit actorSystem: ActorSystem, ec: ExecutionContext): Future[GithubToken] = + userToken.value match { + case "valid_user_token" => Future.successful(GithubToken("gha_token")) + case _ => Future.failed(new RuntimeException("Exception thrown for testing purposes")) + } +} + +object GithubAuthVendingActorSpec { + + 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.baseUrl.map(url => new EcmService(url)) + } +}