Skip to content

[5.8] Add a 'manage' default policy - #28654

Merged
taylorotwell merged 3 commits into
laravel:5.8from
browner12:patch-2
May 30, 2019
Merged

[5.8] Add a 'manage' default policy#28654
taylorotwell merged 3 commits into
laravel:5.8from
browner12:patch-2

Conversation

@browner12

Copy link
Copy Markdown
Contributor

In our normal resource controller, we have index, create, store, edit, show, update, and destroy.

Our default policy has methods that correspond to most of these methods.

view --> show
create --> create and store
update --> edit and update
delete --> destroy

The one clear omission is we have no default policy that corresponds to the index method. The index method traditionally contains a list of all items of a particular Model.

I'm proposing a new default policy called manage that can be used to determine if a User is allowed to access the index resource route.

In our normal resource controller, we have `index`, `create`, `store`, `edit`, `show`, `update`, and `destroy`.

Our default policy has methods that correspond to most of these methods.

`view` --> `show`
`create` --> `create` and `store`
`update` --> `edit` and `update`
`delete` --> `destroy`

The one clear omission is we have no default policy that corresponds to the `index` method. The `index` method traditionally contains a list of all items of a particular Model.

I'm proposing a new default policy called `manage` that can be used to determine if a `User` is allowed to access the `index` resource route.
@Christophvh

Copy link
Copy Markdown

Good idea. But I would call it list. All the words can be seen as permissions:
show, create, store, edit, update could be permissions if we add the model name. Manage seems like a permission where you can manage a complete model. 'List' would suggest you can only show a list of the items.

@browner12

Copy link
Copy Markdown
Contributor Author

Definitely not opposed to a name change. Thanks for the feedback.

@bonzai

bonzai commented May 29, 2019

Copy link
Copy Markdown
Contributor

I think you should also modify the AuthorizesRequests trait:

https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Auth/Access/AuthorizesRequests.php#L105-L115

@devcircus

Copy link
Copy Markdown
Contributor

This was intentionally removed a while back. Taylor has commented why. Not where I can dig for it right now.

@bonzai

bonzai commented May 29, 2019

Copy link
Copy Markdown
Contributor

I know (see link below), but what's the point of the manage policy, if it won't be used by the framework itself?

@taylorotwell

Copy link
Copy Markdown
Member

The reason it was omitted is because it's not as commonly used. If you have a list of comments you would typically just filter the comments the user is allowed to see in the query you run against the database.

However, there are sometimes places where this makes sense. I think I have done it once myself. I would not call it manage though... the suggestion of list was a bit better.

also fixed the comment to use the Plural version of the model name.
@browner12

Copy link
Copy Markdown
Contributor Author

I'm good with 'list' as long as we don't have any conflicts with the PHP native list.

I will say personally I use this everywhere, especially on an administrative backend. I would agree on the frontend you'd only want to list things that belong to a user. A user's comments, a user's payments, etc. But on the backend, as an administrator, you would want to see everything. All the Users, all the Comments, all the Payments. But you may selectively allow different administrative users to access different lists, based on their role.

If this PR gets accepted, I can make the changes in AuthorizesRequest and Gate.

@bonzai

bonzai commented May 30, 2019

Copy link
Copy Markdown
Contributor

Laravel Nova uses viewAny method, so maybe it's a good to use it instead of list?

@taylorotwell

Copy link
Copy Markdown
Member

Ah yes, viewAny... probably worth keeping that to be consistent.

@taylorotwell
taylorotwell merged commit 9c93f77 into laravel:5.8 May 30, 2019
@browner12
browner12 deleted the patch-2 branch May 30, 2019 20:14
@ryanrapini

ryanrapini commented Sep 19, 2019

Copy link
Copy Markdown

Sorry if bumping this issue is not acceptable.

It seems weird to stub out code for the user that is not used. I had the (dangerous) impression that viewAny was going to work out of the box with authorizeResource() since it was stubbed out for me when i created a policy. I tested and found this was not the case.

Is it possible to denote in either the docstring or in the documentation that some of these policies are picked up by authorizeResource() and some are not? Or else don't stub out these policies?

The documentation lists the policies that will be mapped but I was more inclined to believe the stubbed out code than the documentation.

If you are finding this thread through google like I did, I needed to add the following in app\Http\Controllers\Controller.php:

class Controller extends BaseController
{
    use DispatchesJobs;
    use ValidatesRequests;

    use AuthorizesRequests {
        resourceAbilityMap as protected resourceAbilityMapTrait;
    }

    /**
     * Get the map of resource methods to ability names.
     *
     * @return array
     */
    protected function resourceAbilityMap()
    {
        // Map the "viewAny" ability to the "index" function in our policies
        return array_merge($this->resourceAbilityMapTrait(), ['index' => 'viewAny']);
    }
}

credit to laravel/ideas#772 (comment)

@peterjthomson

peterjthomson commented Mar 16, 2020

Copy link
Copy Markdown

For anyone stumbling on this issue, after Laravel 6.0 adding the viewAny method to a Policy now works automatically with the Index function on a resourceful controller. And Taylor is right that permission policies for the Index function is an unusual function in a consumer app, but it's great for admin apps and internal business apps.

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.

7 participants