feat: allow for permanent refresh_token(s)

[bill-robbins-ss]

This is a branch of v0.40.2 that introduces the ability to disable one-time-use refresh tokens (RT). From my reading of issues here in the fosite repository and the Hydra repository, a number of users desire a feature like this. My hope is that this change can be released with a new version v0.40.3 and a new version of Hydra v1.10.7. With these new releases I can update my deployment of Hydra to pick up the new feature.

One time use refresh-tokens can introduce issues where a user's "session" stored in a 3rd party database can be completely disabled, forcing the end-user to go through an OAuth2 flow with Hydra again. The related issues I have included below outline some of the ways this can happen and include some discussion.

For brevity here is a use case involving threads that can permanently disable a session:

1. Execute an OAuth2 flow with Hydra and receive an access_token (AT-0) and refresh_token (RT-0).
2. Thread-A: (AT-0 has expired) use RT-0 to obtain new tokens, AT-1, RT-1
3. Thread-B: (AT-0 has expired) also uses RT-0 to obtain a new RT, but this fails because Thread-1 has already used the token
4. Thread-A: uses AT-1 to call an API endpoint (ie; /userinfo)
5. Thread-A: (AT-1 has expired) use RT-1 to obtain new tokens. This fails because in step 3, Thread-B attempted to use RT-0 after it was used. There is now no way to get back into a functional state and the end-user must go through the flow again.

This can be worked-around by implementing a distributed-lock pattern in the client to Hydra, but I believe this places an undue burden on the client developers. This burden is exacerbated by the nature of implementing OAuth2 flows for an application: each client will likely be an independent 3rd party entity to the entity hosting Hydra. Further, a distributed-locking pattern can not account for the other failure cases, such as network errors.

Note: There is a companion PR in Hydra that adds a configuration option to enable/disable this behavior.

Related issue(s)

#255
Hydra #1831

Checklist

• I have read the contributing guidelines
  and signed the CLA.
• 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 necessary documentation within the code base (if
  appropriate).

Further comments

I implemented this change with the goal of changing as little as possible, hence the use of Context to pass information from Hydra to this library. I have seen no other mechanism for accessing configuration provided in Hydra to fosite, nor is the Strategy Pattern possible here, as HandleTokenEndpointRequest does not use a strategy for revoking a token. Instead HandleTokenEndpointRequest accesses TokenRevocationStorage directly, meaning that this behavior is applied to every Strategy involved. A deeper change could be made to introduce a higher-level strategy for handling token refresh, or introducing a TokenRevocation strategy but this is in conflict with my goal of making a small, precise, change.

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

Man, I wrote a really long review that somehow did not make it through? Sorry about that! So here's the same review and I will double check if GitHub overlords saved it ;)

I think the change is really good from a code perspective. Unfortunately, it is counterintuitive to security best practices laid out in the IETF RFCs, in particular https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.13.2 and other sections in this RFC.

As we do not support sender-constrained refresh tokens (apparently that's a new RFC), we need to implement Refresh token rotation if we want to follow this RFC's recommendations (which we do).

So what alternative do we have? I think you've already figured it out!

Instead of hard-deleting/revoking the token we should probably set an expiry time that is a configured amount of seconds in the future. For example, you set the "refresh token replace grace period" to 5 seconds. When the RT is used, it will be valid for another 5 seconds, and only then will it actually be considered inactive!

What's great about this, you probably don't even need to make changes in fosite. It could all be in the implementation of Ory Hydra's persister!

What do you think about this idea?

[bill-robbins-ss]

Upon further consideration I arrived at the same conclusion; adding the grace period and continuing rotation will be a safer change. I will probably close both of the PRs for this and start a new one.

[aeneasr]

Awesome, thank you for your hard work!

[bill-robbins-ss]

closing in favor of refresh token grace period