dbkaplan / dry-rest-permissions

Rules based permissions for the Django Rest Framework
ISC License
376 stars 59 forks source link

Allow Read single object but not list #32

Closed ohenrik closed 7 years ago

ohenrik commented 7 years ago

I'm trying to disallow list view for simple users while allowing them to see their own profile.

However this is not working:

@staticmethod def has_read_permission(request): return False

@classmethod def has_list_permission(cls, request): return False

def has_object_read_permission(self, request): return self.id == request.user.id

If i set has_read_permission to True the user is also allowed to view the list. If i set it to False the user cannot read their own profile.

dbkaplan commented 7 years ago

The problem you are facing is that granting access (returning True) short circuits the checking process on the global level, but restricting access does not. In other words if you return false on the has_list_permission permission it will see if the more general has_read_permission gives access. This was done for the more common case of when you want to restrict (return false) for all read permissions, but just give access for a single permission.

Anyway...here is how you can achieve what you want: has_read_permission should return False (because you want to restrict all access except for retrieve) has_retrieve_permission should return True (this overrides the has_read_permission for this specific action) has_object_retrieve_permission should return True if self.id == request.user.id

in code:

` @staticmethod def has_read_permission(request): return False

@staticmethod def has_retrieve_permission(request): return False

def has_object_retrieve_permission(self, request): return self.id == request.user.id `

ohenrik commented 7 years ago

Hi, thank you for a quick answer!

I stil can't get this working. It seems has_read_permission oversider everything, so whenever that is set to return False every read action is forbidden (by that i mean i have no access).

So in effect has_retrieve_permission and has_object_retrieve_permission have no effect.

Do i need to use anything other than DRYPermissions when adding the permission class to my view?

dbkaplan commented 7 years ago

hmmm. You may have to do a bit of debugging here. That should work and you shouldn't need more that DRYPermissions.

A couple of things to try when debugging:

  1. What request are you sending? Verify you are sending a retrieve request and that DRF is identifying it that way. Meaning that you are using a url with a specific id...something like /api/profile/342
  2. See which of your permissions functions are being called and in what order. Add a print statement before each return statement in your three has_* functions. You should see that the global retrieve is called and then the object retrieve. The has_read_permissions function should not be called at all for a retrieve request if the other two functions are there.
  3. If this still doesn't help you can add print statements into DRY Rest Permissions at lines 115 and 122 in the generics.py file. These are the areas that determine what permissions get checked on the global level. This should also help tell you what your request is being identified as.

Let me know what you find out.

ohenrik commented 7 years ago
  1. i believe i'm sending a retrieve request. Howerever I'm not sure how to test this (new to django). However If i'm limiting the view to RetrieveAPIView and ListAPIView this still does not make a difference.

  2. It seems like they are all called (twice) when has_read_permission is True and only has_read_permission when has_read_permission is False. So has_read_permission = False makes everything fail.

  3. I'll try this after dinner. :)

ohenrik commented 7 years ago

Also, the DRYPermissionsField method seems to output the correct permissions, however these are not enforced, they just display the correct permissions.

dbkaplan commented 7 years ago

How are you testing your retrieve? Are you using curl or something like that? Seeing each being called twice leads me to believe you are using the DRF browsable API, which does other requests before hand that may get rejected if you shut down read permissions. I would recommend testing using either curl requests or just using a http request from a python script. This way you can make sure you are only calling the api with retrieve.

Also your has_retrieve_permission should always be called first. If it is not then the problem is most likely with the way you are either calling the api or the way you set up the api...not in the permissions.

dbkaplan commented 7 years ago

BTW you can see that the specific action (retrieve) is always checked first on line 115 of https://github.com/dbkaplan/dry-rest-permissions/blob/master/dry_rest_permissions/generics.py The general "read" happens on line 121 only if there is not specific action method.

ohenrik commented 7 years ago

Ok, i finally got this working! :) Thank you for your help and quick answers!

To get it working as i wanted it i needed to use DRF's ModelViewSet. like so:

