Opened 5 weeks ago

Closed 3 days ago

Last modified 3 days ago

#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 Carlton Gibson)

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 Changed 5 weeks ago by Tim Graham

Severity: NormalRelease blocker
Summary: Missing change in permissions for admin actions from release notesDocument the change in permissions required to perform admin actions
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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?

comment:2 Changed 5 weeks ago by Hiroki Kiyohara

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...

Also I agree that the current design is not perfect.

Last edited 5 weeks ago by Hiroki Kiyohara (previous) (diff)

comment:3 Changed 5 weeks ago by Hiroki Kiyohara

Owner: changed from nobody to Hiroki Kiyohara
Status: newassigned

comment:4 Changed 5 weeks ago by Hiroki Kiyohara

Has patch: set

I opened the Pull Request https://github.com/django/django/pull/9970
(Working at DjangoCongress JP 2018 Sprint).

Last edited 5 weeks ago by Hiroki Kiyohara (previous) (diff)

comment:5 Changed 5 weeks ago by Paulo

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 Changed 3 weeks ago by Carlton Gibson

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 Changed 3 weeks ago by Carlton Gibson

Has patch: unset

comment:8 Changed 2 weeks ago by Carlton Gibson

Description: modified (diff)
Summary: Document the change in permissions required to perform admin actionsLimiting visibility of Admin actions to `change` permission only isn't right.

comment:9 Changed 2 weeks ago by Carlton Gibson

Cc: Carlton Gibson added
Has patch: set

comment:10 Changed 2 weeks ago by Carlton Gibson

New PR partially reverting 825f0beda804e48e9197fcf3b0d909f9f548aa47 to restore visibility of actions to all staff users.

_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 Changed 2 weeks ago by Hiroki Kiyohara

Owner: Hiroki Kiyohara deleted
Status: assignednew

I think Carlton Gibson should be assigned this issue, so I deassign myself.

comment:12 Changed 2 weeks ago by Carlton Gibson

Owner: set to Carlton Gibson
Status: newassigned

comment:13 Changed 2 weeks ago by Carlton Gibson

Component: Documentationcontrib.admin

comment:14 Changed 8 days ago by Tim Graham <timograham@…>

In b30f9b13:

Refs #29419, #8936 -- Removed change permission requirement for admin actions.

Partially reverted 825f0beda804e48e9197fcf3b0d909f9f548aa47.

comment:15 Changed 8 days ago by Tim Graham <timograham@…>

In aea0e2b9:

[2.1.x] Refs #29419, #8936 -- Removed change permission requirement for admin actions.

Partially reverted 825f0beda804e48e9197fcf3b0d909f9f548aa47.

Backport of b30f9b131c9489b9d9f21c311ecb46d0aea91381 from master

comment:16 Changed 8 days ago by Tim Graham

Patch needs improvement: set
Summary: Limiting visibility of Admin actions to `change` permission only isn't right.Allow permissioning of admin actions
Type: BugNew feature

comment:17 Changed 6 days ago by Carlton Gibson

Patch needs improvement: unset

OK, bar another review I think the PR is more or less there.

comment:18 Changed 3 days ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 958c7b30:

Fixed #29419 -- Allowed permissioning of admin actions.

comment:19 Changed 3 days ago by Tim Graham <timograham@…>

In 306f1f8:

[2.1.x] Fixed #29419 -- Allowed permissioning of admin actions.

Backport of 958c7b301ead79974db8edd5b9c6588a10a28ae7 from master

Note: See TracTickets for help on using tickets.
Back to Top