enhance/normalize EDITABLE_MITIGATED_DATA handling (#13303)

* enhance/normalize EDITABLE_MITIGATED_DATA handling

* add tests, fix bugs

* fix validation

* fixes

* fix close finding form

Author: valentijnscholten

dojo/api_v2/serializers.py

@@ -1676,6 +1676,8 @@ class Meta:
 
 
 class FindingSerializer(serializers.ModelSerializer):
+    mitigated = serializers.DateTimeField(required=False, allow_null=True)
+    mitigated_by = serializers.PrimaryKeyRelatedField(required=False, allow_null=True, queryset=User.objects.all())
     tags = TagListSerializerField(required=False)
     request_response = serializers.SerializerMethodField()
     accepted_risks = RiskAcceptanceSerializer(
@@ -1772,6 +1774,21 @@ def update(self, instance, validated_data):
         return instance
 
     def validate(self, data):
+        # Enforce mitigated metadata editability (only when non-null values are provided)
+        attempting_to_set_mitigated = any(
+            (field in data) and (data.get(field) is not None)
+            for field in ["mitigated", "mitigated_by"]
+        )
+        user = getattr(self.context.get("request", None), "user", None)
+        if attempting_to_set_mitigated and not finding_helper.can_edit_mitigated_data(user):
+            errors = {}
+            if ("mitigated" in data) and (data.get("mitigated") is not None):
+                errors["mitigated"] = ["Editing mitigated timestamp is disabled (EDITABLE_MITIGATED_DATA=false)"]
+            if ("mitigated_by" in data) and (data.get("mitigated_by") is not None):
+                errors["mitigated_by"] = ["Editing mitigated_by is disabled (EDITABLE_MITIGATED_DATA=false)"]
+            if errors:
+                raise serializers.ValidationError(errors)
+
         if self.context["request"].method == "PATCH":
             is_active = data.get("active", self.instance.active)
             is_verified = data.get("verified", self.instance.verified)
@@ -1841,6 +1858,8 @@ def get_request_response(self, obj):
 
 
 class FindingCreateSerializer(serializers.ModelSerializer):
+    mitigated = serializers.DateTimeField(required=False, allow_null=True)
+    mitigated_by = serializers.PrimaryKeyRelatedField(required=False, allow_null=True, queryset=User.objects.all())
     notes = serializers.PrimaryKeyRelatedField(
         read_only=True, allow_null=True, required=False, many=True,
     )
@@ -1909,6 +1928,21 @@ def create(self, validated_data):
         return new_finding
 
     def validate(self, data):
+        # Ensure mitigated fields are only set when editable is enabled (ignore nulls)
+        attempting_to_set_mitigated = any(
+            (field in data) and (data.get(field) is not None)
+            for field in ["mitigated", "mitigated_by"]
+        )
+        user = getattr(getattr(self.context, "request", None), "user", None)
+        if attempting_to_set_mitigated and not finding_helper.can_edit_mitigated_data(user):
+            errors = {}
+            if ("mitigated" in data) and (data.get("mitigated") is not None):
+                errors["mitigated"] = ["Editing mitigated timestamp is disabled (EDITABLE_MITIGATED_DATA=false)"]
+            if ("mitigated_by" in data) and (data.get("mitigated_by") is not None):
+                errors["mitigated_by"] = ["Editing mitigated_by is disabled (EDITABLE_MITIGATED_DATA=false)"]
+            if errors:
+                raise serializers.ValidationError(errors)
+
         if "reporter" not in data:
             request = self.context["request"]
             data["reporter"] = request.user

dojo/api_v2/views.py

@@ -937,7 +937,7 @@ def close(self, request, pk=None):
                     finding=finding,
                     user=request.user,
                     is_mitigated=finding_close.validated_data["is_mitigated"],
-                    mitigated=(finding_close.validated_data.get("mitigated") if settings.EDITABLE_MITIGATED_DATA else timezone.now()),
+                    mitigated=(finding_close.validated_data.get("mitigated") if finding_helper.can_edit_mitigated_data(request.user) else timezone.now()),
                     mitigated_by=finding_close.validated_data.get("mitigated_by") or (request.user if not finding_helper.can_edit_mitigated_data(request.user) else None),
                     false_p=finding_close.validated_data.get("false_p", False),
                     out_of_scope=finding_close.validated_data.get("out_of_scope", False),

dojo/finding/helper.py

@@ -129,10 +129,13 @@ def update_finding_status(new_state_finding, user, changed_fields=None):
             new_state_finding.mitigated = None
             new_state_finding.mitigated_by = None
 
-    # people may try to remove mitigated/mitigated_by by accident
+    # Ensure mitigated metadata is present for mitigated findings
+    # If values are provided (including custom ones), keep them; if missing, set defaults
     if new_state_finding.is_mitigated:
-        new_state_finding.mitigated = new_state_finding.mitigated or now
-        new_state_finding.mitigated_by = new_state_finding.mitigated_by or user
+        if not new_state_finding.mitigated:
+            new_state_finding.mitigated = now
+        if not new_state_finding.mitigated_by:
+            new_state_finding.mitigated_by = user
 
     if is_new_finding or "active" in changed_fields:
         # finding is being (re)activated
@@ -185,7 +188,7 @@ def filter_findings_by_existence(findings):
 
 
 def can_edit_mitigated_data(user):
-    return settings.EDITABLE_MITIGATED_DATA and user.is_superuser
+    return settings.EDITABLE_MITIGATED_DATA and user and getattr(user, "is_superuser", False)
 
 
 def create_finding_group(finds, finding_group_name):

dojo/finding/views.py

@@ -1144,9 +1144,16 @@ def close_finding(request, fid):
     # we can do this with a Note
     note_type_activation = Note_Type.objects.filter(is_active=True)
     missing_note_types = get_missing_mandatory_notetypes(finding) if len(note_type_activation) else note_type_activation
-    form = CloseFindingForm(missing_note_types=missing_note_types)
+    form = CloseFindingForm(
+        missing_note_types=missing_note_types,
+        can_edit_mitigated_data=finding_helper.can_edit_mitigated_data(request.user),
+    )
     if request.method == "POST":
-        form = CloseFindingForm(request.POST, missing_note_types=missing_note_types)
+        form = CloseFindingForm(
+            request.POST,
+            missing_note_types=missing_note_types,
+            can_edit_mitigated_data=finding_helper.can_edit_mitigated_data(request.user),
+        )
 
         if form.is_valid():
             messages.add_message(request, messages.SUCCESS, "Note Saved.", extra_tags="alert-success")

dojo/forms.py

@@ -1917,15 +1917,15 @@ class CloseFindingForm(forms.ModelForm):
 
     def __init__(self, *args, **kwargs):
         queryset = kwargs.pop("missing_note_types")
+        # must pop custom kwargs before calling parent __init__ to avoid unexpected kwarg errors
+        self.can_edit_mitigated_data = kwargs.pop("can_edit_mitigated_data") if "can_edit_mitigated_data" in kwargs \
+            else False
         super().__init__(*args, **kwargs)
         if len(queryset) == 0:
             self.fields["note_type"].widget = forms.HiddenInput()
         else:
             self.fields["note_type"] = forms.ModelChoiceField(queryset=queryset, label="Note Type", required=True)
 
-        self.can_edit_mitigated_data = kwargs.pop("can_edit_mitigated_data") if "can_edit_mitigated_data" in kwargs \
-            else False
-
         if self.can_edit_mitigated_data:
             self.fields["mitigated_by"].queryset = get_authorized_users(Permissions.Finding_Edit)
             self.fields["mitigated"].initial = self.instance.mitigated

unittests/test_rest_framework.py

@@ -4,6 +4,7 @@
 import pathlib
 import re
 from collections import OrderedDict
+from datetime import timedelta
 from enum import Enum
 from json import dumps
 from pathlib import Path
@@ -14,6 +15,7 @@
 from django.contrib.auth.models import Permission
 from django.test import tag as test_tag
 from django.urls import reverse
+from django.utils import timezone
 from drf_spectacular.drainage import GENERATOR_STATS
 from drf_spectacular.settings import spectacular_settings
 from drf_spectacular.validation import validate_schema
@@ -919,6 +921,102 @@ def test_close_finding_pushes_note_to_jira_when_configured(self):
             self.assertTrue(add_comment_mock.called)
 
 
+class FindingCreateUpdateMitigatedFieldsAPITest(DojoAPITestCase):
+    fixtures = ["dojo_testdata.json"]
+
+    def setUp(self):
+        testuser = User.objects.get(username="admin")
+        token = Token.objects.get(user=testuser)
+        self.client = APIClient()
+        self.client.credentials(HTTP_AUTHORIZATION=f"Token {token.key}")
+        self.admin = testuser
+        self.base_url = "/api/v2/findings/"
+
+    def _minimal_create_payload(self, title: str):
+        return {
+            "test": 3,
+            "found_by": [],
+            "title": title,
+            "date": "2020-05-20",
+            "cwe": 1,
+            "severity": "High",
+            "description": "TEST finding",
+            "mitigation": "MITIGATION",
+            "impact": "HIGH",
+            "references": "",
+            "active": False,
+            "verified": False,
+            "false_p": False,
+            "duplicate": False,
+            "out_of_scope": False,
+            "under_review": False,
+            "under_defect_review": False,
+            "numerical_severity": "S0",
+        }
+
+    def test_create_rejects_mitigated_fields_by_default(self):
+        payload = self._minimal_create_payload("API create with mitigated disallowed")
+        past = (timezone.now() - timedelta(days=7)).replace(microsecond=0)
+        payload.update({
+            "is_mitigated": True,
+            "mitigated": past.isoformat(),
+            "mitigated_by": self.admin.id,
+        })
+
+        response = self.client.post(self.base_url, payload, format="json")
+        self.assertEqual(400, response.status_code, response.content[:1000])
+        self.assertIn("mitigated", response.data)
+        self.assertIn("mitigated_by", response.data)
+
+    def test_update_rejects_mitigated_fields_by_default(self):
+        finding_id = 7
+        past = (timezone.now() - timedelta(days=7)).replace(microsecond=0)
+        payload = {
+            "is_mitigated": True,
+            "mitigated": past.isoformat(),
+            "mitigated_by": self.admin.id,
+        }
+        response = self.client.patch(f"{self.base_url}{finding_id}/", payload, format="json")
+        self.assertEqual(400, response.status_code, response.content[:1000])
+        self.assertIn("mitigated", response.data)
+        self.assertIn("mitigated_by", response.data)
+
+    def test_create_sets_mitigated_fields_when_permitted(self):
+        with patch("dojo.finding.helper.can_edit_mitigated_data", return_value=True):
+            payload = self._minimal_create_payload("API create with mitigated allowed")
+            past = (timezone.now() - timedelta(days=7)).replace(microsecond=0)
+            payload.update({
+                "is_mitigated": True,
+                "mitigated": past.isoformat(),
+                "mitigated_by": self.admin.id,
+            })
+
+            response = self.client.post(self.base_url, payload, format="json")
+            self.assertEqual(201, response.status_code, response.content[:1000])
+
+            created_id = response.data.get("id")
+            self.assertIsNotNone(created_id)
+            created = Finding.objects.get(id=created_id)
+            self.assertEqual(created.mitigated.replace(microsecond=0, tzinfo=created.mitigated.tzinfo), past)
+            self.assertEqual(created.mitigated_by, self.admin)
+
+    def test_update_sets_mitigated_fields_when_permitted(self):
+        finding_id = 7
+        with patch("dojo.finding.helper.can_edit_mitigated_data", return_value=True):
+            past = (timezone.now() - timedelta(days=7)).replace(microsecond=0)
+            payload = {
+                "is_mitigated": True,
+                "mitigated": past.isoformat(),
+                "mitigated_by": self.admin.id,
+            }
+            response = self.client.patch(f"{self.base_url}{finding_id}/", payload, format="json")
+            self.assertEqual(200, response.status_code, response.content[:1000])
+
+            updated = Finding.objects.get(id=finding_id)
+            self.assertEqual(updated.mitigated.replace(microsecond=0, tzinfo=updated.mitigated.tzinfo), past)
+            self.assertEqual(updated.mitigated_by, self.admin)
+
+
 class EndpointStatusTest(BaseClass.BaseClassTest):
     fixtures = ["dojo_testdata.json"]