dbkaplan / dry-rest-permissions

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

Allow create for staff, update for staff or owner #39

Open dbrgn opened 7 years ago

dbrgn commented 7 years ago

I'm adding a user resource to an API.

I want the following permission logic:

So I start out with this:

@staticmethod
def has_write_permission(request):
    return request.user.is_staff

Now staff users can create other users, but regular users can't.

Next, I want to allow users to update themselves, so I add the following:

def has_object_write_permission(self, request):
    return request.user.is_staff or request.user == self

However, the method is never called because the write permission is already denied globally.

So I update the code as follows:

@staticmethod
def has_write_permission(request):
    return True

def has_object_write_permission(self, request):
    return request.user.is_staff or request.user == self

Now the code first calls has_write_permission, then has_object_write_permission. This is fine.

But now any user has the create permission. I want to limit creation to staff. So I add the following method:

@staticmethod
def has_create_permission(request):
    return request.user.is_staff

However, this method is never queried by dry-rest-permissions. Only has_write_permission is called, which already grants the permission.

Am I mis-understanding the concept of dry-rest-permissions?

dbrgn commented 7 years ago

Oh, I think I found the problem. I'm not using viewsets for this specific endpoint.

What do you think about offering has_<httpmethod>_permission for non viewset based views? Would you accept a PR?

dbrgn commented 7 years ago

Actually something like this helped already:

class ActionMixin:
    """
    This mixin adds an ``.action`` attribute to the view based on the
    ``action_map`` attribute, similar to the way a ``ViewSet`` does it.

    Example::

        class UserDetail(ActionMixin, generics.RetrieveUpdateAPIView):
            queryset = models.User.objects.all()
            serializer_class = serializers.UserSerializer
            action_map = {
                'get': 'retrieve',
                'put': 'update',
                'patch': 'partial_update',
            }

    """
    def initialize_request(self, request, *args, **kwargs):
        """
        Set the `.action` attribute on the view,
        depending on the request method.
        """
        request = super().initialize_request(request, *args, **kwargs)
        method = request.method.lower()
        if method == 'options':
            # This is a special case as we always provide handling for the
            # options method in the base `View` class.
            # Unlike the other explicitly defined actions, 'metadata' is implicit.
            self.action = 'metadata'
        else:
            self.action = self.action_map.get(method)
        return request

class DocumentActionMixin(ActionMixin):
    """
    Add an ``.action`` attribute to a document type view.
    """
    action_map = {
        'get': 'retrieve',
        'put': 'update',
        'patch': 'partial_update',
        'delete': 'destroy',
    }

class CollectionActionMixin(ActionMixin):
    """
    Add an ``.action`` attribute to a collection type view.
    """
    action_map = {
        'get': 'list',
        'post': 'create',
    }

But a clear note in the documentation would probably help.

dbkaplan commented 7 years ago

Hey Danilo,

What I would do to get the desired result would be to not use the has_write_permission function at all. Since your use case is more nuanced I would use: has_create_permission (only allow staff) has_update_permission (allow anyone) has_object_write_permission (As you had it with the or statement)

dbrgn commented 7 years ago

@dbkaplan the thing is that apparently when using the DRF browseable API without having has_write_permission and has_read_permission defined, I get exceptions for a page view, because the API checks for available permissions even for methods that I don't even support. Maybe that's related to #40.

dbkaplan commented 7 years ago

That is true. Sorry I didn't realize that was the problem you were having.

I usually get around that by creating the generic has_<write/read>_permission functions and having them return False, but decorating them with @allow_staff_or_superuser. This will only open them up for those situations. I would then still use the other functions as I specified above.

jpipas commented 7 years ago

I was using GenericAPIView's and also had the problem of exceptions when I didn't include has_write_permission, nor was the precedence of has_object_update_permission over the global has_write_permission being followed.

Using the mixin that @dbrgn provided with slight modification to super() (I'm using Python 2.7), I was able to get it to work properly with/using APIViews instead of ViewSets.

Here's my modified mixin in case anyone needs it:

from rest_framework import (
    generics,
)

class ActionMixin:
    """
    This mixin adds an ``.action`` attribute to the view based on the
    ``action_map`` attribute, similar to the way a ``ViewSet`` does it.

    Example::

        class UserDetail(ActionMixin, generics.RetrieveUpdateAPIView):
            queryset = models.User.objects.all()
            serializer_class = serializers.UserSerializer
            action_map = {
                'get': 'retrieve',
                'put': 'update',
                'patch': 'partial_update',
            }

    """
    def initialize_request(self, request, *args, **kwargs):
        """
        Set the `.action` attribute on the view,
        depending on the request method.
        """
        request = super(generics.GenericAPIView, self).initialize_request(request, *args, **kwargs)
        method = request.method.lower()
        if method == 'options':
            # This is a special case as we always provide handling for the
            # options method in the base `View` class.
            # Unlike the other explicitly defined actions, 'metadata' is implicit.
            self.action = 'metadata'
        else:
            self.action = self.action_map.get(method)
        return request