Merge branch 'sk/fix-refresh-worker' into 'master'

Refresh security policies only when user is present in approvers

See merge request gitlab-org/gitlab!139980



Merged-by: Andy Soiron <asoiron@gitlab.com>
Approved-by: Alan (Maciej) Paruszewski <mparuszewski@gitlab.com>
Approved-by: Andy Soiron <asoiron@gitlab.com>
Co-authored-by: Sashi Kumar <skumar@gitlab.com>

ee/app/workers/security/refresh_project_policies_worker.rb

@@ -12,8 +12,26 @@ class RefreshProjectPoliciesWorker
 
     feature_category :security_policy_management
 
+    DELAY_INTERVAL = 30.seconds.to_i
+
     def handle_event(event)
-      ::Security::ScanResultPolicies::SyncProjectWorker.new.perform(event.data[:project_id])
+      project = Project.find_by_id(event.data[:project_id])
+      return unless project
+
+      return unless project.licensed_feature_available?(:security_orchestration_policies)
+      return if Feature.enabled?(:skip_refresh_project_policies, project.root_namespace)
+
+      configurations_with_users = project.all_security_orchestration_policy_configurations.select do |configuration|
+        configuration.active_scan_result_policies.any? do |policy|
+          policy[:actions].any? do |action|
+            action[:user_approvers].present? || action[:user_approvers_ids].present?
+          end
+        end
+      end
+
+      configurations_with_users.each_with_index do |configuration, index|
+        Security::ProcessScanResultPolicyWorker.perform_in(index * DELAY_INTERVAL, project.id, configuration.id)
+      end
     end
   end
 end

ee/config/feature_flags/development/skip_refresh_project_policies.yml

+---
+name: skip_refresh_project_policies
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139980
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/435600
+milestone: '16.8'
+type: development
+group: group::security policies
+default_enabled: false

ee/spec/workers/security/refresh_project_policies_worker_spec.rb

@@ -3,11 +3,24 @@
 require 'spec_helper'
 
 RSpec.describe ::Security::RefreshProjectPoliciesWorker, feature_category: :security_policy_management do
+  let_it_be(:group) { create(:group) }
+  let(:worker) { Security::ProcessScanResultPolicyWorker }
+  let_it_be(:project) { create(:project, group: group) }
   let(:project_member_changed_event) do
-    ::ProjectAuthorizations::AuthorizationsChangedEvent.new(data: { project_id: 123 })
+    ::ProjectAuthorizations::AuthorizationsChangedEvent.new(data: { project_id: project.id })
   end
 
-  let(:worker) { ::Security::ScanResultPolicies::SyncProjectWorker }
+  let(:scan_result_policy) do
+    build(
+      :scan_result_policy,
+      actions: [{ type: 'require_approval', approvals_required: 1, user_approvers_ids: [1] }]
+    )
+  end
+
+  before do
+    stub_licensed_features(security_orchestration_policies: true)
+    stub_feature_flags(skip_refresh_project_policies: false)
+  end
 
   it_behaves_like 'subscribes to event' do
     let(:event) { project_member_changed_event }
@@ -18,11 +31,80 @@
     end
   end
 
-  it 'invokes ::Security::ScanResultPolicies::SyncProjectWorker with the project_id' do
+  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)
+
+      consume_event(subscriber: described_class, event: project_member_changed_event)
+    end
+  end
+
+  context 'when the project has a policy with user_approvers' do
+    let_it_be(:configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    before do
+      allow(configuration).to receive(:active_scan_result_policies).and_return([scan_result_policy])
+      allow_next_found_instance_of(Project) do |instance|
+        allow(instance).to receive(:all_security_orchestration_policy_configurations).and_return([configuration])
+      end
+    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
+  end
+
+  context 'when the project has multiple policy with user_approvers' do
+    let_it_be(:inherited_configuration) do
+      create(:security_orchestration_policy_configuration, project: nil, namespace: group)
+    end
+
+    let_it_be(:configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    before do
+      allow(configuration).to receive(:active_scan_result_policies).and_return([scan_result_policy])
+      allow(inherited_configuration).to receive(:active_scan_result_policies).and_return([scan_result_policy])
+
+      allow_next_found_instance_of(Project) do |instance|
+        allow(instance).to receive(:all_security_orchestration_policy_configurations).and_return([configuration,
+          inherited_configuration])
+      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
+
       consume_event(subscriber: described_class, event: project_member_changed_event)
+    end
+  end
 
-    expect_any_instance_of(worker) do |instance|
-      expect(instance).to receive(:perform).with(123)
+  context 'when the project has multiple policy but only one with user_approvers' do
+    let_it_be(:inherited_configuration) do
+      create(:security_orchestration_policy_configuration, project: nil, namespace: group)
+    end
+
+    let_it_be(:configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    before do
+      allow(inherited_configuration).to receive(:active_scan_result_policies).and_return([scan_result_policy])
+
+      allow_next_found_instance_of(Project) do |instance|
+        allow(instance).to receive(:all_security_orchestration_policy_configurations).and_return([configuration,
+          inherited_configuration])
+      end
+    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