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

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

Author: Carlton Gibson

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 @@ def get_required_permissions(self, method, model_cls):
 
         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 @@ 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):
-        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 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 @@ def test_empty_view_does_not_assert(self):
         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 @@ def test_cannot_read_list_permissions(self):
         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)