has_object_permission does not get called after first permission in Or

[jamesbrobb]

I have an Or containing two permissions. When the first permission fails, the second permissions has_object_permission method does not get called.

From what i can see, it appears to only occur when the first permission does not explicitly define a 'has_object_permission' method. If the method is explicitly defined on the first permission class, then the second permissions has_object_permission method gets called.

[caxap]

Can you provide some code snippets with your permissions classes?

This behavior can came from lazy evaluating for permissions (lazy_until parameter), similar to python logical expressions:

1 or 1/0  # evaluates to 1, second expression will not be evaluated

This way for "Or" expression if first permission will be evaluated to True other ones will non be called at all.

>>> from rest_framework.permissions import BasePermission
>>> from rest_condition import C, Or
>>>
>>> class P1(BasePermission):
...     pass
...
>>> class P2(BasePermission):
...     def has_object_permission(self, request, view, obj):
...         print '>> inside P2'
...         return False
...
>>> Or(P1, P2).has_object_permission(None, None, None)
True
>>> Or(P2, P1).has_object_permission(None, None, None)
>> inside P2
True

Also note that BasePermission class by default return True, so you need explicitly override has_object_permission method to return False.

So if you wont disable lazy evaluation you can try:

>>> import operator
>>> C(P1, P2, reduce_op=operator.or_, lazy_until=None).has_object_permission(None, None, None)
>> inside P2
True

I think snippet above can be default behavior.

[jamesbrobb]

Sorry, i can see how stupid i've been now. Due to the default behaviour of a BasePermission, unless you explicitly override has_object_permission, it'll always returns True and the permission passes.

So in an Or, if you're relying on has_object_permission in any of the permissions (after the first) you have to explicitly override has_object_permission and return False in the previous permission classes to ensure that it get's called. Which i think affects how they'll work as a normal (non rest_condition) permission and also in an And?

[jamesbrobb]

Just noticed your reply, thanks for getting back to me so quickly.

[caxap]

Also to achieve required behavior you can combine conditional permission with plain permissions classes, eg.:

permissions_classes = (P1, Or(P2, P3))

[jamesbrobb]

Sorry, i'm fairly new to python. Is this expected behaviour?

class TestPerm1(permissions.BasePermission):
    pass

class TestPerm2(permissions.BasePermission):
    def has_object_permission(self, request, view, obj):
        print 'TestPerm2.has_permission'
        return False

>>> import operator
>>> from api.permissions import TestPerm1, TestPerm2
>>> from rest_condition import C, And, Or, Not
>>> C(TestPerm1, TestPerm2, reduce_op=operator.or_, lazy_until=None).has_object_permission(None, None, None)
TestPerm2.has_permission
True
>>> 

[caxap]

Looks ok for me, lazy evaluating is disabled (lazy_until=None), so each permission was called. First permissions evaluated to True and second one to False. True or False will get True.

[jamesbrobb]

Ahh, i see, that makes sense. Thanks so much for your help and the app.

[caxap]

Then close the issue.

[jamesbrobb]

I was planning on closing it once i'd done a few more tests. The examples you give work in isolation, as only one of the two permissions methods are being called. But they don't show that it would work when used within DRF.

I'm using these permission classes. All of the methods that return True are only there for output and should be considered the same as the default BasePermission methods, as if no override had been applied.

class TestPerm1(permissions.BasePermission):
    def has_permission(self, request, view):
        print 'TestPerm1.has_permission'
        return False

    #default BasePermission behaviour
    def has_object_permission(self, request, view, obj):
        print 'TestPerm1.has_object_permission'
        return True

class TestPerm2(permissions.BasePermission):
    def has_permission(self, request, view):
        print 'TestPerm2.has_permission'
        return False

    #default BasePermission behaviour   
    def has_object_permission(self, request, view, obj):
        print 'TestPerm2.has_object_permission'
        return True

class TestPerm3(permissions.BasePermission):

    #default BasePermission behaviour
    def has_permission(self, request, view):
        print 'TestPerm3.has_permission'
        return True

    def has_object_permission(self, request, view, obj):
        print 'TestPerm3.has_object_permission'
        return False

The first test uses these permission classes within a DRF project

permission_classes = [C(TestPerm1, TestPerm2, TestPerm3, reduce_op=operator.or_, lazy_until=True)]
#The same as the default Or behaviour
#permission_classes = [Or(TestPerm1, TestPerm2, TestPerm3)]

As all three permissions explicitly fail, i'd expect this Or to deny access, but instead it returns True.

TestPerm1.has_permission
TestPerm2.has_permission
TestPerm3.has_permission
TestPerm1.has_object_permission
True

