[#516] Refactor user checks

backend/bin/liveness_celery.sh

@@ -0,0 +1,25 @@
+#!/bin/bash
+
+    # Exit immediately if a non-zero exit code is returned
+    set -e
+
+    QUEUE_NAME=${CELERY_WORKER_QUEUE:-celery}
+
+    # Check if CELERY_WORKER_NAME is set
+    if [ -z "$CELERY_WORKER_NAME" ]; then
+      # If not set
+      WORKER_NAME="${QUEUE_NAME}@${HOSTNAME}"
+    else
+      # If set and contains the '@' symbol
+      if [[ "$CELERY_WORKER_NAME" != *"@"* ]]; then
+        # If not, update CELERY_WORKER_NAME to celery@${CELERY_WORKER_NAME}
+        WORKER_NAME="celery@${CELERY_WORKER_NAME}"
+      else
+        WORKER_NAME="${CELERY_WORKER_NAME}"
+      fi
+    fi
+
+    echo "$WORKER_NAME"
+
+    # Use CELERY_WORKER_NAME to set the --destination flag
+    # celery --workdir src --app openarchiefbeheer.celery inspect --destination "$WORKER_NAME" active

backend/src/openarchiefbeheer/destruction/api/mixins.py

@@ -0,0 +1,27 @@
+from django.utils.translation import gettext_lazy as _
+
+from rest_framework.exceptions import ValidationError
+
+from ..models import DestructionList
+
+
+class DestructionListChecksMixin:
+    """Check the permissions on a destruction list
+
+    Used by APIView endpoints that create a resource related to a destruction list (for example a review/co-review)
+    and need to check based on the rights of the user, the status and the assignees of the destruction list
+    whether the action is allowed.
+    """
+
+    def create(self, request, *args, **kwargs):
+        try:
+            destruction_list = DestructionList.objects.get(
+                uuid=request.data.get("destruction_list")
+            )
+        except DestructionList.DoesNotExist:
+            raise ValidationError(
+                detail={"destruction_list": _("The destruction list does not exist.")}
+            )
+
+        self.check_object_permissions(self.request, destruction_list)
+        return super().create(request, *args, **kwargs)

backend/src/openarchiefbeheer/destruction/api/permissions.py

@@ -20,6 +20,25 @@ def has_permission(self, request, view):
             "accounts.can_review_destruction"
         ) or request.user.has_perm("accounts.can_review_final_list")
 
+    def has_object_permission(self, request, view, destruction_list):
+        user = request.user
+
+        # User is not assigned
+        if destruction_list.assignee != user:
+            return False
+
+        # User is not permitted based on role + status
+        if (
+            destruction_list.status == ListStatus.ready_to_review
+            and user.has_perm("accounts.can_review_destruction")
+        ) or (
+            destruction_list.status == ListStatus.ready_for_archivist
+            and user.has_perm("accounts.can_review_final_list")
+        ):
+            return True
+
+        return False
+
 
 class CanCoReviewPermission(permissions.BasePermission):
     message = _("You are not allowed to co-review a destruction list.")
@@ -28,7 +47,20 @@ def has_permission(self, request, view):
         return request.user.has_perm("accounts.can_co_review_destruction")
 
     def has_object_permission(self, request, view, destruction_list):
-        return destruction_list.assignees.includes(request.user)
+        user_assignees = destruction_list.assignees.values_list("user__pk", flat=True)
+        user = request.user
+
+        # User is not assigned
+        if user.pk not in user_assignees:
+            return False
+
+        # User is permitted based on role + status
+        if destruction_list.status == ListStatus.ready_to_review and user.has_perm(
+            "accounts.can_co_review_destruction"
+        ):
+            return True
+
+        return False
 
 
 class CanUpdateDestructionList(permissions.BasePermission):

backend/src/openarchiefbeheer/destruction/api/serializers.py

@@ -512,41 +512,6 @@ class Meta:
         )
 
     def validate(self, attrs: dict) -> dict:
-        destruction_list = attrs["destruction_list"]
-        user = self.context["request"].user
-
-        # User is not assigned
-        if destruction_list.assignee != user:
-            raise ValidationError(
-                {
-                    "author": _(
-                        "This user is not currently assigned to the destruction list, "
-                        "so they cannot create a review at this stage."
-                    )
-                }
-            )
-
-        # User is not permitted based on role + status
-        if (
-            (
-                destruction_list.status == ListStatus.ready_to_review
-                and not user.has_perm("accounts.can_review_destruction")
-            )
-            or (
-                destruction_list.status == ListStatus.ready_for_archivist
-                and not user.has_perm("accounts.can_review_final_list")
-            )
-            or destruction_list.status
-            not in [ListStatus.ready_to_review, ListStatus.ready_for_archivist]
-        ):
-            raise ValidationError(
-                {
-                    "author": _(
-                        "The status of this destruction list prevents you from creating a review at this stage."
-                    )
-                }
-            )
-
         zaken_reviews = attrs.get("zaken_reviews", [])
         if (
             attrs["decision"] == ReviewDecisionChoices.rejected
@@ -674,43 +639,15 @@ class Meta:
             "created",
         )
 
