Merge pull request #1137 from open-formulieren/feature/1000-form-api-…

…permissions

[#1000] Implemented and applied custom permission for FormAPI viewsets

src/openforms/accounts/tests/factories.py

@@ -1,4 +1,5 @@
 from django.conf import settings
+from django.contrib.auth.models import Permission
 
 import factory
 
@@ -7,6 +8,18 @@ class UserFactory(factory.django.DjangoModelFactory):
     username = factory.Sequence(lambda n: f"user-{n}")
     password = factory.PostGenerationMethodCall("set_password", "secret")
 
+    @factory.post_generation
+    def user_permissions(self, create, extracted, **kwargs):
+        if not create:
+            return
+
+        if extracted:
+            for permission in extracted:
+                if isinstance(permission, str):
+                    # TODO support appname/model/action, for now lets assume we have unique codenames
+                    permission = Permission.objects.get(codename=permission)
+                self.user_permissions.add(permission)
+
     class Meta:
         model = "accounts.User"
 

src/openforms/appointments/tests/test_admin.py

@@ -1,4 +1,3 @@
-from django.contrib.auth.models import Permission
 from django.urls import reverse
 
 from django_webtest import WebTest
@@ -17,15 +16,9 @@ class AppointmentInfoAdminTests(WebTest):
     @freeze_time("2021-11-26T17:00:00+01:00")
     def test_cancel_and_change_links_only_for_superuser(self):
         normal, staff = non_superusers = [
-            UserFactory.create(),
-            StaffUserFactory.create(),
+            UserFactory.create(user_permissions=["view_appointmentinfo"]),
+            StaffUserFactory.create(user_permissions=["view_appointmentinfo"]),
         ]
-        permission = Permission.objects.get(
-            content_type__app_label="appointments", codename="view_appointmentinfo"
-        )
-        # set up changelist permissions
-        for user in non_superusers:
-            user.user_permissions.add(permission)
 
         # appointment in the future
         AppointmentInfoFactory.create(

src/openforms/config/tests/test_config_check.py

@@ -1,4 +1,3 @@
-from django.contrib.auth.models import Permission
 from django.test import TestCase
 from django.urls import reverse
 
@@ -8,7 +7,6 @@
 class ConfigCheckTests(TestCase):
     def setUp(self):
         super().setUp()
-        self.permission = Permission.objects.get(codename="configuration_overview")
 
     def test_access_permission(self):
         url = reverse("config:overview")
@@ -27,8 +25,7 @@ def test_access_permission(self):
             self.assertEqual(response.status_code, 403)
 
         with self.subTest("staff with permission"):
-            user = StaffUserFactory()
-            user.user_permissions.add(self.permission)
+            user = StaffUserFactory(user_permissions=["configuration_overview"])
             self.client.force_login(user)
             response = self.client.get(url)
             self.assertEqual(response.status_code, 200)

src/openforms/forms/api/permissions.py

@@ -1,12 +1,47 @@
 from rest_framework.permissions import SAFE_METHODS, BasePermission
 
 
-class IsStaffOrReadOnlyNoList(BasePermission):
+class FormAPIPermissions(BasePermission):
     """
-    The request is a staff user, or is a read-only request (but not for a 'list' action).
+    Custom permissions for the FormViewSet and related
+
+    from Github issue #1000:
+
+    1) Write access (put/patch/post/delete) is ONLY given to staff users that have forms.change_form permission
+    2) Read access (get) to everything give to staff users that have form.change_form permission
+    3) Read access (get) to forms.form_LIST given to non-staff users that have forms.view_form permission
+    4) Read access (get) to forms.form_DETAIL given to anonymous users
+
+    x) Anybody can detail/retrieve
     """
 
     def has_permission(self, request, view):
-        if request.user and request.user.is_staff:
+        user = request.user
+
+        # x) anybody can read detail
+        if request.method in SAFE_METHODS and view.action in ("detail", "retrieve"):
             return True
-        return request.method in SAFE_METHODS and view.action != "list"
+
+        # 4) anon users can only read detail (with above)
+        elif not user or not user.is_authenticated:
+            return False
+
+        elif request.method in SAFE_METHODS:
+            # 2) staff with change_form can read everything
+            if user.is_staff and user.has_perm("forms.change_form"):
+                return True
+            # 3) non-staff with view_form can only list
+            elif (
+                not user.is_staff
+                and view.action == "list"
+                and user.has_perm("forms.view_form")
+            ):
+                return True
+            else:
+                return False
+        else:
+            # 1) only staff with change_form can do unsafe operations
+            if user.is_staff and user.has_perm("forms.change_form"):
+                return True
+            else:
+                return False

src/openforms/forms/api/viewsets.py

@@ -31,7 +31,7 @@
 from ..utils import export_form, form_to_json, import_form
 from .filters import FormLogicFilter, FormPriceLogicFilter
 from .parsers import IgnoreConfigurationFieldCamelCaseJSONParser
-from .permissions import IsStaffOrReadOnlyNoList
+from .permissions import FormAPIPermissions
 from .serializers import (
     FormAdminMessageSerializer,
     FormDefinitionDetailSerializer,
@@ -69,7 +69,7 @@ class FormStepViewSet(
 ):
     serializer_class = FormStepSerializer
     queryset = FormStep.objects.all().order_by("order")
-    permission_classes = [IsStaffOrReadOnlyNoList]
+    permission_classes = [FormAPIPermissions]
     lookup_field = "uuid"
 
     def get_serializer_context(self):
@@ -92,9 +92,7 @@ class FormLogicViewSet(
 ):
     serializer_class = FormLogicSerializer
     queryset = FormLogic.objects.all()
-    permission_classes = [
-        permissions.IsAdminUser
-    ]  # TODO: connect with can_change_form admin permission!
+    permission_classes = [FormAPIPermissions]
     filter_backends = (filters.DjangoFilterBackend,)
     filterset_class = FormLogicFilter
     lookup_field = "uuid"
@@ -114,9 +112,7 @@ class FormLogicViewSet(
 class FormPriceLogicViewSet(viewsets.ModelViewSet):
     serializer_class = FormPriceLogicSerializer
     queryset = FormPriceLogic.objects.all()
-    permission_classes = [
-        permissions.IsAdminUser
-    ]  # TODO: connect with can_change_form admin permission!
+    permission_classes = [FormAPIPermissions]
     filter_backends = (filters.DjangoFilterBackend,)
     filterset_class = FormPriceLogicFilter
     lookup_field = "uuid"
@@ -156,7 +152,7 @@ class FormDefinitionViewSet(viewsets.ModelViewSet):
     lookup_field = "uuid"
     # anonymous clients must be able to get the form definitions in the browser
     # The DRF settings apply some default throttling to mitigate abuse
-    permission_classes = [IsStaffOrReadOnlyNoList]
+    permission_classes = [FormAPIPermissions]
 
     def get_serializer_class(self):
         if self.action == "retrieve":
@@ -244,7 +240,7 @@ class FormViewSet(viewsets.ModelViewSet):
     lookup_url_kwarg = "uuid_or_slug"
     # lookup_value_regex = "[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}"
     serializer_class = FormSerializer
-    permission_classes = [IsStaffOrReadOnlyNoList]
+    permission_classes = [FormAPIPermissions]
 
     def get_queryset(self):
         queryset = super().get_queryset()
@@ -435,7 +431,7 @@ def admin_message(self, request, *args, **kwargs):
 class FormVersionViewSet(NestedViewSetMixin, ListModelMixin, viewsets.GenericViewSet):
     serializer_class = FormVersionSerializer
     queryset = FormVersion.objects.all()
-    permission_classes = [permissions.IsAdminUser]
+    permission_classes = [FormAPIPermissions]
     lookup_field = "uuid"
     parent_lookup_kwargs = {"form_uuid_or_slug": "form__uuid"}
 
@@ -465,7 +461,7 @@ class FormsImportAPIView(views.APIView):
     serializer_class = FormImportSerializer
     parser_classes = (parsers.FileUploadParser,)
     authentication_classes = [TokenAuthentication]
-    permission_classes = [permissions.IsAdminUser]
+    permission_classes = [FormAPIPermissions]
 
     @extend_schema(
         summary=_("Import form"),

src/openforms/forms/tests/test_api_form_plugin_options.py

@@ -5,7 +5,7 @@
 from rest_framework import serializers, status
 from rest_framework.test import APITestCase
 
-from openforms.accounts.tests.factories import StaffUserFactory
+from openforms.accounts.tests.factories import SuperUserFactory
 from openforms.payments.base import BasePlugin as PaymentBasePlugin
 from openforms.payments.registry import Registry as PaymentRegistry
 from openforms.registrations.base import BasePlugin as RegistrationBasePlugin
@@ -37,8 +37,8 @@ class PaymentPlugin(PaymentBasePlugin):
 class FormPluginOptionTest(APITestCase):
     def setUp(self):
         super().setUp()
-        self.user = StaffUserFactory()
-        self.client.force_login(self.user)
+        self.user = SuperUserFactory()
+        self.client.force_authenticate(user=self.user)
 
     def test_registration_backend_options(self):
         form = FormFactory.create()

src/openforms/forms/tests/test_api_form_versions.py

@@ -1,13 +1,12 @@
 import datetime
 
-from django.http import HttpRequest
 from django.urls import reverse
 
 from freezegun import freeze_time
 from rest_framework import status
 from rest_framework.test import APITestCase
 
-from openforms.accounts.tests.factories import UserFactory
+from openforms.accounts.tests.factories import StaffUserFactory, UserFactory
 
 from ...submissions.tests.form_logic.factories import FormLogicFactory
 from ..models import FormDefinition, FormLogic, FormStep, FormVersion
@@ -29,34 +28,47 @@ def test_auth_required(self):
 
         self.assertEqual(status.HTTP_401_UNAUTHORIZED, response.status_code)
 
-    def test_must_be_staff_user(self):
-        user = UserFactory.create(username="test", password="test", is_staff=False)
+    def test_staff_required(self):
+        # add the permissions to verify we specifically check is_staff
+        user = UserFactory.create(
+            is_staff=False, user_permissions=["change_form", "view_form"]
+        )
+        self.client.force_authenticate(user=user)
+
+        form = FormFactory.create()
+        url = reverse("api:form-versions-list", args=(form.uuid,))
+
+        response = self.client.post(url)
+
+        self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
+
+    def test_must_have_permission(self):
+        user = StaffUserFactory.create()
+        self.client.force_authenticate(user=user)
+
         form = FormFactory.create()
 
         versions = FormVersion.objects.filter(form=form)
 
         self.assertEqual(0, versions.count())
 
-        self.client.login(
-            request=HttpRequest(), username=user.username, password="test"
-        )
         url = reverse("api:form-versions-list", args=(form.uuid,))
         response = self.client.post(url)
 
         self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
 
     @freeze_time("2020-12-11T10:53:19+01:00")
     def test_save_version(self):
-        user = UserFactory.create(username="test", password="test", is_staff=True)
+        user = StaffUserFactory.create(user_permissions=["change_form"])
+        self.client.force_authenticate(user=user)
+        self.assertEqual(user.user_permissions.all().count(), 1)
+
         form = FormFactory.create()
 
         versions = FormVersion.objects.filter(form=form)
 
         self.assertEqual(0, versions.count())
 
-        self.client.login(
-            request=HttpRequest(), username=user.username, password="test"
-        )
         url = reverse("api:form-versions-list", args=(form.uuid,))
         response = self.client.post(url)
 
@@ -77,7 +89,9 @@ def test_save_version(self):
         )
 
     def test_list_versions(self):
-        user = UserFactory.create(username="test", password="test", is_staff=True)
+        user = StaffUserFactory.create(user_permissions=["change_form"])
+        self.client.force_authenticate(user=user)
+
         form_1 = FormFactory.create()
         form_2 = FormFactory.create()
 
@@ -86,9 +100,6 @@ def test_list_versions(self):
 
         self.assertEqual(2, FormVersion.objects.all().count())
 
-        self.client.login(
-            request=HttpRequest(), username=user.username, password="test"
-        )
         url = reverse("api:form-versions-list", args=(form_1.uuid,))
         response = self.client.get(url)
 
@@ -110,13 +121,12 @@ def test_auth_required(self):
         self.assertEqual(status.HTTP_401_UNAUTHORIZED, response.status_code)
 
     def test_must_be_staff_user(self):
-        user = UserFactory.create(username="test", password="test", is_staff=False)
+        user = UserFactory.create()
+        self.client.force_authenticate(user=user)
+
         form = FormFactory.create()
         form_version = FormVersionFactory.create(form=form)
 
-        self.client.login(
-            request=HttpRequest(), username=user.username, password="test"
-        )
         url = reverse(
             "api:form-versions-restore",
             kwargs={"form_uuid_or_slug": form.uuid, "uuid": form_version.uuid},
@@ -126,6 +136,9 @@ def test_must_be_staff_user(self):
         self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
 
     def test_restore_version(self):
+        user = StaffUserFactory.create(user_permissions=["change_form"])
+        self.client.force_authenticate(user=user)
+
         form_definition = FormDefinitionFactory.create(
             slug="test-definition-2", configuration={"test": "2"}
         )
@@ -139,10 +152,6 @@ def test_restore_version(self):
             export_blob=EXPORT_BLOB,
         )
 
-        user = UserFactory.create(username="test", password="test", is_staff=True)
-        self.client.login(
-            request=HttpRequest(), username=user.username, password="test"
-        )
         url = reverse(
             "api:form-versions-restore",
             kwargs={"form_uuid_or_slug": form.uuid, "uuid": version.uuid},