search

laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.29k stars 780 forks source link

[12.x] Add access token revoked event #1776

Closed axlon closed 3 months ago

axlon commented 3 months ago

This PR adds an event when Passport revokes an access token. This is implemented in the same way as the access token creation event.

hafezdivandari commented 3 months ago

I was thinking about revocation on Passport the other day, and it seems totally unnecessary. I mean every revoke can be a simple delete as we never un-revoke an entity, We just purge revoked entities manually using passport:purge. We may fix this on 13.x? So having an event for this may be unnecessary, what you think?

axlon commented 3 months ago

@hafezdivandari I think that keeping revoked tokens is probably useful in some workflows because it allows you to view revoked tokens in the database (I don't do this myself), this is primarily so because there are no events/callbacks to hook into.

If Passport were to start deleting revoked tokens immediately, I'd say the argument for an event becomes even stronger because after revocation is done there is no way to check if tokens were revoked.

hafezdivandari commented 3 months ago

The problem is that we revoke tokens in some other places without calling AccessTokenRepository::revokeAccessToken(), like:

https://github.com/laravel/passport/blob/ead5067b14697643f6c1d0982746d03374167421/src/Http/Controllers/AuthorizedAccessTokenController.php#L71

If we fix this and remove the revoked entity, you can easily listen for Eloquent model's deleted event instead.

Another problem is that revocation is currently inconsistent, for example we never revoke a client, we always delete it: https://github.com/laravel/passport/blob/ead5067b14697643f6c1d0982746d03374167421/src/Http/Controllers/ClientController.php#L138

I'm going to fix these on 13.x, but this PR makes it just more complex?

axlon commented 3 months ago

Tokens being revoked from outside the repository is indeed a problem for this PR, but its not that big of an issue to fix. The controller in question could be adjusted or the revoke could simply also fire the event, either would work.

Whether an event should be added at all is debatable, Passport is missing a lot of events (I wanted to add a similar event for refresh tokens but immediately ran into inconsistencies there as well). On the other hand there are creation events for both access tokens and refresh tokens.

As for your point about deleting revoked records, personally I think it'd be a bad idea to have Passport delete tokens when they are revoked (as default behavior), because it will be a problem for anyone relying on that data to create insights, for example. Instead it would make more sense to create a revoke action and interface that Passport will call, allowing developers to easily extend/change the default behavior if they want to.

hafezdivandari commented 3 months ago

Whether an event should be added at all is debatable, Passport is missing a lot of events.

It may be much better / safer to define Passport's events as a wrapper for eloquent model's events instead.

As for your point about deleting revoked records, personally I think it'd be a bad idea to have Passport delete tokens when they are revoked (as default behavior), because it will be a problem for anyone relying on that data to create insights, for example. Instead it would make more sense to create a revoke action and interface that Passport will call, allowing developers to easily extend/change the default behavior if they want to.

Revocation suppose to work like soft deletion, but currently it doesn't. Seems easy to fix, I'll try to prepare a PR for this.