Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hard to use "NOT" bitwise operator for permissions due to default values #6598

Closed
adongy opened this issue Apr 17, 2019 · 6 comments
Closed

Comments

@adongy
Copy link

adongy commented Apr 17, 2019

This is not an issue per se, but a consequence of a new behavior and I would like to open up discussions on it.

#6361 recently added support for bitwise not (aka ~) to negate permissions.

Due to how permissions are shared for both view and object checks, relying on calling the correct method (has_permission or has_object_permission), the default value "True" becomes False for any negative check, making the permission extremely restrictive.

Example:

class View(APIView):
    permission_classes = [~IsAuthenticated]
    def retrieve(self, ...):
        return ...

Calling retrieve will call get_object, which in turns calls has_object_permission, returning False (not True), ending the call into a 403.

Im currently circumventing this by specifying which of has_permission or has_object_permission should get negated, but that makes the ~ operator nearly unusable for generic views (it's still very useful if you only need one of the two functions!).

The docs mention that not can be used in the context of an APIView which is what caught me offguard. Maybe a clarification on how it works is needed?

Thanks.

@Tommos0
Copy link

Tommos0 commented Apr 18, 2019

My issue is related I think (#6596). In general I think the split in two methods (has_permission and has_object_permission) is quite confusing and is limiting combining permissions in an intuitive way.

The first has_permission is checked early on the request. The second has_object_permission is checked later, when a handler calls get_object. Since there are two checks, both of which can block, it is impossible to create an OR permission for a combination of the two, without some sort of funky workaround (and indeed it's causing me headaches too thinking about the NOT operator).

Everything would be a lot easier if there is a single method, which optionally receives an object, in such a way that there is only a single point where the permissions are checked.

Should be doable, but it will break backwards compatibility.

@carltongibson
Copy link
Collaborator

Grrr. This has come up a few times. Basic issue is people not implementing both methods. I’m inclined towards something like raising a warning if a permission is composed but doesn’t implement both methods. (And otherwise just leaving the previous behaviour as is.)

@adongy
Copy link
Author

adongy commented Apr 19, 2019

Usually, I'd want to only negate object or view -wide permissions, not both at the same time:

Taking the example of a group permission to access a view, regardless of the object:

class IsStaff(BasePermission):
    def has_permission(self, request, view):
        return request.user.is_staff

    def has_object_permission(self, request, view, obj):
        return True

Negating this with ~ ends up in has_object_permission always returning False, so any detail=True action gets 403-ed.

Returning False for has_object_permission breaks the permission for the non-negated case.

Am I missing the use-case ?

@carltongibson
Copy link
Collaborator

It may just be that in this use-case you need to define a custom permission... but yes, let’s have a think about what if anything we can do to make this work.

@Tommos0
Copy link

Tommos0 commented Apr 23, 2019

I think the general problem is in combining the logical combinators with the two-method approach. As it is I wouldn't use them and define custom permissions instead. In any case, the logical combinators are perhaps not even considered public API (since it's not referenced in the docs)? Was it recently removed?

Still I think a single method is easier. E.g. can we define DjangoObjectPermissions using has_permission only? As it is now, the method will have to determine the object to be accessed from the view & request parameters (which can be done, probably has some performance cost though). An alternative approach could be this method optionally receiving the obj parameter for the relevant requests.

@marcosox
Copy link

marcosox commented Feb 4, 2020

If you are not implementing has_object_permission, you can just make the two methods behave the same when applying bitwise operators:

class MyPermission(BasePermission):

    def has_permission(self, request, view):
        [...]
        return result

    def has_object_permission(self, request, view, obj):
        return self.has_permission(request, view)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants