smart-on-fhir.github.io icon indicating copy to clipboard operation
smart-on-fhir.github.io copied to clipboard

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

Open isaacvetter opened this issue 7 years ago • 20 comments

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?

isaacvetter avatar Mar 24 '17 22:03 isaacvetter

@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.

kpshek avatar Apr 17 '17 18:04 kpshek

Hey @kpshek ,

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

  • Nothing in section 5.3, which you reference, says that the actual endpoint is required, only what's required to implement the endpoint
  • See 5.1, which says "This specification defines a set of standard Claims. They can be requested to be returned either in the UserInfo Response or in the ID Token"
  • Additionally, 15.1 doesn't identify the endpoint as required, and further, says that Signing tokens with RSA SHA 256 is required unless the OP only supports returning ID Tokens from the Token Endpoint (as is the case for the Authorization Code Flow). Which strongly implies a UserInfo endpoint is not required in general.

Isaac

isaacvetter avatar Apr 18 '17 16:04 isaacvetter

@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)?

kpshek avatar Apr 18 '17 18:04 kpshek

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

isaacvetter avatar Apr 18 '17 19:04 isaacvetter

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.

jmandel avatar Apr 18 '17 21:04 jmandel

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?

isaacvetter avatar May 31 '17 15:05 isaacvetter

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.

jmandel avatar Jun 01 '17 01:06 jmandel

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

isaacvetter avatar Jun 01 '17 16:06 isaacvetter

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).

jmandel avatar Jun 02 '17 13:06 jmandel

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.

whitehatguy avatar Jun 02 '17 14:06 whitehatguy

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).

jmandel avatar Jun 02 '17 14:06 jmandel

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

isaacvetter avatar Jun 02 '17 14:06 isaacvetter

@isaacvetter

  • I'd agree that returning the values in the id_token is both simple and secure.
  • I'd also agree it's difficult to determine if it is or isn't valid within the OIDC specification.
  • If we look at this through the lens of an implementer, the OIDC relying party implementer's guide (draft....) is fairly explicit that the point of a resulting access_token is for the express purpose of accessing the UserInfo endpoint. In that regard, not having a UserInfo endpoint may be confusing for solutions that "use the OIDC playbook" in their implementation. If the intent of having the feature is for non-FHIR developers, then perhaps it's best to abide by the practices dictated by the available OIDC implementation guide - it would be our best bet to have compatibility with existing libraries, frameworks, etc. designed to work with OIDC.

whitehatguy avatar Jun 02 '17 15:06 whitehatguy

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 the openid and profile scopes.
  • return an id_token JWT alongside the access_token, when the openid scope is granted to the app. 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. This list may be extended with standard or implementation specific keys. The app must validate the signature of the ID Token according the JWS algorithm. In the case where the JWS alg is "none", the app must reject any claims where the issuer is different from the url of the authorization server. (3.2.2.11)
  • Return the SMART-specific fhirUser claim (see #141) with a url to a FHIR resource representing the current user, within the id_token JWT, when the profile scope is granted to the app.

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

isaacvetter avatar Jul 14 '17 21:07 isaacvetter

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

  • What does requesting the "profile" scope return? Is it the demographics in the id_token and/or the user's FHIR endpoint, and are any of these explicitly required to be returned by the authorization server?
  • Does the resulting access token provide access to the user's FHIR endpoint when authorized with the "profile" scope?

whitehatguy avatar Jul 17 '17 13:07 whitehatguy

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?

isaacvetter avatar Jul 26 '17 20:07 isaacvetter

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)

yashaskram avatar Aug 14 '17 18:08 yashaskram

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 ...

jmandel avatar Sep 09 '17 22:09 jmandel

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

yashaskram avatar Sep 10 '17 00:09 yashaskram

Null

keithbriantomo avatar May 23 '18 08:05 keithbriantomo