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

Controlling state in SMART launch #112

Open
scott-nordicwi opened this issue Feb 4, 2021 · 7 comments
Open

Controlling state in SMART launch #112

scott-nordicwi opened this issue Feb 4, 2021 · 7 comments

Comments

@scott-nordicwi
Copy link

The State parameter is a useful tool to control session data across the SMART launch. For example url paths can be layered onto the base url to indicate that a particular organization or user type is performing the launch, and that data can be stored in the session so it's available after the redirect. This isn't possible if the State value cannot be controlled.

Could we add code to support inclusion of state as a parameter to the authorize() function then set stateKey to the state parameter if it exists? This shouldn't impact current code, but will allow additional development flexibility.

@vlad-ignatov
Copy link
Collaborator

Sounds like an interesting use-case but I'm not sure I'm following. Can you provide a pseudo code example of what you would like to be able to do?

@scott-nordicwi
Copy link
Author

Sure. This is a paradigm I've used in SMART apps a few times. The redirct_uri set in EHR environments has to match exactly with what's passed in so url query parameters cant be added dynamically. This means there's no way to pass data between routes without maintaining some sort of session on the client or server side. This can be done via cookies on the client side, which can be unreliable with EHR-based browsers. Cacheing the data on the server side and using the State value as the key to store and retrieve that data across routes allows you to maintain data across the launch process.

route(/launch?healthcareOrgId=1&otherimportantvariable=2)
create a state value
cache the launch query parameters using the state as the key for the cached object, e.g. in Redis or an SQL table
create the SMART fhir client and pass in the state variable, which returns a redirect to /app

route(/app)
executes typical fhirclient setup
retrieve cached data using the state parameter from the fhirclient. This allows you to identify the SMART login with a specific healthcare org, or use the other important variables passed at launch.

@vlad-ignatov
Copy link
Collaborator

Well, this is how it is supposed to work, except that it uses sessionStorage in the browser instead of cookies. That "state" is created at launch time (when authorize is called) and available after the redirect... The library creates an unique key for that state, so the way to access it could be something like:

const key = sessionStorage.SMART_KEY; // random string
const state = sessionStorage[key];

I don't think you wan't to be able to customize that key name just to get access to the key. What you really need is a way to augment that state. Even though it is theoretically possible to access and modify the state, that might not work in practice because the state is created by a function which will redirect immediately after that...

With that said, what sounds like a better idea to me is to add a placeholder for custom data in the state. That might look like so:

FHIR.oauth2.authorize({
    clientId: "my_web_app",
    scope: "patient/*.read",
    data: { /* my custom data */ }
});
// later...
FHIR.oauth2.ready()
    .then(client => client.getState(”data”))
    .then(console.log)
    .catch(console.error);

What do you think about that?

@scott-nordicwi
Copy link
Author

To be clear, I'm referring to the state string value that is a uuid, not the full object the library refers to as State in getState().

My preference would be that this library just manages the SMART auth concepts. If you allow us to optionally pass in the state uuid then it creates a less "opinionated" experience and shouldn't introduce any breaking change on your side, because we're just replacing an auto-generated value with an optional parameter.

I think the library would be more flexible if it allowed the user to dictate the state value. My use-case is probably the primary driver of this, but I think session management outside of the client object can be useful for other purposes. Allowing the user to pass in a state in lieu of it being auto-generated achieves that.

Your proposed architecture only works if I have access to the fhir.oauth2 client object. I'm currently working on an Auth0 authentication mechanism that uses additional redirects after obtaining the fhir.oauth2 client object. I need some information available from this fhir.oauth2 client object in those routes though. So ultimately I need to use a second session storage mechanism, and I'd prefer to establish that at the initiation of the launch via the State key so I can use the same identifiers throughout the process.

@vlad-ignatov
Copy link
Collaborator

OK, that makes sense. It may be more complicated than it looks though.

  1. If you have control over the state object, it would be your responsibility to maintain it properly. In other words, the library would assume that it can find everything where it used to be.
  2. The bigger problem is that the library is designed to assume that the state is it's own and nobody else can modify it. This might lead to some problems. For example, every time authorize is called it clears any previous state (if any) and creates a brand new one so that the app can be re-authorized. This means that if you add your own stuff to the state and re-visit your launch route, your additions will be lost. How would you address that?

@pnutshellmenace
Copy link

Wondering if this idea can be revived. Using the state parameter in OAuth2 flows is a common way to maintain state throughout the handshake process. A standard OAuth2 server will pass back whatever you pass to it in the state parameter during the authorize request. You can pass a more complicated JSON object by doing JSON.stringify and then base-64 encoding it as the state parameter. For example:

Object state:
{ "uuid": "d1b42a32-73df-4473-ad04-44175d99813f", "referral_code": "ABC" }
Base64 encoded:
eyAidXVpZCI6ICJkMWI0MmEzMi03M2RmLTQ0NzMtYWQwNC00NDE3NWQ5OTgxM2YiLCAicmVmZXJyYWxfY29kZSI6ICJBQkMiIH0K

If &state=eyAidXVpZCI6ICJkMWI0MmEzMi03M2RmLTQ0NzMtYWQwNC00NDE3NWQ5OTgxM2YiLCAicmVmZXJyYWxfY29kZSI6ICJBQkMiIH0K is passed during the authorize request, when it comes back to the redirect_uri, you can base-64 decode and JSON.parse to get the original object back.

The current implementation of the authorize function in this library always generates stateKey as a random 16-character string. It would be nice to have the option of passing in an alternate value to use for the stateKey so that the state parameter can be more useful for situations like this. It could be an optional parameter and the responsibility of the caller to ensure a unique value is passed in (achieved in the above example by using a random uuid as part of what is encoded).

We currently attempt to maintain additional state by setting a cookie before the authorize request and then reading it back in during the completion step of the authorization. This is unreliable in many instances, especially with embedded browsers in some EHRs, which will not preserve cookies used in this way.

@art1c0
Copy link

art1c0 commented Mar 25, 2023

Guys, have you found any solution how to overcome the single SMART_KEY limitation without forking?

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

No branches or pull requests

4 participants