Skip to content
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-3175 Allow testers to specify new or updated client registration attempt #17

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

alisawallace
Copy link
Collaborator

@alisawallace alisawallace commented Dec 18, 2024

Summary

Currently, the dynamic registration success tests assumes all registration attempts are for a new client and expect a 201 response code returned, per IG requirements. This PR allows testers to specify via a test input whether the registration tests are registering a new client or updating an existing client's data. For new clients, the registration success test will require a 201 response code returned; for updated clients, the test will require either a 200 or 201 response code, since the IG states that servers SHOULD (rather than SHALL) return a 200 code for a successful registration modification.

Even though the HL7 UDAP IG does not require servers to support registration modifications, adding support for this capability in the registration tests makes the testing process much easier for system that do support this capability: testers no longer need to manually remove a client's registration entry on their authorization server, or procure a new certificate/issuer identity, to run registration tests more than once. Systems that do not support registration modifications would still be required to make any necessary adjustments so that a 201 Created response is returned on successive registration success test runs.

Testing Guidance

Unit tests have been updated and are passing. Input instructions and test descriptions have also been updated and can be viewed in the web UI.

if udap_client_registration_status == 'new'
assert_response_status(201)
elsif udap_client_registration_status == 'update'
assert_response_status(200)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is considerably more strict than the spec, which just says:

If the Authorization Server returns the same client_id in the registration response for a modification request, it SHOULD also return a 200 OK HTTP response code.

Copy link
Collaborator Author

@alisawallace alisawallace Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt more comfortable with this strict behavior because this is really more of a convenience option for testers that they can opt in to use, rather than a behavior the tests are requiring them to demonstrate. The only thing they are technically required to demonstrate is that a new registration returns a 201 response, it's just that in practice this can be difficult to do (especially in repeated test runs) since we're always working with a limited set of client identities that are likely to already be in the system.

However, we could expand the acceptable range of valid response status codes for the "update" case to make it a little more flexible (accept either 201 or 200? Anything in the 200 range?) What I don't want to do is open it up too much to where the test can technically pass when the modification attempt didn't actually succeed. The next test (registration success contents) would likely catch that but still.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 201 or 200 for an update would be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I updated the code, unit tests, and descriptions to reflect this.

@alisawallace alisawallace merged commit c1301a0 into main Dec 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants