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

Auth info #17

Merged
merged 5 commits into from
Oct 17, 2024
Merged

Auth info #17

merged 5 commits into from
Oct 17, 2024

Conversation

vanessuniq
Copy link
Contributor

@vanessuniq vanessuniq commented Sep 18, 2024

Summary

Update to use auth info and fhirpath evaluator feature from core.

Testing Guidance

bundle install and run server tests locally. Ensure the auth credentials display correctly and validation tests using fhirpath works. See more details about core FHIRPath evaluator, AuthInfo

Screenshot 2024-09-25 at 10 41 08 AM

@vanessuniq vanessuniq marked this pull request as ready for review September 25, 2024 15:05
Copy link
Contributor

@tstrass tstrass left a comment

Choose a reason for hiding this comment

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

This is not related to this PR, but I noticed that suite generation is slightly out of date (non-functional diffs in 4 files), so might make sense to just bring it up to date while we have an open PR.

Edit: Nevermind, this'll be a separate ticket

lib/davinci_pas_test_kit/pas_bundle_validation.rb Outdated Show resolved Hide resolved
lib/davinci_pas_test_kit/generator/templates/suite.rb.erb Outdated Show resolved Hide resolved
Signed-off-by: Vanessa Fotso <[email protected]>
@vanessuniq vanessuniq requested a review from tstrass October 14, 2024 13:11
@karlnaden
Copy link
Collaborator

Trying to read through the linked details on the new auth_info input type and having trouble due to formatting issues on the class page - is that something that is being addressed in core, or should I create a ticket?

@karlnaden
Copy link
Collaborator

Is the Public Auth Type the right default for the PAS server tests? The PAS connection seems more like a backend services type connection, but I don't know enough about SMART yet to be certain? Do you have a sense @vanessuniq and if you agree, could the default selection be updated?

@vanessuniq
Copy link
Contributor Author

Trying to read through the linked details on the new auth_info input type and having trouble due to formatting issues on the class page - is that something that is being addressed in core, or should I create a ticket?

Yes, there's an open PR that addresses this.

@vanessuniq
Copy link
Contributor Author

Is the Public Auth Type the right default for the PAS server tests? The PAS connection seems more like a backend services type connection, but I don't know enough about SMART yet to be certain? Do you have a sense @vanessuniq and if you agree, could the default selection be updated?

Yes, backend services seems more appropriate. I updated to make backend services the default.

@karlnaden
Copy link
Collaborator

Selecting the reference implementation preset enables some strange additional details in the auth info input. Can this be cleaned up in this test kit, or is a core issue?

image

@vanessuniq
Copy link
Contributor Author

vanessuniq commented Oct 15, 2024

Selecting the reference implementation preset enables some strange additional details in the auth info input. Can this be cleaned up in this test kit, or is a core issue?

image

This is the correct behavior. Depending on the auth type and mode, different input fields need to be displayed. The extra fields are the fields the FHIR client may need to automatically refresh the token or request another access token in case the current one has expired. This the logic we used for access mode:

# Access mode
access:
  public:
    - access_token:
    - refresh_token:
# Hide all of these if refresh token is blank
    - client_id:
    - token_url:
    - issue_time:
    - expires_in:

  symmetric_confidential:
    - access_token:
    - refresh_token:
# Hide all of these if refresh token is blank
    - client_id:
    - client_secret:
    - token_url:
    - issue_time:
    - expires_in:

  asymmetric_confidential:
    - access_token:
    - refresh_token:
# Hide all of these if refresh token is blank
    - client_id:
    - token_url:
    - encryption_algorithm:
    - jwks:
    - kid:
    - issue_time:
    - expires_in:

  backend_services:
    - access_token:
# Hide all of these if access token is blank (There's no refresh token for backend services)
    - client_id:
    - token_url:
    - encryption_algorithm:
    - jwks:
    - kid:
    - issue_time:
    - expires_in:

Though, I see that client_id, token_url, and encryption_algorithm are currently missing. I created a ticket to fix that.
Note that all these extra fields are optional.

@karlnaden
Copy link
Collaborator

Selecting the reference implementation preset enables some strange additional details in the auth info input. Can this be cleaned up in this test kit, or is a core issue?

This is the correct behavior. Depending on the auth type and mode, different input fields need to be displayed. The extra fields are the fields the FHIR client may need to automatically refresh the token or request another access token in case the current one has expired.

sounds good - thanks for explaining.

@karlnaden
Copy link
Collaborator

the preset for the RI server still uses the old outh_credentials type for the smart_credentials input. Looks like it doesn't cause a functional problem, but should still be updated to match.

Copy link
Collaborator

@karlnaden karlnaden left a comment

Choose a reason for hiding this comment

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

update the preset type and its good

@vanessuniq vanessuniq merged commit b58e95c into main Oct 17, 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.

3 participants