Add permission checks for moving engagements between products (#14634)

* change-to-moving-engagements

* fix-migration-issue:

* Revert PR #14634 changes (editable=False approach)

Reverting the approach of making Engagement.product editable=False
and splitting serializers. Will replace with proper permission checks
on the destination product when moving engagements.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add permission check on destination product when moving engagements

When a user changes an engagement's product (via API PUT/PATCH or
the UI edit form), verify they have Engagement_Edit permission on
the destination product. Previously only the source product was
checked, allowing users to move engagements to products they lack
write access to.

- API: EngagementSerializer.validate() checks destination product
  permission on update, following the ProductMemberSerializer pattern
- UI: edit_engagement() view checks destination product permission
  before saving
- Tests: 8 new tests covering PATCH, PUT, and UI paths for both
  authorized and unauthorized product moves

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix UI test: form queryset already rejects unauthorized products

The EngForm product queryset is filtered to authorized products, so
submitting an unauthorized product fails form validation (200) before
the view-level permission check runs. Update the test to accept both
200 and 403 -- the key assertion is that the engagement does not move.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix ruff lint: docstring formatting

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Cody Maffucci <46459665+Maffooch@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: 3 people

dojo/api_v2/serializers.py

@@ -1123,6 +1123,18 @@ def validate(self, data):
             if data.get("target_start") > data.get("target_end"):
                 msg = "Your target start date exceeds your target end date"
                 raise serializers.ValidationError(msg)
+        if (
+            self.instance is not None
+            and "product" in data
+            and data.get("product") != self.instance.product
+            and not user_has_permission(
+                self.context["request"].user,
+                data.get("product"),
+                Permissions.Engagement_Edit,
+            )
+        ):
+            msg = "You are not permitted to edit engagements in the destination product"
+            raise PermissionDenied(msg)
         return data
 
     def build_relational_field(self, field_name, relation_info):

dojo/engagement/views.py

@@ -286,6 +286,12 @@ def edit_engagement(request, eid):
         if form.is_valid():
             # first save engagement details
             new_status = form.cleaned_data.get("status")
+            if form.cleaned_data.get("product") != engagement.product:
+                user_has_permission_or_403(
+                    request.user,
+                    form.cleaned_data.get("product"),
+                    Permissions.Engagement_Edit,
+                )
             engagement.product = form.cleaned_data.get("product")
             engagement = form.save(commit=False)
             if (new_status in {"Cancelled", "Completed"}):

unittests/test_permissions_audit.py

@@ -1686,3 +1686,162 @@ def test_risk_acceptance_download_proof_writer_allowed(self):
 
         # Clean up uploaded file
         self.risk_acceptance.path.delete(save=True)
