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 DjangoObjectPermissions to use views that define get_queryset #2904

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api-guide/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ Similar to `DjangoModelPermissions`, but also allows unauthenticated users to ha

This permission class ties into Django's standard [object permissions framework][objectpermissions] that allows per-object permissions on models. In order to use this permission class, you'll also need to add a permission backend that supports object-level permissions, such as [django-guardian][guardian].

As with `DjangoModelPermissions`, this permission must only be applied to views that have a `.queryset` property. Authorization will only be granted if the user *is authenticated* and has the *relevant per-object permissions* and *relevant model permissions* assigned.
As with `DjangoModelPermissions`, this permission must only be applied to views that have a `.queryset` property or `.get_queryset()` method. Authorization will only be granted if the user *is authenticated* and has the *relevant per-object permissions* and *relevant model permissions* assigned.

* `POST` requests require the user to have the `add` permission on the model instance.
* `PUT` and `PATCH` requests require the user to have the `change` permission on the model instance.
Expand Down
12 changes: 9 additions & 3 deletions rest_framework/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
from __future__ import unicode_literals
from django.http import Http404
from django.utils import six
from rest_framework.compat import get_model_name

SAFE_METHODS = ('GET', 'HEAD', 'OPTIONS')
Expand Down Expand Up @@ -109,8 +110,6 @@ def get_required_permissions(self, method, model_cls):
def has_permission(self, request, view):
try:
queryset = view.get_queryset()
except AttributeError:
queryset = getattr(view, 'queryset', None)
except AssertionError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# view.get_queryset() didn't find .queryset
queryset = None
Expand Down Expand Up @@ -172,7 +171,14 @@ def get_required_object_permissions(self, method, model_cls):
return [perm % kwargs for perm in self.perms_map[method]]

def has_object_permission(self, request, view, obj):
model_cls = view.queryset.model
try:
queryset = view.get_queryset()
except AssertionError as exc:
# view.get_queryset() didn't find .queryset
raise six.raise_from(AssertionError(
'Cannot apply DjangoObjectPermissions on a view that '
'does not have `.queryset` property nor redefines `.get_queryset()`.'), exc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use:

... does not have a .get_queryset() method, or a .queryset property.

The "nor" is okay, but may be a bit obscure for some international readers.

(Just noticed we use that here too, which should change.)

model_cls = queryset.model
user = request.user

perms = self.get_required_object_permissions(request.method, model_cls)
Expand Down
21 changes: 21 additions & 0 deletions tests/test_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,18 @@ class ObjectPermissionListView(generics.ListAPIView):
object_permissions_list_view = ObjectPermissionListView.as_view()


class GetQuerysetObjectPermissionInstanceView(generics.RetrieveUpdateDestroyAPIView):
serializer_class = BasicPermSerializer
authentication_classes = [authentication.BasicAuthentication]
permission_classes = [ViewObjectPermissions]

def get_queryset(self):
return BasicPermModel.objects.all()


get_queryset_object_permissions_view = GetQuerysetObjectPermissionInstanceView.as_view()


@unittest.skipUnless(guardian, 'django-guardian not installed')
class ObjectPermissionsIntegrationTests(TestCase):
"""
Expand Down Expand Up @@ -326,6 +338,15 @@ def test_cannot_read_permissions(self):
response = object_permissions_view(request, pk='1')
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)

def test_can_read_get_queryset_permissions(self):
"""
same as ``test_can_read_permissions`` but with a view
that rely on ``.get_queryset()`` instead of ``.queryset``.
"""
request = factory.get('/1', HTTP_AUTHORIZATION=self.credentials['readonly'])
response = get_queryset_object_permissions_view(request, pk='1')
self.assertEqual(response.status_code, status.HTTP_200_OK)

# Read list
def test_can_read_list_permissions(self):
request = factory.get('/', HTTP_AUTHORIZATION=self.credentials['readonly'])
Expand Down