Merge pull request #5376 from rpkilby/django-perms-queryset

DjangoModelPermissions should perform auth check before accessing the view's queryset

docs/topics/release-notes.md

@@ -40,6 +40,10 @@ You can determine your currently installed version using `pip freeze`:
 
 ## 3.6.x series
 
+### 3.6.5
+
+* Fix `DjangoModelPermissions` to ensure user authentication before calling the view's `get_queryset()` method. As a side effect, this changes the order of the HTTP method permissions and authentication checks, and 405 responses will only be returned when authenticated. If you want to replicate the old behavior, see the PR for details. [#5376][gh5376]
+
 ### 3.6.4
 
 **Date**: [21st August 2017][3.6.4-milestone]
@@ -1417,5 +1421,5 @@ For older release notes, [please see the version 2.x documentation][old-release-
 [gh5147]: https://github.com/encode/django-rest-framework/issues/5147
 [gh5131]: https://github.com/encode/django-rest-framework/issues/5131
 
-
-
+<!-- 3.6.5 -->
+[gh5376]: https://github.com/encode/django-rest-framework/issues/5376

rest_framework/permissions.py

@@ -114,32 +114,35 @@ class DjangoModelPermissions(BasePermission):
 
         return [perm % kwargs for perm in self.perms_map[method]]
 
-    def has_permission(self, request, view):
-        # Workaround to ensure DjangoModelPermissions are not applied
-        # to the root view when using DefaultRouter.
-        if getattr(view, '_ignore_model_permissions', False):
-            return True
+    def _queryset(self, view):
+        assert hasattr(view, 'get_queryset') \
+            or getattr(view, 'queryset', None) is not None, (
+            'Cannot apply {} on a view that does not set '
+            '`.queryset` or have a `.get_queryset()` method.'
+        ).format(self.__class__.__name__)
 
         if hasattr(view, 'get_queryset'):
             queryset = view.get_queryset()
             assert queryset is not None, (
                 '{}.get_queryset() returned None'.format(view.__class__.__name__)
             )
-        else:
-            queryset = getattr(view, 'queryset', None)
+            return queryset
+        return view.queryset
 
-        assert queryset is not None, (
-            'Cannot apply DjangoModelPermissions on a view that '
-            'does not set `.queryset` or have a `.get_queryset()` method.'
-        )
+    def has_permission(self, request, view):
+        # Workaround to ensure DjangoModelPermissions are not applied
+        # to the root view when using DefaultRouter.
+        if getattr(view, '_ignore_model_permissions', False):
+            return True
 
+        if not request.user or (
+           not is_authenticated(request.user) and self.authenticated_users_only):
+            return False
+
+        queryset = self._queryset(view)
         perms = self.get_required_permissions(request.method, queryset.model)
 
-        return (
-            request.user and
-            (is_authenticated(request.user) or not self.authenticated_users_only) and
-            request.user.has_perms(perms)
-        )
+        return request.user.has_perms(perms)
 
 
 class DjangoModelPermissionsOrAnonReadOnly(DjangoModelPermissions):
@@ -183,16 +186,8 @@ class DjangoObjectPermissions(DjangoModelPermissions):
         return [perm % kwargs for perm in self.perms_map[method]]
 
     def has_object_permission(self, request, view, obj):
-        if hasattr(view, 'get_queryset'):
-            queryset = view.get_queryset()
-        else:
-            queryset = getattr(view, 'queryset', None)
-
-        assert queryset is not None, (
-            'Cannot apply DjangoObjectPermissions on a view that '
-            'does not set `.queryset` or have a `.get_queryset()` method.'
-        )
-
+        # authentication checks have already executed via has_permission
+        queryset = self._queryset(view)
         model_cls = queryset.model
         user = request.user
 

tests/test_permissions.py

@@ -9,7 +9,7 @@ from django.test import TestCase
 
 from rest_framework import (
     HTTP_HEADER_ENCODING, authentication, generics, permissions, serializers,
-    status
+    status, views
 )
 from rest_framework.compat import ResolverMatch, guardian, set_many
 from rest_framework.filters import DjangoObjectPermissionsFilter
@@ -201,14 +201,45 @@ class ModelPermissionsIntegrationTests(TestCase):
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
     def test_calling_method_not_allowed(self):
-        request = factory.generic('METHOD_NOT_ALLOWED', '/')
+        request = factory.generic('METHOD_NOT_ALLOWED', '/', HTTP_AUTHORIZATION=self.permitted_credentials)
         response = root_view(request)
         self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)
 
-        request = factory.generic('METHOD_NOT_ALLOWED', '/1')
+        request = factory.generic('METHOD_NOT_ALLOWED', '/1', HTTP_AUTHORIZATION=self.permitted_credentials)
         response = instance_view(request, pk='1')
         self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)
 
+    def test_check_auth_before_queryset_call(self):
+        class View(RootView):
+            def get_queryset(_):
+                self.fail('should not reach due to auth check')
+        view = View.as_view()
+
+        request = factory.get('/', HTTP_AUTHORIZATION='')
+        response = view(request)
+        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)
+
+    def test_queryset_assertions(self):
+        class View(views.APIView):
+            authentication_classes = [authentication.BasicAuthentication]
+            permission_classes = [permissions.DjangoModelPermissions]
+        view = View.as_view()
+
+        request = factory.get('/', HTTP_AUTHORIZATION=self.permitted_credentials)
+        msg = 'Cannot apply DjangoModelPermissions on a view that does not set `.queryset` or have a `.get_queryset()` method.'
+        with self.assertRaisesMessage(AssertionError, msg):
+            view(request)
+
+        # Faulty `get_queryset()` methods should trigger the above "view does not have a queryset" assertion.
+        class View(RootView):
+            def get_queryset(self):
+                return None
+        view = View.as_view()
+
+        request = factory.get('/', HTTP_AUTHORIZATION=self.permitted_credentials)
+        with self.assertRaisesMessage(AssertionError, 'View.get_queryset() returned None'):
+            view(request)
+
 
 class BasicPermModel(models.Model):
     text = models.CharField(max_length=100)
@@ -396,7 +427,7 @@ class ObjectPermissionsIntegrationTests(TestCase):
         self.assertListEqual(response.data, [])
 
     def test_cannot_method_not_allowed(self):
-        request = factory.generic('METHOD_NOT_ALLOWED', '/')
+        request = factory.generic('METHOD_NOT_ALLOWED', '/', HTTP_AUTHORIZATION=self.credentials['readonly'])
         response = object_permissions_list_view(request)
         self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)