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

Proposal: Add a new type of custom endpoint to be called on refresh token grant #2570

Closed
wxdao opened this issue Jun 16, 2021 · 11 comments
Closed

Comments

@wxdao
Copy link

wxdao commented Jun 16, 2021

Is your feature request related to a problem? Please describe.

Currently claims of new tokens exchanged with a refresh token are always the ones specified on the initial consent flow, never getting a chance to get updated.

Describe the solution you'd like

Introduce a new external endpoint to participate in token refresh, so that the external identity provider is able to add updated user information to claims, and even run some custom policies to decide whether the flow to be canceled and the refresh token to be revoked.

Other implementations like Auth0 have something like rules run on token refresh flow too.

Additional context

Some more discussions: #2542

@aeneasr
Copy link
Member

aeneasr commented Jun 16, 2021

There are two approaches to solve this:

  • PUSH model would required the IDP to push any claims-related changes to Ory Hydra when updates occur
  • PULL model would required the IDP to implement an endpoint which would be called by Ory Hydra on every refresh

Personally, I would prefer the PUSH model because we could probably add an endpoint which updates the consent state so it would be pretty straight forward to implement (I believe).

@wxdao
Copy link
Author

wxdao commented Jun 16, 2021

PUSH model I think will require the provider to broadcast every user info changes for everyone to hydra server, even though some users may not have any sessions under hydra currently, a little bit too much, or the provider will have to keep track of all active sessions. Also, depending on how a session was first authenticated, probably not every token for a user gets same claims, like if some claims only get populated on specific scopes or login environments.

It more about how a session was initially authenticated that matters. The provider needs this information to decide what to do for a token refresh.

@aeneasr
Copy link
Member

aeneasr commented Jun 18, 2021

True, that might indeed be cumbersome. So the PULL request would only be triggered if configured though, right?

@wxdao
Copy link
Author

wxdao commented Jun 18, 2021

Yes it should be optional.

@aeneasr
Copy link
Member

aeneasr commented Jun 18, 2021

What could potential request and response payloads look like? Should this be supported for refresh tokens only or also for e.g. client credentials?

@wxdao
Copy link
Author

wxdao commented Jun 18, 2021

Request could be like a single element of the response of Lists All Consent Sessions of a Subject:

POST <token-refresh-endpoint-url>

{
  consent_request
  grant_access_token_audience
  grant_scope
  handled_at
  remember
  remember_for
  session
}

Success/continue response could be like the request of Accept a Consent Request but with only the session field:

200

{
  session: {access_token, id_token}
}

Error/canceled response:

any non-200

{
  error
  error_debug
  error_description
  error_hint
  
  should_revoke_refresh_token
}

@wxdao
Copy link
Author

wxdao commented Jun 18, 2021

I don't have any concrete idea about client credentials grant yet. It's a bit weird for the provider to handle parts of its flow since clients are registered on the hydra side, the provider generally only knowing users. Maybe that would be another proposal?

@aeneasr
Copy link
Member

aeneasr commented Jun 21, 2021

POST <token-refresh-endpoint-url>

{
  consent_request
  grant_access_token_audience
  grant_scope
  handled_at
  remember
  remember_for
  session
}

That sounds good to me

200

{
  session: {access_token, id_token}
}

I think that makes sense - it's sort of partially the accept consent payload so it would be easy to re-use. The question is, what happens if these are left empty? Should we expect the endpoint to echo the incoming session here or would empty mean it's the previous value? Is empty null, undefined or an empty object?

any non-200

{
error
error_debug
error_description
error_hint

should_revoke_refresh_token
}

What happens if error/error_debug/error_description are empty? Regarding should_revoke_refresh_token, should only the RT be removed, or the AT as well? Or the whole consent?

I don't have any concrete idea about client credentials grant yet. It's a bit weird for the provider to handle parts of its flow since clients are registered on the hydra side, the provider generally only knowing users. Maybe that would be another proposal?

Yup, now that I see the proposal that makes sense to me.

@wxdao
Copy link
Author

wxdao commented Jun 22, 2021

I think that makes sense - it's sort of partially the accept consent payload so it would be easy to re-use. The question is, what happens if these are left empty? Should we expect the endpoint to echo the incoming session here or would empty mean it's the previous value? Is empty null, undefined or an empty object?

Simply replacing the internal session with the session field responded is easier to implement and understand I think. If nothing were to be changed just echo the value. It avoids complex patching or strategic merge crazy stuff.

What happens if error/error_debug/error_description are empty? Regarding should_revoke_refresh_token, should only the RT be removed, or the AT as well? Or the whole consent?

The whole consent. Maybe the field should be named should_revoke_consent.

@aeneasr
Copy link
Member

aeneasr commented Jun 28, 2021

Simply replacing the internal session with the session field responded is easier to implement and understand I think. If nothing were to be changed just echo the value. It avoids complex patching or strategic merge crazy stuff.

Agreed!

The whole consent. Maybe the field should be named should_revoke_consent.

I think we should keep this functionality out and ask people to use the consent API instead if they want to do that?

@svrakitin
Copy link
Contributor

svrakitin commented Jul 22, 2021

@aeneasr My team is kinda blocked by this so I will have to work on this proposal. Will submit a PR in upcoming days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants