--
Ticket URL: <https://code.djangoproject.com/ticket/18763>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* cc: shelldweller (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:1>
* 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>
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>
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>
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>
* 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>
* 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>
* 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>
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>
Comment (by slurms):
Added documentation, updated pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:10>
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:11>
* 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>
* 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>
* 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>
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:15>
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:16>
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:17>
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:18>
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:19>
* 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>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18763#comment:21>
* 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>