#29419 closed New feature (fixed)
Allow permissioning of admin actions
Reported by: | Paulo | Owned by: | Carlton Gibson |
---|---|---|---|
Component: | contrib.admin | Version: | 2.1 |
Severity: | Release blocker | Keywords: | |
Cc: | Carlton Gibson | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
In the pr that introduced view only admin, there was also a change to require user to have the "change" permission in order to render admin actions.
This is a backwards incompatible change, as such, it would be good to have it in https://docs.djangoproject.com/en/dev/releases/2.1/
Change History (19)
comment:1 by , 6 years ago
Severity: | Normal → Release blocker |
---|---|
Summary: | Missing change in permissions for admin actions from release notes → Document the change in permissions required to perform admin actions |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 6 years ago
I understood. You meant the behavior of ModelAdmin.get_actions
has changed, and it should be described as backward incompatible change.
https://github.com/django/django/blob/2.1a1/django/contrib/admin/options.py#L863-L865
I agree.
We need...
- Writing about it in the release note
- Adding "version changed" note here https://docs.djangoproject.com/en/dev/ref/contrib/admin/actions/#django.contrib.admin.ModelAdmin.get_actions
Also I agree that the current design is not perfect.
- Actions like "CSV Download" requires only "view" permission, so disabling all actions by "change" permission does not make sense for me
- Django should notice which permission is required by each actions https://github.com/django/django/pull/5297#discussion_r42751084
comment:3 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 6 years ago
Has patch: | set |
---|
I opened the Pull Request https://github.com/django/django/pull/9970
(Working at DjangoCongress JP 2018 Sprint).
comment:5 by , 6 years ago
I agree with giving this change another look.
It would be more flexible to leave the permissions handling to the actions themselves.
Apps like django-tablib would no longer allow users to export without permission to change objects.
comment:6 by , 6 years ago
The previous behaviour here is to show all actions regardless of permissions. If then you (for example) try to delete objects without the delete permission you'll hit a permission denied error.
The idea in the PR is that actions won't be available to read-only users, and that change will imply e.g. delete. Whilst nice in theory, neither of those seem to be true.
There's no current mechanism for actions to specify their permissions. That might be a good addition as a separate feature. We could then filter displayed actions on that basis.
For now, the consistent thing would seem to be to continue to display all actions regardless of permission levels, allowing that users may run into permission denied errors, as they will have up to now.
comment:7 by , 6 years ago
Has patch: | unset |
---|
comment:8 by , 6 years ago
Description: | modified (diff) |
---|---|
Summary: | Document the change in permissions required to perform admin actions → Limiting visibility of Admin actions to `change` permission only isn't right. |
comment:9 by , 6 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:10 by , 6 years ago
New PR partially reverting 825f0beda804e48e9197fcf3b0d909f9f548aa47 to restore visibility of actions to all staff users.
- Individual actions should check permissions.
- Additionally, users may follow the steps in Conditionally enabling or disabling actions.
_Maybe_ we could add an allowed_permissions
tuple to the action functions — so delete_selected
would have (delete,)
— that could be examined in the default implementation of get_actions()
. (but that seems like a New Feature beyond the scope of this one...)
comment:11 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I think Carlton Gibson should be assigned this issue, so I deassign myself.
comment:12 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:13 by , 6 years ago
Component: | Documentation → contrib.admin |
---|
comment:16 by , 6 years ago
Patch needs improvement: | set |
---|---|
Summary: | Limiting visibility of Admin actions to `change` permission only isn't right. → Allow permissioning of admin actions |
Type: | Bug → New feature |
comment:17 by , 6 years ago
Patch needs improvement: | unset |
---|
OK, bar another review I think the PR is more or less there.
On second thought, I wonder if this is the best design decision. For example, you would expect the default "deleted selected" bulk action to require the delete permission rather than the change permission, right?