diff --git a/db/migrate/20211108204736_add_policy_idx_to_approval_project_rule.rb b/db/migrate/20211108204736_add_policy_idx_to_approval_project_rule.rb
new file mode 100644
index 0000000000000000000000000000000000000000..90e5fa34817d5b7dd4eb055b55ee5b6ee54dda9a
--- /dev/null
+++ b/db/migrate/20211108204736_add_policy_idx_to_approval_project_rule.rb
@@ -0,0 +1,9 @@
+# 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
diff --git a/db/migrate/20211122215001_add_policy_idx_to_approval_merge_request_rule.rb b/db/migrate/20211122215001_add_policy_idx_to_approval_merge_request_rule.rb
new file mode 100644
index 0000000000000000000000000000000000000000..b1c7bc4d5cea22cd303541a9e184e748cdbf9b70
--- /dev/null
+++ b/db/migrate/20211122215001_add_policy_idx_to_approval_merge_request_rule.rb
@@ -0,0 +1,9 @@
+# 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
diff --git a/db/schema_migrations/20211108204736 b/db/schema_migrations/20211108204736
new file mode 100644
index 0000000000000000000000000000000000000000..6d37b1b118475b85867539ec08f46f2ba14995e0
--- /dev/null
+++ b/db/schema_migrations/20211108204736
@@ -0,0 +1 @@
+9e01b1817e4c578f5be7d7378dc12a8535c2bbbff5ecbc77f5a7cfdb148927f5
\ No newline at end of file
diff --git a/db/schema_migrations/20211122215001 b/db/schema_migrations/20211122215001
new file mode 100644
index 0000000000000000000000000000000000000000..be0fd652eb7b5649781a5073a8192747dbe05c48
--- /dev/null
+++ b/db/schema_migrations/20211122215001
@@ -0,0 +1 @@
+fc29e10717357f7dd57940042d69a6c43a0d17fdf3c951917a76eae8c1d93ba3
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 9a57719ec243364b14f52be8ff2d628a7ef16eec..0c76dab7af9e972606b5db73be5ce4663646a51e 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -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 (
diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
index 55dbf72e3b8864011b40c23bdaff85a542f6e04c..fd1c95c65cbb6781252cc5f01ee042eac3e10e9e 100644
--- a/ee/app/models/approval_project_rule.rb
+++ b/ee/app/models/approval_project_rule.rb
@@ -83,7 +83,7 @@ def vulnerability_states_for_branch(branch = project.default_branch)
def report_approver_attributes
attributes
- .slice('approvals_required', 'name')
+ .slice('approvals_required', 'name', 'orchestration_policy_idx')
.merge(
users: users,
groups: groups,
diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index cdde3fbd211ec0b422a714e01229c0034e8ce2e0..1aa1fa57f00a21f057c77858a611ce958c826ded 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -247,10 +247,10 @@ def any_approver_approval_rules
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
diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb
index 4aaeb5cb8bd97a56ec5b1f855f02ca22e90ce4fc..f968f166e65c00c5c3bea5fa6eaa1aa4f2aa671f 100644
--- a/ee/app/models/approval_wrapped_rule.rb
+++ b/ee/app/models/approval_wrapped_rule.rb
@@ -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 @@ def unactioned_approvers
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)
diff --git a/ee/app/models/approvals/scan_finding_wrapped_rule_set.rb b/ee/app/models/approvals/scan_finding_wrapped_rule_set.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ff18b4d6375dd0f9cddbdb33f87b09f47cac6589
--- /dev/null
+++ b/ee/app/models/approvals/scan_finding_wrapped_rule_set.rb
@@ -0,0 +1,29 @@
+# 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
diff --git a/ee/app/models/approvals/wrapped_rule_set.rb b/ee/app/models/approvals/wrapped_rule_set.rb
new file mode 100644
index 0000000000000000000000000000000000000000..f9cf1f68003d34f30727414ef1f2862f034d5408
--- /dev/null
+++ b/ee/app/models/approvals/wrapped_rule_set.rb
@@ -0,0 +1,30 @@
+# 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
diff --git a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
index d0fe5d2276cf9f30da0a4fd95773e1d9aac7ee0d..8503d9a58ef8f8f0c052f2f5df521ce4273e97fb 100644
--- a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
@@ -5,9 +5,10 @@ module SecurityOrchestrationPolicies
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 @@ def execute
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 @@ def rule_params(rule, rule_index, action_info)
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}"
diff --git a/ee/app/workers/security/create_orchestration_policy_worker.rb b/ee/app/workers/security/create_orchestration_policy_worker.rb
index dee9b8105ce0e96e9fa63180d0444e40dfac555a..6313818cb1ac54b981e9d9dcaf18683074e28489 100644
--- a/ee/app/workers/security/create_orchestration_policy_worker.rb
+++ b/ee/app/workers/security/create_orchestration_policy_worker.rb
@@ -28,9 +28,9 @@ def perform
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
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
index dd6bfcc927e2136f4b46da777bffe4c9252ad456..d759b54cbe10bce694110de7b22a4125a5fc74b6 100644
--- a/ee/spec/models/approval_project_rule_spec.rb
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -139,7 +139,7 @@
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 @@
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
diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb
index b6c01df3e28af10e2993f81e820145950b79aea6..ec79738724e8b316e74bb37d8e944e9ea01665e2 100644
--- a/ee/spec/models/approval_state_spec.rb
+++ b/ee/spec/models/approval_state_spec.rb
@@ -173,6 +173,24 @@ def create_project_member(role, user_attrs = {})
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
diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb
index 5fd0a4559e3068c32ed04bcde5ff07a731bb5a25..361b9079c861930f6fb95abc0d16e732929d929b 100644
--- a/ee/spec/models/approval_wrapped_rule_spec.rb
+++ b/ee/spec/models/approval_wrapped_rule_spec.rb
@@ -201,4 +201,27 @@ def approved_approvers_for_rule_id(rule_id)
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
diff --git a/ee/spec/models/approvals/scan_finding_wrapped_rule_set_spec.rb b/ee/spec/models/approvals/scan_finding_wrapped_rule_set_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..9fa7d21e5e8ca38f7ccffecf68f2e6183a48cd59
--- /dev/null
+++ b/ee/spec/models/approvals/scan_finding_wrapped_rule_set_spec.rb
@@ -0,0 +1,54 @@
+# 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
diff --git a/ee/spec/models/approvals/wrapped_rule_set_spec.rb b/ee/spec/models/approvals/wrapped_rule_set_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ebea102beecd09b06c5bc08973c5a7fff0cea23a
--- /dev/null
+++ b/ee/spec/models/approvals/wrapped_rule_set_spec.rb
@@ -0,0 +1,37 @@
+# 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
diff --git a/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb
index 2542510ce651f857287abf464700d9be07e969a8..ac170028f8ab1f90dad5b358e6869d4331da99e7 100644
--- a/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb
@@ -10,7 +10,7 @@
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)
diff --git a/ee/spec/workers/security/create_orchestration_policy_worker_spec.rb b/ee/spec/workers/security/create_orchestration_policy_worker_spec.rb
index fbc1c91ac6c246f505012cf1c2c3f020d5bd3b13..23e4045586b21f6d1a0e582dbae9c255b96e02aa 100644
--- a/ee/spec/workers/security/create_orchestration_policy_worker_spec.rb
+++ b/ee/spec/workers/security/create_orchestration_policy_worker_spec.rb
@@ -63,9 +63,9 @@
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