Update validation endpoint and remove authentication #8056
Conversation
arnejduranovic
requested review from
stephenkao,
thetaurean,
jimduff-usds and
snesm
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
added
platform
| @@ -178,4 +209,28 @@ class ValidateFunction( | |||
| ) | |||
| .build() | |||
| } | |||
|
|
|||
| /** | |||
| * Return [TopicSender] for a given schema if that schema exists. This lets us wrap the data needed by | |||
| "Internal", | ||
| Sender.Format.valueOf(format), | ||
| CustomerStatus.TESTING, | ||
| schemaName, |
There was a problem hiding this comment.
Is there any possible issue of validating a custom schema error messages through this? Like if someone makes a call to /validateWithSchema?schema=hl7%2fhcintegrations-covid-19&format=HL7 without being part of the hcintegrations Organization. I'd suppose not since the schemas are already open source and there couldn't really be a nefarious use case for this as far as I know, but just thought I'd ask!
There was a problem hiding this comment.
Correct, we concluded that this would be OK. We are also OK with a user calling validateWithClient and "guessing" clients. There is no personal info at risk.
| } | ||
|
|
||
| @Test | ||
| fun `test validate endpoint with okta dot-notation client header - full dotted name`() { | ||
| fun `test validate endpoint with format but missing schemaName`() { |
There was a problem hiding this comment.
Might also be a good idea to test against a call if the schema is provided in the query parameters but it can't be found here!
There was a problem hiding this comment.
Good idea, will test and update
|
Left a couple of very small comments, but this looks and works great! I'll defer approval to the rest of the Platform team, though -- thanks so much for hopping on this! ❤️ |
… validateWithSchema endpoint
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
|
Kudos, SonarCloud Quality Gate passed!
|
| HttpUtilities.internalErrorResponse(request) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handles an incoming validation request after it has been authenticated via the /validate endpoint. |
There was a problem hiding this comment.
Slightly pedantic, but is the second part of this comment still valid (authenticated)?
There was a problem hiding this comment.
Good catch! I can update this
| if (sender != null && sender is HasSchema) { | ||
| schema = workflowEngine.metadata.findSchema(sender.schemaName) | ||
| if (schema == null) { | ||
| // Find schema via SCHEMA_PARAMETER parameter |
There was a problem hiding this comment.
Note: You will need to refactor this as some point as this only supports the covid pipeline.
There was a problem hiding this comment.
Noted, created #8073 to track this work.
| @HttpTrigger( | ||
| name = "validate", | ||
| name = "validateWithClient", |
There was a problem hiding this comment.
Why not just detect what parameter is being passed and run the proper validation? You should not have different API endpoints just to have different parameters.
There was a problem hiding this comment.
Thank you for your comment! If we have a single endpoint, then the user-facing documentation will look like so:
Notice, according to the docs, none of the parameters are NOT required, but actually they are. Passing no params is NOT valid, and there are special combos that are accepted:
- Client
- SchemaName + format (these params cannot be provided without the other)
The only way this would be clear to the user is if we explicitly added additional documentation specifying the valid combinations. Breaking it out into separate endpoints solved that:
Another idea may be to use path/route parameters instead, so like:
- api/validate/{client}
- api/validate/{schemaName}/{format}
But I think this is usually considered bad practice because route parameters should identify a resource in the system, for example:
- /users/{id}
- /organizations/{orgId}/members/{memberId}
In our case, the route parameters would not identify a resource, they would just be placing what are conceptually query parameters into the route parameters.
Compromise?
Would something like the following be better?
- api/validate/withClient (pass client in header, like we do today)
- api/validate/withSchema?schemaName="NAME&format="FORMAT"
Any additional thoughts or thoughts to what I said? I think this is a discussion worth having and I am willing to update to whatever makes most sense.
There was a problem hiding this comment.
I should mention, if I am remembering my past experience with Java Spring Boot correctly, I believe you can specify the same endpoint with different query parameters and, with the proper annotations, it shows up in Swagger Documentation as separate endpoints and is very clear what is going on. Azure Function's HTTPTrigger does not support that but my guess is an Azure Web App, if we ever move to it, probably would. The nice thing about Spring Boot was it would automatically check to see if the required parameters were passed in, no additional code needed.
There was a problem hiding this comment.
Perhaps I can do something like the above but manually? Let me see what I can cook up...
There was a problem hiding this comment.
Okay, I combined the endpoints and manually updated the yml to look like so:
probably should have done this in the first place, but didn't know I could put whatever in the path in yml
There was a problem hiding this comment.
Okay so turns out this is invalid openAPI, so I updated per the openAPI spec:
Doing it this way is what I was trying to avoid in the first place but:
- It's the recommended workaround here https://swagger.io/docs/specification/describing-parameters/
- I agree we should not have separate endpoints in this case
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
prime-router/docs/api/validate.yml
Outdated
| '400': | ||
| description: Bad Request | ||
|
|
||
| /validate?schema={schema}&format={format}: |
There was a problem hiding this comment.
Semantic error at paths./validate?schema={schema}&format={format}
Query strings in paths are not allowed.
I suspect we want to use oneOf: with the two different parameters: blocks intead.
There was a problem hiding this comment.
Hm I didn't get that error locally with my IDEA plugin. What are you using to generate this so I can try too? Will look into oneOf!
There was a problem hiding this comment.
Okay I see this in https://editor.swagger.io/. Will try to find a work around, but oneOf not working so far
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated! Unfortunately, there is not a way to do what I want, the official openAPI docs have a section titled Parameter Dependencies here. I followed their suggestion:
OpenAPI 3.0 does not support parameter dependencies and mutually exclusive parameters. There is an open feature request at OAI/OpenAPI-Specification#256. What you can do is document the restrictions in the parameter description and define the logic in the 400 Bad Request response.
In the future, I will make sure to validate swagger changes @ https://editor.swagger.io/
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
| @@ -70,27 +73,40 @@ abstract class RequestFunction( | |||
| routeTo.filter { workflowEngine.settings.findReceiver(it) == null } | |||
| .forEach { actionLogs.error(InvalidParamMessage("Invalid receiver name: $it")) } | |||
|
|
|||
| var sender: Sender? = null | |||
There was a problem hiding this comment.
This new code is not covered by unit tests
There was a problem hiding this comment.
This abstract class, and specifically this method, is exercised via the tests that test its subclasses: ReportFunctionTests and ValidateFunctionTests. The new (and old) ValidateFunctionTests in particular exercise the code highlighted here, through their calls to ValidateFunction.processRequest. We currently do not have any direct unit tests for the abstract class RequestFunction, but if you would like me to test that method directly in this ticket I can. I just didn't see the need to.
There was a problem hiding this comment.
Also, I just simplified this code, moving almost all schema/format logic to getDummySender to reduce redundancy with the logic in ValidateFunction. So, I added tests for getDummySender since its more complicated now.
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
arnejduranovic
temporarily deployed
to
staging
GitHub Actions
Inactive
This PR:
/validateendpoint into/validateWithClientand/validateWithSchema./validateWithSchemais the new feature that allows a user to validate a message without having to know the client, which is hard to do if the endpoint requires no login.Test Steps:
Changes
Checklist
Testing
./prime testor./gradlew testSmokeagainst local Docker ReportStream container?npm run lint:write?Process
Linked Issues
Specific Security-related subjects a reviewer should pay specific attention to
Security Implications:
This PR makes a couple endpoints public, this means we must have rate limiting, which devops has said we have enabled. Furthermore, these endpoints currently do not display and sensitive information.