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

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

Open
dbrgn opened this issue Mar 17, 2017 · 6 comments
Open

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

dbrgn opened this issue Mar 17, 2017 · 6 comments

Comments

@dbrgn
Copy link

dbrgn commented Mar 17, 2017

I'm adding a user resource to an API.

I want the following permission logic:

  • Staff may do anything
  • Users may read and update themselves

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
Copy link
Author

dbrgn commented Mar 17, 2017

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
Copy link
Author

dbrgn commented Mar 17, 2017

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
Copy link
Owner

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
Copy link
Author

dbrgn commented Mar 19, 2017

@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
Copy link
Owner

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
Copy link

jpipas commented Jul 19, 2017

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

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

3 participants