Add security policy bot comment for policy violations in MRs

This change introduces an automatic comment on the MR
where security policy violations are detected. It adds guidance for
the user on what to do next.

Changelog: added
EE: true

app/models/note.rb

@@ -141,6 +141,7 @@ class Note < ApplicationRecord
   scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) }
   scope :with_suggestions, -> { joins(:suggestions) }
   scope :inc_author, -> { includes(:author) }
+  scope :authored_by, ->(user) { where(author_id: user.id) }
   scope :inc_note_diff_file, -> { includes(:note_diff_file) }
   scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
   scope :inc_relations_for_view, ->(noteable = nil) do

config/sidekiq_queues.yml

@@ -543,6 +543,8 @@
   - 1
 - - security_auto_fix
   - 1
+- - security_generate_policy_violation_comment
+  - 1
 - - security_orchestration_configuration_create_bot
   - 1
 - - security_orchestration_policy_rule_schedule_namespace

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

+# frozen_string_literal: true
+
+module Security
+  module ScanResultPolicies
+    class GeneratePolicyViolationCommentService
+      attr_reader :merge_request, :project, :violated_policy
+
+      def initialize(merge_request, violated_policy)
+        @merge_request = merge_request
+        @project = merge_request.project
+        @violated_policy = violated_policy
+      end
+
+      def execute
+        return unless bot_user
+
+        if bot_comment
+          update_comment
+        elsif violated_policy
+          create_comment
+        end
+      end
+
+      private
+
+      def update_comment
+        body = violated_policy ? violations_note_body : fixed_note_body
+        Notes::UpdateService.new(project, bot_user, note_params(body)).execute(bot_comment)
+      end
+
+      def create_comment
+        Notes::CreateService.new(project, bot_user, note_params(violations_note_body)).execute
+      end
+
+      def bot_comment
+        @bot_comment ||= merge_request.notes.authored_by(bot_user).first
+      end
+
+      def bot_user
+        @bot_user ||= project.security_orchestration_policy_configuration&.bot_user
+      end
+
+      def note_params(body)
+        {
+          note: body,
+          noteable: merge_request
+        }
+      end
+
+      def fixed_note_body
+        _('Security policy violations have been fixed.')
+      end
+
+      def violations_note_body
+        message = <<~TEXT.squish
+          Security and compliance scanners enforced by your organization have completed and identified that approvals
+          are required due to one or more policy violations.
+          Review the policy rules in the MR widget and assign a reviewer to proceed.
+        TEXT
+        <<~MARKDOWN
+          | :warning: **#{_('Policy violation(s) detected')}**|
+          | ----------------------------------------- |
+          | #{_(message)}                                |
+
+          #{format(_('Learn more about [Security & Compliance policies](%{url}).'),
+            url: Rails.application.routes.url_helpers.help_page_url('user/application_security/policies'))}
+        MARKDOWN
+      end
+    end
+  end
+end

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

@@ -15,14 +15,16 @@ def initialize(merge_request:, pipeline:, pipeline_findings:)
       end
 
       def execute
-        return if scan_removed?
-
-        unviolated_rules = merge_request.approval_rules.scan_finding.reject do |approval_rule|
+        violated_rules, unviolated_rules = merge_request.approval_rules.scan_finding.partition do |approval_rule|
           approval_rule = approval_rule.source_rule if approval_rule.source_rule
 
           violates_approval_rule?(approval_rule)
         end
 
+        generate_policy_bot_comment(violated_rules.any? || scan_removed?)
+
+        return if scan_removed?
+
         ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
       end
 
@@ -40,6 +42,13 @@ def violates_approval_rule?(approval_rule)
       def scan_removed?
         (Array.wrap(target_pipeline&.security_scan_types) - pipeline.security_scan_types).any?
       end
+      strong_memoize_attr :scan_removed?
+
+      def generate_policy_bot_comment(violated_policy)
+        return if Feature.disabled?(:security_policy_approval_notification)
+
+        Security::GeneratePolicyViolationCommentWorker.perform_async(merge_request.id, violated_policy)
+      end
 
       def target_pipeline_security_findings
         target_pipeline&.security_findings || Security::Finding.none

ee/app/workers/all_queues.yml

@@ -1722,6 +1722,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: security_generate_policy_violation_comment
+  :worker_name: Security::GeneratePolicyViolationCommentWorker
+  :feature_category: :security_policy_management
+  :has_external_dependencies: false
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: security_orchestration_configuration_create_bot
   :worker_name: Security::OrchestrationConfigurationCreateBotWorker
   :feature_category: :security_policy_management

ee/app/workers/security/generate_policy_violation_comment_worker.rb

+# frozen_string_literal: true
+
+module Security
+  class GeneratePolicyViolationCommentWorker
+    include ApplicationWorker
+
+    idempotent!
+    data_consistency :sticky
+    feature_category :security_policy_management
+
+    def perform(merge_request_id, violated_policy)
+      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
+
+      Security::ScanResultPolicies::GeneratePolicyViolationCommentService.new(merge_request, violated_policy).execute
+    end
+  end
+end

