Re: [Django] #10609: Permissions on admin actions
Django
---------------------------------------+-------------------------------
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: assigned
Milestone: | Component: contrib.admin
Version: 1.1-beta-1 | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+-------------------------------
Changes (by vasiliyeah):
* cc: vvangelovski@… (added)
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:13>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by julien):
See #17097 which is dealing with the special case of
`CommentsAdmin.get_actions()`. I think that this ticket (#10609) should be
fixed first as it is dealing with the issue of permissions and actions in
a more general way.
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:14>
Django
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 1 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Elvard):
* cc: tomas.ehrlich@… (added)
* needs_docs: 1 => 0
* version: 1.1-beta-1 => master
* stage: Accepted => Design decision needed
Comment:
Decorator '''permission_required''' from ''contrib.auth'' can't be used,
since admin actions (even when sometimes they behave like views) take
modeladmin instance as first parameter and request as second (with
queryset as third).
Therefore, I wrote '''action_permission_required''' and
'''action_user_passes_test''' decorators which works similary like
'''permission_required''' and '''user_passes_test''', respectively. The
only difference is that permission can be specified using glob pattern, eg
*.delete_*. The reason behind this is that some actions can be shared
among models like the default ''delete_selected action''.
Documentation and tests are included, but more tests needed. I expect some
discussion about these two new decorators and "glob" pattern, so I'm
submiting incomplete patch right now.
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:15>
Django
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* needs_better_patch: 0 => 1
* stage: Design decision needed => Accepted
Comment:
Thank you for your work on this feature.
A couple administrative related bits:
First it would be a stretch to land this in 1.5 - there are a couple
issues in the current patch - and the feature freeze has already
theoretically passed.
Second - design decision needed should be used sparingly and only when a
tough architectural design decision is required
https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#design-decision-needed
In this case, having non-matching function signatures doesn't mean the
concept is not accepted.
OK - now on to the patch
There should be no carryover of the redirect to login view or raising 403
- those are view related and don't apply in this context- we know we are
already logged into the admin when we are gathering actions.
Likewise, what we are testing, ends up determining what actions are
presented to the user, not whether they will run.
Including the test function a second time inside the wrapped function
doesn't make sense. If you can't see it, you can't run it - this is
currently insured in the response_action method, which would raise a
KeyError at this line:
{{{func, name, description = self.get_actions(request)[action]}}}
with regard to the naming - I can't imagine a place where you would use
these in the same file as the view decorators - so they can share the same
name, the decorator should be descriptive and should make sense in context
- having admin in the name is redundant when used in code that is clearly
enough admin related.
Should this feature really be implemented as decorators at all? We really
don't intend to change what the action does - only assign a test_func
attribute to it.
Finally (ok, this could be a design decision) we should at least explore
whether it would be feasible to do this at a per-object permission level.
Per object permission are a standard part of the permission system - the
contract is that a permission check should get the object they are making
the decision about - but don't have to pay attention to it.
If I've added a permission check that does check objects - I would want
that to be utilized here.
So here is a rough outline of how that might work. We specify something,
perhaps a method on the modeladmin that handles the permssion check like:
This is used with obj=None when gathering the list of actions to display,
and is then used when an action is invoked to create an intermediate page
displaying what objects you do or don't have permission for - maybe with
some option to show only if you don't have permission to act on some
objects. The full QS is then filtered down to those you do have
permission for - and passed to the original, simple, action function.
Like I said that is rough - but should be explored for this feature, even
if it is a bit harder to implement - I don't think we can just skip on
trying to do this while respecting obj level permissions.
I'm marking back to accepted, as the feature is accepted - it is mostly a
question of implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:16>
Django
-------------------------------+------------------------------------
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Elvard):
Thank you for reviewing this patch. I still need to get used to whole
workflow.
Decorators are indeed unnecessary with respect to implementation of
'''render_action''' as mentioned above. I propose add another property to
action, '''passes_test''':
{{{#!python
def delete_selected(modeladmin, request, queryset):
....
delete_selected.short_description = "Delete selected objects"
# Calling modeladmin methods, eg. has_delete_permission,
has_add_permission, etc.
delete_selected.passes_test = "has_delete_permission"
...
# Checking specific permissions
action.passes_test = permission_required("poll.can_vote")
...
# Passing test function directly
action.passes_test = test_func
}}}
where '''permission_required''' is just function which construct test_func
with proper arguments (modeladmin, request, queryset).
'''passes_test''' property is then evaluated and return filtered queryset.
If queryset is non empty, user can trigger action.
{{{#!python
# Example of test_func which allows to delete only entries that are
'owned' by user:
def delete_owned_only(modeladmin, request, queryset):
return queryset.filter(author=request.user)
}}}
String value in passes_test property could be handled like this:
{{{#!python
if hasattr(modeladmin, action.passes_test):
test_func = getattr(modeladmin, action.passes_test)
new_qs = []
for obj in queryset:
if test_func(request, obj):
new_qs.append(obj)
}}}
What would you think about this implementation?
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:17>
Django
-------------------------------+------------------------------------
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Elvard):
Well, after while I realized that this approach doesn't work.
There're two stages of permission check:
1. Check if user can run this action at all
2. Check if there are any objects on which user can run action (per-model
permissions)
Result of first check is whether user can see (and run) action or not.
Result of second check is filtered queryset which is passed to action
function. In case of has_*_permissions, we have to iterate over whole
queryset and, therefore, we can't use it in '''get_actions'''.
I have to think about it, since I have no idea how to implement it. Any
suggestions appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:18>
Django
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Josh Smeaton):
* status: assigned => closed
* resolution: => duplicate
Comment:
I believe the feature described at
https://docs.djangoproject.com/en/2.1/ref/contrib/admin/actions/#setting-
permissions-for-actions covers this, no? I'm not able to find the ticket
implementing that feature just now though.
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:19>
Django
-------------------------------+-------------------------------------
Reporter: leitjohn | Owner: leitjohn
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-------------------------------------
Comment (by Tim Graham):
Yes, it was added in Django 2.1 (#29419).
--
Ticket URL: <https://code.djangoproject.com/ticket/10609#comment:20>