-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Refresh Token Reuse Detection #2383
Conversation
d3b9040
to
39bcce6
Compare
39bcce6
to
151cae4
Compare
@aeneasr No idea what happened to e2e tests, but should be good to go, let me know if I forgot to cover something. We also need some garbage collection in place for refresh tokens. I think I've seen some other PR covering this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had reviewed 90% of the PR and for some reason stopped in the middle of it, which is why I didn't complete it.
@aeneasr No idea what happened to e2e tests, but should be good to go, let me know if I forgot to cover something.
It might make sense to add a full e2e test for refresh tokens. Writing and testing these is covered here: https://github.com/ory/hydra#e2e-tests
We also need some garbage collection in place for refresh tokens. I think I've seen some other PR covering this.
Yes, that was merged!
Added e2e test to go through Auth Code Flow with PKCE in the browser and reuse refresh token. This is closer to the real scenario when it can happen. Hope this can be reused for more e2e tests in context of public clients. CircleCI got an incident, so e2e tests failed again. Did you consider moving tests to Github Actions? :) |
Yeah, unfortunately it's quite an effort to do that so it's gonna take a while as we have lots of tooling around circleci. Sorry about the flaky e2e test - I will rerun the CI. Is this good for another 👀 ? |
@aeneasr Yeah, please review. |
@aeneasr Please let me know if any other change is expected. |
Thank you for bubbling this up in my inbox, I must have missed your push! Taking a look now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! 🎉🎉🎉🎉 Your contribution makes Ory better :)
Sweet! 👍 |
Related issue
Closes #2022
Proposed changes
This PR leverages support for Refresh Token reuse Detection added in ory/fosite#567. It makes hydra's
Persister
stop deleting refresh tokens, but deactivating them similar to whats done for authorization codes.Modules need to be updated if PR for fosite gets merged.
Checklist
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.
Further comments
See ory/fosite#567 for fosite PR.