Re: [Django] #11383: Admin action 'Delete selected' check only global model delete permission
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: cyrus
Type: Bug | Status: assigned
Milestone: | Component: contrib.admin
Version: SVN | Severity: Normal
Resolution: | Keywords: delete permission
Triage Stage: Accepted | admin
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by cyrus):
* owner: nobody => cyrus
* status: new => assigned
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: cyrus
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: kmike84@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner:
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: cyrus =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: adi@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: tomc (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:14>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Al Johri):
Running into exactly this issue. Is this documented somewhere?
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by adsworth):
What would the preferred UX be for this?
1. 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.
2. 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.
3. 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.
4. 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.
5. 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
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by rwlogel):
There is a way to enforce bulk action deletes based on per object
permissions. We used the following approach:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by rwlogel):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: krejcik@… | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: delete permission | Triage Stage: Accepted
admin |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by rwlogel):
I just realized you can use the error message work around to solve the
issue introduced in 2.1.
{{{#!python
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]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11383#comment:19>