feat: Token refresh hook

[svrakitin]

• Add oauth2.refresh_token_hook to configure an endpoint which will be called during refresh_token grant to retrieve updated token claims. Hook will be noop if not configured.
• Add AccessRequestHook interface and RefreshTokenHook implementation.

Related issue(s)

#2570

Checklist

• I have read the contributing guidelines.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

I added github.com/hashicorp/go-cleanhttp as a dependency, but I can avoid it if necessary.

[aeneasr]

This is looking great, I think a few more tests and a bit of config changes and we can merge this! Sorry for the late review :)

[aeneasr]

Could you please add tests:

1. Where the server responds with a non-ok status code
2. Where the server responds with malformed (e.g. empty / unset / incorrect) payloads

Thank you :)

[svrakitin]

Added more tests.

[svrakitin]

@aeneasr Thanks for the comments, I will update the PR this week.

[svrakitin]

Updated PR. Some tests seem flaky.

[codecov]

Codecov Report

Merging #2649 (96324ae) into master (336afa0) will increase coverage by 0.12%.
The diff coverage is 69.86%.

❗ Current head 96324ae differs from pull request most recent head df008a2. Consider uploading reports for the commit df008a2 to get more accurate results

@@            Coverage Diff             @@
##           master    #2649      +/-   ##
==========================================
+ Coverage   52.68%   52.80%   +0.12%     
==========================================
  Files         234      235       +1     
  Lines       14040    14113      +73     
==========================================
+ Hits         7397     7453      +56     
- Misses       6016     6029      +13     
- Partials      627      631       +4     

Impacted Files	Coverage Δ
oauth2/hook.go	63.93% <63.93%> (ø)
driver/config/provider.go	88.99% <100.00%> (+0.10%)	⬆️
driver/registry_base.go	89.28% <100.00%> (+0.21%)	⬆️
oauth2/handler.go	68.19% <100.00%> (+1.18%)	⬆️
persistence/sql/persister_oauth2.go	82.86% <0.00%> (+0.79%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 336afa0...df008a2. Read the comment docs.

[svrakitin]

@aeneasr Please review when you've got time, don't want to leave this hanging.

[aeneasr]

Awesome, thank you! 🎉

[alee792]

Apologies if this isn't the right place, but could this hook be extended to update consent/OIDC claims like ACR and AMR? This could support step-up or forced re-authn.

For example, a user logs in with basic credentials and the identity provider's initial flow grants an id_token with an ACR indicating AAL1. Later out-of-band, the identity provider solicits additional factors without going through the auth code grant again. When queried, the IDP can return those new ACR and AMR values.

If the RefreshHookResponse included these "non-Extra" fields, we could strengthen or modify the authenticator assurance level of the token's session. These fields are already delegated to the identity provider and taken at face value by Hydra.

[svrakitin]

@alee792 Yeah, makes sense to me, will figure out how to plug it in in spare time and see how it works.

I am just not sure if it is not confusing to the consumer if the ID Token. I would probably allow setting ACR, but not AMR, as AMR actually tells you original method of authentication used. We should either append new AMRs or pass original AMRs in hook request so you handle the merge.

You will also probably need a sid claim to link upgraded authentication to the refresh. Would you implement any kind of token binding to ensure that upgraded token can only be used by an actual user?

Ideally your authentication always goes through identity provider through auth code grant or you use some custom grant for that instead of such adhoc scheme.

[alee792]

Good point on AMR. I'm skimming through RFC8176 and this seems to be their overarching guidance:

Parties using this claim will need
to agree upon the meanings of the values used, which may be
context-specific.

With this in mind, we may want to just keep it as malleable as possible. It's a bit contrived, but imagine there's a forced re-authn event where the user authenticates with an entirely different set of factors. The original factors lose significance and it could be unnecessary to keep them. As the RFC suggests, that meaning is up to the CSP and RP to decide.

Would you implement any kind of token binding to ensure that upgraded token can only be used by an actual user?
I don't think Hydra can enforce the binding, but SessionID could be added to the RefreshHookRequest for the IDP to resolve a session.

Your last point makes sense as well. If the IDP is maintaining it's own cookie-based session, the RP could just go through the auth code grant for step-up or reauthn. It's a bit more chatty going through the entire flow again, but there's less risk of skew.

[kszafran]

@svrakitin I just noticed that this feature was released in v1.10.7. I'm trying to understand -- does this hook get called also on the initial flow, when the caller gets the access token for the first time? If not, is there already a way to customize access token claims with hooks during the initial flow?

[svrakitin]

@kszafran You pass initial claims when you accept consent request. Hook is only triggered on refresh.