ee/config/feature_flags/development/security_policy_approval_notification.yml

+---
+name: security_policy_approval_notification
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121732
+rollout_issue_url:
+milestone: '16.1'
+type: development
+group: group::security policies
+default_enabled: false

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::ScanResultPolicies::GeneratePolicyViolationCommentService, feature_category: :security_policy_management do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+
+  describe '#execute' do
+    subject(:execute) { described_class.new(merge_request, violated_policy).execute }
+
+    context 'when bot user exists' do
+      let_it_be(:bot_user) { create(:user, :security_policy_bot) }
+      let_it_be(:policy_configuration) do
+        create(:security_orchestration_policy_configuration, project: project, bot_user: bot_user)
+      end
+
+      let(:expected_violation_note) { 'Policy violation(s) detected' }
+      let(:expected_fixed_note) { 'Security policy violations have been fixed' }
+
+      context 'when there is no bot comment yet' do
+        before do
+          execute
+        end
+
+        context 'when policy has been violated' do
+          let(:violated_policy) { true }
+
+          it 'creates a comment' do
+            note = merge_request.notes.last
+            expect(note.note).to include(expected_violation_note)
+            expect(note.author).to eq(bot_user)
+          end
+        end
+
+        context 'when there was no policy violation' do
+          let(:violated_policy) { false }
+
+          it 'does not create a comment' do
+            expect(merge_request.notes).to be_empty
+          end
+        end
+      end
+
+      context 'when there is already a bot comment' do
+        before do
+          create(:note, project: project, noteable: merge_request, author: bot_user, note: 'Previous comment')
+
+          execute
+        end
+
+        context 'when policy has been violated' do
+          let(:violated_policy) { true }
+
+          it 'updates the comment with a violated note' do
+            note = merge_request.notes.last
+            expect(note.note).to include(expected_violation_note)
+          end
+        end
+
+        context 'when there was no policy violation' do
+          let(:violated_policy) { false }
+
+          it 'updates the comment with fixes note' do
+            note = merge_request.notes.last
+            expect(note.note).to include(expected_fixed_note)
+          end
+        end
+      end
+
+      context 'when there is a comment from another user and there is a violation' do
+        let(:violated_policy) { true }
+
+        before do
+          create(:note, project: project, noteable: merge_request, note: 'Other comment')
+
+          execute
+        end
+
+        it 'creates a bot comment' do
+          note = merge_request.notes.last
+
+          expect(merge_request.notes.count).to eq(2)
+          expect(note.note).to include(expected_violation_note)
+        end
+      end
+    end
+
+    context 'when there is no bot_user assigned to the policy configuration' do
+      let_it_be(:policy_configuration) do
+        create(:security_orchestration_policy_configuration, project: project)
+      end
+
+      let(:violated_policy) { true }
+
+      it 'does not create a bot comment' do
+        expect { execute }.not_to(change { merge_request.notes.count })
+      end
+    end
+  end
+end

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

@@ -21,7 +21,7 @@
       end
     end
 
