-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WM-2500][WM-2502] Fetch Github token from ECM for importing and running private workflows #7392
Changes from 12 commits
a6c1092
9467e26
fb0d6bf
9a26105
2b105ac
c5495f9
60ca5ed
29a412a
1372959
dcbc763
a9861d7
93f98ea
3d3935e
4701188
ab3a534
ea97924
dbd6efd
2d7dd21
05b1e15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package cromwell.services.auth.ecm | ||
|
||
import com.typesafe.config.Config | ||
import net.ceedubs.ficus.Ficus._ | ||
|
||
final case class EcmConfig(baseUrl: String) | ||
|
||
object EcmConfig { | ||
def apply(config: Config): Option[EcmConfig] = config.as[Option[String]]("ecm.base-url").map(EcmConfig(_)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
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 scala.concurrent.{ExecutionContext, Future} | ||
import scala.util.{Success, Try} | ||
import spray.json._ | ||
|
||
class EcmService(baseEcmUrl: String) { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think technically ECM always returns json format, but it's possible for things in the network layer between you and ECM (like the apache proxies, etc) to return html pages There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true. I have updated the comment. I also looked at the error handling in ECM and it seems it should return a standard JSON response in format of ErrorReport in all other status codes. Hence I have updated the helper method to
|
||
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>} | ||
*/ | ||
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 => | ||
Try(responseBodyAsStr.parseJson) match { | ||
case Success(JsObject(fields)) => | ||
fields.get("message").map(_.toString().replaceAll("\"", "")).getOrElse(responseBodyAsStr) | ||
case _ => responseBodyAsStr | ||
} | ||
case _ => responseBodyAsStr | ||
} | ||
|
||
def getGithubAccessToken( | ||
userToken: String | ||
salonishah11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
)(implicit actorSystem: ActorSystem, ec: ExecutionContext): Future[String] = { | ||
|
||
def responseEntityToFutureStr(responseEntity: ResponseEntity): Future[String] = | ||
responseEntity.dataBytes.runFold(ByteString(""))(_ ++ _).map(_.utf8String) | ||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know akka is picky and can throw exceptions if we don't read the bytes in time (within 1 second?). How comfortable are we that this future will evaluate in time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied the pattern we have in Cromwell (see WorkflowCallbackActor.scala and TesAsyncBackendJobExecutionActor.scala) and didn't realize that 1 second was the default timeout. Is there an implicit timeout being imported in these 2 references that I also need to update here? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible that we're hitting this error in cases where we aren't choosing to read the bytes at all, rather than cases where we take too long to read them. We're planning to do WX-1525 next week, which should confirm that one way or the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, should the whole response be loaded into memory as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just make sure that |
||
|
||
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) | ||
Future.failed(new RuntimeException(s"HTTP ${response.status.value}. $errorMessage")) | ||
} | ||
} else responseEntityToFutureStr(response.entity) | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,51 @@ | ||
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 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") | ||
|
||
private lazy val ecmConfigOpt: Option[EcmConfig] = EcmConfig.apply(serviceConfig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI this should work but In this case, I would just rename to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it's in I think what you really want is for the baseUrl to be optional, and for that to be what we're switching on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point that you can't remove values and only redefine them in additional configs. Then in this case it means that ECM base url will always be present so I think we might not need for either base url or the EcmConfig to be Optional. What do you think about simply removing the Optional and assuming ecmConfig will always be defined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on Janet's comment, I have updated the reference.conf file and commented out |
||
lazy val ecmServiceOpt: Option[EcmService] = ecmConfigOpt.map(ecmConfig => new EcmService(ecmConfig.baseUrl)) | ||
|
||
override def receive: Receive = { | ||
case GithubAuthRequest(_) if enabled => | ||
sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) | ||
case GithubAuthRequest(userToken) if enabled => | ||
val respondTo = sender() | ||
Check warning on line 33 in services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala
|
||
ecmServiceOpt match { | ||
case Some(ecmService) => | ||
ecmService.getGithubAccessToken(userToken) onComplete { | ||
case Success(githubToken) => respondTo ! GithubAuthTokenResponse(githubToken) | ||
case Failure(e) => respondTo ! GithubAuthVendingFailure(e.getMessage) | ||
Check warning on line 38 in services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala
|
||
} | ||
case None => | ||
respondTo ! GithubAuthVendingFailure( | ||
"Invalid configuration for service 'GithubAuthVending': missing 'ecm.base-url' value." | ||
) | ||
} | ||
// 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 _ => sender() ! NoGithubAuthResponse | ||
salonishah11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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.apply(config) | ||
|
||
actualEcmConfig shouldBe defined | ||
actualEcmConfig.get.baseUrl shouldBe "https://mock-ecm-url.org" | ||
} | ||
|
||
it should "return None when ECM base url is absent" in { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also have a case for the other config values (to continue the theme... like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes they should be covered here |
||
val config = ConfigFactory.parseString(s""" | ||
|enabled = true | ||
|auth.azure = true | ||
""".stripMargin) | ||
|
||
EcmConfig.apply(config) shouldBe None | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
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.githubb" | ||
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, | ||
"<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}""", | ||
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 for other non-success status codes", | ||
StatusCodes.BadGateway, | ||
"Response error - Bad Gateway", | ||
"Response error - Bad Gateway" | ||
) | ||
) | ||
|
||
behavior of "extractErrorMessage in EcmService" | ||
|
||
forAll(testCases) { (testName, statusCode, responseBodyAsStr, expectedErrorMsg) => | ||
it should testName in { | ||
assert(ecmService.extractErrorMessage(statusCode, responseBodyAsStr) == expectedErrorMsg) | ||
} | ||
Comment on lines
+52
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woohoo for table based tests! |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
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, | ||
NoGithubAuthResponse | ||
} | ||
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 scala.concurrent.{ExecutionContext, Future} | ||
|
||
class GithubAuthVendingActorSpec extends TestKitSuite with AnyFlatSpecLike with Matchers with Eventually { | ||
|
||
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 { | ||
salonishah11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.") | ||
) | ||
} | ||
} | ||
|
||
it should "return Github token if found" in { | ||
val serviceRegistryActor = TestProbe() | ||
val actor = system.actorOf( | ||
Props( | ||
new TestGithubAuthVendingActor(githubAuthEnabledServiceConfig, | ||
ConfigFactory.parseString(""), | ||
serviceRegistryActor.ref | ||
) | ||
) | ||
) | ||
|
||
eventually { | ||
serviceRegistryActor.send(actor, GithubAuthRequest("valid_user_token")) | ||
serviceRegistryActor.expectMsg(GithubAuthTokenResponse("gha_token")) | ||
} | ||
} | ||
|
||
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 | ||
) | ||
) | ||
) | ||
|
||
eventually { | ||
serviceRegistryActor.send(actor, GithubAuthRequest("invalid_user_token")) | ||
serviceRegistryActor.expectMsg(GithubAuthVendingFailure("Exception thrown for testing purposes")) | ||
} | ||
} | ||
} | ||
|
||
class TestEcmService(baseUrl: String) extends EcmService(baseUrl) { | ||
|
||
override def getGithubAccessToken( | ||
userToken: String | ||
)(implicit actorSystem: ActorSystem, ec: ExecutionContext): Future[String] = | ||
userToken match { | ||
case "valid_user_token" => Future.successful("gha_token") | ||
case _ => Future.failed(new RuntimeException("Exception thrown for testing purposes")) | ||
} | ||
} | ||
|
||
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")) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would discourage setting the default to a real Terra service here - might be confusing for non-Terra users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In this case should there be no default? I can comment this out and add comment describing what service it is looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks good!