risk acceptance expiration: keep link with findings (#12737)

* risk acceptance expiration: keep link with findings

* push to jira on reinstate/expiry of Risk Acceptance

* move save call out of jira helper

* fix tests

* remve obsolete comment

Author: valentijnscholten

dojo/finding/views.py

@@ -1410,7 +1410,9 @@ def reopen_finding(request, fid):
         status.save()
     # Clear the risk acceptance, if present
     ra_helper.risk_unaccept(request.user, finding)
-    jira_helper.save_and_push_to_jira(finding)
+    finding.save(dedupe_option=False, push_to_jira=False)
+    if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+        jira_helper.push_to_jira(finding)
 
     reopen_external_issue(finding, "re-opened by defectdojo", "github")
 

dojo/jira_link/helper.py

@@ -142,6 +142,17 @@ def _safely_get_obj_status_for_jira(obj: Finding | Finding_Group, *, isenforced:
     return status or ["Inactive"]
 
 
+def is_keep_in_sync_with_jira(finding):
+    keep_in_sync_enabled = False
+    # Check if there is a jira issue that needs to be updated
+    jira_issue_exists = finding.has_jira_issue or (finding.finding_group and finding.finding_group.has_jira_issue)
+    if jira_issue_exists:
+        # Determine if any automatic sync should occur
+        keep_in_sync_enabled = get_jira_instance(finding).finding_jira_sync
+
+    return keep_in_sync_enabled
+
+
 # checks if a finding can be pushed to JIRA
 # optionally provides a form with the new data for the finding
 # any finding that already has a JIRA issue can be pushed again to JIRA
@@ -1765,7 +1776,14 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
     jira_instance = get_jira_instance(finding)
 
     if resolved:
-        if jira_instance and resolution_name in jira_instance.accepted_resolutions and (finding.test.engagement.product.enable_simple_risk_acceptance or finding.test.engagement.enable_full_risk_acceptance):
+        if (
+            jira_instance
+            and resolution_name in jira_instance.accepted_resolutions
+            and (
+            finding.test.engagement.product.enable_simple_risk_acceptance
+            or finding.test.engagement.enable_full_risk_acceptance
+            )
+        ):
             if not finding.risk_accepted:
                 logger.debug(f"Marking related finding of {jira_issue.jira_key} as accepted.")
                 finding.risk_accepted = True
@@ -1823,26 +1841,6 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
     return status_changed
 
 
-def save_and_push_to_jira(finding):
-    # Manage the jira status changes
-    push_to_jira_decision = False
-    # Determine if the finding is in a group. if so, not push to jira yet
-    finding_in_group = finding.has_finding_group
-    # Check if there is a jira issue that needs to be updated
-    jira_issue_exists = finding.has_jira_issue or (finding.finding_group and finding.finding_group.has_jira_issue)
-    # Only push if the finding is not in a group
-    if jira_issue_exists:
-        # Determine if any automatic sync should occur
-        push_to_jira_decision = is_push_all_issues(finding) \
-            or get_jira_instance(finding).finding_jira_sync
-    # Save the finding
-    finding.save(push_to_jira=(push_to_jira_decision and not finding_in_group))
-    # we only push the group after saving the finding to make sure
-    # the updated data of the finding is pushed as part of the group
-    if push_to_jira_decision and finding_in_group:
-        push_to_jira(finding.finding_group)
-
-
 def get_finding_group_findings_above_threshold(finding_group):
     """Get the findings that are above the minimum threshold"""
     jira_minimum_threshold = 0

dojo/risk_acceptance/helper.py

@@ -24,19 +24,25 @@ def expire_now(risk_acceptance):
         for finding in risk_acceptance.accepted_findings.all():
             if not finding.active:  # not sure why this is important
                 logger.debug("%i:%s: unaccepting/reactivating finding.", finding.id, finding)
+                finding.active = True
+                finding.risk_accepted = False
 
                 # Update any endpoint statuses on each of the findings
                 update_endpoint_statuses(finding, accept_risk=False)
-                risk_unaccept(None, finding, post_comments=False)  # comments will be posted at end
 
                 if risk_acceptance.restart_sla_expired:
                     finding.sla_start_date = timezone.now().date()
-                    finding.save(dedupe_option=False)  # resave if changed after risk_unaccept
+                # this method both saves and pushed to JIRA (no other post processing)
+                finding.save(dedupe_option=False)
+                if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+                    logger.info("pushing finding to JIRA after expiration of risk acceptance")
+                    jira_helper.push_to_jira(finding)
 
                 reactivated_findings.append(finding)
             else:
                 logger.debug("%i:%s already active, no changes made.", finding.id, finding)
 
+        # best effort JIRA integration, no status changes, just a comment
         post_jira_comments(risk_acceptance, risk_acceptance.accepted_findings.all(), expiration_message_creator)
 
     risk_acceptance.expiration_date = timezone.now()
@@ -68,12 +74,16 @@ def reinstate(risk_acceptance, old_expiration_date):
                 finding.risk_accepted = True
                 # Update any endpoint statuses on each of the findings
                 update_endpoint_statuses(finding, accept_risk=True)
+                # this method both saves and pushed to JIRA (no other post processing)
                 finding.save(dedupe_option=False)
+                if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+                    logger.info("pushing finding to JIRA after reinstating risk acceptance")
+                    jira_helper.push_to_jira(finding)
                 reinstated_findings.append(finding)
             else:
                 logger.debug("%i:%s: already inactive, not making any changes", finding.id, finding)
 
-        # best effort JIRA integration, no status changes
+        # best effort JIRA integration, no status changes, just a comment
         post_jira_comments(risk_acceptance, risk_acceptance.accepted_findings.all(), reinstation_message_creator)
 
     risk_acceptance.expiration_date_handled = None
@@ -108,7 +118,12 @@ def remove_finding_from_risk_acceptance(user: Dojo_User, risk_acceptance: Risk_A
     finding.risk_accepted = False
     # Update any endpoint statuses on each of the findings
     update_endpoint_statuses(finding, accept_risk=False)
+    # this method both saves and pushed to JIRA (no other post processing)
     finding.save(dedupe_option=False)
+    if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+        logger.info("pushing finding to JIRA after removal from risk acceptance")
+        jira_helper.push_to_jira(finding)
+
     # best effort jira integration, no status changes
     post_jira_comments(risk_acceptance, [finding], unaccepted_message_creator)
     # Add a note to reflect that the finding was removed from the risk acceptance
@@ -132,7 +147,13 @@ def add_findings_to_risk_acceptance(user: Dojo_User, risk_acceptance: Risk_Accep
             finding.save(dedupe_option=False)
             # Update any endpoint statuses on each of the findings
             update_endpoint_statuses(finding, accept_risk=True)
+
             risk_acceptance.accepted_findings.add(finding)
+
+            if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+                logger.info("pushing finding to JIRA after adding to risk acceptance")
+                jira_helper.push_to_jira(finding)
+
         # Add a note to reflect that the finding was removed from the risk acceptance
         if user is not None:
             finding.notes.add(Notes.objects.create(
@@ -314,6 +335,9 @@ def simple_risk_accept(user: Dojo_User, finding: Finding, *, perform_save=True)
         finding.save(dedupe_option=False)
     # post_jira_comment might reload from database so see unaccepted finding. but the comment
     # only contains some text so that's ok
+    if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+        jira_helper.push_to_jira(finding)
+
     post_jira_comment(finding, accepted_message_creator)
     # Add a note to reflect that the finding was removed from the risk acceptance
     if user is not None:
@@ -344,7 +368,8 @@ def risk_unaccept(user: Dojo_User, finding: Finding, *, perform_save=True, post_
             post_jira_comment(finding, unaccepted_message_creator)
 
         # Update the JIRA obect for this finding
-        jira_helper.save_and_push_to_jira(finding)
+        if jira_helper.is_push_all_issues(finding) or jira_helper.is_keep_in_sync_with_jira(finding):
+            jira_helper.push_to_jira(finding)
 
         # Add a note to reflect that the finding was removed from the risk acceptance
         if user is not None:

dojo/templates/dojo/view_eng.html

@@ -437,7 +437,7 @@ <h4> Risk Acceptance
                                                         Never
                                                     {% endif %}
                                                 </td>
-                                                <td>{{ risk_acceptance.accepted_findings_count }}</td>
+                                                <td><a href="{% url 'view_risk_acceptance' eng.id risk_acceptance.id %}">{{ risk_acceptance.accepted_findings_count }}</a></td>
                                                 {% if risk_acceptance.filename %}
                                                     <td><a href="{% url 'download_risk_acceptance' eng.id risk_acceptance.id %}">Yes</a>
                                                         &nbsp;<i style="position:absolute;" class="fa has-popover fa-info-circle" title="Uploaded proof" data-trigger="hover" data-placement="bottom" data-container="body" data-html="true"
@@ -717,7 +717,7 @@ <h4>Files<span class="pull-right">
         <div class="col-md-4">
             <div class="panel panel-default-secondary">
                 <div class="panel-heading">
-                    <h3 class="panel-title"><span class="fa-solid fa-circle-info fa-fw" aria-hidden="true"></span> 
+                    <h3 class="panel-title"><span class="fa-solid fa-circle-info fa-fw" aria-hidden="true"></span>
                         {% if eng.name %}
                             {{ eng.name }}
                         {% else %}
@@ -1040,25 +1040,25 @@ <h4><span class="fa-solid fa-key" aria-hidden="true"></span>
             $(document).on('keypress', null, 'e', function () {
                 window.location.assign('{% url 'edit_engagement' eng.id %}');
             });
-        
+
             $(document).on('keypress', null, 'a', function () {
                 window.location.assign('{% url 'add_tests' eng.id %}');
             });
-        
+
             $(document).on('keypress', null, 'i', function () {
                 window.location.assign('{% url 'import_scan_results' eng.id %}');
             });
-        
+
             $("a[data-toggle='collapse']").on('click', function () {
                 var i = $($(this).find('i').get(0));
                 i.toggleClass('glyphicon-chevron-up').toggleClass('glyphicon-chevron-down');
             });
-        
+
             //Ensures dropdown has proper zindex
             $('.table-responsive').on('show.bs.dropdown', function () {
             $('.table-responsive').css( "overflow", "inherit" );
             });
-        
+
             $('.table-responsive').on('hide.bs.dropdown', function () {
                 $('.table-responsive').css( "overflow", "auto" );
             })
@@ -1067,15 +1067,15 @@ <h4><span class="fa-solid fa-key" aria-hidden="true"></span>
                 var terms = '';
                 if ($.cookie('highlight')) {
                     terms = $.cookie('highlight').split(' ');
-                    
+
                     for (var i = 0; i < terms.length; i++) {
                         $('body').highlight(terms[i]);
                     }
                 }
-                
+
                 $('input#simple_search').val(terms);
             }
-        
+
             $('#shareQuestionnaireModal').on('show.bs.modal', function (event) {
             var button = $(event.relatedTarget) // Button that triggered the modal
             var path = button.data('whatever') // Extract info from data-* attributes
@@ -1088,8 +1088,8 @@ <h4><span class="fa-solid fa-key" aria-hidden="true"></span>
             modal.find('p#questionnaireURL').text('Questionnaire URL: ' + host + path)
             })
         });
-        
+
         {% include 'dojo/snippets/risk_acceptance_actions_snippet_js.html' %}
-        
+
     </script>
 {% endblock %}

unittests/test_jira_import_and_pushing_api.py

@@ -1,5 +1,6 @@
 # from unittest import skip
 import logging
+from unittest.mock import patch
 
 from crum import impersonate
 from django.urls import reverse
@@ -70,7 +71,6 @@ def setUp(self):
         self.testuser = User.objects.get(username="admin")
         self.testuser.usercontactinfo.block_execution = True
         self.testuser.usercontactinfo.save()
-
         token = Token.objects.get(user=self.testuser)
         self.client = APIClient()
         self.client.credentials(HTTP_AUTHORIZATION="Token " + token.key)
@@ -321,7 +321,7 @@ def add_risk_acceptance(self, eid, data_risk_accceptance, fid=None):
         self.assertEqual(302, response.status_code, response.content[:1000])
         return response
 
-    def test_import_grouped_reopen_expired_sla(self):
+    def test_import_grouped_reopen_expired_risk_acceptance(self):
         # steps
         # import scan, make sure they are in grouped JIRA
         # risk acceptance all the grouped findings, make sure they are closed in JIRA
@@ -374,6 +374,59 @@ def test_import_grouped_reopen_expired_sla(self):
         # by asserting full cassette is played we know all calls to JIRA have been made as expected
         self.assert_cassette_played()
 
+    @patch("dojo.decorators.we_want_async", return_value=False)
+    def test_import_grouped_reopen_expired_risk_acceptance_with_finding_sync(self, mock):
+        # steps
+        # import scan, make sure they are in grouped JIRA
+        # risk acceptance all the grouped findings, make sure they are closed in JIRA
+        # expire risk acceptance on all grouped findings, make sure they are open in JIRA
+        JIRA_Instance.objects.update(finding_jira_sync=True)
+
+        import0 = self.import_scan_with_params(self.npm_groups_sample_filename, scan_type="NPM Audit Scan", group_by="component_name+component_version", push_to_jira=True, verified=True)
+        test_id = import0["test"]
+        self.assert_jira_issue_count_in_test(test_id, 0)
+        self.assert_jira_group_issue_count_in_test(test_id, 3)
+        findings = self.get_test_findings_api(test_id)
+        finding_id = findings["results"][0]["id"]
+
+        ra_data = {
+            "name": "Accept: Unit test",
+            "accepted_findings": [],
+            "recommendation": "A",
+            "recommendation_details": "recommendation 1",
+            "decision": "A",
+            "decision_details": "it has been decided!",
+            "accepted_by": "pointy haired boss",
+            "owner": 1,
+            "expiration_date": "2024-12-31",
+            "reactivate_expired": True,
+            }
+
+        for finding in findings["results"]:
+            ra_data["accepted_findings"].append(finding["id"])
+
+        pre_jira_status = self.get_jira_issue_status(finding_id)
+
+        response = self.add_risk_acceptance(1, data_risk_accceptance=ra_data)
+        self.assertEqual("/engagement/1", response.url)
+
+        # we don't do any explicit push to JIRA here as it should happen automatically
+
+        post_jira_status = self.get_jira_issue_status(finding_id)
+        self.assertNotEqual(pre_jira_status, post_jira_status)
+
+        pre_jira_status = post_jira_status
+        ra = Risk_Acceptance.objects.last()
+        ra_helper.expire_now(ra)
+
+        # we don't do any explicit push to JIRA here as it should happen automatically
+
+        post_jira_status = self.get_jira_issue_status(finding_id)
+        self.assertNotEqual(pre_jira_status, post_jira_status)
+
+        # by asserting full cassette is played we know all calls to JIRA have been made as expected
+        self.assert_cassette_played()
+
     def test_import_with_groups_twice_push_to_jira(self):
         import0 = self.import_scan_with_params(self.npm_groups_sample_filename, scan_type="NPM Audit Scan", group_by="component_name+component_version", push_to_jira=True, verified=True)
         test_id = import0["test"]