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

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

Closed
isaacvetter opened this issue May 13, 2018 · 15 comments · Fixed by #420
Closed
Assignees
Labels
substantive This issue is a substantive change per HL7's ballot rules
Milestone

Comments

@isaacvetter
Copy link
Member

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: smart-on-fhir/smart-on-fhir.github.io#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
Copy link

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
Copy link
Member

jmandel commented May 13, 2018

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
Copy link

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

@jmandel
Copy link
Member

jmandel commented May 13, 2018 via email

@olbrich
Copy link

olbrich commented May 13, 2018

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
Copy link

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
Copy link
Contributor

kpshek commented May 13, 2018

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
Copy link

olbrich commented May 13, 2018

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

@kpshek
Copy link
Contributor

kpshek commented May 13, 2018

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

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

@nschwertner
Copy link

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

@kpshek
Copy link
Contributor

kpshek commented May 13, 2018

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.

@kpshek kpshek added the ballot HL7 ballot comment label May 13, 2018
@jec
Copy link

jec commented May 15, 2018

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

@isaacvetter
Copy link
Member Author

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

@kpshek
Copy link
Contributor

kpshek commented May 30, 2018

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
Copy link
Contributor

kpshek commented Jun 13, 2018

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

👍 For: 10
😑 Abstain: 0
👎 Against: 0

🎉 The motion passed! 🎉

@kpshek kpshek self-assigned this Jun 13, 2018
@kpshek kpshek added the substantive This issue is a substantive change per HL7's ballot rules label Aug 1, 2018
kpshek added a commit that referenced this issue Nov 2, 2018
Additionally, the example JWT has been improved to include the public and private keys used to both sign and verify the signature.
kpshek added a commit that referenced this issue Nov 7, 2018
Additionally, the example JWT has been improved to include the public and private keys used to both sign and verify the signature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
substantive This issue is a substantive change per HL7's ballot rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants