fix(risk_acceptance): stop the expiration job aborting on an unattached risk acceptance (#15376)

* fix(risk_acceptance): stop the expiration job aborting on an unattached risk acceptance

`expiration_handler` raised `AttributeError: 'NoneType' object has no attribute
'product'` and aborted the whole run whenever it reached a risk acceptance that no
engagement points at. `Risk_Acceptance.engagement` is a property over a many-to-many
used as a one-to-many, so it returns None in that state, and both halves of the job
dereferenced it unconditionally while building the notification.

Impact was not limited to the one bad row: every risk acceptance later in the batch was
left unhandled, and because `expiration_date_warned` is only set after the notification,
the same risk acceptance was re-selected and failed the job again on every subsequent run.

Resolve the engagement once through `engagement_to_notify_about()`, which logs and
returns None when there is none, and skip only the notification in that case. The expiry
itself, the finding reactivation and the JIRA comments still happen, and the heads-up
path now marks the risk acceptance as warned either way so it stops being re-selected.

Adds two regression tests covering the warning path and the expiry path; both reproduce
the reported traceback before the change.

* fix(risk_acceptance): derive the notification engagement from the accepted findings

The previous commit stopped the crash but skipped the expiration notification whenever
`Risk_Acceptance.engagement` was empty. That is too blunt: an empty engagement relation
is a normal state for a populated risk acceptance, not a sign there is nobody to notify.

`RiskAcceptanceSerializer.create` only attaches an engagement when the risk acceptance
already has findings; `_accept_risks` and the SonarQube status sync never attach one;
and the Pro plugin tracks the association on its own model rather than on
`Engagement.risk_acceptance`. Skipping on an empty relation would therefore have dropped
expiration notifications for whole classes of risk acceptance that have findings, a
product, and users expecting to hear about them.

Fall back to the engagement of an accepted finding instead - the same derivation
`RiskAcceptanceSerializer` already uses to attach an engagement on create and to repair
an orphan on update. Only a risk acceptance with neither an engagement nor findings is
skipped, because the notification names a product and links to a per-engagement URL and
neither can be produced from an empty one.

Tests now cover the three cases separately: engagement link missing but findings present
(notification still sent, addressed via the derived engagement, asserted on both the
engagement and the product), no engagement and no findings (skipped, still marked warned
so it is not re-selected), and the same on the expiry half. All three fail against
unmodified `bugfix` at helper.py:54 and helper.py:203.

---------

Co-authored-by: DefectDojo Bot <bot@defectdojo.com>

Author: Maffooch

dojo/risk_acceptance/helper.py

@@ -9,13 +9,46 @@
 
 from dojo.celery import app
 from dojo.jira import services as jira_services
-from dojo.models import Dojo_User, Finding, Notes, Risk_Acceptance, System_Settings
+from dojo.models import Dojo_User, Engagement, Finding, Notes, Risk_Acceptance, System_Settings
 from dojo.notifications.helper import create_notification
 from dojo.utils import get_full_url, get_system_setting
 
 logger = logging.getLogger(__name__)
 
 
+def engagement_to_notify_about(risk_acceptance: Risk_Acceptance) -> Engagement | None:
+    """
+    Return an engagement to address a risk acceptance's expiration notification to.
+
+    `Risk_Acceptance.engagement` is a property over `Engagement.risk_acceptance`, a
+    many-to-many used as a one-to-many. It is empty for a risk acceptance that was not
+    created from an engagement page, which is a supported state rather than corrupt data:
+    `RiskAcceptanceSerializer.create` only attaches an engagement when the risk acceptance
+    already has findings, `_accept_risks` and the SonarQube status sync never attach one at
+    all, and the Pro plugin tracks the association on its own model instead of this relation.
+
+    So fall back to the engagement of an accepted finding - the same derivation
+    `RiskAcceptanceSerializer` uses both to attach an engagement on create and to repair an
+    orphan on update. Only a risk acceptance with neither an engagement nor any findings has
+    nothing to notify about; the notification names a product and links to a per-engagement
+    URL, and neither can be produced from an empty risk acceptance.
+    """
+    engagement = risk_acceptance.engagement
+    if engagement is not None:
+        return engagement
+
+    # iterate rather than .first(), to reuse the accepted_findings prefetch
+    accepted_finding = next(iter(risk_acceptance.accepted_findings.all()), None)
+    if accepted_finding is not None:
+        return accepted_finding.test.engagement
+
+    logger.warning(
+        "risk acceptance %i:%s has no engagement and no findings, skipping its expiration notification",
+        risk_acceptance.id, risk_acceptance,
+    )
+    return None
+
+
 def expire_now(risk_acceptance):
     logger.info("Expiring risk acceptance %i:%s with %i findings", risk_acceptance.id, risk_acceptance, len(risk_acceptance.accepted_findings.all()))
 
@@ -49,14 +82,19 @@ def expire_now(risk_acceptance):
     risk_acceptance.expiration_date_handled = timezone.now()
     risk_acceptance.save()
 
+    # the expiry above is the job here, the notification below is best effort
+    engagement = engagement_to_notify_about(risk_acceptance)
+    if engagement is None:
+        return
+
     accepted_findings = risk_acceptance.accepted_findings.all()
     title = "Risk acceptance with " + str(len(accepted_findings)) + " accepted findings has expired for " + \
-            str(risk_acceptance.engagement.product) + ": " + str(risk_acceptance.engagement.name)
+            str(engagement.product) + ": " + str(engagement.name)
 
     create_notification(event="risk_acceptance_expiration", title=title, risk_acceptance=risk_acceptance, accepted_findings=accepted_findings,
-                         reactivated_findings=reactivated_findings, engagement=risk_acceptance.engagement,
-                         product=risk_acceptance.engagement.product,
-                         url=reverse("view_risk_acceptance", args=(risk_acceptance.engagement.id, risk_acceptance.id)))
+                         reactivated_findings=reactivated_findings, engagement=engagement,
+                         product=engagement.product,
+                         url=reverse("view_risk_acceptance", args=(engagement.id, risk_acceptance.id)))
 
 
 def reinstate(risk_acceptance, old_expiration_date):
