search

thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.49k stars 1.12k forks source link

RefreshTokenGrant should not revoke the access token #1347

Closed Starfox64 closed 1 year ago

Starfox64 commented 1 year ago

In the current implementation of the refresh_token grant, the access token associated with the old refresh token is revoked

https://github.com/thephpleague/oauth2-server/blob/591a6311a591b4a41d5e8e7856c6f3c7b64a7c86/src/Grant/RefreshTokenGrant.php#L67

The RFC however makes no mention of this being a requirement, only that the refresh token MAY be revoked which is something that is now configurable.

Sephster commented 1 year ago

Thanks for raising this. We already have an issue for this in #946 so closing accordingly to avoid duplicates

Starfox64 commented 1 year ago

Unless I'm missing something, that issue doesn't seem related to this. I'm not talking about having no expiry on the access token just that they should not be revoked early when using the refresh token grant and should instead be allowed to expire naturally.

Starfox64 commented 1 year ago

@Sephster Sorry for the ping, I don't know if you saw my last message but I think what you mentioned is a different issue.

Sephster commented 1 year ago

Sorry @Starfox64 - the reason I've linked them is that currently, if your access token is revoked, you can't use your refresh token as that has also been revoked or expired. The issue I linked to would resolve this by having indefinite refresh tokens so you aren't stuck in limbo.

I get your point though. It won't be directly solving what you've raised here. What would you like to happen in the library? It seems safer to just revoke the access token to me from a security point of view.

Starfox64 commented 1 year ago

In my case I would still very much like to keep a TTL on the refresh token.

I'm asking this mostly for spec compliance since some clients will expect to be able to reuse access tokens until they expire but it's true that this isn't a problem for most people.

Problems arise however in distributed systems where multiple requests can be in flight which will cause 401 errors when a refresh happens.

In the interest of providing added security on top of the spec we could leave this as is but add a shouldRevokeAccessToken attribute to the refresh grant to leave the door open for those that need to strictly follow the spec.