smart-on-fhir / smart-on-fhir.github.io

SMART on FHIR Docs
Other
83 stars 88 forks source link

Spec should clarify that id_token is required, userinfo endpoint is optional for AS OIDC support. #135

Open isaacvetter opened 7 years ago

isaacvetter commented 7 years ago

The spec currently says this:

Some apps need to authenticate the clinical end-user. This can be accomplished by requesting a pair of OpenID Connect scopes: openid and profile.

When these scopes are requested (and the request is granted), the app will receive an id_token that comes alongside the access token.

The spec doesn't mention a userinfo endpoint, which means it's not required by SMART. Right?

kpshek commented 7 years ago

@isaacvetter - The /userinfo endpoint is defined by the OpenID Connect specification and is required if implementing OpenID Connect.

So, if an authorization servers honors the openid and profile scopes (both of which are defined by OpenID Connect), it needs to implement that /userinfo endpoint.

As it is written today, it is not clear if the openid and profile scopes are required when implementing a SMART compliant authorization server. For SMART apps that do not require any knowledge of the user, these scopes are not needed. However, it can be argued that a robust SMART app should have knowledge of the user to implement things like client-side user auditing and authorization.

isaacvetter commented 7 years ago

Hey @kpshek ,

I don't see where the /userinfo endpoint is explicitly required for the OAuth authorization flow. From the OpenID Connect spec:

Isaac

kpshek commented 7 years ago

@isaacvetter - You're correct! Thanks for setting me straight here.

The standard set of claims (in our case, we're just concerned about profile) can be returned in either the /userinfo endpoint or the ID token.

What do you think about SMART be prescriptive as to which should be used (at a minimum)?

isaacvetter commented 7 years ago

What do you think about SMART be prescriptive as to which should be used (at a minimum)? As a matter of fact, I sure do. :)

I believe that the simplest scenario is for the auth server to return the ID token jwt alongside the access_token. This keeps the number of back and forth exchanges to a minimum, fits within the OIDC spec, is simple enough to auth servers and still gives apps the assurance they need for user authentication.

Thoughts?

Isaac

jmandel commented 7 years ago

Effectively we require an id_token (with a profile url claim inside, identifying the FHIR resource with the user details) today.

What we don't say is "any you need to put the user's name, e-mail address, and other profile details into the id_token, too" — we could, but we'd be replicating what's already in the FHIR resource (and it does make the authz server logic a bit more complicated). I'd be okay either way here.

isaacvetter commented 7 years ago

Hey Josh, All,

Talked with Kevin, we came up with this middle approach:

id_token is required. /userinfo is optional. id_token must contain:

id_token may contain profile, which is the url of the current user's FHIR resource. Clients should make use of the profile field, if populated, and the sub field along with other OIDC recommended or implementation-specific user identifiers.

Thoughts?

jmandel commented 7 years ago

I think this is a fair middle ground! Just so I'm clear: what's the argument against mandating that servers must support the "profile" claim? It would certainly help to know there was some consistently available mechanism.

isaacvetter commented 7 years ago

Hey Josh,

Using a FHIR resource (Person, Patient, Practitioner, etc) to represent the logged-in user makes sense for a real FHIR app, but we're some seeing usage of SMART on FHIR for pure single sign-on without any actual FHIR / API callbacks from the app. I don't want to complicate this nice, simple and valuable SSO use-case by requiring a perhaps, less technically capable app developer to also support FHIR if they don't yet need to.

Isaac

jmandel commented 7 years ago

Understood -- and for this case you'd host a /userinfo endpoint? I wonder if we shouldn't just make /userinfo required everywhere. The goal of this line of thinking is simply: could we agree on one thing that's always available, and some extra optional stuff (rather than a whole bunch of things that are all optional).

whitehatguy commented 7 years ago

Section 5.4 of the OIDC specification would imply a UserInfo endpoint would be required when supporting the profile scope (as an access token is returned in all SMART authorization workflows). I had originally thought it was only necessary to include such claims in the id_token. Upon re-reading 5.4, I would anticipate that libraries using the authorization code grant workflow would expect profile claims to come from the UserInfo endpoint.

jmandel commented 7 years ago

Yeah, I think that's the best interpretation, though I find the text slightly ambiguous:

The Claims requested by the profile, email, address, and phone scope values are returned from the UserInfo Endpoint, as described in Section 5.3.2, when a response_type value is used that results in an Access Token being issued. However, when no Access Token is issued (which is the case for the response_type value id_token), the resulting Claims are returned in the ID Token.

I say this just beacuse conformance language like MUST/SHOULD/SHALL is not used here. It's more like a description. It certainly wouldn't seem to violate the spec, for example, to include these claims in an id_token (even if you also host a /userinfo endpoints where the same claims are available).

