Default authorization backend returns False when queried for object level permissions

[soren@…]

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?

[Anssi Kääriäinen]

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.

[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.

[Tim Graham]

Patches welcome, Adam.

[Tim Graham]

I closed #27528 as a duplicate.

[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
        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.

[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.

[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.

[Mehmet Dogan]

Here is a sample patch:

​https://github.com/doganmeh/django/commit/d85cd3a530984ab5e4cb42f93629a64eb0b65b07

[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,

[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

[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.

[Tobias Bengfort]

I implemented a DefaultObjectPermissionsBackend: ​https://github.com/django/django/pull/10601

[Carlton Gibson]

Unchecking Patch needs improvement to put the new PR back in the queue.

[Carlton Gibson]

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.

[Carlton Gibson]

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…)