bugfix: reimport: close_old_findings must respect service field (#12782)

* reimport: close_old_findings must respect service field

* reimport: close_old_findings must respect service field

* close_old_findings: update docs and help texts

* typo

* reimport docs tweak

* reimport: assert that reopen respect service field

Author: valentijnscholten

docs/content/en/about_defectdojo/faq.md

@@ -48,7 +48,7 @@ There are two different methods to import a report from a security tool into Def
 - **Import** handles the report as a single point-in-time record.  Importing a report creates a Test within DefectDojo that holds the Findings rendered from that report.
 - **Reimport** is used to extend an existing Test.  If you have a more open-ended approach to your testing process, you continuously Reimport the latest version of your report to an existing Test.  DefectDojo will compare the results of the incoming report to your existing data, record any changes, and then adjust the Findings in the Test so that they match the latest report.
 
-Both methods also use **Deduplication** differently: while two discrete Imported Tests in the same Product will identify and label duplicate Findings, Reimport will discard duplicate Findings altogether.
+Both methods also use **Deduplication** differently: while two discrete Imported Tests in the same Product will identify and label duplicate Findings, Reimport will skip duplicates in uploaded reports as theses Findings already exist in Defect Dojo.
 
 Generally speaking - if a point-in-time report is what you need, Import is the best method to use.  If you are continuously running and ingesting reports from a tool, Reimport is the better method for keeping things organized.
 

docs/content/en/open_source/archived_docs/usage/features.md

@@ -386,15 +386,12 @@ details about the deduplication process : switch
 
 ### Deduplication - APIv2 parameters
 
-- `close_old_findings` : if true, findings that are not
-    duplicates and that were in the previous scan of the same type
-    (example ZAP) for the same engagement (or product in case of
-    \"close_old_findings_product_scope\") and that are not present in the new
-    scan are closed (Inactive, Verified, Mitigated). 
-- `close_old_findings_product_scope` : if true, close_old_findings applies
-    to all findings of the same type in the product. Note that
-    \"Deduplication on engagement\" is no longer used to determine the
-    scope of close_old_findings.
+| Parameter | Import behaviour | Reimport Behaviour |
+|-----------|------------------|-------------------|
+| `close_old_findings` | if `true`, findings that are not duplicates and that were in the previous scan of the same type (example ZAP) for the same **engagement** (or product in case of `close_old_findings_product_scope`) and that are not present in the new scan are closed (`Inactive`, `Verified`, `Mitigated`). | if `true`, findings that that are in the same **test** and that are not present in the new scan are closed (`Inactive`, `Verified`, `Mitigated`) |
+| `close_old_findings_product_scope` | if true, `close_old_findings` applies to all findings of the same type in the whole **product**. Note that "Deduplication on engagement" is no longer used to determine the scope of `close_old_findings` | has no effect | 
+
+The `close_old_findings` feature will respect the value of the `service` field to only close findings with an identical `service` value.
 
 ### Deduplication / Similar findings
 

dojo/api_v2/serializers.py

@@ -2302,14 +2302,17 @@ class ImportScanSerializer(CommonImportScanSerializer):
     close_old_findings = serializers.BooleanField(
         required=False,
         default=False,
-        help_text="Select if old findings no longer present in the report get closed as mitigated when importing. "
-        "If service has been set, only the findings for this service will be closed.",
+        help_text="Old findings no longer present in the new report get closed as mitigated when importing. "
+                    "If service has been set, only the findings for this service will be closed. "
+                    "This only affects findings within the same engagement.",
     )
     close_old_findings_product_scope = serializers.BooleanField(
         required=False,
         default=False,
-        help_text="Select if close_old_findings applies to all findings of the same type in the product. "
-        "By default, it is false meaning that only old findings of the same type in the engagement are in scope.",
+        help_text="Old findings no longer present in the new report get closed as mitigated when importing. "
+                    "If service has been set, only the findings for this service will be closed. "
+                    "This only affects findings within the same product."
+                    "By default, it is false meaning that only old findings of the same type in the engagement are in scope.",
     )
     version = serializers.CharField(
         required=False, help_text="Version that was scanned.",
@@ -2380,15 +2383,15 @@ class ReImportScanSerializer(CommonImportScanSerializer):
     # also for ReImport.
     close_old_findings = serializers.BooleanField(
         required=False,
-        default=True,
-        help_text="Select if old findings no longer present in the report get closed as mitigated when importing.",
+        default=False,
+        help_text="Old findings no longer present in the new report get closed as mitigated when importing. "
+                    "If service has been set, only the findings for this service will be closed. "
+                    "This only affects findings within the same test.",
     )
     close_old_findings_product_scope = serializers.BooleanField(
         required=False,
         default=False,
-        help_text="Select if close_old_findings applies to all findings of the same type in the product. "
-        "By default, it is false meaning that only old findings of the same type in the engagement are in scope. "
-        "Note that this only applies on the first call to reimport-scan.",
+        help_text="This has no effect on reimport",
     )
     version = serializers.CharField(
         required=False,

dojo/forms.py

@@ -651,7 +651,7 @@ class ReImportScanForm(forms.Form):
         label="Choose report file",
         allow_empty_file=True,
         required=False)
-    close_old_findings = forms.BooleanField(help_text="Select if old findings no longer present in the report get closed as mitigated when importing.",
+    close_old_findings = forms.BooleanField(help_text="Select if old findings in the same test that are no longer present in the report get closed as mitigated when importing.",
                                             required=False, initial=True)
     version = forms.CharField(max_length=100, required=False, help_text="Version that will be set on existing Test object. Leave empty to leave existing value in place.")
     branch_tag = forms.CharField(max_length=100, required=False, help_text="Branch or Tag that was scanned.")

dojo/importers/default_reimporter.py

@@ -162,7 +162,15 @@ def process_findings(
         at import time
         """
         self.deduplication_algorithm = self.determine_deduplication_algorithm()
-        self.original_items = list(self.test.finding_set.all())
+        # Only process findings with the same service value (or None)
+        # Even though the service values is used in the hash_code calculation,
+        # we need to make sure there are no side effects such as closing findings
+        # for findings with a different service value
+        # https://github.com/DefectDojo/django-DefectDojo/issues/12754
+        original_findings = self.test.finding_set.all().filter(service=self.service)
+        logger.debug(f"original_findings_qyer: {original_findings.query}")
+        self.original_items = list(original_findings)
+        logger.debug(f"original_items: {[(item.id, item.hash_code) for item in self.original_items]}")
         self.new_items = []
         self.reactivated_items = []
         self.unchanged_items = []

dojo/importers/endpoint_manager.py

@@ -52,7 +52,7 @@ def add_endpoints_to_unsaved_finding(
                 finding=finding,
                 endpoint=ep,
                 defaults={"date": finding.date})
-        logger.debug(f"IMPORT_SCAN: {len(endpoints)} imported")
+        logger.debug(f"IMPORT_SCAN: {len(endpoints)} endpoints imported")
         return
 
     @dojo_async_task

unittests/dojo_test_case.py

@@ -594,7 +594,7 @@ def import_scan_with_params(self, filename, scan_type="ZAP Scan", engagement=1,
             return self.import_scan(payload, expected_http_status_code)
 
     def reimport_scan_with_params(self, test_id, filename, scan_type="ZAP Scan", engagement=1, minimum_severity="Low", *, active=True, verified=False, push_to_jira=None,
-                                  tags=None, close_old_findings=True, group_by=None, engagement_name=None, scan_date=None,
+                                  tags=None, close_old_findings=True, group_by=None, engagement_name=None, scan_date=None, service=None,
                                   product_name=None, product_type_name=None, auto_create_context=None, expected_http_status_code=201, test_title=None):
         with Path(filename).open(encoding="utf-8") as testfile:
             payload = {
@@ -640,6 +640,9 @@ def reimport_scan_with_params(self, test_id, filename, scan_type="ZAP Scan", eng
             if scan_date is not None:
                 payload["scan_date"] = scan_date
 
+            if service is not None:
+                payload["service"] = service
+
             return self.reimport_scan(payload, expected_http_status_code=expected_http_status_code)
 
     def endpoint_meta_import_scan_with_params(self, filename, product=1, product_name=None, *,

unittests/test_import_reimport.py

@@ -1335,6 +1335,83 @@ def test_import_param_close_old_findings_with_and_without_service_2(self):
         engagement_findings_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False).count()
         self.assertEqual(engagement_findings_count, 4)
 
+    # close_old_findings functionality: second import to different engagement with different service should not close findings from the first engagement
+    def test_reimport_close_old_findings_different_engagements_different_services(self):
+        logger.debug("importing clair report with service A into engagement 1")
+        with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
+            import1 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, engagement=1, close_old_findings=True, service="service_A")
+
+        test_id = import1["test"]
+        test = self.get_test(test_id)
+        findings = self.get_test_findings_api(test_id)
+        self.log_finding_summary_json_api(findings)
+        # imported count must match count in the report
+        self.assert_finding_count_json(4, findings)
+
+        # imported findings should be active in engagement 1
+        engagement1_findings = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement1_findings.count(), 4)
+
+        # reimporting the same report into the same test with a different service should not close any findings and create 4 new findings
+        self.reimport_scan_with_params(test_id, self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, service="service_B")
+
+        engagement1_active_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement1_active_finding_count.count(), 8)
+        engagement1_mitigated_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=False, is_mitigated=True)
+        self.assertEqual(engagement1_mitigated_finding_count.count(), 0)
+        # verify findings from engagement 1 are still the same (not mitigated/closed)
+        for finding in engagement1_active_finding_count:
+            self.assertTrue(finding.active)
+            self.assertFalse(finding.is_mitigated)
+
+        # reimporting an empty report with service A should close all findings from the first import, but not the reimported ones with service B
+        self.reimport_scan_with_params(test_id, self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, service="service_A")
+
+        engagement1_active_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement1_active_finding_count.count(), 4)
+        engagement1_mitigated_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=False, is_mitigated=True)
+        self.assertEqual(engagement1_mitigated_finding_count.count(), 4)
+
+        for finding in engagement1_active_finding_count:
+            self.assertTrue(finding.active)
+            self.assertFalse(finding.is_mitigated)
+            self.assertEqual(finding.service, "service_B")
+
+        for finding in engagement1_mitigated_finding_count:
+            self.assertFalse(finding.active)
+            self.assertTrue(finding.is_mitigated)
+            self.assertEqual(finding.service, "service_A")
+
+        # reimporting an empty report with service B should close all findings from the second  import, and not reopen any findings
+        self.reimport_scan_with_params(test_id, self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, service="service_B")
+
+        engagement1_active_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement1_active_finding_count.count(), 0)
+        engagement1_mitigated_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=False, is_mitigated=True)
+        self.assertEqual(engagement1_mitigated_finding_count.count(), 8)
+
+        for finding in engagement1_mitigated_finding_count:
+            self.assertFalse(finding.active)
+            self.assertTrue(finding.is_mitigated)
+
+        # reimporting a report with findings and service A should reopen the 4findings with service_A but leave the findings with service_B closed.
+        self.reimport_scan_with_params(test_id, self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, service="service_A")
+
+        engagement1_active_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement1_active_finding_count.count(), 4)
+        engagement1_mitigated_finding_count = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=False, is_mitigated=True)
+        self.assertEqual(engagement1_mitigated_finding_count.count(), 4)
+
+        for finding in engagement1_active_finding_count:
+            self.assertTrue(finding.active)
+            self.assertFalse(finding.is_mitigated)
+            self.assertEqual(finding.service, "service_A")
+
+        for finding in engagement1_mitigated_finding_count:
+            self.assertFalse(finding.active)
+            self.assertTrue(finding.is_mitigated)
+            self.assertEqual(finding.service, "service_B")
+
     def test_import_reimport_generic(self):
         """
         This test do a basic import and re-import of a generic JSON report
@@ -1873,7 +1950,7 @@ def import_scan_with_params_ui(self, filename, scan_type="ZAP Scan", engagement=
 
             return self.import_scan_ui(engagement, payload)
 
-    def reimport_scan_with_params_ui(self, test_id, filename, scan_type="ZAP Scan", minimum_severity="Low", *, active=True, verified=False, push_to_jira=None, tags=None, close_old_findings=True, scan_date=None):
+    def reimport_scan_with_params_ui(self, test_id, filename, scan_type="ZAP Scan", minimum_severity="Low", *, active=True, verified=False, push_to_jira=None, tags=None, close_old_findings=True, scan_date=None, service=None):
         # Mimic old functionality for active/verified to avoid breaking tests
         activePayload = "force_to_true"
         if not active:
@@ -1902,6 +1979,9 @@ def reimport_scan_with_params_ui(self, test_id, filename, scan_type="ZAP Scan",
             if scan_date is not None:
                 payload["scan_date"] = scan_date
 
+            if service is not None:
+                payload["service"] = service
+
             return self.reimport_scan_ui(test_id, payload)
 
 # Observations: