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

refactor: refresh token rotation #838

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

refactor: refresh token rotation #838

wants to merge 3 commits into from

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Nov 29, 2024

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • 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 approval (please contact [email protected]) 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 the necessary documentation within the code base (if appropriate).

Further comments

@aeneasr aeneasr marked this pull request as ready for review December 2, 2024 19:30
Previously, the refresh token handler was using a combination of delete/update storage primitives. This made optimizing and implementing the refresh token handling difficult. Going forward, the RefreshTokenStorage must implement `RotateRefreshToken`. Token creation continues to be separated.

BREAKING CHANGES:

Method `RevokeRefreshTokenMaybeGracePeriod` was removed from `handler/fosite/TokenRevocationStorage`.

Interface `handler/fosite/RefreshTokenStorage` has changed:

- `CreateRefreshToken` now takes an additional argument `accessSignature` to keep track of refresh/access token pairs:
- A new method `RotateRefreshToken` was added, which revokes old refresh tokens and associated access tokens:

```patch
// handler/fosite/storage.go
type RefreshTokenStorage interface {
-	CreateRefreshTokenSession(ctx context.Context, signature string, request fosite.Requester) (err error)
+	CreateRefreshTokenSession(ctx context.Context, signature string, accessSignature string, request fosite.Requester) (err error)

	GetRefreshTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error)
	DeleteRefreshTokenSession(ctx context.Context, signature string) (err error)

+	RotateRefreshToken(ctx context.Context, requestID string, refreshTokenSignature string) (err error)
}
```
@aeneasr aeneasr changed the title Improve refresh refactor: refresh token rotation Dec 4, 2024
@aeneasr aeneasr self-assigned this Dec 4, 2024
handler/oauth2/flow_refresh.go Show resolved Hide resolved
handler/oauth2/flow_refresh.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants