Opened 5 years ago

Last modified 2 months ago

#20218 assigned Bug

Default authorization backend returns False when queried for object level permissions

Reported by: soren@… Owned by: Mehmet Dogan
Component: contrib.auth Version: master
Severity: Normal Keywords: auth
Cc: astronouth7303@…, Mehmet Dogan Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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 (19)

comment:1 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 5 years ago by Tim Graham

Component: Uncategorizedcontrib.auth

comment:3 Changed 4 years ago by Carl Meyer

Type: UncategorizedBug

comment:4 Changed 3 years ago 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.

comment:5 Changed 3 years ago by Tim Graham

Patches welcome, Adam.

comment:6 Changed 16 months ago by Tim Graham

I closed #27528 as a duplicate.

comment:7 in reply to:  description Changed 16 months ago by Jamie Bliss

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
            # 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 Changed 16 months ago by Jamie Bliss

Cc: astronouth7303@… added

comment:9 Changed 2 months ago by Mehmet Dogan

Being unaware of this ticket, I had opened another (should have checked!). Here it is:

Long story here:

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 Changed 2 months ago by Mehmet Dogan

Cc: Mehmet Dogan added

comment:11 Changed 2 months ago 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.

comment:13 Changed 2 months ago by Mehmet Dogan

Has patch: set
Needs documentation: set
Owner: changed from nobody to Mehmet Dogan
Status: newassigned

comment:14 Changed 2 months ago by Mehmet Dogan

sore, astronouth7303:

this is being discussed at developers list:!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 Changed 2 months ago by Mehmet Dogan

Needs documentation: unset

comment:16 Changed 2 months ago by Mehmet Dogan

Version: 1.5master

comment:17 Changed 2 months ago 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 (and following comments)

I posted at more length on the discussion group.

Last edited 2 months ago by Carlton Gibson (previous) (diff)

comment:18 Changed 2 months ago 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.

comment:19 Changed 2 months ago by Carlton Gibson

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top