Merge branch 'mc/fix-bot-comment-for-non-applicable-branches' into 'master'

Prevent policy bot message on non-applicable branches

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144863



Merged-by: Andy Soiron <asoiron@gitlab.com>
Approved-by: Sashi Kumar Kumaresan <skumar@gitlab.com>
Approved-by: Andy Soiron <asoiron@gitlab.com>
Reviewed-by: Andy Soiron <asoiron@gitlab.com>
Co-authored-by: Martin Čavoj <mcavoj@gitlab.com>

config/sidekiq_queues.yml

@@ -713,6 +713,8 @@
   - 2
 - - security_scans_purge_by_job_id
   - 1
+- - security_sync_policy_violation_comment
+  - 1
 - - security_sync_scan_policies
   - 1
 - - security_unenforceable_policy_rules_notification

ee/app/services/ee/merge_requests/update_service.rb

@@ -58,7 +58,7 @@ def sync_any_merge_request_approval_rules(merge_request)
       end
 
       def notify_for_policy_violations(merge_request)
-        ::Security::UnenforceablePolicyRulesNotificationWorker.perform_async(merge_request.id)
+        ::Security::SyncPolicyViolationCommentWorker.perform_async(merge_request.id)
       end
     end
   end

ee/app/services/security/scan_result_policies/update_approvals_service.rb

@@ -37,7 +37,11 @@ def execute
         update_required_approvals(violated_rules, unviolated_rules)
         violations.add(violated_rules.pluck(:scan_result_policy_id), unviolated_rules.pluck(:scan_result_policy_id)) # rubocop:disable CodeReuse/ActiveRecord
         violations.execute
-        generate_policy_bot_comment(merge_request, all_scan_finding_rules, :scan_finding)
+        generate_policy_bot_comment(
+          merge_request,
+          all_scan_finding_rules.applicable_to_branch(merge_request.target_branch),
+          :scan_finding
+        )
       end
 
       private

ee/app/services/security/sync_license_scanning_rules_service.rb

@@ -53,7 +53,10 @@ def remove_required_license_finding_approval(merge_request)
       log_violated_rules(merge_request, violated_rules)
       violations.add(violated_rules.pluck(:scan_result_policy_id), unviolated_rules.pluck(:scan_result_policy_id)) # rubocop:disable CodeReuse/ActiveRecord
       violations.execute
-      generate_policy_bot_comment(merge_request, license_approval_rules, :license_scanning)
+      generate_policy_bot_comment(
+        merge_request,
+        license_approval_rules.applicable_to_branch(merge_request.target_branch),
+        :license_scanning)
     end
 
     def update_required_approvals(merge_request, violated_rules, unviolated_rules)

ee/app/workers/all_queues.yml

@@ -2055,6 +2055,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: security_sync_policy_violation_comment
+  :worker_name: Security::SyncPolicyViolationCommentWorker
+  :feature_category: :security_policy_management
+  :has_external_dependencies: false
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: security_sync_scan_policies
   :worker_name: Security::SyncScanPoliciesWorker
   :feature_category: :security_policy_management

ee/app/workers/security/sync_policy_violation_comment_worker.rb

+# frozen_string_literal: true
+
+module Security
+  class SyncPolicyViolationCommentWorker
+    include ApplicationWorker
+    include ::Security::ScanResultPolicies::PolicyViolationCommentGenerator
+
+    idempotent!
+    data_consistency :sticky
+    feature_category :security_policy_management
+
+    def perform(merge_request_id)
+      merge_request = MergeRequest.find_by_id(merge_request_id)
+
+      unless merge_request
+        logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id))
+        return
+      end
+
+      return unless merge_request.project.licensed_feature_available?(:security_orchestration_policies)
+
+      approval_rules = merge_request.approval_rules
+      generate_comment(merge_request, approval_rules.scan_finding, :scan_finding)
+      generate_comment(merge_request, approval_rules.license_scanning, :license_scanning)
+    end
+
+    private
+
+    def generate_comment(merge_request, rules, report_type)
+      return if rules.blank?
+
+      generate_policy_bot_comment(merge_request, rules.applicable_to_branch(merge_request.target_branch), report_type)
+    end
+  end
+end

ee/spec/services/ee/merge_requests/update_service_spec.rb

@@ -462,8 +462,8 @@ def update_merge_request(opts)
 
       subject(:execute) { update_merge_request(opts) }
 
-      it 'enqueues Security::UnenforceablePolicyRulesNotificationWorker' do
-        expect(Security::UnenforceablePolicyRulesNotificationWorker).to receive(:perform_async).with(merge_request.id)
+      it 'enqueues Security::SyncPolicyViolationCommentWorker' do
+        expect(Security::SyncPolicyViolationCommentWorker).to receive(:perform_async).with(merge_request.id)
 
         execute
       end
