FI-2496 Discovery Test Updates #11
Conversation
…to match IG requirements
|
@Jammjammjamm I still have 2 remaining questions for this ticket:
|
There was a problem hiding this comment.
Do you think the tests should be re-ordered? I maintained the existing test order and added new ones at the end because I would have had to manually re-order the spec tests as well.
We can hold off on that for now. Once this is in, I think we should do a big reorganization to extract all these tests to individual files and give them real ids. Then we can change the order however we like.
Is it worth setting up some mock certs and a JWT to unit test the signed_metadata contents test, either now or in the future? On one hand this is a lot of effort, but on the other hand we'll have to do this to a certain extent anyways when creating a software statement in the dynamic client registration tests.
Let's hold off on it for now, but make a ticket so we don't forget.
| @@ -9,7 +9,7 @@ class DiscoveryGroup < Inferno::TestGroup | |||
| description %( | |||
| Verify that server configuration is made available and conforms with [the | |||
| discovery | |||
| requirements](https://build.fhir.org/ig/HL7/fhir-udap-security-ig/discovery.html). | |||
| requirements](https://hl7.org/fhir/us/udap-security/discovery.html). | |||
There was a problem hiding this comment.
When linking to an IG, we need to make sure to use a version-specific link rather than one that just always points to the most recent version.
| @@ -102,7 +102,9 @@ def assert_array_of_strings(config, field) | |||
| assert_valid_json(config_json) | |||
| config = JSON.parse(config_json) | |||
|
|
|||
| omit_if !config.key?('udap_certifications_required') | |||
| skip_if !config.key?('udap_certifications_supported'), 'Assessment of `udap_certifications_required` field is dependent on values in `udap_certifications_supported` field, which is not present' | |||
There was a problem hiding this comment.
A skip is equivalent to a fail. Wouldn't this be an omit?
|
|
||
| assert_array_of_strings(config, 'grant_types_supported') | ||
|
|
||
| grant_types = config['grant_types_supported'] | ||
|
|
||
| assert grant_types.length() >= 1, "Must include at least 1 supported grant type" |
There was a problem hiding this comment.
| assert grant_types.length() >= 1, "Must include at least 1 supported grant type" | |
| assert grant_types.present?, "Must include at least 1 supported grant type" |
| @@ -175,7 +177,11 @@ def assert_array_of_strings(config, field) | |||
| assert_valid_json(config_json) | |||
| config = JSON.parse(config_json) | |||
|
|
|||
| omit_if !config.key?('authorization_endpoint') | |||
| skip_if (!config.key?('grant_types_supported') || !config['grant_types_supported'].is_a?(Array)), 'Assessment of `authorization_endpoint` field is dependent on values in `grant_types_supported` field, which is not present or correctly formatted' | |||
There was a problem hiding this comment.
This again seems like an omit rather than a skip.
I think this should be broken down into to separate conditions so that the message can be less awkward. Same with the authorization extensions required test below.
|
|
||
| algs_supported = config['token_endpoint_auth_signing_alg_values_supported'] | ||
|
|
||
| assert algs_supported.length() >= 1, 'Must support at least one signature algorithm' |
There was a problem hiding this comment.
| assert algs_supported.length() >= 1, 'Must support at least one signature algorithm' | |
| assert algs_supported.present?, 'Must support at least one signature algorithm' |
|
|
||
| assert profiles_supported.include?('udap_authn'), 'Array must include `udap_authn` value to indicate support for required UDAP JWT-Based Client Authentication profile' | ||
|
|
||
| if (config.key?('grant_types_supported') && config['grant_types_supported'].include?('client_credentials')) |
There was a problem hiding this comment.
| if (config.key?('grant_types_supported') && config['grant_types_supported'].include?('client_credentials')) | |
| if (config['grant_types_supported']&.include?('client_credentials')) |
Summary
Updated discovery tests and specs to conform to the requirements in the most recent published version of the UDAP/Security for Scalable Registration, Authentication, and Authorization IG (STU 1.0.0).
Summary of changes includes:
Testing Guidance
There is no reference server to run these tests against but they can be visualized by running the web browser.
Spec tests were completed for all tests except the
signed_metadatacontents test, which would require a mocked cert, and can be run in the root project directory withbundle exec rspec.