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

RefreshTokenGrant: add option whether to revoke refreshed access tokens #1377

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

Conversation

josiasmontag
Copy link

@josiasmontag josiasmontag commented Sep 29, 2023

Currently, the RefreshTokenGrant immediately revokes an access token when it gets refreshed.

The RFC Section 6 makes no mention that this should happen.

The current behavior sometimes causes issues: Some clients assume the old access token is still valid because it has not reached its expiration date yet. Also, there are race conditions with simultaneous requests when one client refreshes the token and the other client still uses the old, non-expired token.

This PR adds an option revokeRefreshedAccessTokens to configure revoking old access token after refreshing. It defaults to true, wich is the current behaviour.

Also see #1347

@josiasmontag
Copy link
Author

Some additional thoughts:
The current behaviour of revoking the access tokens might even be wrong according to the RFC. Section 1.5 says:

Refresh tokens are issued to the client by the authorization server and are
used to obtain a new access token when the current access token
becomes invalid or expires, or to obtain additional access tokens
with identical or narrower scope
(access tokens may have a shorter
lifetime and fewer permissions than authorized by the resource
owner).

The current implementation does not allow to obtain additional access tokens as it revokes all previous ones.

@Sephster
Copy link
Member

We already have a function for this called revokeRefreshTokens which can be set to true or false. We used to automatically revoke and appreciate this isn't following spec so added in a method to allow implementers to choose whether this is enforced or not.

The boolean should be set in the AuthServer class. Thank you for your PR though. I hope this functionality helps solve your problem.

@Sephster Sephster closed this Oct 25, 2023
@josiasmontag
Copy link
Author

@Sephster I am aware of revokeRefreshTokens which is about revoking the old refresh token, it does not control whether the old access token is revoked. This PR is about adding a setting for revoking the previous access token. Please have a look at the relevant change here.

@Sephster
Copy link
Member

Apologies I had missed this. Will reopen to review later. Thanks for clarifying.

@Sephster Sephster reopened this Oct 25, 2023
@pat0s
Copy link

pat0s commented Feb 19, 2024

Is there any progress on clarifying this PR? @Sephster

@Sephster
Copy link
Member

No not yet. All my efforts are on releasing v9 then this will be picked up along with others. Cheers

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.

None yet

3 participants