Call run_action() even on initial match when scanning for narc outside auto_approve (#23820)

This ensures we trigger actions from NARC matches that happen outside
auto-approve, even if they are the first time NARC is called for that
version, for instance because it was already approved before we enabled
NARC.

Author: diox

src/olympia/reviewers/management/commands/auto_approve.py

@@ -117,8 +117,10 @@ def process(self, version_id):
                         # attached to the upload at that point.
                         # This needs to be run before run_action() and before
                         # auto-approval is attempted, and has to be triggered
-                        # synchronously (no .delay()).
-                        run_narc_on_version(version.pk)
+                        # synchronously (no .delay()). In this case we pass
+                        # run_action_on_match=False since we're going to call
+                        # ScannerResult.run_action() below.
+                        run_narc_on_version(version.pk, run_action_on_match=False)
 
                 if waffle.switch_is_active('run-action-in-auto-approve'):
                     if summary_exists:

src/olympia/reviewers/tests/test_commands.py

@@ -609,7 +609,8 @@ def test_executes_run_narc_on_version_when_switch_is_active(self, run_narc_mock)
         call_command('auto_approve')
 
         assert run_narc_mock.call_count == 1
-        run_narc_mock.assert_called_with(self.version.pk)
+        assert run_narc_mock.call_args[0] == (self.version.pk,)
+        assert run_narc_mock.call_args[1] == {'run_action_on_match': False}
 
     @mock.patch(
         'olympia.reviewers.management.commands.auto_approve.run_narc_on_version'
@@ -622,7 +623,8 @@ def test_only_executes_run_narc_on_version_once(
         call_command('auto_approve')
 
         assert run_narc_mock.call_count == 1
-        run_narc_mock.assert_called_with(self.version.pk)
+        assert run_narc_mock.call_args[0] == (self.version.pk,)
+        assert run_narc_mock.call_args[1] == {'run_action_on_match': False}
 
         run_narc_mock.reset_mock()
         call_command('auto_approve')

src/olympia/scanners/tasks.py

@@ -163,12 +163,12 @@ def run_customs(results, upload_pk):
 
 @task
 @use_primary_db
-def run_narc_on_version(version_pk):
+def run_narc_on_version(version_pk, *, run_action_on_match=True):
     log.info('Starting narc task for Version %s.', version_pk)
     try:
         version = Version.unfiltered.get(pk=version_pk)
         with statsd.timer('devhub.narc'):
-            _run_narc(version=version)
+            _run_narc(version=version, run_action_on_match=run_action_on_match)
     except Exception as exc:
         statsd.incr('devhub.narc.failure')
         log.exception(
@@ -181,7 +181,7 @@ def run_narc_on_version(version_pk):
     log.info('Ending scanner "narc" task for Version %s.', version_pk)
 
 
-def _run_narc(*, version):
+def _run_narc(*, version, run_action_on_match):
     scanner_result, initial_run = ScannerResult.objects.get_or_create(
         scanner=NARC, version=version
     )
@@ -242,14 +242,13 @@ def _run_narc(*, version):
                     )
                 )
 
-    run_action = False
+    has_new_matches = False
     new_results = [json.loads(result) for result in sorted(results)]
-    if not initial_run and new_results != scanner_result.results:
-        # Scanner actions are normally triggered by auto-approve, but here
-        # we're re running the scanner on a version and found more results than
-        # when it ran before. We don't know whether the action has already been
-        # triggered but in case it has we need to do it again.
-        run_action = True
+    if new_results != scanner_result.results:
+        # Either we're scanning this version for the first time, or it's a new
+        # run following some change, but in any case we found new results. We
+        # might need to run ScannerResult.run_action() as a result, see below.
+        has_new_matches = True
     scanner_result.results = new_results
     scanner_result.save()
     if initial_run:
@@ -260,8 +259,9 @@ def _run_narc(*, version):
         statsd.incr(f'devhub.narc{statsd_suffix}.has_matches')
     for scanner_rule in scanner_result.matched_rules.all():
         statsd.incr(f'devhub.narc{statsd_suffix}.rule.{scanner_rule.id}.match')
-    if version and run_action:
+    if not initial_run and has_new_matches:
         statsd.incr(f'devhub.narc{statsd_suffix}.results_differ')
+    if run_action_on_match and has_new_matches:
         ScannerResult.run_action(version, check_mad_results=False)
     return scanner_result
 

src/olympia/scanners/tests/test_tasks.py

@@ -848,6 +848,115 @@ def test_run_on_version_no_new_result(self, run_action_mock, incr_mock):
         # We didn't retrigger the action since the first run (no new matches).
         assert run_action_mock.call_count == 0
 
+    @mock.patch('olympia.scanners.tasks.statsd.incr')
+    @mock.patch.object(ScannerResult, 'run_action')
+    def test_run_action_initial_run(self, run_action_mock, incr_mock):
+        rule = ScannerRule.objects.create(
+            name='always_match_rule',
+            scanner=NARC,
+            definition='^My WebExtension.*$',
+        )
+
+        run_narc_on_version(self.version.pk)
+
+        scanner_results = ScannerResult.objects.all()
+        assert len(scanner_results) == 1
+        narc_result = scanner_results[0]
+        assert narc_result.scanner == NARC
+        assert narc_result.upload is None
+        assert narc_result.version == self.version
+        assert narc_result.has_matches
+        assert list(narc_result.matched_rules.all()) == [rule]
+        assert len(narc_result.results) == 1
+        assert narc_result.results == [
+            {
+                'meta': {
+                    'span': [0, 21],
+                    'locale': None,
+                    'source': 'xpi',
+                    'string': 'My WebExtension Addon',
+                    'pattern': '^My WebExtension.*$',
+                },
+                'rule': 'always_match_rule',
+            },
+        ]
+        assert incr_mock.called
+        assert incr_mock.call_count == 3
+        incr_mock.assert_has_calls(
+            [
+                mock.call('devhub.narc.has_matches'),
+                mock.call(f'devhub.narc.rule.{rule.id}.match'),
+                mock.call('devhub.narc.success'),
+            ]
+        )
+
+        assert run_action_mock.call_count == 1
+        assert run_action_mock.call_args[0] == (self.version,)
+        assert run_action_mock.call_args[1] == {'check_mad_results': False}
+
+    @mock.patch('olympia.scanners.tasks.statsd.incr')
+    @mock.patch.object(ScannerResult, 'run_action')
+    def test_no_run_action_if_no_results(self, run_action_mock, incr_mock):
+        run_narc_on_version(self.version.pk)
+
+        scanner_results = ScannerResult.objects.all()
+        assert len(scanner_results) == 1
+        narc_result = scanner_results[0]
+        assert narc_result.scanner == NARC
+        assert narc_result.upload is None
+        assert narc_result.version == self.version
+        assert not narc_result.has_matches
+        assert not narc_result.matched_rules.all().exists()
+        assert narc_result.results == []
+        assert incr_mock.call_count == 1
+        assert incr_mock.call_args[0] == ('devhub.narc.success',)
+
+        assert run_action_mock.call_count == 0
+
+    @mock.patch('olympia.scanners.tasks.statsd.incr')
+    @mock.patch.object(ScannerResult, 'run_action')
+    def test_no_run_action_if_parameter_is_passed(self, run_action_mock, incr_mock):
+        rule = ScannerRule.objects.create(
+            name='always_match_rule',
+            scanner=NARC,
+            definition='^My WebExtension.*$',
+        )
+
+        run_narc_on_version(self.version.pk, run_action_on_match=False)
+
+        scanner_results = ScannerResult.objects.all()
+        assert len(scanner_results) == 1
+        narc_result = scanner_results[0]
+        assert narc_result.scanner == NARC
+        assert narc_result.upload is None
+        assert narc_result.version == self.version
+        assert narc_result.has_matches
+        assert list(narc_result.matched_rules.all()) == [rule]
+        assert len(narc_result.results) == 1
+        assert narc_result.results == [
+            {
+                'meta': {
+                    'span': [0, 21],
+                    'locale': None,
+                    'source': 'xpi',
+                    'string': 'My WebExtension Addon',
+                    'pattern': '^My WebExtension.*$',
+                },
+                'rule': 'always_match_rule',
+            },
+        ]
+        assert incr_mock.called
+        assert incr_mock.call_count == 3
+        incr_mock.assert_has_calls(
+            [
+                mock.call('devhub.narc.has_matches'),
+                mock.call(f'devhub.narc.rule.{rule.id}.match'),
+                mock.call('devhub.narc.success'),
+            ]
+        )
+
+        assert run_action_mock.call_count == 0
+
 
 class TestRunYara(UploadMixin, TestCase):
     def setUp(self):