fhir-sdk
fhir-sdk copied to clipboard
SMART on FHIR implementation & example
See https://github.com/translating-science/rust-smart-fhir (thanks for the notification).
Some integration into fhir-sdk would be great.
I think I would like to change the auth callback a little bit:
- Make it a Box instead of Arc
- FnMut instead of Fn
- Lock it for the whole period of being called, so only one runes at a time
- Patch request settings without overwriting the whole thing
- Make sure auth_callback isn't called directly again, because someone was waiting on the lock (try_lock and if locked, wait to reuse request settings then, request settings need to be modified while auth_callback is still locked)
- Do we need RwLock for request settings then? Investigate
More orthogonal possibilities:
- Make auth callback a trait and allow both structs and closures/functions. This allows easier state management for auth handling.
- Pass reqwest client to auth callback?
Alright, I addressed all of the ideas from the previous two comments and revamped the authentication handling. It should now be way easier to implement custom auth logic and SMART on FHIR.
I would imagine there could be a struct that holds the SMART configuration and implements LoginManager. Whenever authentication is needed, it can decide whether it refreshes a token or logs in completely fresh based on internal state. The reqwest Client is also already given. Should be quite simple to just add the SMART login as a struct anyone can use by registering it in the auth_callback with the necessary configuration.
Thanks @FlixCoder ! I was able to refactor my code on top of your changes to auth_callback here --> https://github.com/translating-science/rust-smart-fhir/commit/19cf3421fe51dbc6a57e03de0cb2d0a90defa702. The LoginManager changes you made make the implementation much cleaner, so thank you for that.
It is a little cumbersome to reason about locking, especially if you have to refresh the token since this requires an async call to the SMART-on-FHIR token endpoint. I have that implemented here --> https://github.com/translating-science/rust-smart-fhir/commit/19cf3421fe51dbc6a57e03de0cb2d0a90defa702#diff-1730a1cc2e39aa55ce957569d5807f1da1796686531887ddd124875026d431c6R157-R204. I feel like the lock / unlock / attempt refresh / relock pattern has the potential to cause a bug. Would love your thoughts on that.
I would imagine there could be a struct that holds the SMART configuration and implements LoginManager. Whenever authentication is needed, it can decide whether it refreshes a token or logs in completely fresh based on internal state. The reqwest Client is also already given. Should be quite simple to just add the SMART login as a struct anyone can use by registering it in the auth_callback with the necessary configuration.
That is similar to the pattern I've been using. What I've done is:
- Stuff the SMART configuration into a struct here
- The SMART configuration gets bundled into the token
Since the reqwest client is needed to get the initial token, I've also taken the liberty of bundling it into the token. As such, I wind up not using the HTTPClient that is provided to the LoginManager.authenticate call.
It is a little cumbersome to reason about locking, especially if you have to refresh the token since this requires an async call to the SMART-on-FHIR token endpoint.
Why do you have the lock in there actually? The FHIR client does already lock and provide the guarantee to only run one at a time and such. The LoginManager gets mut acces to itself, so I would recommend just keeping the data you need in there directly. So no locking to check if the token is still valid.
In case you need to share some data with a handler in your webserver, I would suggest sharing and locking only that. But that would not have any serious footguns or complications I think. Well unless you lock it, send a request to your server and try to lock it there as well.. :D
As such, I wind up not using the HTTPClient that is provided to the LoginManager.authenticate call.
The HTTPClient that you are provided in the login manager IS a reqwest client and it is the same that you provide in the FhirClient builder ^^
Thanks for your thoughts above! You are correct and making those changes cleans up the code significantly. I've made the changes here --> https://github.com/translating-science/rust-smart-fhir/pull/12
So do you think it makes sense to have some struct for all possible smart of fhir logins? Is this possible? Or are there custom flows like Patient/Practitioner workflows or whatever?
I'm trying to figure that out a bit more. What I've found is that the fields returned by some of the API calls appear to be dependent on the scopes that are requested, and there's some difference between the EHR launch and standalone flows. I've also seen some references in the Cerner SMART-on-FHIR docs that imply there may also be minor API differences that are EHR vendor specific (I think these would be additional fields). I'm hoping to do testing in the Epic and Cerner sandboxes over the next week.