Opened 16 years ago
Last modified 5 years ago
#11383 new Bug
Admin action 'Delete selected' check only global model delete permission
Reported by: | Owned by: | ||
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | delete permission admin |
Cc: | barton@…, Florian Apolloner, bas@…, IanMLewis@…, nils@…, kmike84@…, adi@…, tomc | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Action 'delete_selected' calls ModelAdmin's has_delete_permission method only once without obj argument.
(This action is run from object list with checked records)
It is problem if has_delete_permission contains more complex logic which returns different values for a particular objects.
If one of deleted objects must not be delete whole action should fail.
Simple workaround is always forbid global delete (it means return False if obj argument is not given) and allow delete only for specified objects.
But such solutuion still disallow to do multiple delete on objects which can be deleted separately from it's detail form.
Change History (19)
comment:1 by , 15 years ago
Summary: | Admin actiion 'Delete selected' check only global model delete permission → Admin action 'Delete selected' check only global model delete permission |
---|
comment:2 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 15 years ago
Cc: | added |
---|
comment:4 by , 15 years ago
Cc: | added |
---|
comment:5 by , 14 years ago
Cc: | added |
---|
comment:6 by , 14 years ago
Cc: | added |
---|
comment:9 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:10 by , 13 years ago
Easy pickings: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
UI/UX: | unset |
comment:11 by , 13 years ago
Cc: | added |
---|
comment:12 by , 12 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:13 by , 11 years ago
Cc: | added |
---|
comment:14 by , 10 years ago
Cc: | added |
---|
comment:16 by , 8 years ago
What would the preferred UX be for this?
- on POST, abort on the first object for which modeladmin.has_permission returns
False
, with a message that the user doesn't have permission to delete the obj. - on POST, iterate over all objects in the queryset and collect the ones modeladmin.has_permission returns
False
for. If any can't be deleted then abort the action with a list of records which couldn't be delete. - on POST, iterate over all objects in the queryset and only delete the ones modeladmin.has_permission returns
True
for and add an the admin message for the user stating that N objects were skipped. - on POST, iterate over all objects in the queryset and collect the ones modeladmin.has_permission returns
False
for. If any can't be deleted display them to the user and ask him if he want's to delete the other ones. - on the confirm deletion page split the list in two parts. First part lists the deletable objects, second part lists the non deletable objects. Then on POST submit the deletable object IDs and only delete those.
If we start checking for object permissions here, this actually open the whole box of how to handle the related objects. All the related objects are already iterated using admin.utils.get_deleted_objects, but this only calls user.has_perm
comment:17 by , 5 years ago
There is a way to enforce bulk action deletes based on per object permissions. We used the following approach:
def has_delete_permission(self, request, obj=None): # Only superusers can delete if not request.user.is_superuser: return False if obj: # Code to check if obj is allowed to be deleted can_delete = ..... return can_delete # Check if multiple items are selected to be deleted if ( request.path.startswith('/admin/<path_to_model_being_deleted>') and request.POST and request.POST.get('action') == 'delete_selected' ): # Get list of ids that are going to be deleted id_list = request.POST.getlist(admin.helpers.ACTION_CHECKBOX_NAME) # Use id_list to lookup object or perform some query to see if that object can be deleted for id in id_list: can_delete = .... if not can_delete: # This object can't be deleted so return False to prevent entire request return False # User has delete permissions return True # If you want a custom error message def delete_selected(self, request, queryset): if not self.has_delete_permission(request): # You can also use request.POST.getlist(admin.helpers.ACTION_CHECKBOX_NAME) # to generate a even more detailed message. messages.error(request, "One of the items selected can't be deleted") return return actions.delete_selected(self, request, queryset) delete_selected.short_description = actions.delete_selected.short_description actions = [delete_selected]
Unfortunately this approach no longer generates an appropriate error message as of 2.1 because the has_delete_permission
method is also used to filter the actions. So it will prevent the delete from happening but it just prints a warning No action selected
because the action is filtered out before it can get to the actual delete permission check.
comment:18 by , 5 years ago
If the ModelAdmin._filter_actions_by_permissions
method were updated so that it passed in some variable to the has_permission
function to identify that it was being used to filter the actions the has_delete_permission
could act accordingly. Then when AdminViewPermissionBaseModelAdmin.get_model_perms
calls has_delete_permission
it can differentiate from the action filter and return false with an appropriate error message.
comment:19 by , 5 years ago
I just realized you can use the error message work around to solve the issue introduced in 2.1.
def has_delete_permission(self, request, obj=None): # Only superusers can delete if not request.user.is_superuser: return False if obj: # Code to check if obj is allowed to be deleted can_delete = ..... return can_delete # Bulk deletes will be verified in the overridden delete_selected method below return True # Override the delete_selected action def delete_selected(self, request, queryset): # Check if multiple items are selected to be deleted if ( request.path.startswith('/admin/<path_to_model_being_deleted>') and request.POST and request.POST.get('action') == 'delete_selected' ): # Get list of ids that are going to be deleted id_list = request.POST.getlist(admin.helpers.ACTION_CHECKBOX_NAME) # Use id_list to lookup object or perform some query to see if that object can be deleted for id in id_list: can_delete = .... if not can_delete: messages.error(request, "One of the items selected can't be deleted") return # Don't perform delete # Call the actual delete method return actions.delete_selected(self, request, queryset) delete_selected.short_description = actions.delete_selected.short_description actions = [delete_selected]
Since no one has commented on this issue, I will try to put it another way.
Deleting objects in the admin is inconsistent between
The action delete_selected does not check has_delete_permission for each selected object. Instead, it calls has_delete_permission for all objects.
On the other hand, the admin will check if one has permission to delete the specific object in the view (the change form).
You have to disable the action delete_selected virtually if has_delete_permission is in effect. In my humble opinion, the admin should call has_delete_permission for each selected object with the action delete_selected.