As you can see by the output, even though TestPerm1 has initially failed, it's resurrected and allowed to pass once the default behaviour of it's has_object_permission method is called. This logic doesn't make sense. Surely once a permission fails, it should be considered a failure and stay failed? This is the behaviour that's used for And. Once the permission's has_permission method has failed, the And is correctly considered False, the permission is not given a second chance with a call to it's has_object_permission method. So why would this be considered correct in an Or?

I've tried your suggestion and passed lazy_until=None instead.

permission_classes = [C(TestPerm1, TestPerm2, TestPerm3, reduce_op=operator.or_, lazy_until=None)]

This still returns True, even though this time all methods are called.

TestPerm1.has_permission
TestPerm2.has_permission
TestPerm3.has_permission
TestPerm1.has_object_permission
TestPerm2.has_object_permission
TestPerm3.has_object_permission
True

I understand that the behaviour demonstrated in all these examples, is correct, in relation to the logic you've implemented. But i don't believe it's the logical outcome of an Or.

Shouldn't the behaviour for Or be, that if a permission returns False for either method, then it's considered a fail and can only be considered a pass if both methods return True? That way the default BasePermission behaviour can be left in place and won't affect the outcome of a permission. And you can rely on the fact that a returned False is an explicit, intended fail, that should be treated as such?

Granted, i only have a small amount of experience with DRF, but i'm yet to come across a case where both has_permission and has_object_permission are implemented in the same permission class. Is it not usually the case that it's either one or the other? So there's no requirement to check both methods once one has returned False.

[jamesbrobb]

Overriding the has_object_permission method to return False is not a good solution either, as due to the fact that both has_permission and has_object_permission are called on a single permission, even if has_permission passes due to some explicit logic, the permission is then considered a fail, due to the failure of the second method call.

Take this example,

class TestPerm1(permissions.BasePermission):

    #Explicitly defined custom logic that results in True
    def has_permission(self, request, view):
        print 'TestPerm1.has_permission'
        return True

    #method overriden to return False to force Or to work as expected
    def has_object_permission(self, request, view, obj):
        print 'TestPerm1.has_object_permission'
        return False

class TestPerm2(permissions.BasePermission):

    #Explicitly defined custom logic that results in False
    def has_permission(self, request, view):
        print 'TestPerm2.has_permission'
        return False

    #method overriden to return False to force Or to work as expected
    def has_object_permission(self, request, view, obj):
        print 'TestPerm2.has_object_permission'
        return False

class TestPerm3(permissions.BasePermission):

    #default BasePermission behaviour
    def has_permission(self, request, view):
        print 'TestPerm3.has_permission'
        return True

    #Explicitly defined custom logic that results in False
    def has_object_permission(self, request, view, obj):
        print 'TestPerm3.has_object_permission'
        return False

So even though in this scenario, the first permission has actually passed and returned True (at which point you'd expect the Or to return True), due to the forced override of has_object_permission, to enable Or to work correctly, it forces the permission to then fail and the entire Or fails, denying the user access.

permission_classes = [C(TestPerm1, TestPerm2, TestPerm3, reduce_op=operator.or_, lazy_until=True)]
#The same as the default Or behaviour
#permission_classes = [Or(TestPerm1, TestPerm2, TestPerm3)]

TestPerm1.has_permission
TestPerm1.has_object_permission
TestPerm2.has_object_permission
TestPerm3.has_object_permission
False

and

permission_classes = [C(TestPerm1, TestPerm2, TestPerm3, reduce_op=operator.or_, lazy_until=None)]

TestPerm1.has_permission
TestPerm2.has_permission
TestPerm3.has_permission
TestPerm1.has_object_permission
TestPerm2.has_object_permission
TestPerm3.has_object_permission
False

[caxap]

Ok, first

As all three permissions explicitly fail, i'd expect this Or to deny access, but instead it returns True.

TestPerm1.has_permission
TestPerm2.has_permission
TestPerm3.has_permission
TestPerm1.has_object_permission
True

It's expected logic, as you can see TestPerm3.has_permission return True, and we get Or(False, False, True) == True

Next, you should separate has_permission and has_object_permission flows, as they check permissions on different levels (view vs. object).

If I correctly understood your problem you expect that if has_permission failed for specific permission than has_object_permission should fail too. If it's true, I think this logic should be implemented by developer. There are several solutions to achieve this logic:

1. Separate permission to two permission classes, one with overrided has_permission method and second with has_object_permission
2. Pre save permission state during has_permission check, and next examine it in has_object_permission method:

class Perm1(BasePermission):
    def has_permission(self, request, view):
        result = False  # Some logic to check permission here
        self._has_permission_result = result
        return result

   def has_object_permission(self, request, view, obj):
       if  self._has_permission_result:
           return True  # Some logic to check permission here
       return False

[caxap]

Actually for this case you don't need conditional permissions, use default DRF mechanism

permission_classes = [permissions.IsAuthenticated, IsUserOwner]

Also I don't understand why you need custom class for IsAuthenticated, for And expressions default one should work well.