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

Follow the specs more closely, switch to JWTs.jl, and fix the tests #9

Merged
merged 11 commits into from
Feb 14, 2024

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Jan 18, 2024

This is a larger PR that extends #8:

  • The package follows the specs (https://hl7.org/fhir/smart-app-launch/backend-services.html) more closely: Token endpoint is retrieved from the server configuration available at .well-known/smart-configuration (instead users have to specify the arguably more intuitive base URL of the server), iss and sub are enforced to be the same client_id, and it is mandatory to specify the scope
  • The package is switched to JWTs.jl which, in contrast to JSONWebTokens.jl, already supports header claims required by the specs
  • The PR fixes the tests with the public SMART test server (and adds documentation)

Tests fail currently due to tanmaykm/JWTs.jl#25: https://github.com/JuliaHealth/SMARTBackendServices.jl/actions/runs/7563819763/job/20596913050?pr=9#step:6:128

With the fix in tanmaykm/JWTs.jl#25, tests pass locally.

@devmotion devmotion force-pushed the dw/follow_specs branch 2 times, most recently from 5df875a to d4b5e30 Compare January 18, 2024 01:07
@DilumAluthge
Copy link
Member

So, if I recall correctly, not all servers support /.well-known/smart-configuration. So I think we should first check /.well-known/smart-configuration, but if that doesn't exist (or if it exists but we can't extract all of the required info), we should still fallback to checking /metadata.

@devmotion
Copy link
Member Author

devmotion commented Jan 18, 2024

I could not find the metadata endpoint in the specification (only in Epic-internal docs) whereas .well-known/smart-configuration is explicitly documented and part of the suggested implementation: https://hl7.org/fhir/smart-app-launch/backend-services.html#top-level-steps-for-backend-services-authorization (Therefore I think we should also use it rather than metadata in the EHR Launch implementation.)

@DilumAluthge
Copy link
Member

DilumAluthge commented Jan 18, 2024

I could not find the metadata endpoint in the specification

For the /metadata endpoint in general, the spec is here:

  1. FHIR 4.0.1: https://hl7.org/fhir/R4/http.html#capabilities
  2. FHIR 5.0.0: https://hl7.org/fhir/R5/http.html#capabilities
  3. CI build of FHIR: https://build.fhir.org/http.html#capabilities

For the CapabilityStatement (which is the resource type returned by the /metadata endpoint), the spec is here:

  1. FHIR 4.0.1: https://hl7.org/fhir/R4/capabilitystatement.html
  2. FHIR 5.0.0: https://hl7.org/fhir/R5/capabilitystatement.html
  3. CI build of FHIR: https://build.fhir.org/capabilitystatement.html

For the specific use of the CapabilityStatement in SMART App Launch, you can look at historical versions of the relevant IGs (implementation guides), such as:

  1. SMART App Launch: https://hl7.org/fhir/smart-app-launch/2021May/ (based on FHIR R4)

The reality is that there are still R4 systems out there in the wild that only support /metadata and don't support well-known, so I do think that we need to continue using /metadata as a fallback, at least for the foreseeable future.

@devmotion
Copy link
Member Author

For the specific use of the CapabilityStatement in SMART App Launch, you can look at historical versions of the relevant IGs (implementation guides), such as:

1. SMART App Launch: https://hl7.org/fhir/smart-app-launch/2021May/ (based on FHIR R4)

Even in this document the .well-known/smart-configuration endpoint is mentioned. In fact, it seems that already since version 1.0.0 (from 2018, based on R3) the specification requires that this endpoint exists and reports the token_endpoint: https://hl7.org/fhir/smart-app-launch/1.0.0/conformance/index.html#using-well-known

The reality is that there are still R4 systems out there in the wild that only support /metadata and don't support well-known, so I do think that we need to continue using /metadata as a fallback, at least for the foreseeable future.

I'm sure you have better experience with these systems, so maybe that's what we have to do regardless of the specifications.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (465e6ad) 100.00% compared to head (337cccb) 94.20%.

Files Patch % Lines
src/backend_services.jl 90.90% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master       #9      +/-   ##
===========================================
- Coverage   100.00%   94.20%   -5.80%     
===========================================
  Files            5        4       -1     
  Lines           38       69      +31     
===========================================
+ Hits            38       65      +27     
- Misses           0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/backend_services.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member Author

@DilumAluthge the PR is ready for a re-review, I added a fallback with the metadata endpoint and a test for it.

# Extract the token endpoint from the JSON response
# Ref: https://hl7.org/fhir/smart-app-launch/1.0.0/conformance/index.html#declaring-support-for-oauth2-endpoints
# Ref: https://hl7.org/fhir/R4/capabilitystatement.html
compat_statement = JSON3.read(_metadata_response.body)
Copy link
Member

Choose a reason for hiding this comment

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

Should we use struct-mapping instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial implementation used struct mapping but it felt a bit too verbose since it requires a struct for every level of this nested structure.

test/key/public_jwkset.json Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member

@devmotion Looks like you need some tests to increase code coverage, and then we're good to go here?

@DilumAluthge
Copy link
Member

I've removed Codecov from the list of required checks.

@devmotion devmotion merged commit 84ead00 into master Feb 14, 2024
4 of 6 checks passed
@devmotion devmotion deleted the dw/follow_specs branch February 14, 2024 22:43
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