@@ -471,8 +471,8 @@ def update_merge_request(opts)
       context 'when target_branch is not changing' do
         let(:opts) { {} }
 
-        it 'does not enqueue Security::UnenforceablePolicyRulesNotificationWorker' do
-          expect(Security::UnenforceablePolicyRulesNotificationWorker).not_to receive(:perform_async)
+        it 'does not enqueue Security::SyncPolicyViolationCommentWorker' do
+          expect(Security::SyncPolicyViolationCommentWorker).not_to receive(:perform_async)
 
           execute
         end

ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb

@@ -42,6 +42,8 @@
       end
     end
 
+    let_it_be(:scan_result_policy_read) { create(:scan_result_policy_read) }
+
     let!(:report_approver_rule) do
       create(:report_approver_rule, :scan_finding,
         merge_request: merge_request,
@@ -50,7 +52,7 @@
         vulnerabilities_allowed: vulnerabilities_allowed,
         severity_levels: severity_levels,
         vulnerability_states: vulnerability_states,
-        scan_result_policy_id: create(:scan_result_policy_read).id
+        scan_result_policy_read: scan_result_policy_read
       )
     end
 
@@ -170,10 +172,12 @@
       it_behaves_like 'triggers policy bot comment', :scan_finding, false
 
       context 'when there are other scan_finding violations' do
+        let_it_be(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
         let_it_be(:scan_result_policy_read_other_scan_finding) { create(:scan_result_policy_read, project: project) }
         let_it_be(:approval_project_rule_other) do
           create(:approval_project_rule, :scan_finding, project: project, approvals_required: 1,
-            scan_result_policy_read: scan_result_policy_read_other_scan_finding)
+            scan_result_policy_read: scan_result_policy_read_other_scan_finding,
+            protected_branches: [protected_branch])
         end
 
         let_it_be(:approver_rule_other) do
@@ -198,6 +202,27 @@
       it_behaves_like 'triggers policy bot comment', :scan_finding, true, requires_approval: false
     end
 
+    context 'when targeting an unprotected branch' do
+      let_it_be(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
+      let!(:report_approver_project_rule) do
+        create(:approval_project_rule, :scan_finding, project: project,
+          approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read,
+          protected_branches: [protected_branch])
+      end
+
+      let!(:report_approver_rule) do
+        create(:report_approver_rule, :scan_finding, merge_request: merge_request,
+          approval_project_rule: report_approver_project_rule,
+          approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read)
+      end
+
+      before do
+        merge_request.update!(target_branch: 'non-protected')
+      end
+
+      it_behaves_like 'triggers policy bot comment', :scan_finding, false, requires_approval: false
+    end
+
     context 'when target pipeline is nil' do
       let_it_be(:merge_request) do
         create(:merge_request, source_branch: 'feature', target_branch: 'target-branch')
@@ -431,7 +456,7 @@
               vulnerabilities_allowed: vulnerabilities_allowed,
               severity_levels: severity_levels,
               vulnerability_states: vulnerability_states,
-              scan_result_policy_id: create(:scan_result_policy_read).id
+              scan_result_policy_read: scan_result_policy_read
             )
           end
 

ee/spec/services/security/sync_license_scanning_rules_service_spec.rb

@@ -10,7 +10,7 @@
     create(:merge_request, :with_merge_request_pipeline, source_project: project)
   end
 