@@ -198,17 +236,21 @@ def expiration_handler(*args, **kwargs):
             for risk_acceptance in risk_acceptances:
                 logger.debug("notifying for risk acceptance %i:%s with %i findings", risk_acceptance.id, risk_acceptance, len(risk_acceptance.accepted_findings.all()))
 
-                notification_title = "Risk acceptance with " + str(len(risk_acceptance.accepted_findings.all())) + " accepted findings will expire on " + \
-                    timezone.localtime(risk_acceptance.expiration_date).strftime("%b %d, %Y") + " for " + \
-                    str(risk_acceptance.engagement.product) + ": " + str(risk_acceptance.engagement.name)
+                engagement = engagement_to_notify_about(risk_acceptance)
+                if engagement is not None:
+                    notification_title = "Risk acceptance with " + str(len(risk_acceptance.accepted_findings.all())) + " accepted findings will expire on " + \
+                        timezone.localtime(risk_acceptance.expiration_date).strftime("%b %d, %Y") + " for " + \
+                        str(engagement.product) + ": " + str(engagement.name)
 
-                create_notification(event="risk_acceptance_expiration", title=notification_title, risk_acceptance=risk_acceptance,
-                                    accepted_findings=risk_acceptance.accepted_findings.all(), engagement=risk_acceptance.engagement,
-                                    product=risk_acceptance.engagement.product,
-                                    url=reverse("view_risk_acceptance", args=(risk_acceptance.engagement.id, risk_acceptance.id)))
+                    create_notification(event="risk_acceptance_expiration", title=notification_title, risk_acceptance=risk_acceptance,
+                                        accepted_findings=risk_acceptance.accepted_findings.all(), engagement=engagement,
+                                        product=engagement.product,
+                                        url=reverse("view_risk_acceptance", args=(engagement.id, risk_acceptance.id)))
 
                 post_jira_comments(risk_acceptance, risk_acceptance.accepted_findings.all(), expiration_warning_message_creator, heads_up_days)
 
+                # marked as warned either way, so a risk acceptance nobody can be notified about
+                # is not re-selected on every subsequent run
                 risk_acceptance.expiration_date_warned = timezone.now()
                 risk_acceptance.save()
 

