diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 893be6b11cfa6030bd1edb264faf904bbc952eba..b6ae765d6a9c41793961b25a2d1f3ed88d9b99b7 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -863,6 +863,8 @@ - 1 - - security_create_security_policy_project - 1 +- - security_delete_approval_policy_rules + - 1 - - security_delete_orchestration_configuration - 1 - - security_delete_security_policy diff --git a/ee/app/models/concerns/security/policy_rule.rb b/ee/app/models/concerns/security/policy_rule.rb index 8f2cff31a15d3c8e645bfa45dec2c01c617e02ce..c429413d21f5f3c79ab00d2fa31097ecd92bc81d 100644 --- a/ee/app/models/concerns/security/policy_rule.rb +++ b/ee/app/models/concerns/security/policy_rule.rb @@ -16,6 +16,7 @@ def self.for_policy_type(policy_type) included do self.inheritance_column = :_type_disabled + scope :deleted, -> { where('rule_index < 0') } scope :undeleted, -> { where('rule_index >= 0') } end diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 964e5d482b45d99d799aa73fae73061a95a8f666..77a2be4dcf83230dfeaca19211931749bb76402c 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -179,18 +179,28 @@ def content_by_type end end - def rules + def all_rules if type_approval_policy? - approval_policy_rules.undeleted + approval_policy_rules elsif type_scan_execution_policy? - scan_execution_policy_rules.undeleted + scan_execution_policy_rules elsif type_vulnerability_management_policy? - vulnerability_management_policy_rules.undeleted - else - [] + vulnerability_management_policy_rules end end + def rules + Array.wrap(all_rules&.undeleted) + end + + def max_rule_index + all_rules&.maximum("ABS(rule_index)") || 0 + end + + def next_rule_index + rules.empty? ? 0 : (rules.maximum(:rule_index) + 1) + end + def scope_applicable?(project) strong_memoize_with(:scope_applicable, project) do policy_scope_checker = Security::SecurityOrchestrationPolicies::PolicyScopeChecker.new(project: project) diff --git a/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb b/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb index b935b1b47fa9adce8e160edb54fa6a0b8ad46c58..88c6f0e6dea340b4437e2356fe8f7a4d69ba4010 100644 --- a/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb +++ b/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb @@ -91,6 +91,10 @@ def sync_merge_request_rules def delete_approval_rules(approval_policy_rules) security_policy.delete_approval_policy_rules_for_project(project, approval_policy_rules) + + return if Security::ApprovalPolicyRuleProjectLink.for_policy_rules(approval_policy_rules.select(:id)).exists? + + Security::DeleteApprovalPolicyRulesWorker.perform_in(1.minute, approval_policy_rules.map(&:id)) end def update_approval_rules(approval_policy_rules) diff --git a/ee/app/services/security/security_orchestration_policies/update_security_policies_service.rb b/ee/app/services/security/security_orchestration_policies/update_security_policies_service.rb index 507e3288fbdd8b5095e827abb4d3880fe35af8e3..23a11958e2f9e4e994938919f4f51d048cf3374a 100644 --- a/ee/app/services/security/security_orchestration_policies/update_security_policies_service.rb +++ b/ee/app/services/security/security_orchestration_policies/update_security_policies_service.rb @@ -33,12 +33,9 @@ def update_policy_attributes!(db_policy, yaml_policy) def update_policy_rules(policy, rules_diff) return unless rules_diff - if rules_diff.deleted.any? - mark_rules_for_deletion(rules_diff.deleted, rules_diff.deleted.count + rules_diff.updated.count) - end - + mark_rules_for_deletion(policy, rules_diff.deleted) update_existing_rules(policy, rules_diff.updated) - create_new_rules(policy, rules_diff.created, rules_diff.updated.count) + create_new_rules(policy, rules_diff.created) end def update_existing_rules(policy, updated_rules) @@ -48,19 +45,18 @@ def update_existing_rules(policy, updated_rules) end end - def create_new_rules(policy, created_rules, existing_rules_count) + def create_new_rules(policy, created_rules) + new_index = policy.next_rule_index created_rules.each_with_index do |rule_hash, index| - new_index = existing_rules_count + index - policy.upsert_rule(new_index, rule_hash) + policy.upsert_rule(new_index + index, rule_hash) end end - def mark_rules_for_deletion(deleted_rules, old_rules_count) + def mark_rules_for_deletion(policy, deleted_rules) + new_index = policy.max_rule_index || 1 deleted_rules.each_with_index do |rule_diff, index| rule_record = rule_diff.from - - new_index = old_rules_count + index - rule_record.update!(rule_index: -new_index) + rule_record.update!(rule_index: -(new_index + index)) end end end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 1e006daa108748c331f01843b219c04034d8df2c..de3c78ccfb05bdf15d093196db20ecb8e069579f 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -3063,6 +3063,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: security_delete_approval_policy_rules + :worker_name: Security::DeleteApprovalPolicyRulesWorker + :feature_category: :security_policy_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: security_delete_orchestration_configuration :worker_name: Security::DeleteOrchestrationConfigurationWorker :feature_category: :security_policy_management diff --git a/ee/app/workers/security/delete_approval_policy_rules_worker.rb b/ee/app/workers/security/delete_approval_policy_rules_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..b67a151901f64edeb8fd930d42e0d1513667bac6 --- /dev/null +++ b/ee/app/workers/security/delete_approval_policy_rules_worker.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Security + class DeleteApprovalPolicyRulesWorker + include ApplicationWorker + + ProjectLinkExistsError = Class.new(StandardError) + + feature_category :security_policy_management + data_consistency :sticky + deduplicate :until_executed + idempotent! + + def perform(approval_policy_rule_ids) + if Security::ApprovalPolicyRuleProjectLink.for_policy_rules(approval_policy_rule_ids).exists? + # Raising an error here will make sure that the worker is retried. + raise ProjectLinkExistsError, "Approval policy rules are still linked to projects" + end + + Security::ApprovalPolicyRule.id_in(approval_policy_rule_ids).deleted.delete_all + end + end +end diff --git a/ee/spec/models/security/approval_policy_rule_spec.rb b/ee/spec/models/security/approval_policy_rule_spec.rb index 754c4597d2146a6f18209da00e78446a9adf39b7..0c858e51501937202309c5e98bc8dc01a82d7a7d 100644 --- a/ee/spec/models/security/approval_policy_rule_spec.rb +++ b/ee/spec/models/security/approval_policy_rule_spec.rb @@ -103,6 +103,19 @@ end end + describe '.deleted' do + let_it_be(:rule_with_positive_index) { create(:approval_policy_rule, rule_index: 1) } + let_it_be(:rule_with_zero_index) { create(:approval_policy_rule, rule_index: 0) } + let_it_be(:rule_with_negative_index) { create(:approval_policy_rule, rule_index: -1) } + + it 'returns rules with rule_index lesser than 0' do + result = described_class.deleted + + expect(result).to contain_exactly(rule_with_negative_index) + expect(result).not_to include(rule_with_positive_index, rule_with_zero_index) + end + end + describe '.undeleted' do let_it_be(:rule_with_positive_index) { create(:approval_policy_rule, rule_index: 1) } let_it_be(:rule_with_zero_index) { create(:approval_policy_rule, rule_index: 0) } diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index bba27896d2a5a44a9b77ab14e8cd6a4e21d935dc..731afbe1246d10d7a42498a2884f1ef99d15fd8a 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -509,6 +509,47 @@ end end + describe '#max_rule_index' do + let_it_be(:policy) { create(:security_policy) } + let_it_be(:rule1) { create(:approval_policy_rule, security_policy: policy, rule_index: 0) } + let_it_be(:rule2) { create(:approval_policy_rule, security_policy: policy, rule_index: -2) } + let_it_be(:rule3) { create(:approval_policy_rule, security_policy: policy, rule_index: 1) } + + it 'returns the maximum absolute rule index' do + expect(policy.max_rule_index).to eq(2) + end + + context 'when all_rules is nil' do + before do + allow(policy).to receive(:all_rules).and_return(nil) + end + + it 'returns zero' do + expect(policy.max_rule_index).to eq(0) + end + end + end + + describe '#next_rule_index' do + let_it_be(:policy) { create(:security_policy) } + + context 'when there are no rules' do + it 'returns 0' do + expect(policy.next_rule_index).to eq(0) + end + end + + context 'when there are existing rules' do + let_it_be(:rule1) { create(:approval_policy_rule, security_policy: policy, rule_index: 0) } + let_it_be(:rule2) { create(:approval_policy_rule, security_policy: policy, rule_index: 1) } + let_it_be(:deleted_rule) { create(:approval_policy_rule, security_policy: policy, rule_index: -1) } + + it 'returns the next available index' do + expect(policy.next_rule_index).to eq(2) + end + end + end + describe '#scope_applicable?' do let_it_be(:project) { create(:project) } let(:policy) { build(:security_policy) } diff --git a/ee/spec/services/security/security_orchestration_policies/persist_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/persist_policy_service_spec.rb index 154ce199c468779d12576d4d32496e71d0359b99..785140c289a72d216c2e3992c14cf1c2be2ef87b 100644 --- a/ee/spec/services/security/security_orchestration_policies/persist_policy_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/persist_policy_service_spec.rb @@ -325,7 +325,7 @@ def persist!(policies) expect { persist }.to change { policy_configuration .security_policies.order(policy_index: :asc).flat_map(&:approval_policy_rules).flat_map(&:rule_index) - }.from(contain_exactly(0, 1)).to(contain_exactly(0, -2)) + }.from(contain_exactly(0, 1)).to(contain_exactly(0, -1)) end it 'calls EventPublisher with deleted policies' do diff --git a/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb index f23ee855a2c4352beca3e1e846881888518afe61..bfeaa7ed6eabdd30281b923e5972b4dc8cd16190 100644 --- a/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb @@ -469,6 +469,7 @@ describe '#delete_rules' do let_it_be(:approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } + let_it_be(:other_approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } let_it_be(:project_approval_rule) do create(:approval_project_rule, :scan_finding, @@ -491,6 +492,25 @@ expect { delete_rules }.to change { project.approval_rules.count }.by(-1) end + it 'schedules DeleteApprovalPolicyRulesWorker when rules are not linked to projects' do + expect(Security::DeleteApprovalPolicyRulesWorker).to receive(:perform_in) + .with(1.minute, [approval_policy_rule.id, other_approval_policy_rule.id]) + + delete_rules + end + + context 'when rules are linked to projects' do + before do + create(:approval_policy_rule_project_link, approval_policy_rule: approval_policy_rule, project: project) + end + + it 'does not schedule DeleteApprovalPolicyRulesWorker' do + expect(Security::DeleteApprovalPolicyRulesWorker).not_to receive(:perform_in) + + delete_rules + end + end + it_behaves_like 'calls sync_merge_requests' do let(:action) { delete_rules } end diff --git a/ee/spec/services/security/security_orchestration_policies/update_security_policies_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/update_security_policies_service_spec.rb index 2122cbcff1288101da034fd309b1c2562b9d970a..dd415e9f84ae6230cf0d863c784b8deba5a94f52 100644 --- a/ee/spec/services/security/security_orchestration_policies/update_security_policies_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/update_security_policies_service_spec.rb @@ -107,6 +107,23 @@ end end + context 'when all rules are deleted' do + let(:yaml_policy) do + { + name: 'Updated Policy', + description: 'Updated description', + rules: [] + } + end + + it 'sets the rule_index to negative values' do + service.execute + + expect(approval_policy_rule_1.reload.rule_index).to eq(-2) + expect(approval_policy_rule_2.reload.rule_index).to eq(-3) + end + end + context 'when there are updated rules' do let(:yaml_policy) do { @@ -123,6 +140,25 @@ end end + context 'when there are updated and created rules' do + let(:yaml_policy) do + { + name: 'Updated Policy', + description: 'Updated description', + rules: [rule_content_1, rule_content_3, rule_content_2] + } + end + + it 'updates the existing rule and creates a new rule' do + service.execute + + last_rule = Security::ApprovalPolicyRule.last + expect(last_rule.typed_content.deep_symbolize_keys).to eq(rule_content_2) + expect(last_rule.rule_index).to eq(3) + expect(approval_policy_rule_2.reload.typed_content.deep_symbolize_keys).to eq(rule_content_3) + end + end + context 'when the content of pipeline execution policy gets updated' do let_it_be(:old_config_project) { create(:project, :empty_repo) } let_it_be(:new_config_project) { create(:project, :empty_repo) } diff --git a/ee/spec/workers/security/delete_approval_policy_rules_worker_spec.rb b/ee/spec/workers/security/delete_approval_policy_rules_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..03744900434769e49351f5e531be5ba015676e4d --- /dev/null +++ b/ee/spec/workers/security/delete_approval_policy_rules_worker_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::DeleteApprovalPolicyRulesWorker, feature_category: :security_policy_management do + let_it_be(:approval_policy_rule) { create(:approval_policy_rule, rule_index: -1) } + let_it_be(:other_approval_policy_rule) { create(:approval_policy_rule, rule_index: -2) } + let_it_be(:project) { create(:project) } + + let(:approval_policy_rule_ids) { [approval_policy_rule.id] } + + describe '#perform' do + subject(:perform) { described_class.new.perform(approval_policy_rule_ids) } + + context 'when approval policy rules are linked to projects' do + before do + create(:approval_policy_rule_project_link, approval_policy_rule: approval_policy_rule, project: project) + end + + it 'raises ProjectLinkExistsError' do + expect { perform }.to raise_error(described_class::ProjectLinkExistsError) + end + end + + context 'when approval policy rules are not linked to projects' do + it 'deletes only the specified deleted approval policy rules' do + perform + + expect(Security::ApprovalPolicyRule.exists?(approval_policy_rule.id)).to be false + expect(Security::ApprovalPolicyRule.exists?(other_approval_policy_rule.id)).to be true + end + end + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [approval_policy_rule_ids] } + end + end +end