move evaluation in a signal

Signed-off-by: tdruez <tdruez@aboutcode.org>

Author: tdruez

product_portfolio/tests/test_views.py

@@ -4416,7 +4416,7 @@ def test_post_without_change_perm_returns_404(self):
         response = self.client.post(url)
         self.assertEqual(404, response.status_code)
 
-    @patch("product_portfolio.views.evaluate_ruleset")
+    @patch("vulnerabilities.triage.signals.evaluate_ruleset")
     def test_post_assigns_the_submitted_rulesets(self, mock_evaluate):
         self.client.login(username="nexb_user", password="secret")
         url = self.product1.get_manage_triage_rulesets_url()
@@ -4429,11 +4429,12 @@ def test_post_assigns_the_submitted_rulesets(self, mock_evaluate):
         )
         mock_evaluate.assert_called_once_with(ruleset=self.ruleset, product=self.product1)
 
-    @patch("product_portfolio.views.evaluate_ruleset")
+    @patch("vulnerabilities.triage.signals.evaluate_ruleset")
     def test_post_unassigns_the_deselected_rulesets(self, mock_evaluate):
         ProductTriageRuleset.objects.create(
             product=self.product1, ruleset=self.ruleset, dataspace=self.dataspace
         )
+        mock_evaluate.reset_mock()
         self.client.login(username="nexb_user", password="secret")
         url = self.product1.get_manage_triage_rulesets_url()
         response = self.client.post(url, {"ruleset_uuids": []})

product_portfolio/views.py

@@ -150,7 +150,6 @@
 from vulnerabilities.models import Vulnerability
 from vulnerabilities.models import VulnerabilityAnalysis
 from vulnerabilities.models import get_risk_level
-from vulnerabilities.triage.engine import evaluate_ruleset
 from vulnerabilities.triage.models import AnalysisPreset
 from vulnerabilities.triage.models import ProductTriageRuleset
 from vulnerabilities.triage.models import TriageAction
@@ -2253,13 +2252,14 @@ def manage_triage_rulesets_view(request, dataspace, name, version=""):
         for ruleset in available_rulesets:
             ruleset_uuid = str(ruleset.uuid)
             if ruleset_uuid in submitted_uuids and ruleset_uuid not in current_assignments:
+                # The evaluate_on_assign signal evaluates the ruleset against the product;
+                # wrapping in atomic() rolls the assignment back if that evaluation fails.
                 with transaction.atomic():
                     ProductTriageRuleset.objects.create(
                         product=product,
                         ruleset=ruleset,
                         dataspace=product.dataspace,
                     )
-                    evaluate_ruleset(ruleset=ruleset, product=product)
         for ruleset_uuid, assignment in current_assignments.items():
             if ruleset_uuid not in submitted_uuids:
                 assignment.delete()

vulnerabilities/triage/rules.py

@@ -15,8 +15,6 @@
 
 from policy.rules import BaseRule
 
-# Not shared with policy.rules.TERMINAL_VULNERABILITY_STATES: the two lists are
-# intentionally scoped to their own engine and are not guaranteed to stay identical.
 TRIAGE_TERMINAL_VULNERABILITY_STATES = [
     "resolved",
     "resolved_with_pedigree",

vulnerabilities/triage/signals.py

@@ -42,6 +42,15 @@ def reevaluate_or_delete_on_ruleset_save(sender, instance, created, **kwargs):
         evaluate_ruleset(ruleset=instance, product=assignment.product)
 
 
+@receiver(post_save, sender="vulnerabilities_triage.ProductTriageRuleset")
+def evaluate_on_assign(sender, instance, created, **kwargs):
+    """Evaluate the ruleset against the product as soon as it is assigned."""
+    if not created or not instance.ruleset.enabled:
+        return
+
+    evaluate_ruleset(ruleset=instance.ruleset, product=instance.product)
+
+
 @receiver(post_delete, sender="vulnerabilities_triage.ProductTriageRuleset")
 def delete_triage_records_on_unassign(sender, instance, **kwargs):
     """Delete triage records and associated preset analyses when a ruleset is de-assigned."""
@@ -66,8 +75,9 @@ def reevaluate_on_analysis_change(sender, instance, **kwargs):
     """Re-evaluate triage when an analysis state or reachability is updated."""
     signal = kwargs.get("signal")
     if signal == post_save and instance.applied_by_preset_id:
-        return  # Written by the triage engine itself -- re-evaluating would loop
-    # When a human explicitly deletes their analysis, skip preset application to avoid
+        return  # Written by the triage engine itself, re-evaluating would loop
+
+    # When an user explicitly deletes their analysis, skip preset application to avoid
     # having the engine immediately recreate it.
     is_human_delete = signal == post_delete and not instance.applied_by_preset_id
     reevaluate_product_rulesets(

vulnerabilities/triage/tests/test_signals.py

@@ -42,6 +42,7 @@ def setUp(self):
     def test_evaluates_every_enabled_ruleset_assigned_to_the_product(self, mock_evaluate):
         ruleset = make_triage_ruleset(self.dataspace, enabled=True)
         make_product_triage_ruleset(self.product, ruleset=ruleset)
+        mock_evaluate.reset_mock()  # Called once already by the evaluate_on_assign signal.
 
         reevaluate_product_rulesets(self.product)
 
@@ -62,6 +63,7 @@ def test_skips_disabled_ruleset_assignments(self, mock_evaluate):
     def test_apply_preset_flag_is_forwarded(self, mock_evaluate):
         ruleset = make_triage_ruleset(self.dataspace, enabled=True)
         make_product_triage_ruleset(self.product, ruleset=ruleset)
+        mock_evaluate.reset_mock()  # Called once already by the evaluate_on_assign signal.
 
         reevaluate_product_rulesets(self.product, apply_preset=False)
 
@@ -196,6 +198,38 @@ def test_disabling_then_reenabling_a_ruleset_reuses_the_existing_request(self):
         self.assertEqual(original_request, TriageRecord.objects.get().request)
 
 
+class EvaluateOnAssignSignalTestCase(TestCase):
+    def setUp(self):
+        self.dataspace = Dataspace.objects.create(name="nexB")
+        self.product = make_product(self.dataspace)
+
+    @patch("vulnerabilities.triage.signals.evaluate_ruleset")
+    def test_assigning_an_enabled_ruleset_evaluates_it(self, mock_evaluate):
+        ruleset = make_triage_ruleset(self.dataspace, enabled=True)
+
+        make_product_triage_ruleset(self.product, ruleset=ruleset)
+
+        mock_evaluate.assert_called_once_with(ruleset=ruleset, product=self.product)
+
+    @patch("vulnerabilities.triage.signals.evaluate_ruleset")
+    def test_assigning_a_disabled_ruleset_does_not_evaluate(self, mock_evaluate):
+        ruleset = make_triage_ruleset(self.dataspace, enabled=False)
+
+        make_product_triage_ruleset(self.product, ruleset=ruleset)
+
+        mock_evaluate.assert_not_called()
+
+    @patch("vulnerabilities.triage.signals.evaluate_ruleset")
+    def test_resaving_an_existing_assignment_does_not_reevaluate(self, mock_evaluate):
+        ruleset = make_triage_ruleset(self.dataspace, enabled=True)
+        assignment = make_product_triage_ruleset(self.product, ruleset=ruleset)
+        mock_evaluate.reset_mock()
+
+        assignment.save()
+
+        mock_evaluate.assert_not_called()
+
+
 class DeleteTriageRecordsOnUnassignSignalTestCase(TestCase):
     def setUp(self):
         self.dataspace = Dataspace.objects.create(name="nexB")