From ca9a984a9b930cc0270fbedead02d31cba5122bb Mon Sep 17 00:00:00 2001 From: Robert Schindler Date: Wed, 7 Aug 2019 09:22:48 +0200 Subject: [PATCH] Add mixins for CBV and DRF that check permissions automatically --- README.rst | 63 +++++++ rules/contrib/rest_framework.py | 77 ++++++++ rules/contrib/views.py | 79 ++++++++ .../testsuite/contrib/test_rest_framework.py | 81 ++++++++ tests/testsuite/contrib/test_views.py | 175 +++++++++++++----- tox.ini | 2 + 6 files changed, 426 insertions(+), 51 deletions(-) create mode 100644 rules/contrib/rest_framework.py create mode 100644 tests/testsuite/contrib/test_rest_framework.py diff --git a/README.rst b/README.rst index 8f743b0..9d6395b 100644 --- a/README.rst +++ b/README.rst @@ -61,6 +61,7 @@ Table of Contents - `Permissions in views`_ - `Permissions and rules in templates`_ - `Permissions in the Admin`_ + - `Permissions in Django Rest Framework`_ - `Advanced features`_ @@ -528,6 +529,32 @@ For more information refer to the `Django documentation`_ and the .. _Django documentation: https://docs.djangoproject.com/en/1.9/topics/auth/default/#limiting-access-to-logged-in-users +Checking permission automatically based on view type +++++++++++++++++++++++++++++++++++++++++++++++++++++ + +If you use the mechanisms provided by ``rules.contrib.models`` to register permissions +for your models as described in `Permissions in models`_, there's another convenient +to use mixin for class-based views available for you. + +``rules.contrib.views.AutoPermissionRequiredMixin`` can recognize the type of view +it's used with and check for the corresponding permission automatically. + +This example view would, without any further configuration, automatically check for +the ``"posts.change_post"`` permission, given that the app label is ``"posts"``:: + + from django.views.generic import UpdateView + from rules.contrib.views import AutoPermissionRequiredMixin + from posts.models import Post + + class UpdatePostView(AutoPermissionRequiredMixin, UpdateView): + model = Post + +By default, the generic CRUD views from ``django.views.generic`` are mapped to the +native Django permission types (*add*, *change*, *delete* and *view*). However, +the pre-defined mappings can be extended, changed or replaced altogether when +subclassing ``AutoPermissionRequiredMixin``. See the fully documented source code +for details on how to do that properly. + Permissions and rules in templates ---------------------------------- @@ -630,6 +657,42 @@ different: ``rules`` will ask for the change permission if and only if no rule exists for the view permission. +Permissions in Django Rest Framework +------------------------------------ + +Similar to ``rules.contrib.views.AutoPermissionRequiredMixin``, there is a +``rules.contrib.rest_framework.AutoPermissionViewSetMixin`` for viewsets in Django +Rest Framework. The difference is that it doesn't derive permission from the type +of view but from the API action (*create*, *retrieve* etc.) that's tried to be +performed. Of course, it requires you to use the mixins from ``rules.contrib.models`` +when declaring models the API should operate on. + +Here is a possible ``ModelViewSet`` for the ``Post`` model with fully automated CRUD +permission checking:: + + from rest_framework.serializers import ModelSerializer + from rest_framework.viewsets import ModelViewSet + from rules.contrib.rest_framework import AutoPermissionViewSetMixin + from posts.models import Post + + class PostSerializer(ModelSerializer): + class Meta: + model = Post + fields = "__all__" + + class PostViewSet(AutoPermissionViewSetMixin, ModelViewSet): + queryset = Post.objects.all() + serializer_class = PostSerializer + +By default, the CRUD actions of ``ModelViewSet`` are mapped to the native +Django permission types (*add*, *change*, *delete* and *view*). The ``list`` +action has no permission checking enabled. However, the pre-defined mappings +can be extended, changed or replaced altogether when using (or subclassing) +``AutoPermissionViewSetMixin``. Custom API actions defined via the ``@action`` +decorator may then be mapped as well. See the fully documented source code for +details on how to properly customize the default behaviour. + + Advanced features ================= diff --git a/rules/contrib/rest_framework.py b/rules/contrib/rest_framework.py new file mode 100644 index 0000000..9ca7c74 --- /dev/null +++ b/rules/contrib/rest_framework.py @@ -0,0 +1,77 @@ +from django.core.exceptions import ImproperlyConfigured, PermissionDenied + + +class AutoPermissionViewSetMixin: + """ + Enforces object-level permissions in ``rest_framework.viewsets.ViewSet``, + deriving the permission type from the particular action to be performed.. + + As with ``rules.contrib.views.AutoPermissionRequiredMixin``, this only works when + model permissions are registered using ``rules.contrib.models.RulesModelMixin``. + """ + + # Maps API actions to model permission types. None as value skips permission + # checks for the particular action. + # This map needs to be extended when custom actions are implemented + # using the @action decorator. + # Extend or replace it in subclasses like so: + # permission_type_map = { + # **AutoPermissionViewSetMixin.permission_type_map, + # "close": "change", + # "reopen": "change", + # } + permission_type_map = { + "create": "add", + "destroy": "delete", + "list": None, + "partial_update": "change", + "retrieve": "view", + "update": "change", + } + + def initial(self, *args, **kwargs): + """Ensures user has permission to perform the requested action.""" + super().initial(*args, **kwargs) + + if not self.request.user: + # No user, don't check permission + return + + # Get the handler for the HTTP method in use + try: + if self.request.method.lower() not in self.http_method_names: + raise AttributeError + handler = getattr(self, self.request.method.lower()) + except AttributeError: + # method not supported, will be denied anyway + return + + try: + perm_type = self.permission_type_map[self.action] + except KeyError: + raise ImproperlyConfigured( + "AutoPermissionViewSetMixin tried to check permissions for a " + "request with the {!r} action, but only for the following actions " + "were permission types configured in the " + "permission_type_map: {!r}".format( + self.action, self.permission_type_map + ) + ) + if perm_type is None: + # Skip permission checking for this action + return + + # Determine whether we've to check object permissions (for detail actions) + obj = None + extra_actions = self.get_extra_actions() + # We have to access the unbound function via __func__ + if handler.__func__ in extra_actions: + if handler.detail: + obj = self.get_object() + elif self.action not in ("create", "list"): + obj = self.get_object() + + # Finally, check permission + perm = self.get_queryset().model.get_perm(perm_type) + if not self.request.user.has_perm(perm, obj): + raise PermissionDenied diff --git a/rules/contrib/views.py b/rules/contrib/views.py index f0ca413..fbde87d 100644 --- a/rules/contrib/views.py +++ b/rules/contrib/views.py @@ -9,6 +9,7 @@ from django.utils import six from django.utils.decorators import available_attrs from django.utils.encoding import force_text +from django.views.generic import CreateView, DeleteView, DetailView, UpdateView # These are made available for convenience, as well as for use in Django @@ -49,6 +50,84 @@ def has_permission(self): return self.request.user.has_perms(perms, obj) +class AutoPermissionRequiredMixin(PermissionRequiredMixin): + """ + An extended variant of PermissionRequiredMixin which automatically determines + the permission to check based on the type of view it's used with. + + It works by checking the current view for being an instance of a pre-defined + list of view types. On a match, the corresponding permission type (such as + "add" or "change") is converted into the full model-specific permission name + and checked. See the permission_type_map attribute for the default view type -> + permission type mappings. + + When a view using this mixin has an attribute ``permission_type``, that type is + used directly and overwrites the permission_type_map for the particular view. A + permission type of ``None`` causes permission checking to be skipped. If the + type of permission to check for should depend on dynamic factors other than the + view type, you may overload the ``permission_type`` attribute with a ``property``. + + The ``permission_required`` attribute behaves like it does in + ``PermissionRequiredMixin`` and can be used to specify concrete permission name(s) + to be checked in addition to the automatically derived one. + + NOTE: The model-based permission registration from ``rules.contrib.models`` + must be used with the models for which you create views using this mixin, + because the permission names are derived via ``RulesModelMixin.get_perm()`` + internally. The second requirement is the presence of either an attribute + ``model`` holding the ``Model`` the view acts on, or the ``get_queryset()`` + method as provided by Django's ``SingleObjectMixin``. Hence with the normal + model views, you don't need to care about anything. + """ + + # These reflect Django's default model permissions. If needed, this list can be + # extended or replaced entirely when subclassing, like so: + # permission_type_map = [ + # (SomeCustomViewType, "add"), + # (SomeOtherCustomViewType, "some_fancy_action"), + # *AutoPermissionRequiredMixin.permission_type_map, + # ] + # Note that ordering matters, which is why this is a list and not a dict. The + # first entry for which isinstance(self, view_type) returns True will be used. + permission_type_map = [ + (CreateView, "add"), + (UpdateView, "change"), + (DeleteView, "delete"), + (DetailView, "view"), + ] + + def get_permission_required(self): + """Adds the correct permission to check according to view type.""" + try: + perm_type = self.permission_type + except AttributeError: + # Perform auto-detection by view type + for view_type, _perm_type in self.permission_type_map: + if isinstance(self, view_type): + perm_type = _perm_type + break + else: + raise ImproperlyConfigured( + "AutoPermissionRequiredMixin was used, but permission_type was " + "neither set nor could be determined automatically for {0}. " + "Consider setting permission_type on the view manually or " + "adding {0} to the permission_type_map." + .format(self.__class__.__name__) + ) + + perms = [] + if perm_type is not None: + model = getattr(self, "model", None) + if model is None: + model = self.get_queryset().model + perms.append(model.get_perm(perm_type)) + + # If additional permissions have been defined, consider them as well + if self.permission_required is not None: + perms.extend(super().get_permission_required()) + return perms + + def objectgetter(model, attr_name='pk', field_name='pk'): """ Helper that returns a function suitable for use as the ``fn`` argument diff --git a/tests/testsuite/contrib/test_rest_framework.py b/tests/testsuite/contrib/test_rest_framework.py new file mode 100644 index 0000000..a130900 --- /dev/null +++ b/tests/testsuite/contrib/test_rest_framework.py @@ -0,0 +1,81 @@ +from __future__ import absolute_import + +import sys +import unittest + +from django.contrib.auth.models import AnonymousUser +from django.core.exceptions import ImproperlyConfigured +from django import http +from django.test import TestCase +from rest_framework.decorators import action +from rest_framework.response import Response +from rest_framework.serializers import ModelSerializer +from rest_framework.test import APIRequestFactory +from rest_framework.viewsets import ModelViewSet + +import rules +from rules.contrib.rest_framework import AutoPermissionViewSetMixin + + +@unittest.skipIf(sys.version_info.major < 3, "Python 3 only") +class AutoPermissionRequiredMixinTests(TestCase): + def setUp(self): + from testapp.models import TestModel + + class TestModelSerializer(ModelSerializer): + class Meta: + model = TestModel + fields = "__all__" + + class TestViewSet(AutoPermissionViewSetMixin, ModelViewSet): + queryset = TestModel.objects.all() + serializer_class = TestModelSerializer + permission_type_map = AutoPermissionViewSetMixin.permission_type_map.copy() + permission_type_map["custom_detail"] = "add" + permission_type_map["custom_nodetail"] = "add" + + @action(detail=True) + def custom_detail(self, request): + return Response() + + @action(detail=False) + def custom_nodetail(self, request): + return Response() + + @action(detail=False) + def unknown(self, request): + return Response() + + self.model = TestModel + self.vs = TestViewSet + self.req = APIRequestFactory().get("/") + self.req.user = AnonymousUser() + + def test_predefined_action(self): + # Create should be allowed due to the add permission set on TestModel + self.assertEqual(self.vs.as_view({"get": "create"})(self.req).status_code, 201) + # List should be allowed due to None in permission_type_map + self.assertEqual( + self.vs.as_view({"get": "list"})(self.req, pk=1).status_code, 200 + ) + # Retrieve should be allowed due to the view permission set on TestModel + self.assertEqual( + self.vs.as_view({"get": "retrieve"})(self.req, pk=1).status_code, 200 + ) + # Destroy should be forbidden due to missing delete permission + self.assertEqual( + self.vs.as_view({"get": "destroy"})(self.req, pk=1).status_code, 403 + ) + + def test_custom_actions(self): + # Both should not produce 403 due to being mapped to the add permission + self.assertEqual( + self.vs.as_view({"get": "custom_detail"})(self.req, pk=1).status_code, 404 + ) + self.assertEqual( + self.vs.as_view({"get": "custom_nodetail"})(self.req).status_code, 200 + ) + + def test_unknown_action(self): + with self.assertRaises(ImproperlyConfigured): + self.vs.as_view({"get": "unknown"})(self.req) diff --git a/tests/testsuite/contrib/test_views.py b/tests/testsuite/contrib/test_views.py index db8cc92..aaa2b0d 100644 --- a/tests/testsuite/contrib/test_views.py +++ b/tests/testsuite/contrib/test_views.py @@ -1,12 +1,18 @@ from __future__ import absolute_import -from django.core.exceptions import ImproperlyConfigured +import sys +import unittest + +from django.contrib.auth.models import AnonymousUser +from django.core.exceptions import ImproperlyConfigured, PermissionDenied from django.http import HttpRequest, Http404 -from django.test import TestCase +from django.test import RequestFactory, TestCase from django.urls import reverse from django.utils.encoding import force_str +from django.views.generic import CreateView, View -from rules.contrib.views import objectgetter +import rules +from rules.contrib.views import AutoPermissionRequiredMixin, objectgetter from testapp.models import Book @@ -19,8 +25,8 @@ def test_objectgetter(self): book = Book.objects.get(pk=1) self.assertEqual(book, objectgetter(Book)(request, pk=1)) - self.assertEqual(book, objectgetter(Book, attr_name='id')(request, id=1)) - self.assertEqual(book, objectgetter(Book, field_name='id')(request, pk=1)) + self.assertEqual(book, objectgetter(Book, attr_name="id")(request, id=1)) + self.assertEqual(book, objectgetter(Book, field_name="id")(request, pk=1)) with self.assertRaises(ImproperlyConfigured): # Raise if no `pk` argument is provided to the view @@ -28,7 +34,7 @@ def test_objectgetter(self): with self.assertRaises(ImproperlyConfigured): # Raise if given invalid model lookup field - self.assertEqual(book, objectgetter(Book, field_name='foo')(request, pk=1)) + self.assertEqual(book, objectgetter(Book, field_name="foo")(request, pk=1)) with self.assertRaises(Http404): # Raise 404 if no model instance found @@ -36,111 +42,178 @@ def test_objectgetter(self): def test_permission_required(self): # Adrian can change his book - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('change_book', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("change_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin can change Adrian's book - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('change_book', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("change_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Adrian can delete his book - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('delete_book', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("delete_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin can *not* create a book # Up to Django v2.1, the response was a redirect to login - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('cbv.create_book')) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("cbv.create_book")) self.assertIn(response.status_code, [302, 403]) # Martin can *not* delete Adrian's book and is redirected to login - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('delete_book', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("delete_book", args=(1,))) self.assertEqual(response.status_code, 302) # Martin can *not* delete Adrian's book and an PermissionDenied is raised - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('view_that_raises', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("view_that_raises", args=(1,))) self.assertEqual(response.status_code, 403) # Test views that require a list of permissions # Adrian has both permissions - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('view_with_permission_list', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("view_with_permission_list", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin does not have delete permission - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('view_with_permission_list', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("view_with_permission_list", args=(1,))) self.assertEqual(response.status_code, 302) # Test views that accept a static object as argument # fn is passed to has_perm as-is - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('view_with_object', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("view_with_object", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('view_with_object', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("view_with_object", args=(1,))) self.assertEqual(response.status_code, 302) class CBVMixinTests(TestData, TestCase): def test_get_object_error(self): - self.assertTrue(self.client.login(username='adrian', password='secr3t')) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) with self.assertRaises(AttributeError): - self.client.get(reverse('cbv.change_book_error', args=(1,))) + self.client.get(reverse("cbv.change_book_error", args=(1,))) def test_permission_required_mixin(self): # Adrian can change his book - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('cbv.change_book', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("cbv.change_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin can change Adrian's book - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('cbv.change_book', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("cbv.change_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Adrian can delete his book - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('cbv.delete_book', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("cbv.delete_book", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin can *not* delete Adrian's book # Up to Django v2.1, the response was a redirect to login - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('cbv.delete_book', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("cbv.delete_book", args=(1,))) self.assertIn(response.status_code, [302, 403]) # Martin can *not* delete Adrian's book and an PermissionDenied is raised - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('cbv.view_that_raises', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("cbv.view_that_raises", args=(1,))) self.assertEqual(response.status_code, 403) # Test views that require a list of permissions # Adrian has both permissions - self.assertTrue(self.client.login(username='adrian', password='secr3t')) - response = self.client.get(reverse('cbv.view_with_permission_list', args=(1,))) + self.assertTrue(self.client.login(username="adrian", password="secr3t")) + response = self.client.get(reverse("cbv.view_with_permission_list", args=(1,))) self.assertEqual(response.status_code, 200) - self.assertEqual(force_str(response.content), 'OK') + self.assertEqual(force_str(response.content), "OK") # Martin does not have delete permission # Up to Django v2.1, the response was a redirect to login - self.assertTrue(self.client.login(username='martin', password='secr3t')) - response = self.client.get(reverse('cbv.view_with_permission_list', args=(1,))) + self.assertTrue(self.client.login(username="martin", password="secr3t")) + response = self.client.get(reverse("cbv.view_with_permission_list", args=(1,))) self.assertIn(response.status_code, [302, 403]) + + +@unittest.skipIf(sys.version_info.major < 3, "Python 3 only") +class AutoPermissionRequiredMixinTests(TestCase): + def setUp(self): + from testapp.models import TestModel + + self.model = TestModel + self.req = RequestFactory().get("/") + self.req.user = AnonymousUser() + + def test_predefined_view_type(self): + class TestView(AutoPermissionRequiredMixin, CreateView): + model = self.model + fields = () + + self.assertEqual(TestView.as_view()(self.req).status_code, 200) + + def test_custom_view_type(self): + class CustomViewMixin: + pass + + class TestView(AutoPermissionRequiredMixin, CustomViewMixin, CreateView): + model = self.model + fields = () + permission_type_map = [ + (CustomViewMixin, "unknown_perm") + ] + AutoPermissionRequiredMixin.permission_type_map + raise_exception = True + + with self.assertRaises(PermissionDenied): + TestView.as_view()(self.req) + + def test_unknown_view_type(self): + class TestView(AutoPermissionRequiredMixin, View): + pass + + with self.assertRaises(ImproperlyConfigured): + TestView.as_view()(self.req) + + def test_overwrite_perm_type(self): + class TestView(AutoPermissionRequiredMixin, CreateView): + model = self.model + fields = () + permission_type = "unknown" + raise_exception = True + + with self.assertRaises(PermissionDenied): + TestView.as_view()(self.req) + + def test_disable_perm_checking(self): + class TestView(AutoPermissionRequiredMixin, CreateView): + model = self.model + fields = () + permission_type = None + + self.assertEqual(TestView.as_view()(self.req).status_code, 200) + + def test_permission_required_passthrough(self): + class TestView(AutoPermissionRequiredMixin, CreateView): + model = self.model + fields = () + permission_required = "testapp.unknown_perm" + raise_exception = True + + with self.assertRaises(PermissionDenied): + TestView.as_view()(self.req) diff --git a/tox.ini b/tox.ini index 2ccb053..7af9c14 100644 --- a/tox.ini +++ b/tox.ini @@ -12,6 +12,7 @@ envlist = usedevelop = true deps = coverage + djangorestframework django111: Django~=1.11 django20: Django~=2.0 @@ -23,6 +24,7 @@ commands = usedevelop = false deps = django + djangorestframework commands = python tests/manage.py test testsuite {posargs: -v 2}