filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
17.73k stars 2.78k forks source link

Policies ignored in bulk table actions #764

Closed dive-michiel closed 2 years ago

dive-michiel commented 2 years ago

Package

filament/tables

Package Version

v2.5.4

Laravel Version

v8.74

Livewire Version

v2.8.1

PHP Version

PHP 8.0.0

Bug description

The policy is ignored for bulk actions, and I'm able to see the delete action and delete all rows (even-though it is denied by the policy)

Example policy:

<?php

namespace App\Policies;

use Illuminate\Auth\Access\HandlesAuthorization;

class ProjectPolicy
{
    use HandlesAuthorization;

    public function viewAny()
    {
        return true;
    }

    public function view()
    {
        return true;
    }

    public function create()
    {
        return false;
    }

    public function update()
    {
        return false;
    }

    public function delete()
    {
        return false;
    }

    public function restore()
    {
        return false;
    }

    public function forceDelete()
    {
        return false;
    }
}

Result:

bulk-delete

Steps to reproduce

• Create a policy for a resource to test with that denies everything except viewing

Relevant log output

No response

danharrin commented 2 years ago

That bulk action relies on the deleteAny method of the resource, as the delete method usually checks a specific record.

danharrin commented 2 years ago

I've added more clarification here.

dive-michiel commented 2 years ago

Ok, thanks for the clarification!

In my opinion: I do find it a bit strange to have a deleteAny method. Deleting is a destructive action and if you don't have permission to delete a single record, why would you have permission to delete any/all record(s)?

P.S. congrats on the v2 release, looking very promising.

danharrin commented 2 years ago

If we used the delete method, we'd have to loop through every selected record and check if you have permission to delete them. Since this is a "bulk action", this probably isn't very performant. So, a deleteAny() method that doesn't accept a Model instance parameter is better IMO. Does that make more sense? Or am I missing something? ;)

dive-michiel commented 2 years ago

That's true, you could show the action as long as at least one record has the delete permission, if no records have the permission this will have to loop all - like you said. But if the page sizes aren't huge, this should have only minimal performance implications? I fully understand your concerns and adding the policy list to the docs is good enough for me :)

danharrin commented 2 years ago

I've considered the fact that the pages are only 10 records, but you can still "Select all records" which selects all pages :) Don't wanna go explaining to people why they have (potentially) thousands of queries running to show a "Delete selected" button, if they're using database authorisation.

romaldyminaya commented 2 years ago

Hi @danharrin,

I have the same problem in a proyect I'm working with, I found the solution and I'm pushing it in a few minutes.