Idea for discussion: Moving permission check from 'before' to 'after'

[sergejostir]

Hello.

Currently this package utilizes Gate::before() to check, if the model has specified permission. Since package's Gate::before() are called before Laravel's Gate::before(), we cannot actually use Gate::before() in our main app as it is supposed to (for example to give a user a "superadmin" access).

laravel-permission/src/PermissionRegistrar.php

Line 100 in e382155

$this->gate->before(function (Authorizable $user, string $ability) {

The flow is this:

spatie's before() (currently the permission check is performed here)
laravel's before()

spatie's after() (I propose to move it here)
laravel's after()

Currently spatie's check has absolute priority over our main application, so even though we do something 'before', it's de facto 'after' (after spatie's has already checked for specificed permission; and it's not even called, if spatie found a suitable permission match, so we cannot for example "ban" user in 'before').

If my User is "superadmin" and I do the check in Laravel's before, spatie will go and look for a permission, even thought it's completely useless since that user will be granted access anyway.

My proposal is, to move spatie's permission check to 'after'. That way it will happen after Laravel's before and before Laravel's after and as a result enable us to do our things before or after spatie's check (currently we can only do it after as explained before).

I would like to hear, if some of you have any reservations about this change. I couldn't think of any downside or braking change of top of my head. In my opinion, it would make much more sense.

Alternatively, it could be made configurable, so it would stay as it is by default, but we would be able to move it to 'after' with changing a config value. I'm willing to make a PR for this.

[drbyte]

Since the after feature in Laravel was "fixed" by Joseph Silber awhile back, I've often thought that perhaps all these operations might be better served using that approach.
I explored rewriting things to work primarily with after, but ran into a number of problems (unfortunately I can't recall all of them exactly right at this moment), so I abandoned the idea, at the time.
I welcome a fresh discussion though.

[drbyte]

While it might beg for some refactoring and abstraction, I suppose this could also be configurable, with the current before approach available as an alternate choice.

In a basic app there wouldn't be much likelihood of undesired breaking changes. But more complex apps, particularly those which have been coded with the current before behavior in mind, might need to do some work to adapt.