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

Update smart-configuration and metadata for SMART on FHIR #3697

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

namalu
Copy link
Member

@namalu namalu commented Feb 1, 2024

Description

  • Updates to the behavior of the .well-known/smart-configuration endpoint to better support SMART on FHIR.
  • Updates to the behavior of the capability statement to better support SMART on FHIR.

Testing

  • New unit tests have been developed to ensure the code quality of new functionality and guard against regressions.
  • Existing unit tests have been updated to cover new functionality.
  • Manual testing was performed to verify expected functionality.
  • ONC Certification (g)(10) Standardized API 1.1 SMART on FHIR Discovery tests were executed to ensure conformance.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

Sorry, something went wrong.

@namalu namalu requested a review from a team as a code owner February 1, 2024 21:58
@namalu namalu added this to the S134 milestone Feb 8, 2024
@namalu namalu added Enhancement-Refactor Refactor on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Schema Version unchanged labels Feb 8, 2024
@namalu namalu changed the title Update smart-configuration endpoint behavior Update smart-configuration and metadata for SMART on FHIR Feb 8, 2024
Copy link
Contributor

@mikaelweave mikaelweave left a comment

Choose a reason for hiding this comment

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

Some preliminary comments on the code, haven't looked at tests yet.


using HttpClient client = _httpClientFactory.CreateClient();
using var smartConfigurationRequest = new HttpRequestMessage(HttpMethod.Get, configurationUrl);
HttpResponseMessage response = await client.SendAsync(smartConfigurationRequest, cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you leave Auth/Audience blank in appsettings, and simply run the OSS server, and then hit the .well-known/smart-configuration endpoint, you get an endless loop as the server makes a request against it's own endpoint :).

I also wonder if we can't rely on an Oauth compliant IDP and then provide the "smart" compliance ourselves? I am concerned that there may be few full "smart" compliant IDPs and we may limit the usability of this if we can't augment Oauth to provide a well-known/smart-configuration endpoint from the FHIR service, which in turn relies on the .well-known/openid-configuration of the IDP.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you leave Auth/Audience blank in appsettings, and simply run the OSS server, and then hit the .well-known/smart-configuration endpoint, you get an endless loop as the server makes a request against it's own endpoint :).

I guess I'm not sure why this would happen. If you leave Authority blank, does the SecurityConfiguration.Authentication.Authority value get populated some other way? The GetConfigurationUrl method below expects that if both values are null or empty, null should be returned here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Security is true, but no Auth or Audience is specified we use an in memory Identity Provider which loads users, roles and permitted data actions from role.json. This is for testing purposes. It will publish an authorize and token endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also wonder if we can't rely on an Oauth compliant IDP and then provide the "smart" compliance ourselves? I am concerned that there may be few full "smart" compliant IDPs and we may limit the usability of this if we can't augment Oauth to provide a well-known/smart-configuration endpoint from the FHIR service, which in turn relies on the .well-known/openid-configuration of the IDP.

I have a solution for this and we can talk about where this solution belongs (either this repository or a separate one). The main issue with SMART is that the response from .well-known/smart-configuration contains capabilities representing both the FHIR server and the IDP. The capabilities of the FHIR server and the IDP need to be combined by the implementor. This probably means that there are settings that need to be configured depending on the IDP.

Copy link
Member Author

Choose a reason for hiding this comment

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

If Security is true, but no Auth or Audience is specified we use an in memory Identity Provider which loads users, roles and permitted data actions from role.json. This is for testing purposes. It will publish an authorize and token endpoint.

I added a condition to address this specific case. Let me know what you think.


_messageHandler = new TestHttpMessageHandler();
_httpClientFactory = Substitute.For<IHttpClientFactory>();
_httpClientFactory.CreateClient().Returns(new HttpClient(_messageHandler));

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable

Disposable 'HttpClient' is created but not disposed.
@feordin feordin force-pushed the personal/namalu/smart-configuration branch from e1b5d68 to 2b8e616 Compare September 20, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement-Refactor Refactor on existing functionality. Schema Version unchanged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants