feat: Token Reuse Detection #567
Conversation
There was a problem hiding this comment.
Thank you for your work on this, this looks great already!
I think it would make sense to add an e2e / integration test to https://github.com/ory/fosite/tree/master/integration to check that everything works well together!
Apart from that I think this is pretty much good to go as I think that this should not have any breaking changes, as ErrNotFound still behaves as before!
| @@ -64,7 +64,14 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex | |||
| refresh := request.GetRequestForm().Get("refresh_token") | |||
| signature := c.RefreshTokenStrategy.RefreshTokenSignature(refresh) | |||
| originalRequest, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, request.GetSession()) | |||
| if errors.Is(err, fosite.ErrNotFound) { | |||
| if errors.Is(err, fosite.ErrInactiveToken) { | |||
There was a problem hiding this comment.
If I understand correctly, the idea is that the storage would return a new error code ErrInactiveToken when a refresh token was used or revoked - am I correct with that assumption?
There was a problem hiding this comment.
Yes, exactly, same way it does for auth codes.
|
@aeneasr Thanks. I will add some integration tests. |
|
Awesome, thank you! |
|
@aeneasr Enjoy your holidays. This is not urgent. |
|
@aeneasr Thanks for pleasant contributor experience. I will prepare a PR for hydra as soon as there is a new fosite release. |
|
:) v0.39.0 is out now! |
|
@svrakitin Yes, I must say I always admire how well @aeneasr manages to handle maintaining this project. At the same time pretty opinionated, but still welcoming. :-) A rare mix. |
|
Thank you two, I'm blushing |
This patch adds support for Refresh Token reuse Detection introduced by ory/fosite#567. Ory Hydra's persister no longer deletes refresh tokens when using them, but instead deactivates them - similar to how authorization codes work. Closes #2022
Related issue
ory/hydra#2022
Proposed changes
This change makes
refresh_tokengrant handler to check if we receivedfosite.ErrInactiveTokenerror from storage, which means there is an attempt to refresh access token using previously-used or revoked refresh token.This is similar to what is done in Authorization Code Flow.
Checklist
and signed the CLA.
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.
works.
appropriate).
Further comments
These are changes just for fosite. I have another PR to make for hydra, so its persistent storage stops deleting refresh tokens from database on revocation. Instead it will mark them as inactive similar to what's done for auth codes.