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

OZ-528: Add Oauth2 authentication support to openmrs-fhir component. #57

Merged
merged 3 commits into from
May 8, 2024

Conversation

corneliouzbett
Copy link
Member

This PR introduces Oauth2 authentication support to the openmrs-fhir component.
The OpenmrsFhirConfiguration class has been updated to include a condition to check if Oauth is enabled and accordingly set the Authorization header for the FHIR client.

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Nice work! A few high-level comments:

  • This will fetch a new OAuth2 token for every request. Since OAuth2 tokens have an expiry, it would be better if we could cache the token for a period of time (because without that, this effectively means that each FHIR API request requires two HTTP requests).
  • The OauthProcessor checks that the returned token_type in the response is, in fact, a bearer token. While, for the basic Ozone setup, this should work, it may be best to double-check that.
  • It would be good in the OpenmrsFhirOauth2 component to actually check that the supplied values are present if OAuth is enabled (or at least when calling fetchAuthToken(), since otherwise, this is likely to generate very opaque error messages.

Copy link
Member

@Ruhanga Ruhanga left a comment

Choose a reason for hiding this comment

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

Do we have to reinvent the wheel, why not simply reuse the existing OauthProcessor to handle the auth processing (at-least everything involving the Bearer Token)?

@corneliouzbett
Copy link
Member Author

Do we have to reinvent the wheel, why not simply reuse the existing OauthProcessor to handle the auth processing (at-least everything involving the Bearer Token)?

The OauthProcessor was designed to be used within routes. It's structured in such a way that it fetches the token as part of a route. Leveraging this means invoking the producer template/accessing camelContext within the interceptor and then extracting the token from the message body. This could potentially introduce unnecessary complexity.

@rbuisson rbuisson merged commit 9f77a02 into master May 8, 2024
2 checks passed
@corneliouzbett corneliouzbett deleted the OZ-528 branch May 9, 2024 06:46
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.

4 participants