From 342c477697ce8a7088187e656ede0cfcce360daf Mon Sep 17 00:00:00 2001 From: Jino Tesauro Date: Fri, 3 Apr 2026 15:40:43 -0500 Subject: [PATCH 1/6] change-to-moving-engagements --- dojo/api_v2/serializers.py | 26 +++++++++++++++++++++++++- dojo/api_v2/views.py | 5 +++++ dojo/models.py | 2 +- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/dojo/api_v2/serializers.py b/dojo/api_v2/serializers.py index 342287aba94..10d0be41183 100644 --- a/dojo/api_v2/serializers.py +++ b/dojo/api_v2/serializers.py @@ -1119,7 +1119,7 @@ class Meta: exclude = ("inherited_tags",) def validate(self, data): - if self.context["request"].method == "POST": + if data.get("target_start") and data.get("target_end"): if data.get("target_start") > data.get("target_end"): msg = "Your target start date exceeds your target end date" raise serializers.ValidationError(msg) @@ -1133,6 +1133,30 @@ def build_relational_field(self, field_name, relation_info): return super().build_relational_field(field_name, relation_info) +class EngagementCreateSerializer(serializers.ModelSerializer): + product = serializers.PrimaryKeyRelatedField( + queryset=Product.objects.all(), + ) + tags = TagListSerializerField(required=False) + + class Meta: + model = Engagement + exclude = ("inherited_tags",) + + 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) + return data + + def build_relational_field(self, field_name, relation_info): + if field_name == "notes": + return NoteSerializer, {"many": True, "read_only": True} + if field_name == "files": + return FileSerializer, {"many": True, "read_only": True} + return super().build_relational_field(field_name, relation_info) + + class EngagementToNotesSerializer(serializers.Serializer): engagement_id = serializers.PrimaryKeyRelatedField( queryset=Engagement.objects.all(), many=False, allow_null=True, diff --git a/dojo/api_v2/views.py b/dojo/api_v2/views.py index 85627178af9..e7393583bf3 100644 --- a/dojo/api_v2/views.py +++ b/dojo/api_v2/views.py @@ -455,6 +455,11 @@ class EngagementViewSet( def risk_application_model_class(self): return Engagement + def get_serializer_class(self): + if self.request and self.request.method == "POST": + return serializers.EngagementCreateSerializer + return serializers.EngagementSerializer + def destroy(self, request, *args, **kwargs): instance = self.get_object() if get_setting("ASYNC_OBJECT_DELETE"): diff --git a/dojo/models.py b/dojo/models.py index bcfb39180a4..c9023f95551 100644 --- a/dojo/models.py +++ b/dojo/models.py @@ -1526,7 +1526,7 @@ class Engagement(BaseModel): preset = models.ForeignKey(Engagement_Presets, null=True, blank=True, help_text=_("Settings and notes for performing this engagement."), on_delete=models.CASCADE) reason = models.CharField(max_length=2000, null=True, blank=True) report_type = models.ForeignKey(Report_Type, null=True, blank=True, on_delete=models.CASCADE) - product = models.ForeignKey(Product, on_delete=models.CASCADE) + product = models.ForeignKey(Product, editable=False, on_delete=models.CASCADE) active = models.BooleanField(default=True, editable=False) tracker = models.URLField(max_length=200, help_text=_("Link to epic or ticket system with changes to version."), editable=True, blank=True, null=True) test_strategy = models.URLField(editable=True, blank=True, null=True) From ca4c4453df88afa19a5e0cf28d2822e80602361f Mon Sep 17 00:00:00 2001 From: Jino Tesauro Date: Fri, 3 Apr 2026 16:14:39 -0500 Subject: [PATCH 2/6] fix-migration-issue: --- .../0264_alter_engagement_product_and_more.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 dojo/db_migrations/0264_alter_engagement_product_and_more.py diff --git a/dojo/db_migrations/0264_alter_engagement_product_and_more.py b/dojo/db_migrations/0264_alter_engagement_product_and_more.py new file mode 100644 index 00000000000..7752e3c96c9 --- /dev/null +++ b/dojo/db_migrations/0264_alter_engagement_product_and_more.py @@ -0,0 +1,24 @@ +# Generated by Django 5.2.12 on 2026-04-03 20:48 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('dojo', '0263_language_type_unique_language'), + ] + + operations = [ + migrations.AlterField( + model_name='engagement', + name='product', + field=models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, to='dojo.product'), + ), + migrations.AlterField( + model_name='engagementevent', + name='product', + field=models.ForeignKey(db_constraint=False, db_index=False, editable=False, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', related_query_name='+', to='dojo.product'), + ), + ] From 72861d0dc32f4654d78f53a737ec2ac7f4e6a610 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Tue, 14 Apr 2026 14:27:17 -0600 Subject: [PATCH 3/6] 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) --- dojo/api_v2/serializers.py | 26 +------------------ dojo/api_v2/views.py | 5 ---- .../0264_alter_engagement_product_and_more.py | 24 ----------------- dojo/models.py | 2 +- 4 files changed, 2 insertions(+), 55 deletions(-) delete mode 100644 dojo/db_migrations/0264_alter_engagement_product_and_more.py diff --git a/dojo/api_v2/serializers.py b/dojo/api_v2/serializers.py index 10d0be41183..342287aba94 100644 --- a/dojo/api_v2/serializers.py +++ b/dojo/api_v2/serializers.py @@ -1119,7 +1119,7 @@ class Meta: exclude = ("inherited_tags",) def validate(self, data): - if data.get("target_start") and data.get("target_end"): + if self.context["request"].method == "POST": if data.get("target_start") > data.get("target_end"): msg = "Your target start date exceeds your target end date" raise serializers.ValidationError(msg) @@ -1133,30 +1133,6 @@ def build_relational_field(self, field_name, relation_info): return super().build_relational_field(field_name, relation_info) -class EngagementCreateSerializer(serializers.ModelSerializer): - product = serializers.PrimaryKeyRelatedField( - queryset=Product.objects.all(), - ) - tags = TagListSerializerField(required=False) - - class Meta: - model = Engagement - exclude = ("inherited_tags",) - - 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) - return data - - def build_relational_field(self, field_name, relation_info): - if field_name == "notes": - return NoteSerializer, {"many": True, "read_only": True} - if field_name == "files": - return FileSerializer, {"many": True, "read_only": True} - return super().build_relational_field(field_name, relation_info) - - class EngagementToNotesSerializer(serializers.Serializer): engagement_id = serializers.PrimaryKeyRelatedField( queryset=Engagement.objects.all(), many=False, allow_null=True, diff --git a/dojo/api_v2/views.py b/dojo/api_v2/views.py index e7393583bf3..85627178af9 100644 --- a/dojo/api_v2/views.py +++ b/dojo/api_v2/views.py @@ -455,11 +455,6 @@ class EngagementViewSet( def risk_application_model_class(self): return Engagement - def get_serializer_class(self): - if self.request and self.request.method == "POST": - return serializers.EngagementCreateSerializer - return serializers.EngagementSerializer - def destroy(self, request, *args, **kwargs): instance = self.get_object() if get_setting("ASYNC_OBJECT_DELETE"): diff --git a/dojo/db_migrations/0264_alter_engagement_product_and_more.py b/dojo/db_migrations/0264_alter_engagement_product_and_more.py deleted file mode 100644 index 7752e3c96c9..00000000000 --- a/dojo/db_migrations/0264_alter_engagement_product_and_more.py +++ /dev/null @@ -1,24 +0,0 @@ -# Generated by Django 5.2.12 on 2026-04-03 20:48 - -import django.db.models.deletion -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('dojo', '0263_language_type_unique_language'), - ] - - operations = [ - migrations.AlterField( - model_name='engagement', - name='product', - field=models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, to='dojo.product'), - ), - migrations.AlterField( - model_name='engagementevent', - name='product', - field=models.ForeignKey(db_constraint=False, db_index=False, editable=False, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', related_query_name='+', to='dojo.product'), - ), - ] diff --git a/dojo/models.py b/dojo/models.py index c9023f95551..bcfb39180a4 100644 --- a/dojo/models.py +++ b/dojo/models.py @@ -1526,7 +1526,7 @@ class Engagement(BaseModel): preset = models.ForeignKey(Engagement_Presets, null=True, blank=True, help_text=_("Settings and notes for performing this engagement."), on_delete=models.CASCADE) reason = models.CharField(max_length=2000, null=True, blank=True) report_type = models.ForeignKey(Report_Type, null=True, blank=True, on_delete=models.CASCADE) - product = models.ForeignKey(Product, editable=False, on_delete=models.CASCADE) + product = models.ForeignKey(Product, on_delete=models.CASCADE) active = models.BooleanField(default=True, editable=False) tracker = models.URLField(max_length=200, help_text=_("Link to epic or ticket system with changes to version."), editable=True, blank=True, null=True) test_strategy = models.URLField(editable=True, blank=True, null=True) From b8a5c0f42fc3198e48791e820772589606ca31c1 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Tue, 14 Apr 2026 14:30:11 -0600 Subject: [PATCH 4/6] 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) --- dojo/api_v2/serializers.py | 12 +++ dojo/engagement/views.py | 6 ++ unittests/test_permissions_audit.py | 152 ++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+) diff --git a/dojo/api_v2/serializers.py b/dojo/api_v2/serializers.py index 342287aba94..7c9bbd6ae79 100644 --- a/dojo/api_v2/serializers.py +++ b/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): diff --git a/dojo/engagement/views.py b/dojo/engagement/views.py index 1b94ace21f4..7dad0433f8e 100644 --- a/dojo/engagement/views.py +++ b/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"}): diff --git a/unittests/test_permissions_audit.py b/unittests/test_permissions_audit.py index 8904c6270be..e3dc2ad020e 100644 --- a/unittests/test_permissions_audit.py +++ b/unittests/test_permissions_audit.py @@ -1425,3 +1425,155 @@ 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.""" + 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.assertEqual(response.status_code, 403) + self.engagement.refresh_from_db() + self.assertEqual(self.engagement.product, self.product_a) From 648fdb922c01d32ad304aa5685514965420c895d Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:33:11 -0600 Subject: [PATCH 5/6] 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) --- unittests/test_permissions_audit.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/unittests/test_permissions_audit.py b/unittests/test_permissions_audit.py index e3dc2ad020e..c4d49dc2f41 100644 --- a/unittests/test_permissions_audit.py +++ b/unittests/test_permissions_audit.py @@ -1563,7 +1563,13 @@ def test_ui_move_to_authorized_product(self): 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.""" + """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 = { @@ -1574,6 +1580,6 @@ def test_ui_move_to_unauthorized_product(self): "status": "Not Started", } response = client.post(url, form_data) - self.assertEqual(response.status_code, 403) + self.assertIn(response.status_code, [200, 403]) self.engagement.refresh_from_db() self.assertEqual(self.engagement.product, self.product_a) From 9ed0616e26ac6e4a27ac9d2bc414c80a000c420b Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:36:45 -0600 Subject: [PATCH 6/6] Fix ruff lint: docstring formatting Co-Authored-By: Claude Opus 4.6 (1M context) --- unittests/test_permissions_audit.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/unittests/test_permissions_audit.py b/unittests/test_permissions_audit.py index c4d49dc2f41..6f0705f8d3c 100644 --- a/unittests/test_permissions_audit.py +++ b/unittests/test_permissions_audit.py @@ -1563,7 +1563,8 @@ def test_ui_move_to_authorized_product(self): 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. + """ + 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