[Django] #20218: Default authorization backend returns False when queried for object level permissions
Django
permissions
-------------------------------+--------------------
Reporter: soren@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords: auth
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
The default auth backend, django.contrib.auth.backends.ModelBackend
unconditioally returns False when queried through User.has_perm() if
has_perm is passed an object.
I understand that erring on the side of caution is generally sound, but
doing it this way forces generic consumers of the auth framework (e.g.
Tastypie in my case) to know whether to pass an obj or not depending on
the authentication backend chosen. Always passing an obj to has_perm will
result in all requests being denied if using the default backend. Not
passing it makes it impossible to apply object level permissions even
though I've configured a capable authz backend for this.
Ticket #12462 suggests this is intentional, but doesn't give much of a
rationale. It seems to me that if you don't want a user to be able to edit
all objects of type XXX, don't give them the "app.change_XXX"?
If this isn't considered a bug, can you offer some advice on how to deal
with this situation from a generic application like Tastypie? How should
it determine when to pass an obj or not?
--
Ticket URL: <https://code.djangoproject.com/ticket/20218>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Yeah, I think the proper way would be to check the generic permission and
return True/False based on that. It seems incorrect that user.has_perm()
will return False if given any object, but True if no obj is given. The
interpretation seems to be that the user has permission for all objects,
but not for any single object which seems a bit strange.
Could get_all_permissions, when given an obj, return the set of all
permissions applicable to that obj. That is, if obj is Model subclass,
then query for all permissions for the obj's contenttype.
I am marking this as accepted. This will need very careful consideration
from backwards compatibility viewpoint. It might be the resolution will
need to be wontfix due to that.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:1>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* component: Uncategorized => contrib.auth
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:2>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:3>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adam-iris):
I want to bump this, because I just spent 30 minutes trying to figure out
why I wasn't seeing the behavior I expected from the documentation. (My
expectation was exactly the behavior proposed here -- if ''obj'' is a
model, return the permissions based on the content type.)
If nothing else, the documentation should note that what it's describing
is '''inapplicable under the default configuration'''.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:4>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Patches welcome, Adam.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:5>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I closed #27528 as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:6>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jamie Bliss):
Replying to [ticket:20218 soren@…]:
> Ticket #12462 suggests this is intentional, but doesn't give much of a
rationale. It seems to me that if you don't want a user to be able to edit
all objects of type XXX, don't give them the "app.change_XXX"?
I think the rationale is that the current behavior handles the general
permissions case (no object) while still falling through and allowing
another provider to handle the object-specific case.
The snippet to I used to work around this.
{{{#!python
class UseGeneralPermissions:
"""
Permissions provider that does object-level permissions by using
general permissions.
"""
def has_perm(self, user_obj, perm, obj=None):
if obj is None:
return False
else:
# Retry using general permissions
return user_obj.has_perm(perm)
}}}
As to how general and object-level permissions interact? That is
completely unspecified, and there isn't a clear answer how it should be.
1. A general given overrides an object-level ungiven, 2. A general ungiven
overrides an object-level given. 1 still allows efficient bulk operations
but UIs must check permissions on each object to know to display actions.
2 allows UIs to display actions optimistically, but can prevent efficient
bulk operations.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:7>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jamie Bliss):
* cc: astronouth7303@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:8>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mehmet Dogan):
Being unaware of this ticket, I had opened another (should have checked!).
Here it is:
Long story here: https://github.com/django-guardian/django-
guardian/issues/49
Short story: for authorization backends checking object level permissions
(like guardian) usually requires calling the django's default
authorization backend as a fallback to the more general set of
permissions:
{{{
if user.has_perm('foo.change_bar', obj=bar) or
user.has_perm('foo.change_bar'):
...
}}}
However, this not only looks ugly, but also requires polling of all the
backends twice, and thus, is a performance loss.
First, and possibly the best, solution to this is that, django does not
deny permission if obj argument is provided, but just ignores it. This is
also very logical, one who has a permission for the entire model/table,
would also have it for an instance/row. This way by properly ordering
backends in the settings, it could be a fallback solution for the lower
level checkers. This might be the move in the right direction, although it
is backwards incompatible.
A second solution is a keyword argument, such as `fallback_to_model=None`,
that will allow lower-level checkers mimic the model level permissions
that django does. Obviously, this is not DRY. But is needed if the first
solution is not accepted to get the necessary permissions with one round
of polling, and without cluttering the code. If it was accepted, it would
still be a useful addition since it would allow backends to prefer to
handle the fallback by themselves. Or, it would allow users who fallback
by default override that behavior and not fallback (via a value of
`False`), i.e., when object level permissions are definitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:9>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mehmet Dogan):
* cc: Mehmet Dogan (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:10>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mehmet Dogan):
Here is what I propose in terms of working around the backward
compatibility that seems to have kept it from being solved for so long.
1) define a global setting, say:
`OBJECT_PERMISSION_FALLBACK_TO_MODEL=False`. This is to help maintain the
default behavior (unless the setting is changed of course).
2) (as mentioned in the above comment) define a keyword argument at the
method level for occasional override, say: `fallback_to_model=None`.
Default value of `None` means it will be ignored in favor of the global
setting, otherwise, it will take precedence.
I can work on a patch if found reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:11>
Django
permissions
Reporter: soren@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mehmet Dogan):
Here is a sample patch:
https://github.com/doganmeh/django/commit/d85cd3a530984ab5e4cb42f93629a64eb0b65b07
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:12>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mehmet Dogan):
* owner: nobody => Mehmet Dogan
* needs_docs: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:13>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mehmet Dogan):
sore, astronouth7303:
this is being discussed at developers list:
https://groups.google.com/forum/#!topic/django-developers/MLWfvPPVwDk
please provide feedback, if you can. or, show support, if you want this to
be solved, and agree with my proposed solution. regards,
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:14>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mehmet Dogan):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:15>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mehmet Dogan):
* version: 1.5 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:16>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
I see three approaches here:
1. Close as "Won't Fix", as mentioned as possibility in initial comment
here.
2. Accept the BC change, with deprecation/migration path.
3. Break out the permissions aspect of `ModelBackend` in order to make it
pluggable.
The pain of 2 makes 1 more appealing.
These is some discussion of 3 on
https://code.djangoproject.com/ticket/13539#comment:16
I posted at more length on the discussion group.
https://groups.google.com/d/msg/django-developers/MLWfvPPVwDk/jWaYQkOUAAAJ
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:17>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by astandley):
I've gone back and forth on this issue, especially after reviewing all the
links Carlton has posted pertaining to it. I think at this point my
favored solution is the one proposed on ticket 13539 of adding a
`DefaultObjPermsBackend`, and setting it as part of the
`AUTHENTICATION_BACKEND` defaults. It combines some of the best aspects of
options 1, 2, and 3.
-It minimizes BC issues. Only installations which do not specify the
setting are effected. Migration/Depreciation warnings could be handled in
`django.conf.__init__`
-It changes the new default behavior of `user.has_perm(perm, obj)` to
return True as expected.
-It allows users a choice in behavior.
-It offers a documented example of model/object permission backends
working together.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:18>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:19>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tobias Bengfort):
I implemented a `DefaultObjectPermissionsBackend`:
https://github.com/django/django/pull/10601
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:20>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
Comment:
Unchecking `Patch needs improvement` to put the new PR back in the queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:21>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:22>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tobias Bengfort):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:23>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
As I [https://github.com/django/django/pull/10636#issuecomment-467902972
commented on the PR], I think this should be addressed with a
`ModelBackend` subclass **in django-guardian** that ignores the `obj`
parameter to checks, so that the "superset" behaviour is available.
Something like...
{{{
from django.contrib.auth.backends import ModelBackend
class ImpliedObjectPermissionsBackend(ModelBackend):
"""
Ignores `obj` parameter to permission checks to apply general
permissions
at object-level.
Use in place of `ModelBackend` in settings.AUTHENTICATION_BACKENDS
to avoid
the need to check permissions twice when using Guardian:
user.has_perm('foo.change_bar', obj=bar)
Rather than:
user.has_perm('foo.change_bar', obj=bar) or
user.has_perm('foo.change_bar')
...when using ModelBackend.
"""
def has_perm(self, user, perm, obj=None):
return super().has_perm(perm)
...
}}}
This would be a small and natural addition to django-guardian, where it's
totally out of place in django itself.
As such, as old as this ticket is, I'm inclined to close this as
`wontfix`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:24>
Django
permissions
Reporter: soren@… | Owner: Mehmet Dogan
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* has_patch: 1 => 0
* type: Bug => New feature
* stage: Accepted => Unreviewed
Comment:
Given that I closed the PR, I’m removing the “Has Patch”. Given that,
there’s not likely any patch acceptable, so I **will** close it.
Since this was Accepted originally, django-guardian has become (and been
for a long time now) the go-to solution for object permissions. Advocating
that this code belongs there is an option that wasn’t originally
available. Hence I’m happy with the change in decision. If people want to
push for inclusion then the mailing list is the place to go. (But given
that we have django-guardian…)
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:25>
Django
permissions
------------------------------+----------------------------------------
Reporter: soren@… | Owner: Mehmet Dogan
Component: contrib.auth | Version: master
Severity: Normal | Resolution: wontfix
Keywords: auth | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+----------------------------------------
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/20218#comment:26>