Filament•7mo ago

DeleteBulkAction and ForceDeleteBulkAction policies

I had a look to the code of those 2 actions and I don't see any check if the user is authorized or not to delete or forceDelete the record. Is it managed somewhere else ? If not, as all FI stuff remains on authorisations and policies, does it make sense to check that in the process of those actions. I also understand that table builder can also be used outside of filament admin panel, so does is means we have to customise each process to check authorisation ?

16 Replies

David VincentOP•7mo ago

Same thought regarding RestoreBulkAction

awcodes•7mo ago

https://filamentphp.com/docs/3.x/panels/resources/getting-started#authorization

Getting started - Panel Builder - Filament

David VincentOP•7mo ago

Maybe I missed smth, deleteAny policy action does not require any model, right ? So we apply delete on a collection of records without checking individual policy. Tell me if I'm wrong ? "Filament uses the deleteAny() method because iterating through multiple records and checking the delete() policy is not very performant." mentioned in the doc any way, for the time being i use

 Tables\Actions\DeleteBulkAction::make()
            ->authorize(static::canDeleteAny())
            ->using(function (Collection $records) {
                $records->each(function ($record) {
                    if (static::canDelete($record)) {
                        $record->delete();
                    }
                });
            })

 Tables\Actions\DeleteBulkAction::make()
            ->authorize(static::canDeleteAny())
            ->using(function (Collection $records) {
                $records->each(function ($record) {
                    if (static::canDelete($record)) {
                        $record->delete();
                    }
                });
            })

even if it's not performant

awcodes•7mo ago

I think the ‘any’ actions are used primarily to show or hide the actions since it’s assumed that if the user can perform the action on any record then the actual record isn’t relevant.

David VincentOP•7mo ago

humm, you can display a list of records (viewAny) with some you can delete and some not, it does not mean you can deleteAny

awcodes•7mo ago

I’d have to do some more digging into the code to see where the actual policy is being checked.

David VincentOP•7mo ago

So you can say, you remove the deleteAny permission, but in that case you don't have access to the bulkAction

awcodes•7mo ago

I think at that level you’d have to use roles and permissions for the user. Ie, user can viewAny but they can’t deleteAny. I see what you’re getting at though. A custom bulk delete might be necessary for your use case then. Just hope no one is trying to bulk delete 100s or 1000s of records. 😅

David VincentOP•7mo ago

Yes but from a general perspective, the actual behaviour allows a user to deleteAny (assuming he has the permission) and delete a record for which he has not the permission to. Strange behaviour to me

awcodes•7mo ago

That’s fair. Dan or Dennis probably know more about the internals of it than I do. I didn’t say to tag them. Lol.

David VincentOP•7mo ago

oups sorry deleted

awcodes•7mo ago

Hopefully one of them will see the thread and respond.

Dennis Koch•7mo ago

even if it's not performant

Yeah, I guess that's why we don't check auth for every record by default. That goes for all BulkActions.

allows a user to deleteAny (assuming he has the permission) and delete a record for which he has not the permission to.

I know that sometimes permissions need to be a bit more fine grained, but doesn't "delete any" imply that he can "delete any" record?

David VincentOP•7mo ago

I understand your point, even if I don't totally agree. Sometimes you're right, but there are many situations where even if you can delete any, there are some records which can't be deleted. example 1 : a tasks table, each task has an owner, so I can have the viewAny permission, deleteAny permission (to gain access to the bulkAction) but I can only delete mine. Another example will be a table which contains system's entries and entries created by the user, the user can only delete the ones he created, not the system ones. There are many situations. I understand the performance issue, but in my situation I prefer to have a better system logic so I understand that I have to implement my own logic. Maybe the documentation should warn more the users on this.

Zen Nitiruj•7mo ago

I totally agree. DeleteBulkAction still allow to users even we unable them in Policy to not allow delete and not allow deleteAny. It does not make sense to custom set here. This should be added to next version. Also fo view, I cannot view the record, if I don't also enable set viewAny. That means I cannot directly open a page if I don't set view any for that user. I think view a record should not need to view any.

Dennis Koch•7mo ago

DeleteBulkAction still allow to users even we unable them in Policy to not allow delete and not allow deleteAny.

If you don't allow deleteAny this should be respected.

Gaming

Programming

DeleteBulkAction and ForceDeleteBulkAction policies