Custom session fields along with user_token and access_token

sidharthramesh commented 3 years ago

Preflight checklist

[X] I could not find a solution in the existing issues, docs, nor discussions.
[X] I agree to follow this project's Code of Conduct.
[X] I have read and am following this repository's Contribution Guidelines.
[ ] This issue affects my Ory Cloud project.
[ ] I have joined the Ory Community Slack.
[X] I am signed up to the Ory Security Patch Newsletter.

Describe your problem

First off, Hydra is very well thought out! The ability to have full control over everything except the core OAuth2 API makes this the only available framework to use in some situations.

One such situation is SMART on FHIR - a specification for healthcare that extends OAuth2 to provide secure flows from Electronic Health Records to different third-party applications. Note that this issue has already been discussed in this community thread, with no resolution.

Again, Hydra is a perfect candidate to implement such a specification. It's just missing one thing - the ability to set arbitrary session data along with id_token and access_token.

An example of the token response expected from the /token endpoint according to the specification:

{
  "access_token": "i8hweunweunweofiwweoijewiwe",
  "token_type": "bearer",
  "expires_in": 3600,
  "scope": "patient/Observation.read patient/Patient.read",
  "intent": "client-ui-name",
  "patient":  "123",
  "encounter": "456"
}

This is not currently supported by Hydra and throws a 400 error upon accepting a consent request by POSTing to /oauth2/auth/requests/consent/accept with additional session attributes:

{
  "grant_scope": ["openid", "offline_access"],
  "grant_access_token_audience": ["https://my-audience.com"],
  "remember": true,
  "remember_for": 3600,
  "session": {
    "access_token": {
      "foo": "This field will be available when introspecting the Access Token"
    },
    "id_token": {
      "bar": "This field will be available as a claim in the ID Token"
    },
    "fhirUser": "http://fhirserver.org/Patient/4434",
    "intent": "vaccination-records"
  }

Describe your ideal solution

The Accept a Consent Request API should accept additional parameters in the session object.

Workarounds or alternatives

Setting the additional attributes within the id_token is a workaround, but, the specification makes it clear that the id_token should only be provided if the openid profile scopes are used, and the user consents to them.

And in many cases, the user denies access to view their personal data, and the id_token will never be available. However, the app still needs to function, and needs other parameters like "patient", and "intent". The ideal solution would be to allow this in the token response.

Version

1.10.7

Additional Context

A screenshot from the SMART on FHIR specifications page. This is the expected token response:

Again, we use the ORY stack a lot at Medblocks, and I thank the team for this wonderful piece of software.

sidharthramesh commented 3 years ago

For reference, as per RFC6749 Section 4.1.4, additional parameters (example_parameter) are allowed in the response:

{
       "access_token":"2YotnFZFEjr1zCsicMWpAA",
       "token_type":"example",
       "expires_in":3600,
       "refresh_token":"tGzv3JOkF0XG5Qx2TlKWIA",
       "example_parameter":"example_value"
     }

Also, for reference, Okta has an implementation of SMART on FHIR. Their core platform is combined with a token hook to accomplish the custom fields in the token response. However, having this natively as part of the Hydra API would be awesome. Are there any security implications or bigger hurdles to implementing this? I can have a look at this along with #2663 - which I've almost figured out.

aeneasr commented 3 years ago

You're probably looking for the oauth2.allowed_top_level_claims config which governs this!

daniel-eichinger-bl commented 2 years ago

In my opinion the oauth2.allowed_top_level_claims does not cover this feature request entirely.

The SMART on FHIR standard requires some additional JSON parameters in the /token response: see SMART on FHIR 2.0.0 - Scopes and Launch Context.

As a result I think it would be great if Ory Hydra could accept additional parameters in the session object, when the Consent Flow is accepted and return these in the /token response next to the access token, id token, etc.

aeneasr commented 2 years ago

You're right, I think this is not supported right now. Such a feature would need to be implemented in Ory Fosite ( https://github.com/ory/fosite ) with the explicit requirement of sending this data as part of SMART - and once integrated there it would need to be added in Ory Hydra's consent management.

If such an effort is started, please start it against this PR to avoid massive merge conflicts https://github.com/ory/hydra/pull/3013

sidharthramesh commented 2 years ago

Hey @aeneasr, Will you be interested in having this feature in Fosite and Hydra?

I've been looking at Fosite's codebase, and I can see a few ways in which this can be added.

sidharthramesh commented 2 years ago

Can this Extra parameter in Session be used for storing results that have to be returned in the token response? Is this being used for something else?

Maybe something special like Extra.TokenExtras can be used? Otherwise what about adding another object to the Session struct directly like TokenExtras of type map[string]interface{} json: "token_extras" ? https://github.com/ory/hydra/blob/60b93459a08798f4ed5d57a94653924b83cfd597/oauth2/session.go#L40-L48

sidharthramesh commented 2 years ago

Also, if I'm not wrong, there's no need to modify fosite's code here right?

Since we could pass these extra token parameters as just Extra, and return it in the ToMap function

https://github.com/ory/fosite/blob/575ae6d63ed6fd00259fdf273c1e88185cb088f4/access_response.go#L73-L77

sidharthramesh commented 2 years ago

Hey @aeneasr, any possibility of accepting this as a feature? Should I start a PR on fosite?

aeneasr commented 2 years ago

Yes, maybe doing it in Hydra only does the job!

github-actions[bot] commented 1 year ago

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

open a PR referencing and resolving the issue;
leave a comment on it and discuss ideas on how you could contribute towards resolving it;
leave a comment and describe in detail why this issue is critical for your use case;
open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️

ory / hydra