smart-on-fhir / client-js

JavaScript client for FHIR
Other
294 stars 211 forks source link

Support of non JWTs (JSON Web Tokens), additional authorization params & TS-wrapper #27

Closed ies1 closed 6 years ago

ies1 commented 6 years ago

Hi there

First of all, I would like to thank for the library and the awesome tutorials and documentation on http://docs.smarthealthit.org/authorization/.

The authorization documentation (http://docs.smarthealthit.org/authorization/) in chapter "SMART authorization and resource retrieval" part three "App exchanges authorization code for access token" says:

"Defining the format and content of the access token is left up to the organization that issues the access token and holds the requested resource."

But now, as far as I understood it from debugging, the bb-client ready function expects a jw token (after successful authentication with a "validTokenResponse").

Line 301 of bb-client in src folder: var payloadCheck = jwt.decode(tokenResponse.access_token);

Without a JSON Web Token the app crashes on the next line, cause there is no key 'exp' in the decoded non-jwt token that returned 'null' (means var payloadCheck === null).

I would like to ask, if there is already a plan to make the client-js library work with non-jwts or if there is any workaround?


My second request/question is; is there a way to add additional request body params during the "completeCodeFlow"?

Line 126-130 lines in the bb-client.js:

if (state.client.secret) { 
    headers['Authorization'] = 'Basic ' + btoa(state.client.client_id + ':' + state.client.secret);
} else {
    data['client_id'] = state.client.client_id;
}

Our oAuth 2.0 server expects always the client_id in the body. Even if there is a client_secret. Would it be possible to implement a functionality that makes it possible to select/define which way the params will be sent?

The oAuth 2.0 documentation (https://tools.ietf.org/html/rfc6749) says that the request header with Authorization should be the default way and the parameters in the request body is the alternative.

For example (with extra line breaks for display purposes only):

 `Authorization: Basic czZCaGRSa3F0Mzo3RmpmcDBaQnIxS3REUmJuZlZkbUl3`

Alternatively, the authorization server MAY support including the client credentials in the request-body using the following parameters:

client_id REQUIRED. The client identifier issued to the client during the registration process described by Section 2.2.

client_secret REQUIRED. The client secret. The client MAY omit the parameter if the client secret is an empty string.


In addition, I would like to ask, if there is a plan to make an TypeScript version of this library or if there is a "TS-Wrapper" for it?

Many thanks and greeting

jmandel commented 6 years ago

Thanks for the comments here!

I've captured your first point in https://github.com/smart-on-fhir/client-js/issues/28 -- this is a bug introduced in a recent contribution and we should fix it.

Re: communicating a client id, it's true that the OAuth spec allows it to appear optionally; the SMART spec says something more specific, since it's a profile on OAuth designed to ensure more consistent behavior:

image

Hope this makes sense!

Re: typescript, we aren't currently working on this but would love contributions in this direction. (Overall, it's getting to be time for a ground-up rewrite...)

ies1 commented 6 years ago

Thanks for the quick answer and for opening the issue!

To the client_id: Our server is designed for "public apps". He is more like an EPR than an EHR, where the patients can store personal health-related data from third party apps (like Smartphone apps) and share the data for clinical trials. Therefore, the client id will never be optional. Now the OAuth sever expects the client_id to be always in the request body and not only when no client_secret is set. Atm. most of these developed "public apps" have a secret.

With the doc part above the table you've posted, I now know, that we should not send / define client_secrets for such public apps. So as far as I understand, we should deactivate / delete the secrets of these apps. image

Do I understand this correctly?


TypeScript: I'll send you a PM next week for some discussion and contributions on this point.

jmandel commented 6 years ago

It's correct that if you're only supporting public apps, then there should be no client_secrets involved.