Skip to content
Snippets Groups Projects
Commit d1561c3a authored by Martin Čavoj's avatar Martin Čavoj :palm_tree:
Browse files

Merge branch '411656-spike-investigate-and-prepare-poc-for-approval-notification-mvc' into 'master'

Add security bot comment for policy violations in MRs

See merge request gitlab-org/gitlab!121732



Merged-by: default avatarMartin Čavoj <mcavoj@gitlab.com>
Reviewed-by: default avatarAndy Soiron <asoiron@gitlab.com>
parents 72d85a10 d4bc812e
No related branches found
No related tags found
No related merge requests found
Showing
with 341 additions and 4 deletions
...@@ -141,6 +141,7 @@ class Note < ApplicationRecord ...@@ -141,6 +141,7 @@ class Note < ApplicationRecord
scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) } scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) }
scope :with_suggestions, -> { joins(:suggestions) } scope :with_suggestions, -> { joins(:suggestions) }
scope :inc_author, -> { includes(:author) } scope :inc_author, -> { includes(:author) }
scope :authored_by, ->(user) { where(author: user) }
scope :inc_note_diff_file, -> { includes(:note_diff_file) } scope :inc_note_diff_file, -> { includes(:note_diff_file) }
scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) } scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
scope :inc_relations_for_view, ->(noteable = nil) do scope :inc_relations_for_view, ->(noteable = nil) do
......
...@@ -543,6 +543,8 @@ ...@@ -543,6 +543,8 @@
- 1 - 1
- - security_auto_fix - - security_auto_fix
- 1 - 1
- - security_generate_policy_violation_comment
- 1
- - security_orchestration_configuration_create_bot - - security_orchestration_configuration_create_bot
- 1 - 1
- - security_orchestration_policy_rule_schedule_namespace - - security_orchestration_policy_rule_schedule_namespace
......
# 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
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 ||= User.security_bot
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
...@@ -15,14 +15,16 @@ def initialize(merge_request:, pipeline:, pipeline_findings:) ...@@ -15,14 +15,16 @@ def initialize(merge_request:, pipeline:, pipeline_findings:)
end end
def execute def execute
return if scan_removed? violated_rules, unviolated_rules = merge_request.approval_rules.scan_finding.partition do |approval_rule|
unviolated_rules = merge_request.approval_rules.scan_finding.reject do |approval_rule|
approval_rule = approval_rule.source_rule if approval_rule.source_rule approval_rule = approval_rule.source_rule if approval_rule.source_rule
violates_approval_rule?(approval_rule) violates_approval_rule?(approval_rule)
end end
generate_policy_bot_comment(violated_rules.any? || scan_removed?)
return if scan_removed?
ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any? ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
end end
...@@ -40,6 +42,13 @@ def violates_approval_rule?(approval_rule) ...@@ -40,6 +42,13 @@ def violates_approval_rule?(approval_rule)
def scan_removed? def scan_removed?
(Array.wrap(target_pipeline&.security_scan_types) - pipeline.security_scan_types).any? (Array.wrap(target_pipeline&.security_scan_types) - pipeline.security_scan_types).any?
end end
strong_memoize_attr :scan_removed?
def generate_policy_bot_comment(violated_policy)
return if Feature.disabled?(:security_policy_approval_notification, pipeline.project)
Security::GeneratePolicyViolationCommentWorker.perform_async(merge_request.id, violated_policy)
end
def target_pipeline_security_findings def target_pipeline_security_findings
target_pipeline&.security_findings || Security::Finding.none target_pipeline&.security_findings || Security::Finding.none
......
...@@ -1704,6 +1704,15 @@ ...@@ -1704,6 +1704,15 @@
:weight: 1 :weight: 1
:idempotent: true :idempotent: true
:tags: [] :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 - :name: security_orchestration_configuration_create_bot
:worker_name: Security::OrchestrationConfigurationCreateBotWorker :worker_name: Security::OrchestrationConfigurationCreateBotWorker
:feature_category: :security_policy_management :feature_category: :security_policy_management
......
# 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
return if Feature.disabled?(:security_policy_approval_notification, merge_request.project)
Security::ScanResultPolicies::GeneratePolicyViolationCommentService.new(merge_request, violated_policy).execute
end
end
end
---
name: security_policy_approval_notification
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121732
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/413519
milestone: '16.1'
type: development
group: group::security policies
default_enabled: false
# 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 }
let_it_be(:bot_user) { User.security_bot }
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
let_it_be_with_reload(:bot_comment) do
create(:note, project: project, noteable: merge_request, author: bot_user, note: 'Previous comment')
end
before do
execute
end
context 'when policy has been violated' do
let(:violated_policy) { true }
it 'updates the comment with a violated note' do
expect(bot_comment.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
expect(bot_comment.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
end
...@@ -21,7 +21,7 @@ ...@@ -21,7 +21,7 @@
end end
end end
let(:report_approver_rule) do let!(:report_approver_rule) do
create(:report_approver_rule, :scan_finding, create(:report_approver_rule, :scan_finding,
merge_request: merge_request, merge_request: merge_request,
approvals_required: 2, approvals_required: 2,
...@@ -80,16 +80,47 @@ ...@@ -80,16 +80,47 @@
end end
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: project)
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 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) } 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 'does not update approvals_required'
it_behaves_like 'triggers policy bot comment', true
end end
context 'when there are no violated approval rules' do context 'when there are no violated approval rules' do
let(:vulnerabilities_allowed) { 100 } let(:vulnerabilities_allowed) { 100 }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
context 'when target pipeline is nil' do context 'when target pipeline is nil' do
...@@ -98,6 +129,8 @@ ...@@ -98,6 +129,8 @@
end end
it_behaves_like 'does not update approvals_required' it_behaves_like 'does not update approvals_required'
it_behaves_like 'triggers policy bot comment', true
end end
context 'when the number of findings in current pipeline exceed the allowed limit' do context 'when the number of findings in current pipeline exceed the allowed limit' do
...@@ -127,6 +160,8 @@ ...@@ -127,6 +160,8 @@
context 'when vulnerabilities count exceeds the allowed limit' do context 'when vulnerabilities count exceeds the allowed limit' do
it_behaves_like 'does not update approvals_required' it_behaves_like 'does not update approvals_required'
it_behaves_like 'triggers policy bot comment', true
end end
context 'when new findings are introduced and it exceeds the allowed limit' do context 'when new findings are introduced and it exceeds the allowed limit' do
...@@ -140,6 +175,8 @@ ...@@ -140,6 +175,8 @@
it_behaves_like 'does not update approvals_required' 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 context 'when there are no new dismissed vulnerabilities' do
let(:vulnerabilities_allowed) { 0 } let(:vulnerabilities_allowed) { 0 }
...@@ -215,34 +252,46 @@ ...@@ -215,34 +252,46 @@
let(:vulnerability_states) { %w[detected newly_detected] } let(:vulnerability_states) { %w[detected newly_detected] }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
context 'when vulnerability_states has new_needs_triage' do context 'when vulnerability_states has new_needs_triage' do
let(:vulnerability_states) { %w[detected new_needs_triage] } let(:vulnerability_states) { %w[detected new_needs_triage] }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
context 'when vulnerability_states has new_dismissed' do context 'when vulnerability_states has new_dismissed' do
let(:vulnerability_states) { %w[detected new_dismissed] } let(:vulnerability_states) { %w[detected new_dismissed] }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
context 'when vulnerability_states has new_needs_triage and new_dismissed' do context 'when vulnerability_states has new_needs_triage and new_dismissed' do
let(:vulnerability_states) { %w[detected new_needs_triage new_dismissed] } let(:vulnerability_states) { %w[detected new_needs_triage new_dismissed] }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
context 'when vulnerabilities count exceeds the allowed limit' do context 'when vulnerabilities count exceeds the allowed limit' do
it_behaves_like 'does not update approvals_required' it_behaves_like 'does not update approvals_required'
it_behaves_like 'triggers policy bot comment', true
end end
context 'when vulnerabilities count does not exceed the allowed limit' do context 'when vulnerabilities count does not exceed the allowed limit' do
let(:vulnerabilities_allowed) { 6 } let(:vulnerabilities_allowed) { 6 }
it_behaves_like 'sets approvals_required to 0' it_behaves_like 'sets approvals_required to 0'
it_behaves_like 'triggers policy bot comment', false
end end
end end
...@@ -252,6 +301,8 @@ ...@@ -252,6 +301,8 @@
end end
it_behaves_like 'does not update approvals_required' it_behaves_like 'does not update approvals_required'
it_behaves_like 'triggers policy bot comment', true
end end
end end
end end
......
# 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) }
subject(:worker) { described_class.new }
it_behaves_like 'an idempotent worker' do
let(:job_args) { [merge_request.id, true] }
end
describe '#perform' do
it 'calls Security::ScanResultPolicies::GeneratePolicyViolationCommentService#execute' do
expect_next(Security::ScanResultPolicies::GeneratePolicyViolationCommentService, merge_request, true)
.to receive(:execute)
worker.perform(merge_request.id, true)
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 nothing' do
expect(Security::ScanResultPolicies::GeneratePolicyViolationCommentService).not_to receive(:new)
worker.perform(merge_request.id, true)
end
end
context 'with a non-existing merge request' do
it 'does nothing' do
expect(Security::ScanResultPolicies::GeneratePolicyViolationCommentService).not_to receive(:new)
worker.perform(non_existing_record_id, true)
end
end
end
end
end
...@@ -26580,6 +26580,9 @@ msgstr "" ...@@ -26580,6 +26580,9 @@ msgstr ""
msgid "Learn more about X.509 signed commits" msgid "Learn more about X.509 signed commits"
msgstr "" 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}." msgid "Learn more about adding certificates to your project by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}."
msgstr "" msgstr ""
   
...@@ -34072,6 +34075,9 @@ msgstr "" ...@@ -34072,6 +34075,9 @@ msgstr ""
msgid "Policy project doesn't exist" msgid "Policy project doesn't exist"
msgstr "" msgstr ""
   
msgid "Policy violation(s) detected"
msgstr ""
msgid "PolicyRuleMultiSelect|%{firstLabel} +%{numberOfAdditionalLabels} more" msgid "PolicyRuleMultiSelect|%{firstLabel} +%{numberOfAdditionalLabels} more"
msgstr "" msgstr ""
   
...@@ -40574,6 +40580,9 @@ msgstr "" ...@@ -40574,6 +40580,9 @@ msgstr ""
msgid "Security dashboard" msgid "Security dashboard"
msgstr "" 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})" msgid "Security report is out of date. Please update your branch with the latest changes from the target branch (%{targetBranchName})"
msgstr "" msgstr ""
   
...@@ -1695,6 +1695,30 @@ def expect_expiration(noteable) ...@@ -1695,6 +1695,30 @@ def expect_expiration(noteable)
end end
end 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
context 'With ID integer' do
subject(:notes_by_author) { described_class.authored_by(author.id) }
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
end end
describe 'banzai_render_context' do describe 'banzai_render_context' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment