Commit 8271f091 authored by Luke Duncalfe's avatar Luke Duncalfe

Merge branch 'load_only_one_scan_finding_rule_per_mr' into 'master'

Load only one scan_finding rule per MR

See merge request gitlab-org/gitlab!71858
parents 758c2fd6 db5acf82
# frozen_string_literal: true
class AddPolicyIdxToApprovalProjectRule < Gitlab::Database::Migration[1.0]
enable_lock_retries!
def change
add_column :approval_project_rules, :orchestration_policy_idx, :integer, limit: 2
end
end
# frozen_string_literal: true
class AddPolicyIdxToApprovalMergeRequestRule < Gitlab::Database::Migration[1.0]
enable_lock_retries!
def change
add_column :approval_merge_request_rules, :orchestration_policy_idx, :integer, limit: 2
end
end
9e01b1817e4c578f5be7d7378dc12a8535c2bbbff5ecbc77f5a7cfdb148927f5
\ No newline at end of file
fc29e10717357f7dd57940042d69a6c43a0d17fdf3c951917a76eae8c1d93ba3
\ No newline at end of file
......@@ -10547,6 +10547,7 @@ CREATE TABLE approval_merge_request_rules (
report_type smallint,
section text,
modified_from_project_rule boolean DEFAULT false NOT NULL,
orchestration_policy_idx smallint,
CONSTRAINT check_6fca5928b2 CHECK ((char_length(section) <= 255))
);
......@@ -10616,7 +10617,8 @@ CREATE TABLE approval_project_rules (
vulnerabilities_allowed smallint DEFAULT 0 NOT NULL,
severity_levels text[] DEFAULT '{}'::text[] NOT NULL,
report_type smallint,
vulnerability_states text[] DEFAULT '{newly_detected}'::text[] NOT NULL
vulnerability_states text[] DEFAULT '{newly_detected}'::text[] NOT NULL,
orchestration_policy_idx smallint
);
CREATE TABLE approval_project_rules_groups (
......@@ -83,7 +83,7 @@ class ApprovalProjectRule < ApplicationRecord
def report_approver_attributes
attributes
.slice('approvals_required', 'name')
.slice('approvals_required', 'name', 'orchestration_policy_idx')
.merge(
users: users,
groups: groups,
......
......@@ -247,10 +247,10 @@ class ApprovalState
def wrapped_rules
strong_memoize(:wrapped_rules) do
merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch)
grouped_merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch).group_by(&:report_type)
merge_request_rules.map! do |rule|
ApprovalWrappedRule.wrap(merge_request, rule)
grouped_merge_request_rules.flat_map do |report_type, merge_request_rules|
Approvals::WrappedRuleSet.wrap(merge_request, merge_request_rules, report_type).wrapped_rules
end
end
end
......
......@@ -23,7 +23,7 @@ class ApprovalWrappedRule
def_delegators(:@approval_rule,
:regular?, :any_approver?, :code_owner?, :report_approver?,
:overridden?, :id, :name, :users, :groups, :code_owner,
:overridden?, :id, :users, :groups, :code_owner,
:source_rule, :rule_type, :approvals_required, :section, :to_global_id)
def self.wrap(merge_request, rule)
......@@ -109,6 +109,12 @@ class ApprovalWrappedRule
approvers - approved_approvers
end
def name
return approval_rule.name unless approval_rule.scan_finding?
approval_rule.name.gsub(/\s\d+\z/, '')
end
private
def filter_approvers(approvers)
......
# frozen_string_literal: true
module Approvals
class ScanFindingWrappedRuleSet < WrappedRuleSet
extend ::Gitlab::Utils::Override
override :wrapped_rules
def wrapped_rules
strong_memoize(:wrapped_rules) do
if ::Feature.enabled?(:scan_result_policy, merge_request.project)
grouped_merge_request_rules = approval_rules.group_by(&:orchestration_policy_idx)
grouped_merge_request_rules.map do |_, merge_request_rules|
wrapped_rules_sorted_by_approval(merge_request_rules).first
end
else
[]
end
end
end
private
def wrapped_rules_sorted_by_approval(merge_request_rules)
merge_request_rules.map! do |rule|
ApprovalWrappedRule.wrap(merge_request, rule)
end
merge_request_rules.sort_by {|rule| rule.approved? ? 1 : 0}
end
end
end
# frozen_string_literal: true
module Approvals
class WrappedRuleSet
include Gitlab::Utils::StrongMemoize
attr_reader :merge_request, :approval_rules
def self.wrap(merge_request, rules, report_type)
if report_type == Security::ScanResultPolicy::SCAN_FINDING
ScanFindingWrappedRuleSet.new(merge_request, rules)
else
WrappedRuleSet.new(merge_request, rules)
end
end
def initialize(merge_request, approval_rules)
@merge_request = merge_request
@approval_rules = approval_rules
end
def wrapped_rules
strong_memoize(:wrapped_rules) do
approval_rules.map do |rule|
ApprovalWrappedRule.wrap(merge_request, rule)
end
end
end
end
end
......@@ -5,9 +5,10 @@ module Security
class ProcessScanResultPolicyService
MAX_LENGTH = 25
def initialize(policy_configuration:, policy:)
def initialize(policy_configuration:, policy:, policy_index:)
@policy_configuration = policy_configuration
@policy = policy
@policy_index = policy_index
@project = policy_configuration.project
@author = policy_configuration.policy_last_updated_by
end
......@@ -20,7 +21,7 @@ module Security
private
attr_reader :policy_configuration, :policy, :project, :author
attr_reader :policy_configuration, :policy, :project, :author, :policy_index
def create_new_approval_rules
action_info = policy[:actions].find { |action| action[:type] == Security::ScanResultPolicy::REQUIRE_APPROVAL }
......@@ -43,12 +44,13 @@ module Security
severity_levels: rule[:severity_levels],
user_ids: project.users.get_ids_by_username(action_info[:approvers]),
vulnerabilities_allowed: rule[:vulnerabilities_allowed],
report_type: :scan_finding
report_type: :scan_finding,
orchestration_policy_idx: policy_index
}
end
def rule_name(policy_name, rule_index)
truncated = policy_name.truncate(MAX_LENGTH, omission: '')
truncated = policy_name.truncate(MAX_LENGTH)
return truncated if rule_index == 0
"#{truncated} #{rule_index + 1}"
......
......@@ -28,9 +28,9 @@ module Security
configuration.transaction do
configuration.approval_rules.scan_finding.delete_all
configuration.active_scan_result_policies.each do |policy|
configuration.active_scan_result_policies.each_with_index do |policy, policy_index|
Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyService
.new(policy_configuration: configuration, policy: policy)
.new(policy_configuration: configuration, policy: policy, policy_index: policy_index)
.execute
end
end
......
......@@ -139,7 +139,7 @@ RSpec.describe ApprovalProjectRule do
context "when there is a project rule for each report type" do
with_them do
subject { create(:approval_project_rule, report_type, :requires_approval, project: project) }
subject { create(:approval_project_rule, report_type, :requires_approval, project: project, orchestration_policy_idx: 1) }
let!(:result) { subject.apply_report_approver_rules_to(merge_request) }
......@@ -149,6 +149,7 @@ RSpec.describe ApprovalProjectRule do
specify { expect(result.name).to be(:default_name) }
specify { expect(result.rule_type).to be(:report_approver) }
specify { expect(result.report_type).to be(:report_type) }
specify { expect(result.orchestration_policy_idx).to be 1 }
end
end
end
......
......@@ -173,6 +173,24 @@ RSpec.describe ApprovalState do
end
end
context 'with multiple scan_finding rules' do
before do
2.times {create_rule({ rule_type: :report_approver, report_type: :scan_finding })}
2.times {create_rule({ rule_type: :report_approver, report_type: :scan_finding, orchestration_policy_idx: 1 })}
end
it 'returns one rule for each orchestration_policy_idx' do
approval_rules = subject.wrapped_approval_rules
expect(approval_rules.count).to be(2)
expect(approval_rules).to all(be_instance_of(ApprovalWrappedRule))
policy_indices = approval_rules.map { |rule| rule.approval_rule.orchestration_policy_idx }
expect(policy_indices).to contain_exactly(nil, 1)
end
end
describe '#approval_needed?' do
context 'when feature not available' do
it 'returns false' do
......
......@@ -201,4 +201,27 @@ RSpec.describe ApprovalWrappedRule do
expect(subject.approvals_required).to eq(19)
end
end
describe '#name' do
let(:rule_name) { 'approval rule 2' }
let(:rule) { create(:approval_merge_request_rule, report_type: report_type, name: rule_name) }
context 'with report_type set to scan_finding' do
let(:report_type) { :scan_finding }
let(:expected_rule_name) { 'approval rule' }
it 'returns rule name without the sequential notation' do
expect(subject.name).not_to eq(rule_name)
expect(subject.name).to eq(expected_rule_name)
end
end
context 'with report_type other than scan_finding' do
let(:report_type) { :vulnerability }
it 'returns rule name as is' do
expect(subject.name).to eq(rule_name)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Approvals::ScanFindingWrappedRuleSet do
let(:report_type) { Security::ScanResultPolicy::SCAN_FINDING }
let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:approver) { create(:user) }
let_it_be(:approval_rules) { create_list(:approval_merge_request_rule, 2, :scan_finding, merge_request: merge_request, users: [approver]) }
let(:approval_rules_list) { approval_rules }
subject { described_class.wrap(merge_request, approval_rules_list, report_type).wrapped_rules }
describe '#wrapped_rules' do
context 'with feature flag disabled' do
before do
stub_feature_flags(scan_result_policy: false)
end
it {is_expected.to be_empty }
end
it 'returns only one rule' do
expect(subject.count).to be 1
end
context 'with various orchestration_policy_idx' do
let(:orchestration_policy_idx) { 0 }
let(:approval_rules_w_policy_idx) { create_list(:approval_merge_request_rule, 2, :scan_finding, merge_request: merge_request, orchestration_policy_idx: orchestration_policy_idx, users: [approver]) }
let(:approval_rules_list) { approval_rules + approval_rules_w_policy_idx }
it 'returns one rule for each orchestration_policy_idx' do
expect(subject.count).to be 2
orchestration_policy_indices = subject.map { |wrapped_rule| wrapped_rule.approval_rule.orchestration_policy_idx }
expect(orchestration_policy_indices).to contain_exactly(nil, orchestration_policy_idx)
end
context 'with unapproved rules' do
let(:unapproved_rule) { create(:approval_merge_request_rule, :scan_finding, merge_request: merge_request, orchestration_policy_idx: orchestration_policy_idx, users: [approver], approvals_required: 5) }
let(:approval_rules_list) { approval_rules + approval_rules_w_policy_idx + [unapproved_rule] }
it 'returns sorted based on approval' do
selected_rules = subject.select { |wrapped_rule| wrapped_rule.approval_rule.orchestration_policy_idx == orchestration_policy_idx }
expect(selected_rules.count).to be 1
expect(selected_rules.first.id).to be unapproved_rule.id
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Approvals::WrappedRuleSet do
let(:report_type) { nil }
let(:merge_request) { build(:merge_request) }
let(:approval_merge_request_rule) { build(:approval_merge_request_rule, merge_request: merge_request, report_type: report_type) }
let(:grouped_approval_wrapped_rules) { described_class.wrap(merge_request, [approval_merge_request_rule], report_type) }
describe '.wrap' do
subject { grouped_approval_wrapped_rules }
context "with report_type set to #{Security::ScanResultPolicy::SCAN_FINDING}" do
let(:report_type) { Security::ScanResultPolicy::SCAN_FINDING }
it { is_expected.to be_instance_of(Approvals::ScanFindingWrappedRuleSet) }
end
context 'with any other report_type' do
it { is_expected.to be_instance_of(described_class) }
end
end
describe '#wrapped_rules' do
subject { grouped_approval_wrapped_rules.wrapped_rules }
it 'returns an array of ApprovalWrappedRule' do
expect(subject.count).to be 1
expect(subject.first).to be_instance_of(ApprovalWrappedRule)
end
it "returns ApprovalWrappedRule with attributes as provided to #{described_class.name}" do
expect(subject.first).to have_attributes(merge_request: merge_request, approval_rule: approval_merge_request_rule)
end
end
end
......@@ -10,7 +10,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyS
let(:policy) { build(:scan_result_policy, name: 'Test Policy') }
let(:policy_yaml) { Gitlab::Config::Loader::Yaml.new(policy.to_yaml).load! }
let(:project) { policy_configuration.project }
let(:service) { described_class.new(policy_configuration: policy_configuration, policy: policy) }
let(:service) { described_class.new(policy_configuration: policy_configuration, policy: policy, policy_index: 0) }
before do
allow(policy_configuration).to receive(:policy_last_updated_by).and_return(project.owner)
......
......@@ -63,9 +63,9 @@ RSpec.describe Security::CreateOrchestrationPolicyWorker do
end
end
active_policies[:scan_result_policy].each do |policy|
active_policies[:scan_result_policy].each_with_index do |policy, policy_index|
expect_next_instance_of(Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyService,
policy_configuration: configuration, policy: policy) do |service|
policy_configuration: configuration, policy: policy, policy_index: policy_index) do |service|
expect(service).to receive(:execute)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment