Opened 12 years ago

Closed 5 years ago

#18763 closed New feature (fixed)

Shortcut to get users by permission

Reported by: Sergiy Kuzmenko Owned by: Nick Pope
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Sergiy Kuzmenko, pelletier, berker.peksag@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In my apps I often need to get the list of users who have a specific permission. But it seems there is no shortcut to do this in Django itself (unless I'm missing something obvious). And getting users by permission is slightly more complicated than may appear at first sight because user permission can be set at user level or at group level, and also we need to pay special attention to superusers.

So I usually end up doing something like this:

from django.contrib.auth.models import User
from django.db.models import Q

def get_users_by_permission_q(permission_name, include_superusers=True):
    """ Returns the Q object suitable for querying users by permission. If include_superusers
    is true (default) all superusers will be also included. Otherwise
    only users with explicitely set permissions will be included. """
    (appname, codename) = permission_name.split(".")
    
    query = \
        Q(user_permissions__codename=codename, user_permissions__content_type__app_label=appname) | \
        Q(groups__permissions__codename=codename, groups__permissions__content_type__app_label=appname)    
    
    if include_superusers:
        query |= Q(is_superuser=True)
    
    # The above query may return multiple instances of User objects if user
    # has overlapping permissions. Hence we are using a nested query by unique
    # user ids.
    return {'pk__in': User.objects.filter(query).distinct().values('pk')}

def get_users_by_permission(permission_name, include_superusers=True):
    """ Returns the queryset of User objects with the given permission. Permission name
    is in the form appname.permission similar to the format
    required by django.contrib.auth.decorators.permission_required
    """
    return User.objects.filter( get_users_by_permission_q(permission_name, include_superusers) )

And them in my models.py:

class MyModel:
    my_fk_field = models.ForeignKey(User, limit_choices_to=dict(is_active=True, \
        *get_users_by_permission_q("myapp.change_my_model", False)))

Or in my views:

users = get_users_by_permission("myapp.change_my_model")

It would be nice to have something like this in django/contrib/auth/utils.py

If this proposal gets accepted I can contribute a patch.

Change History (22)

comment:1 by Sergiy Kuzmenko, 12 years ago

Cc: Sergiy Kuzmenko added

comment:2 by pelletier, 12 years ago

Cc: pelletier added
Triage Stage: UnreviewedDesign decision needed

To my mind that's indeed an interesting feature, so I mark the ticket has DDN in order to get the opinion of a core developer.

In addition, here two cents on your code:

  • I think this kind of code belongs to UserManager, so as we could do User.objects.get_user_with_perm(...) or something like that.
  • Having the "reverse" method on Permission objects could also be nice: aperm.get_users().
  • Maybe instead of using a string to identify the permission (which is apparently not the natural key here) you should just take a Permission object and use it in the query. I think it would make the code clearer and also allow you to ensure the permission exists. Or the argument could be a natural key, then you retrieve and permission using Permission.objcets.get_by_natural_key() and use it in query.

comment:3 by Sergiy Kuzmenko, 12 years ago

It would be also useful to have this functionality somehow exposed on the admin site. Right now there is no easy way to find out who is authorized to do what.

@pelletier:

  • UserManager sounds like the right way to do it but I initially decided against it because I wanted a quick and dirty is_superuser flag. Otherwise I'd have to manually construct the Q object every time I'd want to include superusers into the list.
  • Having the "reverse" method would be useful indeed.
  • I used the string instead of Permission object because it's how django operates already (e.g., in user.has_perm('foo.add_bar'))

comment:4 by leckey.ryan@…, 11 years ago

I would propose for_perm and for_perms methods on both the user and the default object manager.

So statements like the following could happen:

user.for_perm('foo.bar')     # Queryset of users that have that permission
user.for_perm('foo.bar', o)  # Queryset of users that have that permission for 

Poll.objects.for_perm(user, 'foo.bar')   # Queryset of all Polls where the user has the permission for

# .. `for_perms` would be much the same except with an iterable for permission names like `has_perms`

Of course this should be hooked through the permission backend and not special-cased for Permission objects.

I could write a patch to add the for_perm and for_perms hooks as well as implementation for the ModelBackend if the above sounds fine. Thought about this for awhile now. Thoughts?

comment:5 by anonymous, 11 years ago

On the above I meant User.objects.for_perm instead of user.for_perm

comment:6 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

Even though this pattern is almost always a symptom of misuse of the permissions system, it can be handy once in a while.

This should be implemented as a method in the manager for User: User.objects.with_perm('foo.bar'). Keep the API simple.

comment:7 by Michael Kelly, 10 years ago

Has patch: set
Owner: changed from nobody to Michael Kelly
Status: newassigned

Added a pull request for this issue: https://github.com/django/django/pull/2551

comment:8 by jorgecarleitao, 10 years ago

Patch needs improvement: set

At the moment, permissions for a given user are checked on ModelBackends. The PermissionsMixin goes to all backends and checks if any ModelBackend.has_perm returns True to that given permission.

This means that permissions are not uniquely defined by the database relations between users and permissions; they also rely on the hard coded conditions on the has_perm of the backend. For instance, the default Backend also checks if the user is active.

Thus, I'm not sure we can have a list of users with a given permission just by looking at the database relationships. If get_users_by_permission("can_delete_database") is not telling anything about the permission, I wonder if it should exist...

Besides, this could easily be used to (misleadingly) create a security breach:

  1. create a permission "can_delete_database", and a custom backend that says that the user can delete the database if it has user_permission "can_delete_database" and fulfills a hard coded and very restricted condition user.is_system_admin.
  1. check that the user has permission to delete the database using if user in get_users_by_permission("can_delete_database") (why should I use user.has_perm("can_delete_database") when I can cache the result of get_users_by_permission(X) and use for all users?)
  1. boom

I'm not sure we want to incentivate a developer to use 2 to check permissions.

(Point initially raised on github, https://github.com/django/django/pull/2551#issuecomment-42316616, and now transcripted to here)

comment:9 by Nick Sandford, 10 years ago

Moved with_perm checking to the authentication backends. Current strategy is to use the top-most backend that defines with_perm. django.contrib.auth.backends.ModelBackend falls back to checking the same as was previously on the UserManager. This allows custom authentication backends to specify their own behaviour for with_perm, that takes obj into account.

It is now possible to do User.objects.with_perm('can_delete', obj) along with User.objects.with_perm('can_delete').

Pull request here: https://github.com/django/django/pull/2951

Last edited 10 years ago by Nick Sandford (previous) (diff)

comment:10 by Nick Sandford, 10 years ago

Added documentation, updated pull request.

comment:11 by Nick Sandford, 10 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:12 by Nick Sandford, 10 years ago

Triage Stage: Ready for checkinAccepted

Had a bit of a derp moment, I need someone else to review it before it's RFC. Sorry.

comment:13 by Tim Graham, 10 years ago

Patch needs improvement: set

There seem to be some (possibly non-deterministic) test failures introduced by the patch. See Jenkins for details.

comment:14 by Berker Peksag, 8 years ago

Cc: berker.peksag@… added
Owner: changed from Michael Kelly to Berker Peksag
Patch needs improvement: unset

comment:15 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:16 by Berker Peksag, 8 years ago

Patch needs improvement: unset

comment:17 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:18 by Berker Peksag, 5 years ago

Patch needs improvement: unset

comment:19 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:20 by Nick Pope, 5 years ago

Owner: changed from Berker Peksag to Nick Pope
Patch needs improvement: unset

New PR.

comment:21 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 400ec512:

Fixed #18763 -- Added ModelBackend/UserManager.with_perm() methods.

Co-authored-by: Nick Pope <nick.pope@…>

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