isaacvetter commented 7 years ago

Hey @jmandel , @whitehatguy ,

I agree that it's always better to require a minimal set of features than have a much larger optional set of features.

I'm hoping to return identity claims in id_token, not support /userinfo and optionally support profile. This seems simple and secure, it's not entirely clear to me that is or isn't valid according to OIDC.

Isaac

whitehatguy commented 7 years ago

@isaacvetter

isaacvetter commented 7 years ago

Hey @whitehatguy , @jmandel , @kpshek, and everyone else interested in this topic.

I'm going to create a PR with the intent of documenting this plan (which also accounts for @kpshek 's #141).

To be conformant with SMART on FHIR, an EHR's authorization server must

Support for the /userinfo endpoint is optional.

What am I missing? Any concerns, feedback, 👍 with this approach?

Isaac

http://openid.net/specs/openid-connect-core-1_0.html

whitehatguy commented 7 years ago

The only clarifications that I think may need to be addressed are:

isaacvetter commented 7 years ago

Hey @whitehatguy ,

Good clarifying questions, here's what I think makes sense:

What does requesting the "profile" scope return?

The granting of the profile scope causes the AS to return the fhirUser claim (#141) within the id_token JWT. The value of fhriUser is an absolute url to the EHR's FHIR resource that best represents the current user.

explicitly required to be returned by the authorization server?

Yes. When the AS grants the id_token scope, it must return the OIDC required keys (iss, sub, aud, exp, iat).

When both the openid and profile scopes are granted, the AS must return the fhirUser claim.

Does the resulting access token provide access to the user's FHIR endpoint when authorized with the "profile" scope?

Good point, it would be pretty frustrating for it not to. Yes, it should. When an AS grants the profile scope, it authorizes the app to retrieve information about the current user. It's likely more information than strictly necessary due to the nature of FHIR's fairly large resources. Any implementation has the choice of representing users with a stripped down Person resource in lieu of a rich Patient or Practitioner resource.

Also, implied above is that the profile scope can't be granted without also granting the openid scope. Optimally AS's will always grant these together, but it would also be valid for an AS to throw an error when an app requests the profile scope without the openid scope.

Any concerns with the above?

yashaskram commented 7 years ago

Based on the PR https://github.com/smart-on-fhir/smart-on-fhir.github.io/pull/144, I see that the current proposal for Auth server is to leverage profile scope to serve fhirUser claim as part of the id_token (fhirUser within id_token jwt as an extension of OIDC spec [custom OIDC claim])

If one of the intent is to provide non-FHIR way to do SSO through SMART on FHIR, then why use profile scope to serve fhirUser claim? Per @whitehatguy 's comment, would it be best to abide by the practices dictated by the available OIDC in supporting profile scope

As proposed in issue #141, can we have a new scope - fhirUser (in addition to profile scope) to serve fhirUser as an OAuth claim similar to other launch parameters (outside id_token jwt). The support of profile and in turn /userinfo endpoint can be optional (this is upto further discussion). Technically app vendor's can simply use the id_token for SSO, based on the sub claim in the OIDC id_token which uniquely identify the user/subject. But I can understand user information either through /userinfo endpoint or fhirUser fhir url would be key for practical implementation of SSO to get user information like name etc in addition to the user identity (sub)

jmandel commented 7 years ago

issuer is different from the url of the authorization server

@isaacvetter What is "the URL of the authorization server"? Does this mean the authorize URL, the token URL, the dynreg URL, or something else?

E.g. one server could have an authorization server at http://authorize.ehr.com with services like http://authorize.ehr.com/authorize, http://authorize.ehr.com/token, and http://authorize.ehr.com/register ; and another server could have services like http://authorize.ehr.com, http://token.ehr.com, and http://register.ehr.com ...

yashaskram commented 7 years ago

Does this mean the authorize URL, the token URL, the dynreg URL, or something else?

It is not auth or token or reg URL but something else. It is issuer which is openid meta data https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata.

Further, the id_token JWT must be signed using a JWS as outlined in the OIDC specification. Per the OIDC specification, required keys in the id_token JWT are: iss, sub, aud, exp, iat.

with above proposal to support jws, the question is how does SMART app get the jwks_uri to inturn get the JSON web key. I think we are in need to incrementally add more open id metadata fields like issuer, jwks_uri to fhir conformance in addition to oauth, token, reg end point uris or instead add the openid base url to the conformance so that SMART app can do .well-know/openid-configuration.

https://id.mitre.org/connect/.well-known/openid-configuration

keithbriantomo commented 6 years ago

Null