-  let_it_be(:pipeline) do
+  let_it_be_with_reload(:pipeline) do
     create(
       :ee_ci_pipeline,
       :success,
@@ -64,14 +64,24 @@
       let(:license_states) { ['newly_detected'] }
       let(:match_on_inclusion_license) { true }
       let(:approvals_required) { 1 }
+      let_it_be(:protected_branch) do
+        create(:protected_branch, name: merge_request.target_branch, project: project)
+      end
 
       let(:scan_result_policy_read) do
         create(:scan_result_policy_read, license_states: license_states,
           match_on_inclusion_license: match_on_inclusion_license)
       end
 
+      let!(:license_finding_project_rule) do
+        create(:approval_project_rule, :license_scanning, project: project,
+          approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read,
+          protected_branches: [protected_branch])
+      end
+
       let!(:license_finding_rule) do
         create(:report_approver_rule, :license_scanning, merge_request: merge_request,
+          approval_project_rule: license_finding_project_rule,
           approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read)
       end
 
@@ -126,6 +136,15 @@
           it_behaves_like 'triggers policy bot comment', :license_scanning, true, requires_approval: false
         end
 
+        context 'when targeting an unprotected branch' do
+          before do
+            merge_request.update!(target_branch: 'non-protected')
+            pipeline.update!(ref: 'non-protected')
+          end
+
+          it_behaves_like 'triggers policy bot comment', :license_scanning, false, requires_approval: false
+        end
+
         context 'when most recent base pipeline lacks SBOM report' do
           let(:pipeline_without_sbom) do
             create(

ee/spec/workers/security/sync_policy_violation_comment_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SyncPolicyViolationCommentWorker, feature_category: :security_policy_management do
+  describe '#perform' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+    let(:merge_request_id) { merge_request.id }
+    let(:licensed_feature) { true }
+    let(:approvals_required) { 1 }
+    let_it_be(:protected_branch) { create(:protected_branch, project: project, name: merge_request.target_branch) }
+    let_it_be(:scan_result_policy_read) { create(:scan_result_policy_read) }
+    let_it_be(:scan_finding_project_rule) do
+      create(:approval_project_rule, :scan_finding, project: project, protected_branches: [protected_branch],
+        scan_result_policy_read: scan_result_policy_read)
+    end
+
+    let!(:scan_finding_rule) do
+      create(:report_approver_rule, :scan_finding, merge_request: merge_request,
+        approval_project_rule: scan_finding_project_rule, approvals_required: approvals_required,
+        scan_result_policy_read: scan_result_policy_read)
+    end
+
+    let_it_be(:license_scanning_project_rule) do
+      create(:approval_project_rule, :license_scanning, project: project, protected_branches: [protected_branch],
+        scan_result_policy_read: scan_result_policy_read)
+    end
+
+    let!(:license_scanning_rule) do
+      create(:report_approver_rule, :license_scanning, merge_request: merge_request,
+        approval_project_rule: scan_finding_project_rule, approvals_required: approvals_required,
+        scan_result_policy_read: scan_result_policy_read)
+    end
+
+    before do
+      stub_licensed_features(security_orchestration_policies: licensed_feature)
+    end
+
+    subject(:perform) { described_class.new.perform(merge_request_id) }
+
+    it_behaves_like 'an idempotent worker' do
+      let(:job_args) { [merge_request_id] }
+    end
+
+    it 'enqueues Security::GeneratePolicyViolationCommentWorker' do
+      expect(Security::GeneratePolicyViolationCommentWorker)
+        .to receive(:perform_async).with(merge_request.id,
+          { 'report_type' => 'scan_finding', 'violated_policy' => false, 'requires_approval' => true })
+      expect(Security::GeneratePolicyViolationCommentWorker)
+        .to receive(:perform_async).with(merge_request.id,
+          { 'report_type' => 'license_scanning', 'violated_policy' => false, 'requires_approval' => true })
+
+      perform
+    end
+
+    context 'when there are violations' do
+      before do
+        create(:scan_result_policy_violation, scan_result_policy_read: scan_result_policy_read,
+          merge_request: merge_request, project: project)
+      end
+
+      it 'enqueues Security::GeneratePolicyViolationCommentWorker with correct params' do
+        expect(Security::GeneratePolicyViolationCommentWorker)
+          .to receive(:perform_async).with(merge_request.id,
+            { 'report_type' => 'scan_finding', 'violated_policy' => true, 'requires_approval' => true })
+        expect(Security::GeneratePolicyViolationCommentWorker)
+          .to receive(:perform_async).with(merge_request.id,
+            { 'report_type' => 'license_scanning', 'violated_policy' => true, 'requires_approval' => true })
+
+        perform
+      end
+
+      context 'when approvals are optional' do
+        let(:approvals_required) { 0 }
+
+        it 'enqueues Security::GeneratePolicyViolationCommentWorker with correct params' do
+          expect(Security::GeneratePolicyViolationCommentWorker)
+            .to receive(:perform_async).with(merge_request.id,
+              { 'report_type' => 'scan_finding', 'violated_policy' => true, 'requires_approval' => false })
+          expect(Security::GeneratePolicyViolationCommentWorker)
+            .to receive(:perform_async).with(merge_request.id,
+              { 'report_type' => 'license_scanning', 'violated_policy' => true, 'requires_approval' => false })
+
+          perform
+        end
+      end
+    end
+
+    context 'with a non-existing merge request' do
+      let(:merge_request_id) { non_existing_record_id }
+
+      it 'does not enqueue the worker' do
+        expect(Security::GeneratePolicyViolationCommentWorker).not_to receive(:perform_async)
+
+        perform
+      end
+    end
+
+    context 'when feature is not licensed' do
+      let(:licensed_feature) { false }
+
+      it 'does not enqueue the worker' do
+        expect(Security::GeneratePolicyViolationCommentWorker).not_to receive(:perform_async)
+
+        perform
+      end
+    end
+  end
+end