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

[5.4] Gate resources #19124

Merged
merged 3 commits into from
May 12, 2017
Merged

[5.4] Gate resources #19124

merged 3 commits into from
May 12, 2017

Conversation

browner12
Copy link
Contributor

Problem

I love policies for their ability to group authorization rules. However, I am personally not a huge fan of the auto-resolution of models to policies. While they work fine for simple models, they quickly break down when dealing with nested resources.

Moving everything to gates allows us to better handle these nested resources. However, gates can get very lengthy to define. Writing them as closures in your AuthServiceProvider could quickly turn into a very large file. Defining them using the class@method style gets us a little closer, but I think we can make it even better.

Solution

This PR pulls from the concept of a route resource, and defines a simple method to quickly generate multiple gates for a particular resource. Rather that defining gates like

Gate::define('client.view', 'ClientPolicy@view');
Gate::define('client.create', 'ClientPolicy@create');
Gate::define('client.update', 'ClientPolicy@update');
Gate::define('client.delete', 'ClientPolicy@delete');

we can take advantage of a new resource method on the Gate.

Gate::resource('client', 'ClientPolicy');

which will generate those 4 gates for us. We can use them normally with

Gate::check('client.view', $client);
Gate::check('client.create');
Gate::check('client.update', $client);
Gate::check('client.delete', $client);

Now defining authentication rules for nested resources becomes incredibly easy.

Gate::resource('client.document', 'ClientDocumentPolicy');
Gate::resource('user.document', 'UserDocumentPolicy');

and use them like

Gate::check('client.document.view', [$client, $document]);
Gate::check('client.document.create', [$client]);
Gate::check('client.document.update', [$client, $document]);
Gate::check('client.document.delete', [$client, $document]);

The abilities are also easily customizable. If you don't like 'view', 'create', 'update', and 'delete', you can define your own.

$abilities = [
    'ability1' => 'foo',
    'ability2' => 'bar',
];

The key defines the ability name, and the value defines the method. You can apply them as follows

Gate::resource('client.document', 'ClientDocumentPolicy', $abilities);
Gate::resource('user.document', 'UserDocumentPolicy', $abilities);

and called by

Gate::check('client.document.ability1');  //checks method 'foo' on the class 'ClientDocumentPolicy'
Gate::check('client.document.ability2');  //checks method 'bar' on the class 'ClientDocumentPolicy'

To be clear, this PR does not take away any of the existing functionality of Gates or Policies. The user can decide if they want to continue binding Policies to Models, or simply use gates instead.

While I'm sure there are lots of improvements we could make here, I think this is a very good first step, and gives us some very useful additional functionality.

browner12 added 3 commits May 9, 2017 09:40
this allows us to succinctly define abilities for resources
- by default we will use the ‘view’, ‘create’, ‘update’, and ‘delete’,
but the user can override it if desired.
- also renamed ‘base’ to ‘name’ to keep it consistent with resource
routes.
@taylorotwell
Copy link
Member

Can you define more how the current situation "breaks down" with nested models? It's been working great for me.

@browner12
Copy link
Contributor Author

If you could share some insight on how you're doing it with nested resources, I'd love to see it.

Sorry, 'breaks down', was maybe not the appropriate phrase. Here's my 2 cents on the problems I've run into.

We'll stick with our App\Client, App\User, and App\Document examples. Both Clients and Users can have many Documents. And we'll start with the corresponding policies: ClientPolicy, UserPolicy, and DocumentPolicy. And we'll register them as such:

$policies = [
    'App\Client'   => 'App\Policies\ClientPolicy',
    'App\User'     => 'App\Policies\UserPolicy',
    'App\Document' => 'App\Policies\DocumentPolicy',
];

So the authorization we're looking at is whether a User can CRUD Documents. We're going to assume the logic is different for authorizing a Client who owns a Document, and a User who owns it.

The first option I'll address is housing the logic in the owner's policy. We will start with the call to the authorize method in the controller.

class ClientDocumentController
{
    public function update(Client $client, Document $document)
    {
        $this->authorize('document-update', [$client, $document]);
    }
}

Because we pass a Client object as the first argument, the Gate will resolve it to the ClientPolicy. We cannot simply use an ability name of update, because the ClientPolicy may be responsible for authorizing other models the Client owns, or making changes to the client itself.

class ClientPolicy
{
    public function documentUpdate(User $user, Client $client, Document $document)
    {
        return $client->rep_id == $user->id || $document->author_id == $user->id ;
    }
}

This allows us to keep our methods very clean, but we litter the Policy with a lot of methods. In this scenario, the Policy needs to house all of these methods because anytime we pass the Client as the first argument, we are auto-resolving to the ClientPolicy.


Another option is to house this logic in the DocumentPolicy. Our controller gets cleaned up a little:

class ClientDocumentController
{
    public function update(Client $client, Document $document)
    {
        $this->authorize('update', [$document]);
    }
}

but now our policy absorbs that code

class DocumentPolicy
{
    public function update(User $user, Document $document)
    {
        if ($document->owner instanceof Client) {
            return $document->owner->rep_id == $user->id;
        }

        if ($document->owner instanceof User) {
            return $document->owner->id == $user->id;
        }
    }
}

While this is definitely a feasible option, I would argue that all of these conditional checks are not particularly fun to read. And this is for a Model that only has 2 owners. It's very easy to imagine one with many more owners, making these conditionals even longer.


The way I would use this resource method would allow us to have very distinct Policy classes with 1 purpose, and very succinct methods.

Gate::resource('client', 'ClientPolicy');
Gate::resource('user', 'UserPolicy');
Gate::resource('client.document', 'ClientDocumentPolicy');
Gate::resource('user.document', 'UserDocumentPolicy');
class ClientDocumentPolicy
{
    public function view(User $user, Client $client, Document $document)
    {
        return $client->rep_id == $user->id;
    }

    public function create(User $user, Client $client)
    {
        return $client->rep_id == $user->id;
    }

    public function update(User $user, Client $client, Document $document)
    {
        return $client->rep_id == $user->id && $document->author_id == $user->id;
    }

    public function delete(User $user, Client $client, Document $document)
    {
        return ($client->rep_id == $user->id || $client_manager_id == $user->id) && !$document->isProtected();
    }
}

Using this approach reminds me a little of Jeffrey's video on staying true to the 7 resourceful method.


Even if you're happy with the current Policies and nested models, I think it would still be beneficial to have this method for people who want to only use Gates, and avoid the auto-resolving of Policies. Again, this PR does not remove the Policies, so if they still work for people they can go on ahead using them.

I hope I explained myself well, let me know if you have other questions or concerns.

@taylorotwell taylorotwell merged commit 79c28d6 into laravel:5.4 May 12, 2017
@browner12 browner12 deleted the gate-resources branch May 12, 2017 13:51
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