CromIAM check user authorization before forwarding request to Cromwell #4348
Conversation
| @@ -165,8 +165,6 @@ trait QuerySupport extends RequestSupport { | |||
| } | |||
|
|
|||
| object QuerySupport { | |||
| def hasCollectionLabel(labels: Iterable[String]): Boolean = labels exists { _.startsWith(s"$CollectionLabelName:") } | |||
There was a problem hiding this comment.
Removed this function as it was not being used anymore
|
👍 |
| @@ -19,7 +19,7 @@ class QuerySupportSpec extends FlatSpec with Matchers with ScalatestRouteTest wi | |||
|
|
|||
| val queryPath = "/api/workflows/v1/query" | |||
| val getQuery = s"$queryPath?status=Submitted&label=foo:bar&label=foo:baz" | |||
| val badGetQuery = s"$queryPath?status=Submitted?labelor=foo:bar&label=foo:baz" | |||
| val badGetQuery = s"$queryPath?status=Submitted&labelor=foo:bar&label=foo:baz" | |||
| val helloWorldWdl = HelloWorld.workflowSource() | ||
| val helloWorldInputs = HelloWorld.rawInputs | ||
|
|
||
| val workflowSource = Multipart.FormData.BodyPart("workflowSource", HttpEntity(MediaTypes.`application/json`, helloWorldWdl)) |
|
|
||
| "Submit query" should "forward the request to Cromwell for authorized SAM user" in { | ||
| Post(submitPath).withHeaders(goodAuthHeaders).withEntity(formData) ~> submitRoute ~> check { | ||
| status shouldEqual StatusCodes.InternalServerError |
There was a problem hiding this comment.
I haven't dug into the mocks here but a 500 seems strange for a "success" expectation. 🙂
There was a problem hiding this comment.
Yes it did seem strange to me as well, but I didn't change it as QuerySupportSpec also uses forwardToCromwell and expects InternalServerError. I will try and do what @Horneth mentioned in the comment below. Thanks!
|
|
||
| "Submit query" should "forward the request to Cromwell for authorized SAM user" in { | ||
| Post(submitPath).withHeaders(goodAuthHeaders).withEntity(formData) ~> submitRoute ~> check { | ||
| status shouldEqual StatusCodes.InternalServerError |
There was a problem hiding this comment.
TOL: Looking at the mock code I was able to figure out that this is the expected response because the mock Cromwell client always returns 500, but it's not very obvious here IMO that a test for "user is authorized" should return 500.
I think it would be nice if possible to have the mock cromwell client respond differently based on the request (like you did for SAM). It could send back 200 for authorized and maybe throw an exception otherwise.
This would also have the added benefit to fail any test where Cromwell receives a request that wasn't authorized by CromIAM. For instance in the test below, it would make sure that on top of CromIAM responding with 400, it really didn't send the request to Cromwell.
There was a problem hiding this comment.
Good point. I will try that. Thanks!
Closes #4284