close old findings: make test cases test default behaviour (#12842)

* reimport: test cases must use default value in api

* reimport: test cases must use default value in api

* update comment for UI default

* reimport: remove explicit close_old_findings params to use default

* import: rely on default close_old_findings False for API tests

Author: valentijnscholten

unittests/dojo_test_case.py

@@ -536,7 +536,7 @@ def get_results_by_id(self, results: list, object_id: int) -> dict | None:
         return None
 
     def import_scan_with_params(self, filename, scan_type="ZAP Scan", engagement=1, minimum_severity="Low", *, active=True, verified=False,
-                                push_to_jira=None, endpoint_to_add=None, tags=None, close_old_findings=False, group_by=None, engagement_name=None,
+                                push_to_jira=None, endpoint_to_add=None, tags=None, close_old_findings=None, group_by=None, engagement_name=None,
                                 product_name=None, product_type_name=None, auto_create_context=None, expected_http_status_code=201, test_title=None,
                                 scan_date=None, service=None, force_active=True, force_verified=True):
 
@@ -546,9 +546,11 @@ def import_scan_with_params(self, filename, scan_type="ZAP Scan", engagement=1,
                     "scan_type": scan_type,
                     "file": testfile,
                     "version": "1.0.1",
-                    "close_old_findings": close_old_findings,
             }
 
+            if close_old_findings is not None:
+                payload["close_old_findings"] = close_old_findings
+
             if active is not None:
                 payload["active"] = active
 
@@ -594,7 +596,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, service=None,
+                                  tags=None, close_old_findings=None, 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 = {
@@ -604,9 +606,11 @@ def reimport_scan_with_params(self, test_id, filename, scan_type="ZAP Scan", eng
                     "scan_type": scan_type,
                     "file": testfile,
                     "version": "1.0.1",
-                    "close_old_findings": close_old_findings,
             }
 
+            if close_old_findings is not None:
+                payload["close_old_findings"] = close_old_findings
+
             if test_id is not None:
                 payload["test"] = test_id
 

unittests/test_import_reimport.py

@@ -998,9 +998,42 @@ def test_import_0_reimport_3_active_verified(self):
         # - zap2 and zap5 closed
         self.assertEqual(notes_count_before + 2, self.db_notes_count())
 
+    # import 1 and then reimport 2 without closing old findings should result in old findings being closed as the default is True in serializers.py
+    # for the UI tests this also works as the default is True in the (re)import methods in this test class
+    # - reimport should mitigate the zap1
+    def test_import_reimport_without_close_old_findings(self):
+        # https://github.com/DefectDojo/django-DefectDojo/pull/12837
+        logger.debug("reimporting updated zap xml report and using default value for close_old_findings (True)")
+
+        import1 = self.import_scan_with_params(self.zap_sample1_filename)
+
+        test_id = import1["test"]
+        findings = self.get_test_findings_api(test_id)
+        self.assert_finding_count_json(4, findings)
+
+        with assertTestImportModelsCreated(self, reimports=1, affected_findings=2, closed=1, created=1, untouched=3):
+            reimport1 = self.reimport_scan_with_params(test_id, self.zap_sample2_filename)
+
+        test_id = reimport1["test"]
+        self.assertEqual(test_id, test_id)
+
+        findings = self.get_test_findings_api(test_id)
+        self.assert_finding_count_json(5, findings)
+
+        mitigated = 0
+        not_mitigated = 0
+        for finding in findings["results"]:
+            logger.debug(finding)
+            if finding["is_mitigated"]:
+                mitigated += 1
+            else:
+                not_mitigated += 1
+        self.assertEqual(mitigated, 1)
+        self.assertEqual(not_mitigated, 4)
+
     # import 1 and then reimport 2 without closing old findings
     # - reimport should not mitigate the zap1
-    def test_import_reimport_without_closing_old_findings(self):
+    def test_import_reimport_with_close_old_findings_false(self):
         logger.debug("reimporting updated zap xml report and keep old findings open")
 
         import1 = self.import_scan_with_params(self.zap_sample1_filename)
@@ -1015,12 +1048,9 @@ def test_import_reimport_without_closing_old_findings(self):
         test_id = reimport1["test"]
         self.assertEqual(test_id, test_id)
 
-        findings = self.get_test_findings_api(test_id, verified=False)
+        findings = self.get_test_findings_api(test_id)
         self.assert_finding_count_json(5, findings)
 
-        findings = self.get_test_findings_api(test_id, verified=True)
-        self.assert_finding_count_json(0, findings)
-
         mitigated = 0
         not_mitigated = 0
         for finding in findings["results"]:
@@ -1030,7 +1060,7 @@ def test_import_reimport_without_closing_old_findings(self):
             else:
                 not_mitigated += 1
         self.assertEqual(mitigated, 0)
-        self.assertEqual(not_mitigated, 0)
+        self.assertEqual(not_mitigated, 5)
 
     # some parsers generate 1 finding for each vulnerable file for each vulnerability
     # i.e
@@ -1211,7 +1241,7 @@ def test_import_6_reimport_6_gitlab_dep_scan_component_name_and_version(self):
     def test_import_param_close_old_findings_with_additional_endpoint(self):
         logger.debug("importing clair report with additional endpoint")
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
-            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, endpoint_to_add=1)
+            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, endpoint_to_add=1)
 
         test_id = import0["test"]
         test = self.get_test(test_id)