-    let(:report_approver_rule) do
+    let!(:report_approver_rule) do
       create(:report_approver_rule, :scan_finding,
         merge_request: merge_request,
         approvals_required: 2,
@@ -80,16 +80,47 @@
       end
     end
 
+    shared_examples_for 'triggers policy bot comment' do |violated_policy|
+      context 'when feature flag "security_policy_approval_notification" is enabled' do
+        before do
+          stub_feature_flags(security_policy_approval_notification: true)
+        end
+
+        it 'enqueues Security::GeneratePolicyViolationCommentWorker' do
+          expect(Security::GeneratePolicyViolationCommentWorker).to receive(:perform_async)
+                                                                      .with(merge_request.id, violated_policy)
+
+          service
+        end
+      end
+
+      context 'when feature flag "security_policy_approval_notification" is disabled' do
+        before do
+          stub_feature_flags(security_policy_approval_notification: false)
+        end
+
+        it 'does not enqueue Security::GeneratePolicyViolationCommentWorker' do
+          expect(Security::GeneratePolicyViolationCommentWorker).not_to receive(:perform_async)
+
+          service
+        end
+      end
+    end
+
     context 'when security scan is removed in current pipeline' do
       let_it_be(:pipeline) { create(:ee_ci_pipeline, project: project, ref: merge_request.source_branch) }
 
       it_behaves_like 'does not update approvals_required'
+
+      it_behaves_like 'triggers policy bot comment', true
     end
 
     context 'when there are no violated approval rules' do
       let(:vulnerabilities_allowed) { 100 }
 
       it_behaves_like 'sets approvals_required to 0'
+
+      it_behaves_like 'triggers policy bot comment', false
     end
 
     context 'when target pipeline is nil' do
@@ -98,6 +129,8 @@
       end
 
       it_behaves_like 'does not update approvals_required'
+
+      it_behaves_like 'triggers policy bot comment', true
     end
 
     context 'when the number of findings in current pipeline exceed the allowed limit' do
@@ -127,6 +160,8 @@
 
       context 'when vulnerabilities count exceeds the allowed limit' do
         it_behaves_like 'does not update approvals_required'
+
+        it_behaves_like 'triggers policy bot comment', true
       end
 
       context 'when new findings are introduced and it exceeds the allowed limit' do
@@ -140,6 +175,8 @@
 
         it_behaves_like 'does not update approvals_required'
 
+        it_behaves_like 'triggers policy bot comment', true
+
         context 'when there are no new dismissed vulnerabilities' do
           let(:vulnerabilities_allowed) { 0 }
 
@@ -215,34 +252,46 @@
           let(:vulnerability_states) { %w[detected newly_detected] }
 
           it_behaves_like 'sets approvals_required to 0'
+
+          it_behaves_like 'triggers policy bot comment', false
         end
 
         context 'when vulnerability_states has new_needs_triage' do
           let(:vulnerability_states) { %w[detected new_needs_triage] }
 
           it_behaves_like 'sets approvals_required to 0'
+
+          it_behaves_like 'triggers policy bot comment', false
         end
 
         context 'when vulnerability_states has new_dismissed' do
           let(:vulnerability_states) { %w[detected new_dismissed] }
 
           it_behaves_like 'sets approvals_required to 0'
+
+          it_behaves_like 'triggers policy bot comment', false
         end
 
         context 'when vulnerability_states has new_needs_triage and new_dismissed' do
           let(:vulnerability_states) { %w[detected new_needs_triage new_dismissed] }
 
           it_behaves_like 'sets approvals_required to 0'
+
+          it_behaves_like 'triggers policy bot comment', false
         end
 
         context 'when vulnerabilities count exceeds the allowed limit' do
           it_behaves_like 'does not update approvals_required'
+
+          it_behaves_like 'triggers policy bot comment', true
         end
 
         context 'when vulnerabilities count does not exceed the allowed limit' do
           let(:vulnerabilities_allowed) { 6 }
 
           it_behaves_like 'sets approvals_required to 0'
+
+          it_behaves_like 'triggers policy bot comment', false
         end
       end
 
@@ -252,6 +301,8 @@
         end
 
         it_behaves_like 'does not update approvals_required'
+
+        it_behaves_like 'triggers policy bot comment', true
       end
     end
   end

ee/spec/workers/security/generate_policy_violation_comment_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::GeneratePolicyViolationCommentWorker, feature_category: :security_policy_management do
+  include AfterNextHelpers
+
+  describe '#perform' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+    let_it_be(:violated_policy) { true }
+
+    subject(:worker) { described_class.new }
+
+    it_behaves_like 'an idempotent worker' do
+      let(:job_args) { [merge_request.id, violated_policy] }
+    end
+
+    describe '#perform' do
+      it 'calls Security::ScanResultPolicies::GeneratePolicyViolationCommentService#execute' do
+        expect_next(Security::ScanResultPolicies::GeneratePolicyViolationCommentService, merge_request, violated_policy)
+          .to receive(:execute)
+
+        worker.perform(merge_request.id, violated_policy)
+      end
+
+      context 'with a non-existing merge request' do
+        it 'does nothing' do
+          expect(::MergeRequests::ResolveTodosService).not_to receive(:new)
+
+          worker.perform(non_existing_record_id, violated_policy)
+        end
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -26373,6 +26373,9 @@ msgstr ""
 msgid "Learn more about X.509 signed commits"
 msgstr ""
+msgid "Learn more about [Security & Compliance policies](%{url})."
+msgstr ""
+
 msgid "Learn more about adding certificates to your project by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}."
 msgstr ""
@@ -33880,6 +33883,9 @@ msgstr ""
 msgid "Policy project doesn't exist"
 msgstr ""
+msgid "Policy violation(s) detected"
+msgstr ""
+
 msgid "PolicyRuleMultiSelect|%{firstLabel} +%{numberOfAdditionalLabels} more"
 msgstr ""
@@ -40358,6 +40364,9 @@ msgstr ""
 msgid "Security dashboard"
 msgstr ""
+msgid "Security policy violations have been fixed."
+msgstr ""
+
 msgid "Security report is out of date. Please update your branch with the latest changes from the target branch (%{targetBranchName})"
 msgstr ""

spec/models/note_spec.rb

@@ -1695,6 +1695,19 @@ def expect_expiration(noteable)
         end
       end
     end
+
+    describe '.authored_by' do
+      subject(:notes_by_author) { described_class.authored_by(author) }
+
+      let(:author) { create(:user) }
+
+      it 'returns the notes with the matching author' do
+        note = create(:note, author: author)
+        create(:note)
+
+        expect(notes_by_author).to contain_exactly(note)
+      end
+    end
   end
 
   describe 'banzai_render_context' do