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

Reuse or revoke existing or access and refresh tokens on new auth #1372

Closed ctadlock closed 9 months ago

ctadlock commented 9 months ago

As a part of the auth code grant, a given use will end up with an access token and refresh token for a specific client. If the use goes through the auth code grant process again (regardless of if the previous tokens are expired or not), a new access token and refresh token are created. This seems a bit strange to me. Why would it not either (a) reuse the existing tokens or (b) revoke the existing tokens and issue a new one. What am I missing? Is this by design, if so could someone point me to additional information on this?

The only related logic I see is that when a refresh token is refreshed that it will revoke the existing access token and refresh token. It would seem that it should revoke all tokens for that user/client; even the comment makes me feel thats the intention.

https://github.com/thephpleague/oauth2-server/blob/ab7714d073844497fd222d5d0a217629089936bc/src/Grant/RefreshTokenGrant.php#L66-L85

Sephster commented 9 months ago

There is nothing on this in the spec as far as I'm aware but I don't think there needs to be. Once a client has an access token, why would they go through the process again?

ctadlock commented 9 months ago

There is nothing on this in the spec as far as I'm aware but I don't think there needs to be. Once a client has an access token, why would they go through the process again?

If they logged in on another computer. We are building a Desktop App and its very common for the same account to be used on multiple workstations.

eugene-borovov commented 9 months ago

@ctadlock tihs is refresh token rotation.

ctadlock commented 9 months ago

@ctadlock tihs is refresh token rotation.

Thanks. Its related, but not exact.

To simplify my question.. is it OK for the same user/client pair to have multiple unexpired and unrevoken access and refresh tokens?

Im using a modified version of Laravel Passport and it even has code to check to see if a valid token exists, but it only uses it to bypass the authorization step. It then gets handed off to this package which always issues new tokens without regard to any existing active tokens.

https://github.com/laravel/passport/blob/286baeb1be934654fe4eef147b24d3c8a9a3e08d/src/Http/Controllers/AuthorizationController.php#L101

eugene-borovov commented 9 months ago

is it OK for the same user/client pair to have multiple unexpired and unrevoken access and refresh tokens?

No, it isn`t. There are some considerations not to do so.

You can use multiple token pairs to organize multiple user sessions but tokens must be renewed.

ctadlock commented 9 months ago

That is what happens if you use this library with Laravel Passport. Its not clear to me if the fix would be in this library or Passport. It would seem to me given the code link I put in my first post that it should be in this library. Im not at a level of expertise to know yet, nor submit a PR to resolve it.

Here is the result from my system with the same user (account_id) being authorized to the same client (application_id) on two devices.

image

Sephster commented 9 months ago

If this is for a desktop app I think that app should ensure stored credentials are tied to the logged in user in much the same way as other desktop apps do. Is this not possible?

ctadlock commented 9 months ago

If this is for a desktop app I think that app should ensure stored credentials are tied to the logged in user in much the same way as other desktop apps do. Is this not possible?

Many of the desktop apps I use use OAuth; GitKraken...

Sephster commented 9 months ago

Do they all definitely have shared credentials though? If I logged into your machine would Git Kraken allow me to have access to all your repos?

Sephster commented 9 months ago

Closing this as I think it is something your app needs to handle rather than this library