diff --git a/app/events/project_authorizations/authorizations_added_event.rb b/app/events/project_authorizations/authorizations_added_event.rb
new file mode 100644
index 0000000000000000000000000000000000000000..521a862218d307e53db6fb4a5d833e152e2957d9
--- /dev/null
+++ b/app/events/project_authorizations/authorizations_added_event.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module ProjectAuthorizations
+ class AuthorizationsAddedEvent < ::Gitlab::EventStore::Event
+ def schema
+ {
+ 'type' => 'object',
+ 'required' => %w[project_id user_ids],
+ 'properties' => {
+ 'project_id' => { 'type' => 'integer' },
+ 'user_ids' => { 'type' => 'array' }
+ }
+ }
+ end
+ end
+end
diff --git a/app/models/project_authorizations/changes.rb b/app/models/project_authorizations/changes.rb
index ac52bdfdb07dc75937a294a6f12b5bc7a2e68e7d..256207d2cc74aa0bed2863e754cc6ec98cd3a3cf 100644
--- a/app/models/project_authorizations/changes.rb
+++ b/app/models/project_authorizations/changes.rb
@@ -21,6 +21,7 @@ def initialize
@authorizations_to_add = []
@affected_project_ids = Set.new
@removed_user_ids = Set.new
+ @added_user_ids = Set.new
yield self
end
@@ -61,6 +62,7 @@ def should_delete_authorizations_for_project?
def add_authorizations
insert_all_in_batches(authorizations_to_add)
@affected_project_ids += authorizations_to_add.pluck(:project_id)
+ @added_user_ids += authorizations_to_add.pluck(:user_id)
end
def delete_authorizations_for_user
@@ -139,23 +141,52 @@ def user_ids
end
def publish_events
+ publish_changed_event
+ publish_removed_event
+ publish_added_event
+ end
+
+ def publish_changed_event
+ # This event is used to add policy approvers to approval rules by re-syncing all project policies which is costly.
+ # If the feature flag below is enabled, the policies won't be re-synced and
+ # the approvers will be added via `AuthorizationsAddedEvent`.
+ return if ::Feature.enabled?(:add_policy_approvers_to_rules)
+
@affected_project_ids.each do |project_id|
::Gitlab::EventStore.publish(
::ProjectAuthorizations::AuthorizationsChangedEvent.new(data: { project_id: project_id })
)
end
- return if ::Feature.disabled?(:user_approval_rules_removal) || @removed_user_ids.blank?
+ end
- @affected_project_ids.each do |project_id|
- @removed_user_ids.to_a.each_slice(EVENT_USER_BATCH_SIZE).each do |user_ids_batch|
- ::Gitlab::EventStore.publish(
- ::ProjectAuthorizations::AuthorizationsRemovedEvent.new(data: {
- project_id: project_id,
- user_ids: user_ids_batch
- })
- )
+ def publish_removed_event
+ return if ::Feature.disabled?(:user_approval_rules_removal)
+ return if @removed_user_ids.none?
+
+ events = @affected_project_ids.flat_map do |project_id|
+ @removed_user_ids.to_a.each_slice(EVENT_USER_BATCH_SIZE).map do |user_ids_batch|
+ ::ProjectAuthorizations::AuthorizationsRemovedEvent.new(data: {
+ project_id: project_id,
+ user_ids: user_ids_batch
+ })
+ end
+ end
+ ::Gitlab::EventStore.publish_group(events)
+ end
+
+ def publish_added_event
+ return if ::Feature.disabled?(:add_policy_approvers_to_rules)
+ return if @added_user_ids.none?
+
+ events = @affected_project_ids.flat_map do |project_id|
+ @added_user_ids.to_a.each_slice(EVENT_USER_BATCH_SIZE).map do |user_ids_batch|
+ ::ProjectAuthorizations::AuthorizationsAddedEvent.new(data: {
+ project_id: project_id,
+ user_ids: user_ids_batch
+ })
end
end
+ ::Gitlab::EventStore.publish_group(events)
end
end
end
diff --git a/config/feature_flags/development/add_policy_approvers_to_rules.yml b/config/feature_flags/development/add_policy_approvers_to_rules.yml
new file mode 100644
index 0000000000000000000000000000000000000000..1d5584c518141a991afd4bd52d06b108b3576dd1
--- /dev/null
+++ b/config/feature_flags/development/add_policy_approvers_to_rules.yml
@@ -0,0 +1,8 @@
+---
+name: add_policy_approvers_to_rules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138809
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/434385
+milestone: '16.8'
+type: development
+group: group::security policies
+default_enabled: false
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 34a41c39f3120d8443fbe14585fd62b79ee7f119..fc36a9d0b045a96a21d4a25e77b577a2e9a04b45 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -681,6 +681,8 @@
- 1
- - security_scan_execution_policies_rule_schedule
- 1
+- - security_scan_result_policies_add_approvers_to_rules
+ - 1
- - security_scan_result_policies_sync_any_merge_request_approval_rules
- 1
- - security_scan_result_policies_sync_project
diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
index 016f5b22a5019ec6ae406dee1f068f9665faa394..63543c554432c327c2a2a2547b41fe2c4c8e9c29 100644
--- a/ee/app/models/approval_project_rule.rb
+++ b/ee/app/models/approval_project_rule.rb
@@ -38,6 +38,7 @@ class ApprovalProjectRule < ApplicationRecord
scope :report_approver_without_scan_finding, -> { report_approver.where.not(report_type: [:scan_finding, :license_scanning]) }
scope :for_all_branches, -> { where.missing(:protected_branches).where(applies_to_all_protected_branches: false) }
scope :for_all_protected_branches, -> { where.missing(:protected_branches).where(applies_to_all_protected_branches: true) }
+ scope :for_project, ->(project_id) { where(project_id: project_id) }
alias_method :code_owner, :code_owner?
diff --git a/ee/app/models/concerns/security/scan_result_policy.rb b/ee/app/models/concerns/security/scan_result_policy.rb
index 715b19695c75e545d4025b1bfaeeb1d93be1ad75..a079a15b7a8f6341ba509e70fe87c5612e4e878e 100644
--- a/ee/app/models/concerns/security/scan_result_policy.rb
+++ b/ee/app/models/concerns/security/scan_result_policy.rb
@@ -65,6 +65,13 @@ def active_scan_result_policies
scan_result_policies&.select { |config| config[:enabled] }&.first(LIMIT)
end
+ def applicable_scan_result_policies_for_project(project)
+ strong_memoize_with(:applicable_scan_result_policies_for_project, project) do
+ policy_scope_service = ::Security::SecurityOrchestrationPolicies::PolicyScopeService.new(project: project)
+ active_scan_result_policies.select { |policy| policy_scope_service.policy_applicable?(policy) }
+ end
+ end
+
def scan_result_policies
policy_by_type(:scan_result_policy)
end
diff --git a/ee/app/services/security/scan_result_policies/add_approvers_to_rules_service.rb b/ee/app/services/security/scan_result_policies/add_approvers_to_rules_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..1e8f6469e848a4237d39ae161d952f2c06932f58
--- /dev/null
+++ b/ee/app/services/security/scan_result_policies/add_approvers_to_rules_service.rb
@@ -0,0 +1,107 @@
+# frozen_string_literal: true
+
+module Security
+ module ScanResultPolicies
+ class AddApproversToRulesService < BaseProjectService
+ INSERT_BATCH_SIZE = 100
+ APPROVAL_RULES_ATTRIBUTES = %i[id orchestration_policy_idx].freeze
+
+ def execute(user_ids)
+ params = build_bulk_insert_params(user_ids)
+ insert_users_to_approval_rules(params)
+ end
+
+ private
+
+ def build_bulk_insert_params(user_ids)
+ project_usernames = username_map(user_ids)
+
+ project.all_security_orchestration_policy_configurations.flat_map do |configuration|
+ policies = configuration.applicable_scan_result_policies_for_project(project)
+ next if policies.blank?
+
+ policy_indexes_for_users = policy_indexes_for_users(user_ids, policies, project_usernames)
+ referenced_policy_indexes = policy_indexes_for_users.values.flatten.uniq
+ next if referenced_policy_indexes.none?
+
+ build_approval_rules_for_users(configuration, policy_indexes_for_users, referenced_policy_indexes)
+ end.compact
+ end
+
+ def build_approval_rules_for_users(configuration, policy_indexes_for_users, referenced_policy_indexes)
+ project_rules = rules_by_idx(approval_project_rules(configuration, referenced_policy_indexes))
+ merge_request_rules = rules_by_idx(approval_merge_request_rules(configuration, referenced_policy_indexes))
+
+ policy_indexes_for_users.map do |user_id, policy_indexes|
+ project_rule_ids = project_rules.values_at(*policy_indexes).compact
+ merge_request_rule_ids = merge_request_rules.values_at(*policy_indexes).compact
+
+ build_user_approval_rules_params(user_id, project_rule_ids, merge_request_rule_ids)
+ end
+ end
+
+ def rules_by_idx(approval_rules)
+ approval_rules.to_h { |rule| [rule.orchestration_policy_idx, rule.id] }
+ end
+
+ def policy_indexes_for_users(user_ids, policies, project_usernames)
+ user_ids.select { |user_id| project_usernames[user_id].present? }.filter_map do |user_id|
+ policy_indexes = policy_indexes_for_user(policies, project_usernames, user_id)
+ next if policy_indexes.none?
+
+ [user_id, policy_indexes]
+ end.to_h
+ end
+
+ def policy_indexes_for_user(policies, usernames, user_id)
+ policies.filter_map.with_index do |policy, index|
+ index if policy[:actions]&.any? do |action|
+ action[:user_approvers_ids]&.include?(user_id) || action[:user_approvers]&.include?(usernames[user_id])
+ end
+ end
+ end
+
+ def username_map(user_ids)
+ project.team.users.id_in(user_ids).select(:id, :username).to_h { |user| [user.id, user.username] }
+ end
+
+ def build_user_approval_rules_params(user_id, project_rule_ids, merge_request_rule_ids)
+ project_rule_params = build_project_rule_params(project_rule_ids, user_id)
+ merge_request_rule_params = build_merge_request_rule_params(merge_request_rule_ids, user_id)
+
+ [project_rule_params, merge_request_rule_params]
+ end
+
+ def insert_users_to_approval_rules(bulk_insert_params)
+ ApplicationRecord.transaction do
+ bulk_insert_params.each_slice(INSERT_BATCH_SIZE) do |params_batch|
+ project_rule_params = params_batch.flat_map(&:first)
+ merge_request_rule_params = params_batch.flat_map(&:second)
+
+ ApprovalProjectRulesUser.insert_all(project_rule_params) if project_rule_params.present?
+ ApprovalMergeRequestRulesUser.insert_all(merge_request_rule_params) if merge_request_rule_params.present?
+ end
+ end
+ end
+
+ def approval_project_rules(configuration, policy_indexes)
+ configuration.approval_project_rules
+ .for_project(project.id).for_policy_index(policy_indexes).select(*APPROVAL_RULES_ATTRIBUTES)
+ end
+
+ def approval_merge_request_rules(configuration, policy_indexes)
+ configuration.approval_merge_request_rules.for_unmerged_merge_requests
+ .for_merge_request_project(project.id).for_policy_index(policy_indexes)
+ .select(*APPROVAL_RULES_ATTRIBUTES)
+ end
+
+ def build_project_rule_params(approval_rule_ids, user_id)
+ approval_rule_ids.map { |rule_id| { approval_project_rule_id: rule_id, user_id: user_id } }
+ end
+
+ def build_merge_request_rule_params(approval_rule_ids, user_id)
+ approval_rule_ids.map { |rule_id| { approval_merge_request_rule_id: rule_id, user_id: user_id } }
+ end
+ end
+ end
+end
diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml
index b32d344c50aae54fc48a35434e4f52d9437ee8ca..5b44994fd943c29ca6268ea67159a5a513d1f9db 100644
--- a/ee/app/workers/all_queues.yml
+++ b/ee/app/workers/all_queues.yml
@@ -1911,6 +1911,15 @@
:weight: 1
:idempotent: false
:tags: []
+- :name: security_scan_result_policies_add_approvers_to_rules
+ :worker_name: Security::ScanResultPolicies::AddApproversToRulesWorker
+ :feature_category: :security_policy_management
+ :has_external_dependencies: false
+ :urgency: :low
+ :resource_boundary: :unknown
+ :weight: 1
+ :idempotent: true
+ :tags: []
- :name: security_scan_result_policies_sync_any_merge_request_approval_rules
:worker_name: Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker
:feature_category: :security_policy_management
diff --git a/ee/app/workers/security/refresh_project_policies_worker.rb b/ee/app/workers/security/refresh_project_policies_worker.rb
index d04324d7a3f6a49da280c1c35d0e124f577aa8d2..0901f471bc697def98947920cfa36ba004f23694 100644
--- a/ee/app/workers/security/refresh_project_policies_worker.rb
+++ b/ee/app/workers/security/refresh_project_policies_worker.rb
@@ -15,6 +15,8 @@ class RefreshProjectPoliciesWorker
DELAY_INTERVAL = 30.seconds.to_i
def handle_event(event)
+ return if ::Feature.enabled?(:add_policy_approvers_to_rules)
+
project = Project.find_by_id(event.data[:project_id])
return unless project
diff --git a/ee/app/workers/security/scan_result_policies/add_approvers_to_rules_worker.rb b/ee/app/workers/security/scan_result_policies/add_approvers_to_rules_worker.rb
new file mode 100644
index 0000000000000000000000000000000000000000..b5af8f05208a62a4a5d77e992ff8a1e32583573d
--- /dev/null
+++ b/ee/app/workers/security/scan_result_policies/add_approvers_to_rules_worker.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Security
+ module ScanResultPolicies
+ class AddApproversToRulesWorker
+ include Gitlab::EventStore::Subscriber
+
+ data_consistency :sticky
+ feature_category :security_policy_management
+ idempotent!
+
+ def handle_event(event)
+ return if ::Feature.disabled?(:add_policy_approvers_to_rules)
+
+ user_ids = event.data[:user_ids]
+ return if user_ids.blank?
+
+ project_id = event.data[:project_id]
+ project = Project.find_by_id(project_id)
+
+ unless project
+ logger.info(structured_payload(message: 'Project not found.', project_id: project_id))
+ return
+ end
+
+ return unless project.licensed_feature_available?(:security_orchestration_policies)
+
+ Security::ScanResultPolicies::AddApproversToRulesService.new(project: project).execute(user_ids)
+ end
+ end
+ end
+end
diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb
index e0875d389f8e1a0611e1a9171364d77678b14964..d0a28c7f5c4144b4a55ddf29694b438bd4bc1f5b 100644
--- a/ee/lib/ee/gitlab/event_store.rb
+++ b/ee/lib/ee/gitlab/event_store.rb
@@ -44,6 +44,8 @@ def configure!(store)
delay: 1.minute
store.subscribe ::MergeRequests::RemoveUserApprovalRulesWorker,
to: ::ProjectAuthorizations::AuthorizationsRemovedEvent
+ store.subscribe ::Security::ScanResultPolicies::AddApproversToRulesWorker,
+ to: ::ProjectAuthorizations::AuthorizationsAddedEvent
store.subscribe ::Security::RefreshComplianceFrameworkSecurityPoliciesWorker,
to: ::Projects::ComplianceFrameworkChangedEvent
end
diff --git a/ee/spec/lib/ee/gitlab/event_store_spec.rb b/ee/spec/lib/ee/gitlab/event_store_spec.rb
index 53a4a3838d337b580913b7457b7496ff07680bff..5fc311900a5ae6db7e1d2351b80114fc3302c5a0 100644
--- a/ee/spec/lib/ee/gitlab/event_store_spec.rb
+++ b/ee/spec/lib/ee/gitlab/event_store_spec.rb
@@ -20,6 +20,7 @@
::Members::MembersAddedEvent,
::ProjectAuthorizations::AuthorizationsChangedEvent,
::ProjectAuthorizations::AuthorizationsRemovedEvent,
+ ::ProjectAuthorizations::AuthorizationsAddedEvent,
::Projects::ComplianceFrameworkChangedEvent
)
end
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
index aab79ca80d21b1475bd2f39bc0c43e3ba31cf4c6..dd812051267b52b137af3bbdf00ed9937172af54 100644
--- a/ee/spec/models/approval_project_rule_spec.rb
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -109,6 +109,18 @@
end
end
+ describe '.for_project' do
+ it 'returns approval rules belonging to a project' do
+ project1 = create(:project)
+ project2 = create(:project)
+
+ rule_for_project1 = create(:approval_project_rule, project: project1)
+ create(:approval_project_rule, project: project2)
+
+ expect(described_class.for_project(project1)).to contain_exactly(rule_for_project1)
+ end
+ end
+
describe '.regular_or_any_approver scope' do
it 'returns regular or any-approver rules' do
any_approver_rule = create(:approval_project_rule, rule_type: :any_approver)
diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb
index 1af5d1b2a50c66a4698b38eeb591de5b30e8a628..57cec4f5954f2fc2664465dddaf0148c87ccb9f7 100644
--- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb
+++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb
@@ -1436,6 +1436,35 @@
end
end
+ describe '#applicable_scan_result_policies_for_project' do
+ let_it_be(:namespace_settings) do
+ create(:namespace_settings, toggle_security_policies_policy_scope: true)
+ end
+
+ let_it_be(:group) { create(:group, namespace_settings: namespace_settings) }
+ let_it_be(:project) { create(:project, :repository, group: group) }
+ let(:policy_yaml) do
+ build(:orchestration_policy_yaml, scan_result_policy: [
+ build(:scan_result_policy, name: 'Active policy'),
+ build(:scan_result_policy, name: 'Disabled policy', enabled: false),
+ build(:scan_result_policy, name: 'Not applicable policy', policy_scope: {
+ projects: {
+ excluding: [{ id: project.id }]
+ }
+ })
+ ])
+ end
+
+ subject(:applicable_policies) do
+ security_orchestration_policy_configuration.applicable_scan_result_policies_for_project(project)
+ end
+
+ it 'returns only active applicable policies' do
+ expect(applicable_policies).to be_one
+ expect(applicable_policies.first[:name]).to eq('Active policy')
+ end
+ end
+
describe '#scan_result_policies' do
let(:policy_yaml) { fixture_file('security_orchestration.yml', dir: 'ee') }
diff --git a/ee/spec/services/security/scan_result_policies/add_approvers_to_rules_service_spec.rb b/ee/spec/services/security/scan_result_policies/add_approvers_to_rules_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..598388135274d28685b65219c6fafce70afa37a5
--- /dev/null
+++ b/ee/spec/services/security/scan_result_policies/add_approvers_to_rules_service_spec.rb
@@ -0,0 +1,221 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::ScanResultPolicies::AddApproversToRulesService, feature_category: :security_policy_management do
+ let_it_be(:user1) { create(:user) }
+ let_it_be(:user2) { create(:user) }
+ let_it_be(:user_not_referenced_in_policy) { create(:user) }
+
+ let_it_be(:namespace_settings) do
+ create(:namespace_settings, toggle_security_policies_policy_scope: true)
+ end
+
+ let_it_be(:namespace) do
+ create(:group, :with_security_orchestration_policy_configuration, namespace_settings: namespace_settings)
+ end
+
+ let_it_be(:project1) { create(:project, :repository, group: namespace) }
+ let_it_be(:project2) { create(:project, :repository, group: namespace) }
+ let_it_be_with_reload(:merge_request1) { create(:merge_request, source_project: project1, target_project: project1) }
+
+ let(:user_ids) { [user1.id, user2.id, user_not_referenced_in_policy.id] }
+ let(:configuration) { namespace.security_orchestration_policy_configuration }
+ let(:policies_yaml) do
+ build(:orchestration_policy_yaml, scan_result_policy: [
+ build(:scan_result_policy, name: 'Active policy', actions: [
+ { type: 'require_approval', approvals_required: 1,
+ user_approvers_ids: [user1.id], user_approvers: [user2.username] }
+ ]),
+ build(:scan_result_policy, name: 'Disabled policy', enabled: false, actions: [
+ { type: 'require_approval', approvals_required: 1, user_approvers_ids: [user1.id] }
+ ])
+ ])
+ end
+
+ let(:policies_not_applicable_yaml) do
+ build(:orchestration_policy_yaml, scan_result_policy: [
+ build(:scan_result_policy, name: 'Active policy',
+ actions: [
+ { type: 'require_approval', approvals_required: 1,
+ user_approvers_ids: [user1.id], user_approvers: [user2.username] }
+ ],
+ policy_scope: {
+ projects: {
+ excluding: [{ id: project1.id }]
+ }
+ })
+ ])
+ end
+
+ before do
+ allow_next_instance_of(Repository) do |repository|
+ allow(repository).to receive(:blob_data_at).and_return(policies_yaml)
+ end
+ end
+
+ describe '#execute' do
+ let(:project_rule_expected_to_change) do
+ create(:approval_project_rule, project: project1, security_orchestration_policy_configuration: configuration,
+ orchestration_policy_idx: 0)
+ end
+
+ let!(:merge_request_rule_expected_to_change) do
+ create(:approval_merge_request_rule, merge_request: merge_request1,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 0)
+ end
+
+ subject(:execute) { described_class.new(project: project1).execute(user_ids) }
+
+ shared_examples_for 'adds users to the project rule' do
+ it 'adds users to the project rule' do
+ expect { execute }.to change { project_rule.users.count }.by(2)
+ expect(project_rule.reload.users).to contain_exactly(user1, user2)
+ end
+ end
+
+ shared_examples_for 'does not add users to the project rule' do
+ it 'does not add users to the project rule' do
+ expect { execute }.not_to change { project_rule.users.count }
+ end
+ end
+
+ shared_examples_for 'adds users to the merge request rule' do
+ it 'adds users to the merge request rule' do
+ expect { execute }.to change { merge_request_rule.users.count }.by(2)
+ expect(merge_request_rule.reload.users).to contain_exactly(user1, user2)
+ end
+ end
+
+ shared_examples_for 'does not add users to the merge request rule' do
+ it 'does not add users to the merge request rule' do
+ expect { execute }.not_to change { merge_request_rule.users.count }
+ end
+ end
+
+ context 'when users have access to the project' do
+ before_all do
+ project1.add_developer(user1)
+ project1.add_developer(user2)
+ project1.add_developer(user_not_referenced_in_policy)
+ end
+
+ describe 'project rules' do
+ let!(:project_rule) { project_rule_expected_to_change }
+
+ it_behaves_like 'adds users to the project rule'
+
+ context 'when project rule belongs to a different policy index' do
+ let!(:project_rule) do
+ create(:approval_project_rule, project: project1,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 1)
+ end
+
+ it_behaves_like 'does not add users to the project rule'
+ end
+
+ context 'when project rule belongs to other configuration' do
+ let_it_be(:configuration_other) do
+ create(:security_orchestration_policy_configuration, project: project2)
+ end
+
+ let!(:project_rule) do
+ create(:approval_project_rule, project: project1,
+ security_orchestration_policy_configuration: configuration_other, orchestration_policy_idx: 0)
+ end
+
+ it_behaves_like 'does not add users to the project rule'
+ end
+
+ context 'when project rule belongs to other project' do
+ let!(:project_rule) do
+ create(:approval_project_rule, project: project2,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 0)
+ end
+
+ it_behaves_like 'does not add users to the project rule'
+ end
+
+ context 'when policy is not applicable for the project' do
+ let!(:project_rule) { project_rule_expected_to_change }
+ let(:policies_yaml) { policies_not_applicable_yaml }
+
+ it_behaves_like 'does not add users to the project rule'
+ end
+ end
+
+ describe 'merge request rules' do
+ let!(:merge_request_rule) { merge_request_rule_expected_to_change }
+
+ it_behaves_like 'adds users to the merge request rule'
+
+ context 'when merge request rule belongs to the project rule' do
+ let!(:merge_request_rule) do
+ create(:approval_merge_request_rule, merge_request: merge_request1,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 0,
+ approval_project_rule: project_rule_expected_to_change)
+ end
+
+ it_behaves_like 'adds users to the merge request rule'
+ end
+
+ context 'when merge request is merged' do
+ let!(:merge_request_rule) { merge_request_rule_expected_to_change }
+
+ before do
+ merge_request1.mark_as_merged!
+ end
+
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+
+ context 'when policies are not applicable to the project' do
+ let!(:merge_request_rule) { merge_request_rule_expected_to_change }
+ let(:policies_yaml) { policies_not_applicable_yaml }
+
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+
+ context 'when merge request rule belongs to a different policy index' do
+ let!(:merge_request_rule) do
+ create(:approval_merge_request_rule, merge_request: merge_request1,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 1)
+ end
+
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+
+ context 'when merge request rule belongs to other configuration' do
+ let_it_be(:configuration_other) do
+ create(:security_orchestration_policy_configuration, project: project2)
+ end
+
+ let!(:merge_request_rule) do
+ create(:approval_merge_request_rule, merge_request: merge_request1,
+ security_orchestration_policy_configuration: configuration_other, orchestration_policy_idx: 0)
+ end
+
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+
+ context 'when merge request rule belongs to a merge request in other project' do
+ let_it_be(:merge_request2) { create(:merge_request, source_project: project2, target_project: project2) }
+ let!(:merge_request_rule) do
+ create(:approval_merge_request_rule, merge_request: merge_request2,
+ security_orchestration_policy_configuration: configuration, orchestration_policy_idx: 0)
+ end
+
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+ end
+ end
+
+ context 'when users do not have access to the project' do
+ let!(:project_rule) { project_rule_expected_to_change }
+ let!(:merge_request_rule) { merge_request_rule_expected_to_change }
+
+ it_behaves_like 'does not add users to the project rule'
+ it_behaves_like 'does not add users to the merge request rule'
+ end
+ end
+end
diff --git a/ee/spec/workers/security/refresh_project_policies_worker_spec.rb b/ee/spec/workers/security/refresh_project_policies_worker_spec.rb
index dc22a2952b5c5d21b0c13e7a23201352035be51d..29bed67de0ebafd6d3974bd3c2d841c842ba5162 100644
--- a/ee/spec/workers/security/refresh_project_policies_worker_spec.rb
+++ b/ee/spec/workers/security/refresh_project_policies_worker_spec.rb
@@ -31,13 +31,9 @@
end
end
- context 'when skip_refresh_project_policies is enabled' do
- before do
- stub_feature_flags(skip_refresh_project_policies: true)
- end
-
- it 'does not invoke Security::ProcessScanResultPolicyWorker' do
- expect(worker).not_to receive(:perform_async)
+ shared_examples_for 'does not invoke the worker' do
+ it 'does not invoke Security::ProcessScanResultPolicyWorker worker' do
+ expect(worker).not_to receive(:perform_in)
consume_event(subscriber: described_class, event: project_member_changed_event)
end
@@ -53,10 +49,26 @@
end
end
- it 'invokes Security::ProcessScanResultPolicyWorker' do
- expect(worker).to receive(:perform_in).with(0, project.id, configuration.id)
+ it_behaves_like 'does not invoke the worker'
- consume_event(subscriber: described_class, event: project_member_changed_event)
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'invokes Security::ProcessScanResultPolicyWorker' do
+ expect(worker).to receive(:perform_in).with(0, project.id, configuration.id)
+
+ consume_event(subscriber: described_class, event: project_member_changed_event)
+ end
+
+ context 'when skip_refresh_project_policies is enabled' do
+ before do
+ stub_feature_flags(skip_refresh_project_policies: true)
+ end
+
+ it_behaves_like 'does not invoke the worker'
+ end
end
end
@@ -77,11 +89,19 @@
end
end
- it 'invokes Security::ProcessScanResultPolicyWorker with incremental delay' do
- expect(worker).to receive(:perform_in).with(0, project.id, configuration.id).ordered
- expect(worker).to receive(:perform_in).with(30, project.id, inherited_configuration.id).ordered
+ it_behaves_like 'does not invoke the worker'
- consume_event(subscriber: described_class, event: project_member_changed_event)
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'invokes Security::ProcessScanResultPolicyWorker with incremental delay' do
+ expect(worker).to receive(:perform_in).with(0, project.id, configuration.id).ordered
+ expect(worker).to receive(:perform_in).with(30, project.id, inherited_configuration.id).ordered
+
+ consume_event(subscriber: described_class, event: project_member_changed_event)
+ end
end
end
@@ -101,10 +121,18 @@
end
end
- it 'invokes Security::ProcessScanResultPolicyWorker with incremental delay' do
- expect(worker).to receive(:perform_in).with(0, project.id, inherited_configuration.id)
+ it_behaves_like 'does not invoke the worker'
- consume_event(subscriber: described_class, event: project_member_changed_event)
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'invokes Security::ProcessScanResultPolicyWorker with incremental delay' do
+ expect(worker).to receive(:perform_in).with(0, project.id, inherited_configuration.id)
+
+ consume_event(subscriber: described_class, event: project_member_changed_event)
+ end
end
end
end
diff --git a/ee/spec/workers/security/scan_result_policies/add_approvers_to_rules_worker_spec.rb b/ee/spec/workers/security/scan_result_policies/add_approvers_to_rules_worker_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..90c2fa544c5d701a7476c9d165f78d8b41353c96
--- /dev/null
+++ b/ee/spec/workers/security/scan_result_policies/add_approvers_to_rules_worker_spec.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::ScanResultPolicies::AddApproversToRulesWorker, feature_category: :security_policy_management do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:project) { create(:project) }
+
+ let(:project_id) { project.id }
+ let(:user_ids) { [user.id] }
+ let(:data) { { project_id: project_id, user_ids: user_ids } }
+ let(:authorizations_event) { ProjectAuthorizations::AuthorizationsAddedEvent.new(data: data) }
+ let(:licensed_feature) { true }
+
+ before do
+ stub_licensed_features(security_orchestration_policies: licensed_feature)
+ end
+
+ it_behaves_like 'subscribes to event' do
+ let(:event) { authorizations_event }
+
+ it 'calls Security::ScanResultPolicies::AddApproversToRulesService' do
+ expect_next_instance_of(
+ Security::ScanResultPolicies::AddApproversToRulesService,
+ project: project
+ ) do |service|
+ expect(service).to receive(:execute).with([user.id])
+ end
+
+ consume_event(subscriber: described_class, event: authorizations_event)
+ end
+ end
+
+ context 'when the project does not exist' do
+ let(:project_id) { non_existing_record_id }
+
+ it 'logs and does not call Security::ScanResultPolicies::AddApproversToRulesService' do
+ expect(Sidekiq.logger).to receive(:info).with(
+ hash_including('message' => 'Project not found.', 'project_id' => project_id)
+ )
+ expect(Security::ScanResultPolicies::AddApproversToRulesService).not_to receive(:new)
+
+ expect { consume_event(subscriber: described_class, event: authorizations_event) }.not_to raise_exception
+ end
+ end
+
+ context 'when the user_ids are empty' do
+ let(:user_ids) { [] }
+
+ it 'does not call Security::ScanResultPolicies::AddApproversToRulesService' do
+ expect(Security::ScanResultPolicies::AddApproversToRulesService).not_to receive(:new)
+
+ expect { consume_event(subscriber: described_class, event: authorizations_event) }.not_to raise_exception
+ end
+ end
+
+ context 'when the feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'does not call Security::ScanResultPolicies::AddApproversToRulesService' do
+ expect(Security::ScanResultPolicies::AddApproversToRulesService).not_to receive(:new)
+
+ expect { consume_event(subscriber: described_class, event: authorizations_event) }.not_to raise_exception
+ end
+ end
+
+ context 'when the feature is not licensed' do
+ let(:licensed_feature) { false }
+
+ it 'does not call Security::ScanResultPolicies::AddApproversToRulesService' do
+ expect(Security::ScanResultPolicies::AddApproversToRulesService).not_to receive(:new)
+
+ expect { consume_event(subscriber: described_class, event: authorizations_event) }.not_to raise_exception
+ end
+ end
+end
diff --git a/spec/models/project_authorizations/changes_spec.rb b/spec/models/project_authorizations/changes_spec.rb
index d6ccfccbcbe1123fd8ebbaecdd0f3b2512e74a3e..714144841fbbaf8e6c46900d156b3caa85edde2c 100644
--- a/spec/models/project_authorizations/changes_spec.rb
+++ b/spec/models/project_authorizations/changes_spec.rb
@@ -28,36 +28,49 @@
end
shared_examples_for 'publishes AuthorizationsChangedEvent' do
- it 'publishes a AuthorizationsChangedEvent event with project id' do
- project_ids.each do |project_id|
- project_data = { project_id: project_id }
- project_event = instance_double('::ProjectAuthorizations::AuthorizationsChangedEvent', data: project_data)
+ it 'does not publish a AuthorizationsChangedEvent event' do
+ expect(::Gitlab::EventStore).not_to receive(:publish)
+ .with(an_instance_of(::ProjectAuthorizations::AuthorizationsChangedEvent))
- allow(::ProjectAuthorizations::AuthorizationsChangedEvent).to receive(:new)
- .with(data: project_data)
- .and_return(project_event)
+ apply_project_authorization_changes
+ end
- allow(::Gitlab::EventStore).to receive(:publish)
- expect(::Gitlab::EventStore).to receive(:publish).with(project_event)
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
end
- apply_project_authorization_changes
+ it 'publishes a AuthorizationsChangedEvent event with project id' do
+ allow(::Gitlab::EventStore).to receive(:publish)
+ project_ids.each do |project_id|
+ project_data = { project_id: project_id }
+ project_event = instance_double('::ProjectAuthorizations::AuthorizationsChangedEvent', data: project_data)
+
+ allow(::ProjectAuthorizations::AuthorizationsChangedEvent).to receive(:new)
+ .with(data: project_data)
+ .and_return(project_event)
+
+ expect(::Gitlab::EventStore).to receive(:publish).with(project_event)
+ end
+
+ apply_project_authorization_changes
+ end
end
end
shared_examples_for 'publishes AuthorizationsRemovedEvent' do
it 'publishes a AuthorizationsRemovedEvent event with project id' do
- project_ids.each do |project_id|
+ allow(::Gitlab::EventStore).to receive(:publish_group)
+ project_events = project_ids.map do |project_id|
project_data = { project_id: project_id, user_ids: user_ids }
project_event = instance_double('::ProjectAuthorizations::AuthorizationsRemovedEvent', data: project_data)
allow(::ProjectAuthorizations::AuthorizationsRemovedEvent).to receive(:new)
.with(data: project_data)
.and_return(project_event)
-
- allow(::Gitlab::EventStore).to receive(:publish)
- expect(::Gitlab::EventStore).to receive(:publish).with(project_event)
+ project_event
end
+ expect(::Gitlab::EventStore).to receive(:publish_group).with(project_events)
apply_project_authorization_changes
end
@@ -69,7 +82,43 @@
it 'does not publish a AuthorizationsRemovedEvent event' do
expect(::Gitlab::EventStore).not_to(
- receive(:publish).with(an_instance_of(::ProjectAuthorizations::AuthorizationsRemovedEvent))
+ receive(:publish_group).with(
+ array_including(an_instance_of(::ProjectAuthorizations::AuthorizationsRemovedEvent))
+ )
+ )
+
+ apply_project_authorization_changes
+ end
+ end
+ end
+
+ shared_examples_for 'publishes AuthorizationsAddedEvent' do
+ it 'publishes a AuthorizationsAddedEvent event with project id' do
+ allow(::Gitlab::EventStore).to receive(:publish_group)
+ project_events = project_ids.map do |project_id|
+ project_data = { project_id: project_id, user_ids: user_ids }
+ project_event = instance_double('::ProjectAuthorizations::AuthorizationsAddedEvent', data: project_data)
+
+ allow(::ProjectAuthorizations::AuthorizationsAddedEvent).to receive(:new)
+ .with(data: project_data)
+ .and_return(project_event)
+ project_event
+ end
+ expect(::Gitlab::EventStore).to receive(:publish_group).with(project_events)
+
+ apply_project_authorization_changes
+ end
+
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'does not publish a AuthorizationsAddedEvent event' do
+ expect(::Gitlab::EventStore).not_to(
+ receive(:publish_group).with(array_including(
+ an_instance_of(::ProjectAuthorizations::AuthorizationsAddedEvent))
+ )
)
apply_project_authorization_changes
@@ -88,8 +137,23 @@
shared_examples_for 'does not publish AuthorizationsRemovedEvent' do
it 'does not publish a AuthorizationsRemovedEvent event' do
- expect(::Gitlab::EventStore).not_to receive(:publish)
- .with(an_instance_of(::ProjectAuthorizations::AuthorizationsRemovedEvent))
+ expect(::Gitlab::EventStore).not_to(
+ receive(:publish_group).with(
+ array_including(an_instance_of(::ProjectAuthorizations::AuthorizationsRemovedEvent))
+ )
+ )
+
+ apply_project_authorization_changes
+ end
+ end
+
+ shared_examples_for 'does not publish AuthorizationsAddedEvent' do
+ it 'does not publish a AuthorizationsAddedEvent event' do
+ expect(::Gitlab::EventStore).not_to(
+ receive(:publish_group).with(
+ array_including(an_instance_of(::ProjectAuthorizations::AuthorizationsAddedEvent))
+ )
+ )
apply_project_authorization_changes
end
@@ -101,6 +165,7 @@
let_it_be(:project_2) { create(:project) }
let_it_be(:project_3) { create(:project) }
let(:project_ids) { [project_1.id, project_2.id, project_3.id] }
+ let(:user_ids) { [user.id] }
let(:authorizations_to_add) do
[
@@ -155,6 +220,7 @@
it_behaves_like 'logs the detail', batch_size: 2
it_behaves_like 'publishes AuthorizationsChangedEvent'
+ it_behaves_like 'publishes AuthorizationsAddedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
context 'when the GitLab installation does not have a replica database configured' do
@@ -166,6 +232,7 @@
it_behaves_like 'inserts the rows in batches, as per the `per_batch` size, without a delay between batches'
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
+ it_behaves_like 'publishes AuthorizationsAddedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
end
end
@@ -178,6 +245,7 @@
it_behaves_like 'inserts the rows in batches, as per the `per_batch` size, without a delay between batches'
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
+ it_behaves_like 'publishes AuthorizationsAddedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
end
end
@@ -242,6 +310,7 @@
it_behaves_like 'logs the detail', batch_size: 2
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
context 'when the GitLab installation does not have a replica database configured' do
before do
@@ -253,6 +322,7 @@
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
end
@@ -265,6 +335,7 @@
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
context 'when the user_ids list is empty' do
@@ -273,6 +344,7 @@
it_behaves_like 'does not removes project authorizations of the users in the current project'
it_behaves_like 'does not publish AuthorizationsChangedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
context 'when the user_ids list is nil' do
@@ -281,6 +353,7 @@
it_behaves_like 'does not removes project authorizations of the users in the current project'
it_behaves_like 'does not publish AuthorizationsChangedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
end
@@ -344,6 +417,7 @@
it_behaves_like 'logs the detail', batch_size: 2
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
context 'when the GitLab installation does not have a replica database configured' do
before do
@@ -355,6 +429,7 @@
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
end
@@ -367,6 +442,7 @@
it_behaves_like 'does not log any detail'
it_behaves_like 'publishes AuthorizationsChangedEvent'
it_behaves_like 'publishes AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
context 'when the project_ids list is empty' do
@@ -375,6 +451,7 @@
it_behaves_like 'does not removes any project authorizations from the current user'
it_behaves_like 'does not publish AuthorizationsChangedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
context 'when the user_ids list is nil' do
@@ -383,6 +460,7 @@
it_behaves_like 'does not removes any project authorizations from the current user'
it_behaves_like 'does not publish AuthorizationsChangedEvent'
it_behaves_like 'does not publish AuthorizationsRemovedEvent'
+ it_behaves_like 'does not publish AuthorizationsAddedEvent'
end
end
end
diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb
index af151f93dc7c82a09ad5f40782de8a7990827c16..c08b40e9528dbce8ae6cbc59d2e357f885b535ee 100644
--- a/spec/services/members/create_service_spec.rb
+++ b/spec/services/members/create_service_spec.rb
@@ -119,14 +119,34 @@
before do
# validations will fail because we try to invite them to the project as a guest
source.group.add_developer(member)
+ allow(Gitlab::EventStore).to receive(:publish)
end
- it 'triggers the members added and authorizations changed events' do
+ it 'triggers the authorizations changed events' do
expect(Gitlab::EventStore)
- .to receive(:publish)
- .with(an_instance_of(ProjectAuthorizations::AuthorizationsChangedEvent))
+ .to receive(:publish_group)
+ .with(array_including(an_instance_of(ProjectAuthorizations::AuthorizationsAddedEvent)))
.and_call_original
+ execute_service
+ end
+
+ context 'when feature flag "add_policy_approvers_to_rules" is disabled' do
+ before do
+ stub_feature_flags(add_policy_approvers_to_rules: false)
+ end
+
+ it 'triggers the authorizations changed event' do
+ expect(Gitlab::EventStore)
+ .to receive(:publish)
+ .with(an_instance_of(ProjectAuthorizations::AuthorizationsChangedEvent))
+ .and_call_original
+
+ execute_service
+ end
+ end
+
+ it 'triggers the members added event' do
expect(Gitlab::EventStore)
.to receive(:publish)
.with(an_instance_of(Members::MembersAddedEvent))