unittests/test_risk_acceptance.py

@@ -1,6 +1,7 @@
 import copy
 import datetime
 import logging
+from unittest.mock import patch
 
 from dateutil.relativedelta import relativedelta
 from django.db.models import Q
@@ -299,3 +300,150 @@ def test_expiration_handler(self):
         # after handling no ra should be select for anything
         self.assertFalse(any(ra in to_warn for ra in [ra1, ra2, ra3]))
         self.assertFalse(any(ra in to_expire for ra in [ra1, ra2, ra3]))
+
+    def detach_from_engagements(self, ra):
+        """
+        Leave a risk acceptance without an engagement.
+
+        Not corrupt data: `RiskAcceptanceSerializer.create` only attaches an engagement when
+        the risk acceptance already has findings, and the Pro plugin tracks the association on
+        its own model rather than on `Engagement.risk_acceptance`.
+        """
+        for engagement in ra.engagement_set.all():
+            engagement.risk_acceptance.remove(ra)
+        ra.refresh_from_db()
+        self.assertIsNone(ra.engagement, msg="setup failed: risk acceptance is still attached to an engagement")
+
+    def notified_risk_acceptances(self, mock_create_notification):
+        return [call.kwargs.get("risk_acceptance") for call in mock_create_notification.call_args_list]
+
+    # Regression: expiration_handler aborted the entire run with
+    # "AttributeError: 'NoneType' object has no attribute 'product'" as soon as it reached a
+    # risk acceptance not attached to any engagement, so every remaining risk acceptance in the
+    # batch was left unhandled and the job failed again on every subsequent run.
+    def test_expiration_handler_warns_via_findings_when_engagement_link_is_missing(self):
+        ra1, ra2, ra3 = self.create_multiple_ras()
+        system_settings = System_Settings.objects.get()
+        system_settings.risk_acceptance_notify_before_expiration = 10
+        system_settings.save()
+        heads_up_days = system_settings.risk_acceptance_notify_before_expiration
+
+        # both are inside the heads-up window, ra1 (created first, so handled first) has no engagement
+        ra1.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days - 1)
+        ra2.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days - 1)
+        ra3.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days + 1)
+        ra1.save()
+        ra2.save()
+        ra3.save()
+        expected_engagement = ra1.accepted_findings.first().test.engagement
+        self.detach_from_engagements(ra1)
+
+        with patch("dojo.risk_acceptance.helper.create_notification") as mock_create_notification:
+            ra_helper.expiration_handler()
+
+        ra1.refresh_from_db()
+        ra2.refresh_from_db()
+
+        # the missing link is recovered from the accepted findings rather than dropping the notification
+        notified = self.notified_risk_acceptances(mock_create_notification)
+        self.assertIn(ra1, notified, msg=f"risk acceptance with findings but no engagement was not notified about, got {notified}")
+        self.assertIn(ra2, notified, msg=f"expected a notification for the attached risk acceptance, got {notified}")
+
+        ra1_call = next(c for c in mock_create_notification.call_args_list if c.kwargs.get("risk_acceptance") == ra1)
+        self.assertEqual(
+            ra1_call.kwargs.get("engagement"), expected_engagement,
+            msg=f"expected engagement={expected_engagement}, notified with {ra1_call.kwargs.get('engagement')}",
+        )
+        self.assertEqual(
+            ra1_call.kwargs.get("product"), expected_engagement.product,
+            msg=f"expected product={expected_engagement.product}, notified with {ra1_call.kwargs.get('product')}",
+        )
+
+        self.assertIsNotNone(ra1.expiration_date_warned, msg="risk acceptance with no engagement link was never warned")
+        self.assertIsNotNone(ra2.expiration_date_warned, msg="attached risk acceptance was never warned")
+
+    # Regression: the shape produced by the Pro standalone "New Risk Acceptance" page, which
+    # pre-fills an expiration date and posts no findings, so the risk acceptance is selected by
+    # the heads-up query while having neither an engagement nor anything to derive one from.
+    def test_expiration_handler_skips_risk_acceptance_with_no_engagement_and_no_findings(self):
+        ra1, ra2, ra3 = self.create_multiple_ras()
+        system_settings = System_Settings.objects.get()
+        system_settings.risk_acceptance_notify_before_expiration = 10
+        system_settings.save()
+        heads_up_days = system_settings.risk_acceptance_notify_before_expiration
+
+        for ra in (ra1, ra2, ra3):
+            ra.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days + 1)
+            ra.save()
+
+        empty_ra = Risk_Acceptance.objects.create(
+            name="Accept: no findings, no engagement",
+            owner=self.get_test_admin(),
+            expiration_date=datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days - 1),
+        )
+        attached_ra = ra1
+        attached_ra.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days - 1)
+        attached_ra.save()
+        self.assertIsNone(empty_ra.engagement, msg="setup failed: expected no engagement")
+        self.assertFalse(empty_ra.accepted_findings.exists(), msg="setup failed: expected no accepted findings")
+        self.assertIn(
+            empty_ra, ra_helper.get_almost_expired_risk_acceptances_to_handle(heads_up_days=heads_up_days),
+            msg="setup failed: the empty risk acceptance is not selected by the heads-up query",
+        )
+
+        with patch("dojo.risk_acceptance.helper.create_notification") as mock_create_notification:
+            ra_helper.expiration_handler()
+
+        empty_ra.refresh_from_db()
+        attached_ra.refresh_from_db()
+
+        # nothing to name a product with, so no notification - but the job must not die over it
+        notified = self.notified_risk_acceptances(mock_create_notification)
+        self.assertNotIn(empty_ra, notified, msg="a risk acceptance with no findings has no product to notify about")
+        self.assertIn(attached_ra, notified, msg=f"the rest of the batch was abandoned, notified: {notified}")
+
+        self.assertIsNotNone(
+            empty_ra.expiration_date_warned,
+            msg="empty risk acceptance stays unwarned and is re-selected, failing the job again on every run",
+        )
+        self.assertIsNotNone(attached_ra.expiration_date_warned, msg="attached risk acceptance was never warned")
+
+    # Regression: same root cause, on the expiry half of the job.
+    def test_expiration_handler_expires_risk_acceptance_without_engagement_link(self):
+        ra1, ra2, ra3 = self.create_multiple_ras()
+        system_settings = System_Settings.objects.get()
+        system_settings.risk_acceptance_notify_before_expiration = 10
+        system_settings.save()
+        heads_up_days = system_settings.risk_acceptance_notify_before_expiration
+
+        # ra3 is past its expiration date and has no engagement, ra1 is only due a heads-up
+        ra1.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days - 1)
+        ra2.expiration_date = datetime.datetime.now(datetime.UTC) + relativedelta(days=heads_up_days + 1)
+        ra3.expiration_date = datetime.datetime.now(datetime.UTC) - relativedelta(days=5)
+        ra1.save()
+        ra2.save()
+        ra3.save()
+        findings = list(ra3.accepted_findings.all())
+        expected_engagement = ra3.accepted_findings.first().test.engagement
+        self.detach_from_engagements(ra3)
+
+        with patch("dojo.risk_acceptance.helper.create_notification") as mock_create_notification:
+            ra_helper.expiration_handler()
+
+        ra1.refresh_from_db()
+        ra3.refresh_from_db()
+
+        # the expiry happens and the notification still goes out, addressed via the findings
+        self.assertIsNotNone(ra3.expiration_date_handled, msg="risk acceptance with no engagement link was never expired")
+        self.assert_all_active_not_risk_accepted(findings)
+
+        notified = self.notified_risk_acceptances(mock_create_notification)
+        self.assertIn(ra3, notified, msg=f"expired risk acceptance with findings was not notified about, got {notified}")
+        ra3_call = next(c for c in mock_create_notification.call_args_list if c.kwargs.get("risk_acceptance") == ra3)
+        self.assertEqual(
+            ra3_call.kwargs.get("engagement"), expected_engagement,
+            msg=f"expected engagement={expected_engagement}, notified with {ra3_call.kwargs.get('engagement')}",
+        )
+
+        # the rest of the batch is still processed instead of being abandoned mid-run
+        self.assertIsNotNone(ra1.expiration_date_warned, msg="the run stopped at the risk acceptance with no engagement link")