-
Notifications
You must be signed in to change notification settings - Fork 0
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
FI-3220 Add scope and aud parameters as optional inputs for authorization code redirect test #16
Conversation
), | ||
optional: true | ||
|
||
input :udap_authorization_code_request_aud, |
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.
Is there a reason not to just use a checkbox input or configuration option for 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.
Yes, I originally attempted a checkbox but then there would have been two inputs for it: the checkbox input to use/not use the aud
param and then the udap_fhir_base_url
(originally input in the Discovery group) to get the actual value needed for the request. I thought it may be kind of confusing when running from the authorization and authentication group level (group 1.3), to have an input for the aud
param value and then also a checkbox to opt to use it even though the value is already present, as opposed to "if you want to use this parameter, provide an input value for it; if it's empty the parameter won't be used," which felt simpler.
That said, I didn't really love any options I thought of for this input, and am open to cleaner suggestions.
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 don't think this should be a distinct input from the base url. So I think I would vote for a checkbox plus base url input.
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 updated the tests to use the checkbox and base url inputs.
Summary
Currently, the authorization code redirect test has no means of providing a
scope
parameter to the authorization endpoint. However, RFC 6749 Section 4.1.1, as referenced in the HL7 UDAP STU 1.0 IG, states that inclusion of ascope
parameter is optional, and testers should therefore be able to provide it as an optional input.This PR adds an optional
scope
input for the authorization code redirect test. It also adds an optional input for theaud
parameter, which is not mentioned in the HL7 UDAP IG or RFC 6749 but is required for SMART App Launch. These changes are effectively doing the same thing as the authorization redirect test created in the SMART-UDAP Harmonization Test Kit.My justification for including the
aud
parameter here, despite it not being required by RFC 6749 or the HL7 UDAP IG, is that it would reduce duplicated code across test kits. If this PR is merged, then the SMART-UDAP test kit can just import this updated test from the baseline UDAP test kit instead of having its own redirect test. It would also make the redirect test more reusable in other UDAP test contexts where an implementation is likely to support both SMART and UDAP.If we really don't want to allow testers to add an
aud
parameter in this test kit, we can always configure that input to be locked at the group level.Testing Guidance
Unit tests were updated to account for changes. We don't have a reference implementation to run against, but inputs can be seen in the web UI.