cds-hooks / docs

CDS Hooks website & specification
http://cds-hooks.org
Apache License 2.0
166 stars 61 forks source link

Specify that an implementation specific tenant/billing_code can be added to the JWT #205

Closed isaacvetter closed 6 years ago

isaacvetter commented 6 years ago

During the HL7 FHIR Connectathon in Koeln, we had a CDS Hooks breakout session and talked through the tenant / billing_code discussion that originated with this SMART issue: https://github.com/smart-on-fhir/smart-on-fhir.github.io/issues/133

We talked through this issue for CDS Hooks and started to get consensus that an implementation specific key/value pair in the JWT body is a reasonable method to suggest in the specification.

grahamegrieve commented 6 years ago

I like it in the JWT, and that works for CDS-hooks, but I worry that putting in the JWT is too late for some applications - they'll want to know this before auth not after. However it will be hard to be secure if it's ut in the launch URL

jmandel commented 6 years ago

The JWT is available right up front -- a CDS Service can validate it first, then apply logic... or can look inside it before validating. I'm not sure how this affects your assessment @grahamegrieve

grahamegrieve commented 6 years ago

it is in the CDS-hooks context, but not in the context of a smart app launched from the EHR

jmandel commented 6 years ago

That's a SMART issue though. For non-sensitive information in SMART, we could add another ehr-launch-time query param to convey this information. We could even migrate from the current "iss" and "launch" parameters to a signed JWT. But that's a separate discussion.

On Sun, May 13, 2018, 13:27 Grahame Grieve notifications@github.com wrote:

it is in the CDS-hooks context, but not in the context of a smart app launched from the EHR

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cds-hooks/docs/issues/205#issuecomment-388619827, or mute the thread https://github.com/notifications/unsubscribe-auth/AATHAX1X3SEpKbmJa7FQDiwAhNwVzoQvks5tyBiGgaJpZM4T8vWc .

olbrich commented 6 years ago

To a large extent this problem can be resolved by allowing implementers to include additional fields in the service request that would be specified by a service provider in their implementation guide.

nschwertner commented 6 years ago

The proposed approach to use extend the JWT to be the carrier of vendor/site/etc claims might open new possibilities beyond our current approach of:

a) Inferring the vendor/site from the the FHIR Base/Service URI at launch time b) Injecting custom parameters in the SMART launch URI or otherwise using unique launch URIs per site

While (b) would work for a SMART app, it it not that trivial to implement in a CDS Hooks service scenario without turning it into a hack. The nice part of JWT is that it would work equally well for a SMART app and a CDS Hooks service with the added benefit of signature-based authenticity validation of the claims.

I wonder though if there is anything simpler to implement than a JWT to the same effect. For example, the CDS service invocation (the payload of the POST req) already contains context that is trusted, then why not extend that context with other useful claims?

kpshek commented 6 years ago

I wonder though if there is anything simpler to implement than a JWT to the same effect. For example, the CDS service invocation (the payload of the POST req) already contains context that is trusted, then why not extend that context with other useful claims?

The reason why I think placing this information in the JWT is most appropriate because it seems to be related to the EHR authentication process as well as information already in the JWT (namely, the issuer represented by the iss).

Here is a concrete proposal for a new private claim in the JWT payload:

Field Optionality Type Value
tenant REQUIRED string An opaque string identifying the healthcare organization that is invoking the CDS Hooks request.

Per rfc7519, regarding the claims:

All the names are short because a core goal of JWTs is for the representation to be compact.

Additionally, since we are defining a private claim here, also from rfc7519:

Unlike Public Claim Names, Private Claim Names are subject to collision and should be used with caution.

I am proposing a private claim (as opposed to public) as I don't see a need register a new public claim in the IANA "JSON Web Token Claims" registry. I am not concerned with claim name collisions since we are defining what fields we use in the JWT and there are no public claims with the proposed name today.

This new JWT payload claim satisfies the immediate need expressed in the original issue description.

For the needs expressed by @olbrich, we could add this additional statement to our specification:

"An EHR and CDS Service may negotiate additional custom private claims in the JWT payload to communicate additional data. These custom private claims should be named with a prefix of "x-" (extension) to avoid name collisions with possible future CDS Hooks claims."

Thoughts?

olbrich commented 6 years ago

@kpshek Perhaps entity, it's less tied to a particular use case.

kpshek commented 6 years ago

Perhaps entity, it's less tied to a particular use case.

I wouldn't be opposed to using entity as the claim name.

nschwertner commented 6 years ago

entity or even instance sound abstract enough to me. What is the rationale behind making this a "required" claim?

kpshek commented 6 years ago

What is the rationale behind making this a "required" claim?

My thinking was that every EHR is going to have some identifier (already in use or can create a new identifier) that represents this concept.

I know we're focused on CDS Hooks here, but in my experience with production SMART on FHIR apps, nearly all of them need this value (which is why the large EHR vendors provide this value as part of the SMART app launch). This reality was another reason why I am proposing this field is required.

jec commented 6 years ago

In one implementation, I called mine rqe for "requesting entity."

isaacvetter commented 6 years ago

How would entity be different from sub in the JWT? ... and why?

kpshek commented 6 years ago

How would entity be different from sub in the JWT? ... and why?

I wouldn't be opposed to using the sub field for this. rfc 7519 defines sub as:

4.1.2. "sub" (Subject) Claim

The "sub" (subject) claim identifies the principal that is the subject of the JWT. The claims in a JWT are normally statements about the subject. The subject value MUST either be scoped to be locally unique in the context of the issuer or be globally unique. The processing of this claim is generally application specific. The "sub" value is a case-sensitive string containing a StringOrURI value. Use of this claim is OPTIONAL.

We could argue that the subject of our JWT is that entity/organization. I was hoping that whatever field name we came up with, we could use this same name as a new launch context parameter in SMART. I'd be fine using sub here in the context of a JWT as we can explain that but a field named sub seems a bit odder in the SMART on FHIR launch context parameter use case.

With all of that being said, I've been beaten down on this particular topic for a long time so I'm fine with whatever name we decide to use -- sub or something equally as generic (like entity). 😝

kpshek commented 6 years ago

:telephone_receiver: CDS Working Group Vote (6-13-2018)

Meeting notes: http://wiki.hl7.org/index.php?title=File:2018-06-13_CDS_WG_Call_Minutes.docx

@isaacvetter moved the following disposition, seconded by @kpshek.

We will add the following new private claim to the JWT:

Field Optionality Type Value
tenant OPTIONAL string An opaque string identifying the healthcare organization that is invoking the CDS Hooks request.

:+1: For: 10 :expressionless: Abstain: 0 :-1: Against: 0

:tada: The motion passed! :tada: