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

Make sure refreshing OAuth token is idempotent #3650

Merged
merged 7 commits into from
Dec 11, 2024

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Dec 10, 2024

Fixes #2795

This can be reviewed commit by commit

It is basically:

  • keep track of the 'chain' of refresh tokens, so record the 'next' token when consuming one
  • recording when access tokens are first used (either through introspection or the userinfo endpoint)
  • make the cleanup job remove 'revoked' tokens, not the expired ones anymore. This is to make sure we keep the info whether the access token was used or not
  • then implement the "idempotence" logic on the refresh token grant

Copy link

cloudflare-workers-and-pages bot commented Dec 10, 2024

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: e64e5f5
Status: ✅  Deploy successful!
Preview URL: https://7a059f05.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-refresh-idempotence.matrix-authentication-service-docs.pages.dev

View logs

@sandhose sandhose marked this pull request as ready for review December 10, 2024 16:55
@sandhose sandhose requested a review from reivilibre December 10, 2024 16:56

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
This will help us determine whether we had a double-refresh happening

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
If we continue deleting expired tokens, we might not record whether the
token was used or not, and not know what to do in case of
a double-refresh.

Revoked tokens are safe to delete.

This also reduces the frequency of the cleanup job to once an hour.

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
This lets us track 'revoked' tokens separately from 'consumed' tokens.

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
This allows using a refresh token multiple times, as long as the new
pair of tokens were not used in the meantime.
@sandhose sandhose force-pushed the quenting/refresh-idempotence branch from 89331a3 to 76137c4 Compare December 11, 2024 08:22
&state.clock,
&mut repo,
&session,
Duration::microseconds(5 * 60 * 1000 * 1000),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there really no way to do Duration::seconds(5 * 60) lol? :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the only one that can't panic, and clippy will complain that we don't document that potential panic I think? 😬

@reivilibre
Copy link
Contributor

('idempotent' seems like a misleading word since the old tokens get revoked, rather than returned again, but not sure if there's a more accurate word that matches it better)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: reivilibre <oliverw@element.io>
@sandhose sandhose merged commit 1dbed14 into main Dec 11, 2024
19 checks passed
@sandhose sandhose deleted the quenting/refresh-idempotence branch December 11, 2024 13:15
@sandhose sandhose added T-Enhancement New feature of request A-Next-Gen-Auth Related to the next generation authentication APIs T-Defect Something isn't working and removed T-Enhancement New feature of request labels Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Next-Gen-Auth Related to the next generation authentication APIs T-Defect Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh token request should be idempotent
2 participants