+
+
+class TestEngagementMovePermission(DojoTestCase):
+
+    """Moving an engagement to another product requires Engagement_Edit on the destination."""
+
+    @classmethod
+    def setUpTestData(cls):
+        cls.owner_role = Role.objects.get(name="Owner")
+        cls.product_type = Product_Type.objects.create(name="Eng Move Test PT")
+
+        cls.product_a = Product.objects.create(
+            name="Eng Move Product A", description="Test", prod_type=cls.product_type,
+        )
+        cls.product_b = Product.objects.create(
+            name="Eng Move Product B", description="Test", prod_type=cls.product_type,
+        )
+        cls.product_c = Product.objects.create(
+            name="Eng Move Product C", description="Test", prod_type=cls.product_type,
+        )
+
+        cls.user = Dojo_User.objects.create_user(
+            username="eng_move_owner",
+            password="testTEST1234!@#$",  # noqa: S106
+            is_active=True,
+        )
+        Product_Member.objects.create(product=cls.product_a, user=cls.user, role=cls.owner_role)
+        # No membership on product_b -- user cannot move engagements there
+        Product_Member.objects.create(product=cls.product_c, user=cls.user, role=cls.owner_role)
+
+    def setUp(self):
+        self.engagement = Engagement.objects.create(
+            name="Move Test Engagement",
+            product=self.product_a,
+            target_start=datetime.date.today(),
+            target_end=datetime.date.today(),
+        )
+
+    def _api_client(self):
+        token, _ = Token.objects.get_or_create(user=self.user)
+        client = APIClient()
+        client.credentials(HTTP_AUTHORIZATION="Token " + token.key)
+        return client
+
+    def _ui_client(self):
+        client = Client()
+        client.login(username="eng_move_owner", password="testTEST1234!@#$")  # noqa: S106
+        return client
+
+    # ── API: PATCH ────────────────────────────────────────────────────
+
+    def test_api_patch_move_to_authorized_product(self):
+        """PATCH with product the user has access to should succeed."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        response = client.patch(url, {"product": self.product_c.id}, format="json")
+        self.assertEqual(response.status_code, 200, response.content)
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_c)
+
+    def test_api_patch_move_to_unauthorized_product(self):
+        """PATCH with product the user lacks access to should be denied."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        response = client.patch(url, {"product": self.product_b.id}, format="json")
+        self.assertEqual(response.status_code, 403, response.content)
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_a)
+
+    def test_api_patch_same_product(self):
+        """PATCH with the same product should succeed without extra permission check."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        response = client.patch(url, {"product": self.product_a.id}, format="json")
+        self.assertEqual(response.status_code, 200, response.content)
+
+    def test_api_patch_without_product_field(self):
+        """PATCH without product field should succeed (no spurious check)."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        response = client.patch(url, {"version": "1.0"}, format="json")
+        self.assertEqual(response.status_code, 200, response.content)
+
+    # ── API: PUT ──────────────────────────────────────────────────────
+
+    def test_api_put_move_to_authorized_product(self):
+        """PUT with product the user has access to should succeed."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        payload = {
+            "name": "Move Test Engagement",
+            "product": self.product_c.id,
+            "target_start": str(datetime.date.today()),
+            "target_end": str(datetime.date.today()),
+            "engagement_type": "Interactive",
+            "status": "Not Started",
+        }
+        response = client.put(url, payload, format="json")
+        self.assertEqual(response.status_code, 200, response.content)
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_c)
+
+    def test_api_put_move_to_unauthorized_product(self):
+        """PUT with product the user lacks access to should be denied."""
+        client = self._api_client()
+        url = reverse("engagement-detail", args=(self.engagement.id,))
+        payload = {
+            "name": "Move Test Engagement",
+            "product": self.product_b.id,
+            "target_start": str(datetime.date.today()),
+            "target_end": str(datetime.date.today()),
+            "engagement_type": "Interactive",
+            "status": "Not Started",
+        }
+        response = client.put(url, payload, format="json")
+        self.assertEqual(response.status_code, 403, response.content)
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_a)
+
+    # ── UI ────────────────────────────────────────────────────────────
+
+    def test_ui_move_to_authorized_product(self):
+        """Edit engagement form moving to authorized product should succeed."""
+        client = self._ui_client()
+        url = reverse("edit_engagement", args=(self.engagement.id,))
+        form_data = {
+            "product": self.product_c.id,
+            "target_start": datetime.date.today().strftime("%Y-%m-%d"),
+            "target_end": datetime.date.today().strftime("%Y-%m-%d"),
+            "lead": self.user.id,
+            "status": "Not Started",
+        }
+        response = client.post(url, form_data)
+        self.assertIn(response.status_code, [200, 302], response.content[:500])
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_c)
+
+    def test_ui_move_to_unauthorized_product(self):
+        """
+        Edit engagement form moving to unauthorized product should be denied.
+
+        The form's product queryset is filtered to authorized products, so
+        submitting an unauthorized product fails form validation (200 with
+        errors) before the view-level permission check runs.  Either way the
+        engagement must NOT move.
+        """
+        client = self._ui_client()
+        url = reverse("edit_engagement", args=(self.engagement.id,))
+        form_data = {
+            "product": self.product_b.id,
+            "target_start": datetime.date.today().strftime("%Y-%m-%d"),
+            "target_end": datetime.date.today().strftime("%Y-%m-%d"),
+            "lead": self.user.id,
+            "status": "Not Started",
+        }
+        response = client.post(url, form_data)
+        self.assertIn(response.status_code, [200, 403])
+        self.engagement.refresh_from_db()
+        self.assertEqual(self.engagement.product, self.product_a)