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

Gate ability registration #45807

Closed
wants to merge 4 commits into from

Conversation

browner12
Copy link
Contributor

@browner12 browner12 commented Jan 26, 2023

Currently to register a group of authorization abilities, we can use the resource() method, which defines our standard CRUD options. This is fine in a lot of cases, but if you want to adjust those basic options, you now need to pass a clunky third parameter in the resource() method to get the desired abilities.

With this PR we now have the ability to auto-register any methods defined in a class as a Gate ability! It uses the convention of the method name being the same as the ability name.

Imagine we have a class of gates:

class UserPolicy
{
    public function viewAny(User $user): bool {}
    public function view(User $user, User $model): bool {}
    public function create(User $user): bool {}
    public function update(User $user, User $model): bool {}
    public function delete(User $user, User $model): bool {}
    public function restore(User $user, User $model): bool {}
    public function forceDelete(User $user, User $model): bool {}
}

To register this in our AuthServiceProvider we can now use:

$gate->register('users', \App\Policies\UserPolicy::class);

and all 7 of our methods will be defined with the following ability names:

users.viewAny
users.view
users.create
users.update
users.delete
users.restore
users.forceDelete

Some people will probably be thinking, why not just use actual Laravel Policies? I've talked previously (#19124) about why I prefer defined gates vs policies, but it boils down to the fact policies will auto resolve the authorization rule based on the first Model it receives, and this isn't always desirable. Defined gates work more similarly to Routes, and allow you to be explicit about which authorization rule is called.


The one thing I'm not really sure about with this PR is if there are performance concerns when dealing with Reflection. I wasn't seeing any issues in my testing, but if anyone has any knowledge on this, would love to hear it.

given a class, it will automatically define all methods in the class as gates.
- create dummy test classes
- assert methods from parent class **are not** defined
- assert methods from traits **are not** defined
- assert methods from class **are** defined
@browner12
Copy link
Contributor Author

Tests passed locally. Look like it had some kind of issue with Docker.

Could a maintainer re-trigger the tests, looks like I am unable to. Thanks!

@taylorotwell
Copy link
Member

Hey there!

I personally wonder if we should make Gate macroable instead. I went back and revisited the original discussion from 2017 regarding why you prefer not to use policies. I think I still am not sure I totally agree with that original premise.

Using your original example in 2017 of users, clients, and documents, I would place the determination to update a document on the document policy and do something like the following:

public function update(User $user, Document $document, User|Client $hasDocuments)
{
    return match (true) {
        $hasDocuments instanceof User => $someCondition,
        $hasDocuments instanceof Client => $someOtherCondition,
    };
}

Of course, you could have a HasDocuments interface on the third argument where that interface defines a ownsDocument method or something that allows you to delegate the check to that class. In your 2017 thread you defined multiple policies for documents and put the conditions for updating a document in each policy. I'm not sure that is really a marked improvement over the example above in my personal opinion.

So, rather than going further down the path of this use case of auto registration of gates -> class methods, I think I would rather make Gate macroable so that you can define this convenience method for yourself.

@browner12
Copy link
Contributor Author

@taylorotwell Thanks for the feedback. I should have probably avoided the Gate vs Policy debate for this, and instead focused on how this PR makes things better for Gate users. This method is a clear improvement over the resource() method way of defining gates. Maybe the better solution would be to update the resource() method in 10.x with the new functionality.

I would obviously love for this feature to be added to core, since it's a big improvement over the existing resource() method, and would be helpful for the Gate users out there. Will definitely take making Gate macroable as a consolation prize if that's the direction you think is best.

Thanks

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.

2 participants