class UserViewSet(viewsets.ModelViewSet):
    permission_classes = (DRYPermissions, ) # (DRYPermissions,)
    queryset = AdminUser.objects.all() # pylint: disable=E1101

    def get_serializer_class(self):
        if self.request.user.is_superuser:
            return serializers.UserSerializer
        return serializers.BaseUserSerializer

(the get_serializer_class is just so i can deliver more fields to admins than to other users, is there a better way to do this?).

I had previously used ListCreateAPIView and RetrieveUpdateDestroyAPIView

urls:

    url(r'^users/(?P<pk>[0-9]+)/$', views.UserViewSet.as_view({
        'get': 'retrieve',
        'put': 'update',
        'delete': 'destroy'})),
    url(r'^users/?$', views.UserViewSet.as_view({'get': 'list', 'post': 'create'})),

And my model (proxy model for djangos user):

class AdminUser(User):
    # pylint: disable=E1101

    class Meta(User.Meta):
        proxy = True

    @staticmethod
    def base_allow(request, groups, other):
        if request.user.is_superuser:
            return True
        elif (request.user.groups.filter(
                name__in=groups).exists()):
            return True
        else:
            return other

    @staticmethod
    @authenticated_users
    def has_write_permission(request):
        return AdminUser.base_allow(request, ['admin', 'superadmin'], True, )

    @staticmethod
    @authenticated_users
    def has_read_permission(request):
        print('read global')
        return AdminUser.base_allow(request, ['admin', 'superadmin'], False, )

    @staticmethod
    @authenticated_users
    def has_list_permission(request):
        print('retrieve global')
        return AdminUser.base_allow(request,
                                    ['admin', 'superadmin'],
                                    False, )

    @staticmethod
    @authenticated_users
    def has_retrieve_permission(request):
        # pylint: disable=W0613
        print('retrieve global')
        return True

    @staticmethod
    @authenticated_users
    def has_create_permission(request):
        print('retrieve global')
        return AdminUser.base_allow(request,
                                    ['admin', 'superadmin'],
                                    False, )

    @authenticated_users
    def has_object_read_permission(self, request):
        print('read object')
        return self.base_allow(request, ['admin', 'superadmin'],
                               self.id == request.user.id, )

    @authenticated_users
    def has_object_destroy_permission(self, request):
        print('read object')
        return self.base_allow(request, ['admin', 'superadmin'],
                               self.id == request.user.id, )

    @authenticated_users
    def has_object_retrieve_permission(self, request):
        print('read object')
        return self.base_allow(request, ['admin', 'superadmin'],
                               self.id == request.user.id, )

    @authenticated_users
    def has_object_write_permission(self, request):
        return self.base_allow(request,
                               ['admin', 'superadmin'],
                               self.id == request.user.id, )
    @authenticated_users
    def has_object_update_permission(self, request):
        return self.base_allow(request,
                               ['admin', 'superadmin'],
                               self.id == request.user.id, )

This is a whole lot of permissions, basically superusers, admins, superadmins may do pretty much everything, while others (at this point) may only view and update their own records.

To restrict reading of records but not the entire list i need to set has_read_permission = false, has_retrieve_permission = True, has_object_retrieve_permission = True and finally has_list_permission = False.

One thing that still confuses me is why i need to set has_write_permission = True to allow has_object_write_permission = True to have effect?

dbkaplan commented 7 years ago

The way you are using the get_serializer_class to return more fields is the way I would do it as well.

I would replace all the checks you are doing for superusers with the decorator @allow_staff_or_superuser. This is part of the DRY package. You can see how to use it in the readme.

As for the permissions I think you can edit this down a bit. It sounds like superusers can do everything and everyone else can retrieve, update, and delete their own record.

I think you need the following to return False, but with @allow_staff_or_superuser decorators has_create_permission has_list_permission

I think you need the following to return True has_read_permission has_write_permission

I think you need the following to check if the record being written to or read from is the requester, also with an @allow_staff_or_superuser decorator has_object_write_permission has_object_read_permission

This will open up read permissions on the global level, while shutting down create and list specifically, since there is no object level for them. Then on the object level (for retrieve, update, and destroy) you check to make sure whatever object you are reading or writing is owned by the requester.