-    def validate(self, attrs: dict) -> dict:
-        destruction_list = attrs["destruction_list"]
-        user_assignees = destruction_list.assignees.values_list("user__pk", flat=True)
+    def create(self, validated_data: dict) -> DestructionListReview:
         user = self.context["request"].user
+        validated_data.update({"author": user})
 
-        # User is not assigned
-        if user.pk not in user_assignees:
-            raise ValidationError(
-                {
-                    "author": _(
-                        "This user is not currently assigned to the destruction list, "
-                        "so they cannot create a co-review at this stage."
-                    )
-                }
-            )
-
-        # User is not permitted based on role + status
-        if (
-            destruction_list.status == ListStatus.ready_to_review
-            and not user.has_perm("accounts.can_co_review_destruction")
-        ) or destruction_list.status != ListStatus.ready_to_review:
-            raise ValidationError(
-                {
-                    "author": _(
-                        "The status of this destruction list prevents you from creating a co-review at this stage."
-                    )
-                }
-            )
-
-        return {"author": user, **attrs}
-
-    def create(self, validated_data: dict) -> DestructionListReview:
         co_review = super().create(validated_data)
         logevent.destruction_list_co_reviewed(
             destruction_list=validated_data["destruction_list"],
             co_review=co_review,
-            user=validated_data["author"],
+            user=user,
         )
         return co_review
 

backend/src/openarchiefbeheer/destruction/api/viewsets.py

@@ -46,6 +46,7 @@
     DestructionListReviewItemFilterset,
     ReviewResponseFilterset,
 )
+from .mixins import DestructionListChecksMixin
 from .permissions import (
     CanAbortDestruction,
     CanCoReviewPermission,
@@ -532,6 +533,7 @@ def get_queryset(self):
     ),
 )
 class DestructionListReviewViewSet(
+    DestructionListChecksMixin,
     mixins.ListModelMixin,
     mixins.CreateModelMixin,
     viewsets.GenericViewSet,
@@ -567,6 +569,7 @@ def get_permissions(self):
     ),
 )
 class DestructionListCoReviewViewSet(
+    DestructionListChecksMixin,
     mixins.ListModelMixin,
     mixins.CreateModelMixin,
     viewsets.GenericViewSet,

backend/src/openarchiefbeheer/destruction/tests/endpoints/test_co_reviews.py

@@ -186,15 +186,51 @@ def test_create_not_assigned(self):
                 "list_feedback": "gh-497",
             },
         )
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertEqual(
-            response.data["author"],
-            [
-                _(
-                    "This user is not currently assigned to the destruction list, so they cannot create a co-review at this stage."
-                )
-            ],
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+
+    def test_if_list_in_wrong_status_cannot_be_reviewed(self):
+        assignees = DestructionListAssigneeFactory.create_batch(
+            3,
+            user__post__can_co_review_destruction=True,
+        )
+        co_reviewer = assignees[2]
+        destruction_list = DestructionListFactory.create(
+            status=ListStatus.ready_for_archivist
+        )
+        destruction_list.assignees.set(assignees)
+
+        self.client.force_login(co_reviewer.user)
+        url = reverse("api:destruction-list-co-reviews-list")
+        response = self.client.post(
+            url,
+            {
+                "destruction_list": destruction_list.uuid,
+                "list_feedback": "gh-497",
+            },
+        )
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+
+    def test_if_user_not_a_co_reviewer_cannot_create_review(self):
+        assignees = DestructionListAssigneeFactory.create_batch(
+            3,
+            user__post__can_co_review_destruction=False,
+        )
+        co_reviewer = assignees[2].user
+        destruction_list = DestructionListFactory.create(
+            status=ListStatus.ready_to_review
+        )
+        destruction_list.assignees.set(assignees)
+
+        self.client.force_login(co_reviewer)
+        url = reverse("api:destruction-list-co-reviews-list")
+        response = self.client.post(
+            url,
+            {
+                "destruction_list": destruction_list.uuid,
+                "list_feedback": "gh-497",
+            },
         )
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
     def test_create_no_list_feedback(self):
         co_reviewer = DestructionListAssigneeFactory.create(role=ListRole.co_reviewer)
@@ -238,3 +274,26 @@ def test_create(self):
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
         self.assertEqual(co_review.author, co_reviewer.user)
         self.assertEqual(co_review.list_feedback, "gh-497")
+
+    def test_destruction_list_does_not_exist(self):
+        co_reviewer = DestructionListAssigneeFactory.create(role=ListRole.co_reviewer)
+        destruction_list = DestructionListFactory.create(
+            status=ListStatus.ready_to_review
+        )
+        destruction_list.assignees.add(co_reviewer)
+
+        self.client.force_login(co_reviewer.user)
+        url = reverse("api:destruction-list-co-reviews-list")
+        response = self.client.post(
+            url,
+            {
+                "destruction_list": "7cf58293-cf87-48a1-9857-7dea3dba00e0",
+                "list_feedback": "Tralala",
+            },
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(
+            response.json()["destructionList"],
+            _("The destruction list does not exist."),
+        )