Opened 12 years ago
Closed 6 years ago
#20218 closed New feature (wontfix)
Default authorization backend returns False when queried for object level permissions
Reported by: | Owned by: | Mehmet Dogan | |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | auth |
Cc: | astronouth7303@…, Mehmet Dogan | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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?
Change History (26)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Component: | Uncategorized → contrib.auth |
---|
comment:3 by , 11 years ago
Type: | Uncategorized → Bug |
---|
comment:4 by , 10 years ago
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.
comment:7 by , 8 years ago
Replying to 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.
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.
comment:8 by , 8 years ago
Cc: | added |
---|
comment:9 by , 7 years ago
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.
comment:10 by , 7 years ago
Cc: | added |
---|
comment:11 by , 7 years ago
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.
comment:12 by , 7 years ago
Here is a sample patch:
https://github.com/doganmeh/django/commit/d85cd3a530984ab5e4cb42f93629a64eb0b65b07
comment:13 by , 7 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Owner: | changed from | to
Status: | new → assigned |
comment:14 by , 7 years ago
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,
comment:15 by , 7 years ago
Needs documentation: | unset |
---|
comment:16 by , 7 years ago
Version: | 1.5 → master |
---|
comment:17 by , 7 years ago
I see three approaches here:
- Close as "Won't Fix", as mentioned as possibility in initial comment here.
- Accept the BC change, with deprecation/migration path.
- 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 (and following comments)
I posted at more length on the discussion group. https://groups.google.com/d/msg/django-developers/MLWfvPPVwDk/jWaYQkOUAAAJ
comment:18 by , 7 years ago
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.
comment:19 by , 7 years ago
Patch needs improvement: | set |
---|
comment:20 by , 6 years ago
I implemented a DefaultObjectPermissionsBackend
: https://github.com/django/django/pull/10601
comment:21 by , 6 years ago
Patch needs improvement: | unset |
---|
Unchecking Patch needs improvement
to put the new PR back in the queue.
comment:22 by , 6 years ago
Needs documentation: | set |
---|
comment:23 by , 6 years ago
Needs documentation: | unset |
---|
comment:24 by , 6 years ago
As I 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
.
comment:25 by , 6 years ago
Has patch: | unset |
---|---|
Triage Stage: | Accepted → Unreviewed |
Type: | Bug → New feature |
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…)
comment:26 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.