Opened 8 years ago

Last modified 17 months ago

#10609 assigned New feature

Permissions on admin actions

Reported by: leitjohn Owned by: leitjohn
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: code_djangoproject_com@…, vvangelovski@…, tomas.ehrlich@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Often one would not want an option to appear in the actions select box in the changelist. For instance, you may not want a user who can't delete to see "delete_selected".

I propose that we enable the admin to use the auth decorators (permission_required, etc).

Index: options.py
===================================================================
--- options.py	(revision 10163)
+++ options.py	(working copy)
@@ -423,7 +423,11 @@
         for klass in [self.admin_site] + self.__class__.mro()[::-1]:
             for action in getattr(klass, 'actions', []):
                 func, name, description = self.get_action(action)
-                actions[name] = (func, name, description)
+		
+                # check permission
+                test = getattr(func, "test_func", None)
+                if test == None or test(request.user):
+                    actions[name] = (func, name, description)
         return actions

Attachments (2)

action_permissions.patch (754 bytes) - added by leitjohn 8 years ago.
patch
10609-1.patch (16.3 KB) - added by Tomáš Ehrlich 4 years ago.
Added new decorators, documentation and tests

Download all attachments as: .zip

Change History (20)

Changed 8 years ago by leitjohn

Attachment: action_permissions.patch added

patch

comment:1 Changed 8 years ago by leitjohn

Cc: code_djangoproject_com@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 8 years ago by Malcolm Tredinnick

milestone: 1.1 beta1.1
Version: 1.01.1-beta-1

*Maybe* this is in scope for 1.1, maybe not. But it's certainly not a 1.1-beta milestone, since that had passed even before the ticket was opened.

comment:3 Changed 8 years ago by leitjohn

Status: newassigned

Opps, that was a little ambitious of me :)

comment:4 Changed 7 years ago by Alex Gaynor

Component: Uncategorizeddjango.contrib.admin

comment:5 Changed 7 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:6 Changed 7 years ago by Jacob

milestone: 1.11.2

This is a feature add, so I'm punting to 1.2. For now, this is possible by overriding ModelAdmin.get_actions.

comment:7 Changed 7 years ago by James Bennett

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:8 Changed 6 years ago by Julien Phalip

#13709 was a dupe and has a different patch.

comment:9 Changed 6 years ago by Julien Phalip

#11383 is a related issue.

comment:10 Changed 6 years ago by Ramiro Morales

#15599 was another duplicate.

comment:11 Changed 5 years ago by Chris Beaven

Severity: Normal
Type: New feature

comment:12 Changed 5 years ago by Julien Phalip

Needs documentation: set
Needs tests: set

comment:13 Changed 5 years ago by Vasil Vangelovski

Cc: vvangelovski@… added
Easy pickings: unset
UI/UX: unset

comment:14 Changed 5 years ago by Julien Phalip

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.

comment:15 Changed 4 years ago by Tomáš Ehrlich

Cc: tomas.ehrlich@… added
Needs documentation: unset
Triage Stage: AcceptedDesign decision needed
Version: 1.1-beta-1master

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.
http://lockerdome.com/blog

Last edited 17 months ago by johnhomes (previous) (diff)

Changed 4 years ago by Tomáš Ehrlich

Attachment: 10609-1.patch added

Added new decorators, documentation and tests

comment:16 Changed 4 years ago by Preston Holmes

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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:

https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.has_delete_permission

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.

comment:17 Changed 4 years ago by Tomáš Ehrlich

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:

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.

# 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:

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?

comment:18 Changed 4 years ago by Tomáš Ehrlich

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.

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