[Django] #18763: Shortcut to get users by permission
Django
------------------------------+--------------------
Reporter: shelldweller | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by shelldweller):
* cc: shelldweller (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:1>
Django
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by pelletier):
* cc: pelletier (added)
* stage: Unreviewed => Design decision needed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:2>
Django
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by shelldweller):
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')`)
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:3>
Django
Reporter: shelldweller | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by leckey.ryan@…):
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:
{{{#!python
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:4>
Django
Reporter: shelldweller | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anonymous):
On the above I meant `User.objects.for_perm` instead of `user.for_perm`
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:5>
Django
Reporter: shelldweller | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:6>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Osmose):
* owner: nobody => Osmose
* status: new => assigned
* has_patch: 0 => 1
Comment:
Added a pull request for this issue:
https://github.com/django/django/pull/2551
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:7>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jorgecarleitao):
* needs_better_patch: 0 => 1
Comment:
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`.
2. 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?)
3. 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:8>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by slurms):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:9>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by slurms):
Added documentation, updated pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:10>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by slurms):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:11>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by slurms):
* stage: Ready for checkin => Accepted
Comment:
Had a bit of a derp moment, I need someone else to review it before it's
RFC. Sorry.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:12>
Django
Reporter: shelldweller | Owner: Osmose
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
Comment:
There seem to be some (possibly non-deterministic) test failures
introduced by the patch. See Jenkins for details.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:13>
Django
Reporter: shelldweller | Owner: berkerpeksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* cc: berker.peksag@… (added)
* needs_better_patch: 1 => 0
* owner: Osmose => berkerpeksag
Comment:
[https://github.com/django/django/pull/7153 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:14>
Django
Reporter: shelldweller | Owner: berkerpeksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:15>
Django
Reporter: shelldweller | Owner: berkerpeksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:16>
Django
Reporter: shelldweller | Owner: Berker Peksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:17>
Django
Reporter: Sergiy Kuzmenko | Owner: Berker Peksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Berker Peksag):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:18>
Django
Reporter: Sergiy Kuzmenko | Owner: Berker Peksag
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:19>
Django
Reporter: Sergiy Kuzmenko | Owner: Nick Pope
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* owner: Berker Peksag => Nick Pope
* needs_better_patch: 1 => 0
Comment:
New [https://github.com/django/django/pulls/11625 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:20>
Django
Reporter: Sergiy Kuzmenko | Owner: Nick Pope
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:21>
Django
Reporter: Sergiy Kuzmenko | Owner: Nick Pope
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"400ec5125ec32e3b18d267bbb4f3aab09d741ce4" 400ec512]:
{{{
#!CommitTicketReference repository=""
revision="400ec5125ec32e3b18d267bbb4f3aab09d741ce4"
Fixed #18763 -- Added ModelBackend/UserManager.with_perm() methods.
Co-authored-by: Nick Pope <nick...@flightdataservices.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:22>