@@ -1229,19 +1259,51 @@ def test_import_param_close_old_findings_with_additional_endpoint(self):
             self.assertEqual(finding.endpoints.count(), 1)
             self.assertEqual(finding.endpoints.first().id, 1)
 
-        # reimport empty report
+        # reimport empty report to close old findings
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, closed=4):
             self.import_scan_with_params(self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, endpoint_to_add=1)
 
         # all findings from import0 should be closed now
         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, 0)
 
+    # import clair scan, testing:
+    # parameter endpoint_to_add: each imported finding should be related to endpoint with id=1
+    # close_old_findings functionality: secony (empty) import should not close all findings from the first import as we rely on the default value of close_old_findings=False
+    def test_import_param_close_old_findings_default_with_additional_endpoint(self):
+        logger.debug("importing clair report with additional endpoint")
+        with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
+            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, endpoint_to_add=1)
+
+        test_id = import0["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 the engagement
+        engagement_findings = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
+        self.assertEqual(engagement_findings.count(), 4)
+
+        # findings should have only one endpoint, added with endpoint_to_add
+        for finding in engagement_findings:
+            self.assertEqual(finding.endpoints.count(), 1)
+            self.assertEqual(finding.endpoints.first().id, 1)
+
+        # reimport empty report with no explicit close_old_findings parameter should not close old findings
+        with assertTestImportModelsCreated(self, imports=1, affected_findings=0, closed=0):
+            self.import_scan_with_params(self.clair_empty, scan_type=self.scan_type_clair, endpoint_to_add=1)
+
+        # all findings from import0 should be closed now
+        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 (empty) import should close all findings from the first import when setting the same service
     def test_import_param_close_old_findings_with_same_service(self):
         logger.debug("importing clair report with same service")
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
-            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, service="service_1")
+            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, service="service_1")
 
         test_id = import0["test"]
         test = self.get_test(test_id)
@@ -1254,7 +1316,7 @@ def test_import_param_close_old_findings_with_same_service(self):
         engagement_findings = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
         self.assertEqual(engagement_findings.count(), 4)
 
-        # reimport empty report
+        # reimport empty report to close old findings
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, closed=4):
             self.import_scan_with_params(self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, service="service_1")
 
@@ -1266,7 +1328,7 @@ def test_import_param_close_old_findings_with_same_service(self):
     def test_import_param_close_old_findings_with_different_services(self):
         logger.debug("importing clair report with different services")
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
-            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, service="service_1")
+            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, service="service_1")
 
         test_id = import0["test"]
         test = self.get_test(test_id)
@@ -1279,7 +1341,7 @@ def test_import_param_close_old_findings_with_different_services(self):
         engagement_findings = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
         self.assertEqual(engagement_findings.count(), 4)
 
-        # reimport empty report
+        # reimport empty report to close old findings
         with assertTestImportModelsCreated(self, imports=1, affected_findings=0, closed=0):
             self.import_scan_with_params(self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, service="service_2")
 
@@ -1290,7 +1352,7 @@ def test_import_param_close_old_findings_with_different_services(self):
     # close_old_findings functionality: second (empty) import should not close findings from the first import when setting a service in the first import but none in the second import
     def test_import_param_close_old_findings_with_and_without_service_1(self):
         with assertTestImportModelsCreated(self, imports=1, affected_findings=4, created=4):
-            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, close_old_findings=True, service="service_1")
+            import0 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, service="service_1")
 
         test_id = import0["test"]
         test = self.get_test(test_id)
@@ -1303,7 +1365,7 @@ def test_import_param_close_old_findings_with_and_without_service_1(self):
         engagement_findings = Finding.objects.filter(test__engagement_id=1, test__test_type=test.test_type, active=True, is_mitigated=False)
         self.assertEqual(engagement_findings.count(), 4)
 
-        # reimport empty report
+        # reimport empty report to close old findings
         with assertTestImportModelsCreated(self, imports=1, affected_findings=0, closed=0):
             self.import_scan_with_params(self.clair_empty, scan_type=self.scan_type_clair, close_old_findings=True, service=None)
 
@@ -1339,7 +1401,7 @@ def test_import_param_close_old_findings_with_and_without_service_2(self):
     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")
+            import1 = self.import_scan_with_params(self.clair_few_findings, scan_type=self.scan_type_clair, engagement=1, service="service_A")
 
         test_id = import1["test"]
         test = self.get_test(test_id)
@@ -1353,7 +1415,7 @@ def test_reimport_close_old_findings_different_engagements_different_services(se
         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")
+        self.reimport_scan_with_params(test_id, self.clair_few_findings, scan_type=self.scan_type_clair, 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)
@@ -1365,7 +1427,7 @@ def test_reimport_close_old_findings_different_engagements_different_services(se
             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")
+        self.reimport_scan_with_params(test_id, self.clair_empty, scan_type=self.scan_type_clair, 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)
@@ -1383,7 +1445,7 @@ def test_reimport_close_old_findings_different_engagements_different_services(se
             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")
+        self.reimport_scan_with_params(test_id, self.clair_empty, scan_type=self.scan_type_clair, 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)
@@ -1395,7 +1457,7 @@ def test_reimport_close_old_findings_different_engagements_different_services(se
             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")
+        self.reimport_scan_with_params(test_id, self.clair_few_findings, scan_type=self.scan_type_clair, 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)
@@ -1954,7 +2016,10 @@ 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, service=None):
+    # For UI tests we cannot rely on the default for close_old_findings True as when we leave out the field in the request,
+    # Django (or rathet HTML FORM spec) will interpret that as False. So we explicitly set it to True here.
+    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: