From 1f189b79b851b018b3eb76a44c88383a7b8902ca Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Sun, 6 Feb 2022 12:24:43 -0500
Subject: [PATCH 1/7] Add approvers when editing scan result policies

taking into consideration the existing feature
flag. It also split a couple of fields which
were previously only used for scan execution scan.

Changelog: changed
EE: true
---
 .../scan_execution_policy_editor.vue          |  9 ++-
 .../scan_result_policy/lib/index.js           |  5 +-
 .../scan_result_policy_editor.vue             | 10 ++-
 .../threat_monitoring/policy_editor.js        | 12 ++-
 .../projects/security/policies_controller.rb  | 13 ++++
 .../projects/security/policies_helper.rb      |  9 ++-
 .../fetch_policy_approvers_service.rb         | 43 ++++++++++
 .../projects/security/policies/edit.html.haml |  2 +-
 .../scan_execution_policy_editor_spec.js      | 15 ++--
 .../scan_result_policy_editor_spec.js         | 17 ++--
 .../projects/security/policies_helper_spec.rb |  9 ++-
 .../security/policies_controller_spec.rb      | 64 ++++++++++++++-
 .../fetch_policy_approvers_service_spec.rb    | 78 +++++++++++++++++++
 13 files changed, 245 insertions(+), 41 deletions(-)
 create mode 100644 ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
 create mode 100644 ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb

diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
index d47a325360ab5c..c0ed179af257f2 100644
--- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
+++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
@@ -28,11 +28,11 @@ export default {
     PolicyEditorLayout,
   },
   inject: [
-    'disableScanExecutionUpdate',
+    'disableScanPolicyUpdate',
     'policyEditorEmptyStateSvgPath',
     'projectId',
     'projectPath',
-    'scanExecutionDocumentationPath',
+    'scanPolicyDocumentationPath',
   ],
   props: {
     assignedPolicyProject: {
@@ -62,6 +62,7 @@ export default {
       newlyCreatedPolicyProject: null,
       policy: fromYaml(yamlEditorValue),
       yamlEditorValue,
+      documentationPath: `${this.scanPolicyDocumentationPath}#scan-execution-policy-editor`,
     };
   },
   computed: {
@@ -137,7 +138,7 @@ export default {
 
 <template>
   <policy-editor-layout
-    v-if="!disableScanExecutionUpdate"
+    v-if="!disableScanPolicyUpdate"
     :custom-save-button-text="$options.i18n.createMergeRequest"
     :default-editor-mode="$options.DEFAULT_EDITOR_MODE"
     :editor-modes="$options.EDITOR_MODES"
@@ -153,7 +154,7 @@ export default {
   <gl-empty-state
     v-else
     :description="$options.i18n.notOwnerDescription"
-    :primary-button-link="scanExecutionDocumentationPath"
+    :primary-button-link="documentationPath"
     :primary-button-text="$options.i18n.notOwnerButtonText"
     :svg-path="policyEditorEmptyStateSvgPath"
     title=""
diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js
index c1ee5a6b1b7aa8..df2c462e6aa1e8 100644
--- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js
+++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js
@@ -16,10 +16,9 @@ rules:
     severity_levels:
       - critical
     vulnerability_states:
-      - newly_added
+      - newly_detected
 actions:
   - type: require_approval
     approvals_required: 1
-    user_approvers:
-      - security_user
+    user_approvers: []
 `;
diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
index a4c42ad1afe335..29fc565bcea07f 100644
--- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
+++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
@@ -28,11 +28,12 @@ export default {
     PolicyEditorLayout,
   },
   inject: [
-    'disableScanExecutionUpdate',
+    'disableScanPolicyUpdate',
     'policyEditorEmptyStateSvgPath',
     'projectId',
     'projectPath',
-    'scanExecutionDocumentationPath',
+    'scanPolicyDocumentationPath',
+    'scanResultPolicyApprovers',
   ],
   props: {
     assignedPolicyProject: {
@@ -62,6 +63,7 @@ export default {
       newlyCreatedPolicyProject: null,
       policy: fromYaml(yamlEditorValue),
       yamlEditorValue,
+      documentationPath: `${this.scanPolicyDocumentationPath}#scan-result-policy-editor`,
     };
   },
   computed: {
@@ -138,7 +140,7 @@ export default {
 
 <template>
   <policy-editor-layout
-    v-if="!disableScanExecutionUpdate"
+    v-if="!disableScanPolicyUpdate"
     :custom-save-button-text="$options.i18n.createMergeRequest"
     :default-editor-mode="$options.DEFAULT_EDITOR_MODE"
     :editor-modes="$options.EDITOR_MODES"
@@ -154,7 +156,7 @@ export default {
   <gl-empty-state
     v-else
     :description="$options.i18n.notOwnerDescription"
-    :primary-button-link="scanExecutionDocumentationPath"
+    :primary-button-link="documentationPath"
     :primary-button-text="$options.i18n.notOwnerButtonText"
     :svg-path="policyEditorEmptyStateSvgPath"
     title=""
diff --git a/ee/app/assets/javascripts/threat_monitoring/policy_editor.js b/ee/app/assets/javascripts/threat_monitoring/policy_editor.js
index f5c4e5d2109f53..caa7ba7842b8dc 100644
--- a/ee/app/assets/javascripts/threat_monitoring/policy_editor.js
+++ b/ee/app/assets/javascripts/threat_monitoring/policy_editor.js
@@ -17,7 +17,7 @@ export default () => {
   const {
     assignedPolicyProject,
     defaultEnvironmentId,
-    disableScanExecutionUpdate,
+    disableScanPolicyUpdate,
     environmentsEndpoint,
     createAgentHelpPath,
     networkPoliciesEndpoint,
@@ -29,7 +29,8 @@ export default () => {
     projectPath,
     projectId,
     environmentId,
-    scanExecutionDocumentationPath,
+    scanPolicyDocumentationPath,
+    scanResultApprovers,
   } = el.dataset;
 
   // We require the project to have at least one available environment.
@@ -59,19 +60,22 @@ export default () => {
     props.existingPolicy = { type: policyType, ...JSON.parse(policy) };
   }
 
+  const scanResultPolicyApprovers = scanResultApprovers ? JSON.parse(scanResultApprovers) : [];
+
   return new Vue({
     el,
     apolloProvider,
     provide: {
       createAgentHelpPath,
-      disableScanExecutionUpdate: parseBoolean(disableScanExecutionUpdate),
+      disableScanPolicyUpdate: parseBoolean(disableScanPolicyUpdate),
       networkDocumentationPath,
       policyEditorEmptyStateSvgPath,
       policyType,
       projectId,
       projectPath,
       policiesPath,
-      scanExecutionDocumentationPath,
+      scanPolicyDocumentationPath,
+      scanResultPolicyApprovers,
     },
     store,
     render(createElement) {
diff --git a/ee/app/controllers/projects/security/policies_controller.rb b/ee/app/controllers/projects/security/policies_controller.rb
index 95fc81449fbfc9..c6a59ffc27fa9f 100644
--- a/ee/app/controllers/projects/security/policies_controller.rb
+++ b/ee/app/controllers/projects/security/policies_controller.rb
@@ -22,6 +22,7 @@ def index
       def edit
         @policy_name = URI.decode_www_form_component(params[:id])
         @policy = policy
+        @approvers = approvers
 
         render_404 if @policy.nil?
       end
@@ -89,6 +90,18 @@ def default_policy
       def policy_configuration
         @policy_configuration ||= project.security_orchestration_policy_configuration
       end
+
+      def approvers
+        return unless Feature.enabled?(:scan_result_policy, project) && @policy_type == :scan_result_policy
+
+        result = ::Security::SecurityOrchestrationPolicies::FetchPolicyApproversService.new(
+          policy: @policy,
+          project: project,
+          current_user: @current_user
+        ).execute
+
+        result[:approvers]
+      end
     end
   end
 end
diff --git a/ee/app/helpers/projects/security/policies_helper.rb b/ee/app/helpers/projects/security/policies_helper.rb
index f533611410212a..8beba567d791e5 100644
--- a/ee/app/helpers/projects/security/policies_helper.rb
+++ b/ee/app/helpers/projects/security/policies_helper.rb
@@ -15,15 +15,15 @@ def assigned_policy_project(project)
     }
   end
 
-  def orchestration_policy_data(project, policy_type = nil, policy = nil, environment = nil)
+  def orchestration_policy_data(project, policy_type = nil, policy = nil, environment = nil, approvers = nil)
     return unless project
 
-    disable_scan_execution_update = !can_update_security_orchestration_policy_project?(project)
+    disable_scan_policy_update = !can_update_security_orchestration_policy_project?(project)
 
     {
       assigned_policy_project: assigned_policy_project(project).to_json,
       default_environment_id: project.default_environment&.id || -1,
-      disable_scan_execution_update: disable_scan_execution_update.to_s,
+      disable_scan_policy_update: disable_scan_policy_update.to_s,
       network_policies_endpoint: project_security_network_policies_path(project),
       create_agent_help_path: help_page_url('user/clusters/agent/install/index'),
       environments_endpoint: project_environments_path(project),
@@ -35,7 +35,8 @@ def orchestration_policy_data(project, policy_type = nil, policy = nil, environm
       project_path: project.full_path,
       project_id: project.id,
       policies_path: project_security_policies_path(project),
-      scan_execution_documentation_path: help_page_path('user/application_security/policies/index', anchor: 'scan-execution-policy-editor')
+      scan_policy_documentation_path: help_page_path('user/application_security/policies/index'),
+      scan_result_approvers: approvers&.to_json
     }
   end
 end
diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
new file mode 100644
index 00000000000000..0994121798235f
--- /dev/null
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class FetchPolicyApproversService
+      include BaseServiceUtility
+
+      def initialize(policy:, project:, current_user:)
+        @policy = policy
+        @project = project
+        @current_user = current_user
+      end
+
+      def execute
+        success({ approvers: approvers })
+      end
+
+      private
+
+      attr_reader :policy, :project, :current_user
+
+      def approvers
+        action = policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
+
+        return unless action
+
+        API::Entities::UserBasic.represent(user_approvers(action)) + API::Entities::PublicGroupDetails.represent(group_approvers(action))
+      end
+
+      def user_approvers(action)
+        user_names = action[:user_approvers] || []
+        user_ids = action[:user_approvers_ids] || []
+        project.team.users.by_ids_or_usernames(user_ids, user_names)
+      end
+
+      def group_approvers(action)
+        group_paths = action[:group_approvers] || []
+        group_ids = action[:group_approvers_ids] || []
+        Group.unscoped.public_to_user(current_user).by_ids_or_paths(group_ids, group_paths)
+      end
+    end
+  end
+end
diff --git a/ee/app/views/projects/security/policies/edit.html.haml b/ee/app/views/projects/security/policies/edit.html.haml
index 3183b26f01aab7..f1411cd72d3dd6 100644
--- a/ee/app/views/projects/security/policies/edit.html.haml
+++ b/ee/app/views/projects/security/policies/edit.html.haml
@@ -1,6 +1,6 @@
 - add_to_breadcrumbs s_("SecurityOrchestration|Policies"), project_security_policies_path(@project)
 - breadcrumb_title s_("SecurityOrchestration|Edit policy")
 - page_title s_("SecurityOrchestration|Edit policy")
-- data = orchestration_policy_data(@project, @policy_type, @policy, @environment)
+- data = orchestration_policy_data(@project, @policy_type, @policy, @environment, @approvers)
 
 #js-policy-builder-app{ data: data }
diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
index db4c6414f7d356..0342a1f17efd6b 100644
--- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
+++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
@@ -55,7 +55,7 @@ describe('ScanExecutionPolicyEditor', () => {
   let wrapper;
   const defaultProjectPath = 'path/to/project';
   const policyEditorEmptyStateSvgPath = 'path/to/svg';
-  const scanExecutionDocumentationPath = 'path/to/docs';
+  const scanPolicyDocumentationPath = 'path/to/docs';
   const assignedPolicyProject = {
     branch: 'main',
     fullPath: 'path/to/existing-project',
@@ -68,11 +68,11 @@ describe('ScanExecutionPolicyEditor', () => {
         ...propsData,
       },
       provide: {
-        disableScanExecutionUpdate: false,
+        disableScanPolicyUpdate: false,
         policyEditorEmptyStateSvgPath,
         projectId: 1,
         projectPath: defaultProjectPath,
-        scanExecutionDocumentationPath,
+        scanPolicyDocumentationPath,
         ...provide,
       },
     });
@@ -141,12 +141,11 @@ describe('ScanExecutionPolicyEditor', () => {
 
   describe('when a user is not an owner of the project', () => {
     it('displays the empty state with the appropriate properties', async () => {
-      factory({ provide: { disableScanExecutionUpdate: true } });
+      factory({ provide: { disableScanPolicyUpdate: true } });
       await nextTick();
-      expect(findEmptyState().props()).toMatchObject({
-        primaryButtonLink: scanExecutionDocumentationPath,
-        svgPath: policyEditorEmptyStateSvgPath,
-      });
+      expect(findEmptyState().props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
+      expect(findEmptyState().props('primaryButtonLink')).toMatch('scan-execution-policy-editor');
+      expect(findEmptyState().props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
     });
   });
 });
diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
index a6d5932cf588f5..0a8666da7bf30e 100644
--- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
+++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
@@ -39,11 +39,12 @@ describe('ScanResultPolicyEditor', () => {
   let wrapper;
   const defaultProjectPath = 'path/to/project';
   const policyEditorEmptyStateSvgPath = 'path/to/svg';
-  const scanExecutionDocumentationPath = 'path/to/docs';
+  const scanPolicyDocumentationPath = 'path/to/docs';
   const assignedPolicyProject = {
     branch: 'main',
     fullPath: 'path/to/existing-project',
   };
+  const scanResultPolicyApprovers = [];
 
   const factory = ({ propsData = {}, provide = {} } = {}) => {
     wrapper = shallowMount(ScanResultPolicyEditor, {
@@ -52,11 +53,12 @@ describe('ScanResultPolicyEditor', () => {
         ...propsData,
       },
       provide: {
-        disableScanExecutionUpdate: false,
+        disableScanPolicyUpdate: false,
         policyEditorEmptyStateSvgPath,
         projectId: 1,
         projectPath: defaultProjectPath,
-        scanExecutionDocumentationPath,
+        scanPolicyDocumentationPath,
+        scanResultPolicyApprovers,
         ...provide,
       },
     });
@@ -122,11 +124,10 @@ describe('ScanResultPolicyEditor', () => {
 
   describe('when a user is not an owner of the project', () => {
     it('displays the empty state with the appropriate properties', async () => {
-      factory({ provide: { disableScanExecutionUpdate: true } });
-      expect(findEmptyState().props()).toMatchObject({
-        primaryButtonLink: scanExecutionDocumentationPath,
-        svgPath: policyEditorEmptyStateSvgPath,
-      });
+      factory({ provide: { disableScanPolicyUpdate: true } });
+      expect(findEmptyState().props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
+      expect(findEmptyState().props('primaryButtonLink')).toMatch('scan-result-policy-editor');
+      expect(findEmptyState().props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
     });
   });
 });
diff --git a/ee/spec/helpers/projects/security/policies_helper_spec.rb b/ee/spec/helpers/projects/security/policies_helper_spec.rb
index 645f7d2cdec90a..f3fc250b3cb2c1 100644
--- a/ee/spec/helpers/projects/security/policies_helper_spec.rb
+++ b/ee/spec/helpers/projects/security/policies_helper_spec.rb
@@ -35,12 +35,13 @@
   end
 
   describe '#orchestration_policy_data' do
+    let(:approvers) { %w(approver1 approver2) }
     let(:owner) { project.first_owner }
     let(:base_data) do
       {
         assigned_policy_project: "null",
         default_environment_id: -1,
-        disable_scan_execution_update: "false",
+        disable_scan_policy_update: "false",
         network_policies_endpoint: kind_of(String),
         create_agent_help_path: kind_of(String),
         environments_endpoint: kind_of(String),
@@ -52,7 +53,8 @@
         environment_id: environment&.id,
         policy: policy&.to_json,
         policy_type: policy_type,
-        scan_execution_documentation_path: kind_of(String)
+        scan_policy_documentation_path: kind_of(String),
+        scan_result_approvers: approvers&.to_json
       }
     end
 
@@ -61,12 +63,13 @@
       allow(helper).to receive(:can?).with(owner, :update_security_orchestration_policy_project, project) { true }
     end
 
-    subject { helper.orchestration_policy_data(project, policy_type, policy, environment) }
+    subject { helper.orchestration_policy_data(project, policy_type, policy, environment, approvers) }
 
     context 'when a new policy is being created' do
       let(:environment) { nil }
       let(:policy) { nil }
       let(:policy_type) { nil }
+      let(:approvers) { nil }
 
       it { is_expected.to match(base_data) }
     end
diff --git a/ee/spec/requests/projects/security/policies_controller_spec.rb b/ee/spec/requests/projects/security/policies_controller_spec.rb
index d43916c03c0b60..ff4ec618213b00 100644
--- a/ee/spec/requests/projects/security/policies_controller_spec.rb
+++ b/ee/spec/requests/projects/security/policies_controller_spec.rb
@@ -11,10 +11,11 @@
   let_it_be(:policy) { build(:scan_execution_policy) }
   let_it_be(:type) { 'scan_execution_policy' }
   let_it_be(:index) { project_security_policies_url(project) }
-  let_it_be(:edit) { edit_project_security_policy_url(project, id: policy[:name], type: type) }
   let_it_be(:new) { new_project_security_policy_url(project) }
   let_it_be(:feature_enabled) { true }
 
+  let(:edit) { edit_project_security_policy_url(project, id: policy[:name], type: type) }
+
   before do
     project.add_developer(user)
     sign_in(user)
@@ -39,6 +40,13 @@
           expect(app.attributes['data-policy-type'].value).to eq(type)
         end
 
+        it 'does not contain any approver data' do
+          get edit
+          app = Nokogiri::HTML.parse(response.body).at_css('div#js-policy-builder-app')
+
+          expect(app['data-scan-result-approvers']).to be_nil
+        end
+
         context 'when type is container_runtime' do
           let_it_be(:type) { 'container_policy' }
           let_it_be(:environment) { create(:environment, :with_review_app, project: project) }
@@ -84,6 +92,58 @@
             expect(app.attributes['data-policy-type'].value).to eq(type)
             expect(app.attributes['data-environment-id'].value).to eq(environment_id.to_s)
           end
+
+          it 'does not contain any approver data' do
+            get edit
+            app = Nokogiri::HTML.parse(response.body).at_css('div#js-policy-builder-app')
+
+            expect(app['data-scan-result-approvers']).to be_nil
+          end
+        end
+
+        context 'with scan result policy type' do
+          let_it_be(:type) {'scan_result_policy'}
+          let_it_be(:policy) {build(:scan_result_policy)}
+          let_it_be(:approvers) { [{ id: 1, name: 'one' }, { id: 2, name: "two" }] }
+
+          let(:service) { instance_double('::Security::SecurityOrchestrationPolicies::FetchPolicyApproversService', execute: { approvers: approvers }) }
+
+          before do
+            stub_feature_flags(scan_result_policy: true)
+            allow_next_instance_of(Repository) do |repository|
+              allow(repository).to receive(:blob_data_at).and_return({ scan_result_policy: [policy] }.to_yaml)
+            end
+            allow(::Security::SecurityOrchestrationPolicies::FetchPolicyApproversService).to receive(:new).with(policy: policy, project: project, current_user: user).and_return(service)
+          end
+
+          it 'renders the edit page with approvers data' do
+            get edit
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response).to render_template(:edit)
+
+            app = Nokogiri::HTML.parse(response.body).at_css('div#js-policy-builder-app')
+
+            expect(app['data-policy']).to eq(policy.to_json)
+            expect(app['data-policy-type']).to eq(type)
+            expect(app['data-scan-result-approvers']).to eq(approvers.to_json)
+          end
+
+          context 'with feature flag disabled' do
+            before do
+              stub_feature_flags(scan_result_policy: false)
+            end
+
+            it 'renders the edit page without approvers data' do
+              get edit
+
+              app = Nokogiri::HTML.parse(response.body).at_css('div#js-policy-builder-app')
+
+              expect(app['data-policy']).to eq(policy.to_json)
+              expect(app['data-policy-type']).to eq(type)
+              expect(app['data-scan-result-approvers']).to be_nil
+            end
+          end
         end
 
         context 'when type is missing' do
@@ -143,7 +203,7 @@
         end
 
         context 'when policy yaml is invalid' do
-          let_it_be(:policy) { 'invalid' }
+          let_it_be(:policy) { { name: 'invalid' } }
 
           it 'redirects to policy file' do
             get edit
diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
new file mode 100644
index 00000000000000..8744e66b1b95a7
--- /dev/null
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SecurityOrchestrationPolicies::FetchPolicyApproversService do
+  describe '#execute' do
+    let(:group) { create(:group) }
+    let(:project) { create(:project, :public, namespace: group) }
+    let(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+    let(:policy) { build(:scan_result_policy, actions: [action]) }
+    let(:user) { create(:user) }
+
+    subject(:service) do
+      described_class.new(policy: policy, current_user: user, project: project)
+    end
+
+    before do
+      group.add_user(user, :owner)
+    end
+
+    context 'with user approver' do
+      let(:action) { { type: "require_approval", approvals_required: 1, user_approvers: [user.username] } }
+
+      it 'returns user approvers' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers]).to all(be_a(API::Entities::UserBasic))
+        expect(response[:approvers].as_json.first).to include(id: user.id, username: user.username, state: user.state)
+      end
+    end
+
+    context 'with group approver' do
+      let(:action) { { type: "require_approval", approvals_required: 1, group_approvers_ids: [group.id] } }
+
+      it 'returns group approvers' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers]).to all(be_a(API::Entities::PublicGroupDetails))
+        expect(response[:approvers].as_json.first).to include(id: group.id, full_name: group.full_name, full_path: group.full_path, name: group.name)
+      end
+    end
+
+    context 'with policy equals to nil' do
+      let(:policy) { nil }
+
+      it 'returns nil' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers]).to be_nil
+      end
+    end
+
+    context 'with action equals to nil' do
+      let(:action) { nil }
+
+      it 'returns nil' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers]).to be_nil
+      end
+    end
+
+    context 'with action of an unknown type' do
+      let(:action) { { type: "random_type", approvals_required: 1, group_approvers_ids: [group.id] } }
+
+      it 'returns nil' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers]).to be_nil
+      end
+    end
+  end
+end
-- 
GitLab


From 068e62d9e392e913432ad37785310416e291dfdc Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Tue, 8 Feb 2022 10:36:59 -0500
Subject: [PATCH 2/7] Address backend reviews by adding more specs and

returning empty array instead of nil.
---
 .../fetch_policy_approvers_service.rb         |  2 +-
 .../fetch_policy_approvers_service_spec.rb    | 25 ++++++++++++++-----
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
index 0994121798235f..94bc03a79559ab 100644
--- a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
@@ -22,7 +22,7 @@ def execute
       def approvers
         action = policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
 
-        return unless action
+        return [] unless action
 
         API::Entities::UserBasic.represent(user_approvers(action)) + API::Entities::PublicGroupDetails.represent(group_approvers(action))
       end
diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
index 8744e66b1b95a7..d2d45effdc6f7f 100644
--- a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -42,36 +42,49 @@
       end
     end
 
+    context 'with both user and group approvers' do
+      let(:action) { { type: "require_approval", approvals_required: 1, group_approvers: [group.path], user_approvers_ids: [user.id] } }
+
+      it 'returns all approvers' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:approvers].map(&:class)).to match_array([API::Entities::UserBasic, API::Entities::PublicGroupDetails])
+        expect(response[:approvers].as_json.first).to include(id: user.id, username: user.username, state: user.state)
+        expect(response[:approvers].as_json.last).to include(id: group.id, full_name: group.full_name, full_path: group.full_path, name: group.name)
+      end
+    end
+
     context 'with policy equals to nil' do
       let(:policy) { nil }
 
-      it 'returns nil' do
+      it 'returns an empty array' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_nil
+        expect(response[:approvers]).to be_empty
       end
     end
 
     context 'with action equals to nil' do
       let(:action) { nil }
 
-      it 'returns nil' do
+      it 'returns an empty array' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_nil
+        expect(response[:approvers]).to be_empty
       end
     end
 
     context 'with action of an unknown type' do
       let(:action) { { type: "random_type", approvals_required: 1, group_approvers_ids: [group.id] } }
 
-      it 'returns nil' do
+      it 'returns sn empty array' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_nil
+        expect(response[:approvers]).to be_empty
       end
     end
   end
-- 
GitLab


From ce44b133b064d8cbb4d4aae77239186128a38ecd Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Wed, 9 Feb 2022 08:22:37 -0500
Subject: [PATCH 3/7] Address frontend maintainer review

by extracting repetitive calls into a variable
in the specs and replacing string interpolation
by setUrlFragment.
---
 .../scan_execution_policy_editor.vue                     | 7 +++++--
 .../scan_result_policy/scan_result_policy_editor.vue     | 7 +++++--
 .../scan_execution_policy_editor_spec.js                 | 9 ++++++---
 .../scan_result_policy/scan_result_policy_editor_spec.js | 9 ++++++---
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
index c0ed179af257f2..054b8b2cddf883 100644
--- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
+++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue
@@ -1,6 +1,6 @@
 <script>
 import { GlEmptyState } from '@gitlab/ui';
-import { joinPaths, visitUrl } from '~/lib/utils/url_utility';
+import { joinPaths, visitUrl, setUrlFragment } from '~/lib/utils/url_utility';
 import { __, s__ } from '~/locale';
 import {
   EDITOR_MODES,
@@ -62,7 +62,10 @@ export default {
       newlyCreatedPolicyProject: null,
       policy: fromYaml(yamlEditorValue),
       yamlEditorValue,
-      documentationPath: `${this.scanPolicyDocumentationPath}#scan-execution-policy-editor`,
+      documentationPath: setUrlFragment(
+        this.scanPolicyDocumentationPath,
+        'scan-execution-policy-editor',
+      ),
     };
   },
   computed: {
diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
index 29fc565bcea07f..811e5ccec3d699 100644
--- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
+++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue
@@ -1,6 +1,6 @@
 <script>
 import { GlEmptyState } from '@gitlab/ui';
-import { joinPaths, visitUrl } from '~/lib/utils/url_utility';
+import { joinPaths, visitUrl, setUrlFragment } from '~/lib/utils/url_utility';
 import { __, s__ } from '~/locale';
 import {
   EDITOR_MODES,
@@ -63,7 +63,10 @@ export default {
       newlyCreatedPolicyProject: null,
       policy: fromYaml(yamlEditorValue),
       yamlEditorValue,
-      documentationPath: `${this.scanPolicyDocumentationPath}#scan-result-policy-editor`,
+      documentationPath: setUrlFragment(
+        this.scanPolicyDocumentationPath,
+        'scan-result-policy-editor',
+      ),
     };
   },
   computed: {
diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
index 0342a1f17efd6b..0b66f1e0b20b61 100644
--- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
+++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js
@@ -21,6 +21,7 @@ import { SECURITY_POLICY_ACTIONS } from 'ee/threat_monitoring/components/policy_
 jest.mock('~/lib/utils/url_utility', () => ({
   joinPaths: jest.requireActual('~/lib/utils/url_utility').joinPaths,
   visitUrl: jest.fn().mockName('visitUrlMock'),
+  setUrlFragment: jest.requireActual('~/lib/utils/url_utility').setUrlFragment,
 }));
 
 const newlyCreatedPolicyProject = {
@@ -143,9 +144,11 @@ describe('ScanExecutionPolicyEditor', () => {
     it('displays the empty state with the appropriate properties', async () => {
       factory({ provide: { disableScanPolicyUpdate: true } });
       await nextTick();
-      expect(findEmptyState().props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
-      expect(findEmptyState().props('primaryButtonLink')).toMatch('scan-execution-policy-editor');
-      expect(findEmptyState().props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
+      const emptyState = findEmptyState();
+
+      expect(emptyState.props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
+      expect(emptyState.props('primaryButtonLink')).toMatch('scan-execution-policy-editor');
+      expect(emptyState.props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
     });
   });
 });
diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
index 0a8666da7bf30e..b1ca7c27183d2d 100644
--- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
+++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js
@@ -21,6 +21,7 @@ import { SECURITY_POLICY_ACTIONS } from 'ee/threat_monitoring/components/policy_
 jest.mock('~/lib/utils/url_utility', () => ({
   joinPaths: jest.requireActual('~/lib/utils/url_utility').joinPaths,
   visitUrl: jest.fn().mockName('visitUrlMock'),
+  setUrlFragment: jest.requireActual('~/lib/utils/url_utility').setUrlFragment,
 }));
 
 const newlyCreatedPolicyProject = {
@@ -125,9 +126,11 @@ describe('ScanResultPolicyEditor', () => {
   describe('when a user is not an owner of the project', () => {
     it('displays the empty state with the appropriate properties', async () => {
       factory({ provide: { disableScanPolicyUpdate: true } });
-      expect(findEmptyState().props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
-      expect(findEmptyState().props('primaryButtonLink')).toMatch('scan-result-policy-editor');
-      expect(findEmptyState().props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
+      const emptyState = findEmptyState();
+
+      expect(emptyState.props('primaryButtonLink')).toMatch(scanPolicyDocumentationPath);
+      expect(emptyState.props('primaryButtonLink')).toMatch('scan-result-policy-editor');
+      expect(emptyState.props('svgPath')).toBe(policyEditorEmptyStateSvgPath);
     });
   });
 });
-- 
GitLab


From 79b695332b5b8203feb985908e4093969986864c Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Wed, 9 Feb 2022 12:07:21 -0500
Subject: [PATCH 4/7] Address backend maintainer suggestions

by moving the presenters out of the service into

controller as per dev guidelines. It adds guard
so to not query if there is no users or groups.
---
 .../projects/security/policies_controller.rb  |  4 ++-
 .../fetch_policy_approvers_service.rb         | 24 ++++++++--------
 .../security/policies_controller_spec.rb      |  7 +++--
 .../fetch_policy_approvers_service_spec.rb    | 28 ++++++++++---------
 4 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/ee/app/controllers/projects/security/policies_controller.rb b/ee/app/controllers/projects/security/policies_controller.rb
index c6a59ffc27fa9f..85dce280b1e514 100644
--- a/ee/app/controllers/projects/security/policies_controller.rb
+++ b/ee/app/controllers/projects/security/policies_controller.rb
@@ -100,7 +100,9 @@ def approvers
           current_user: @current_user
         ).execute
 
-        result[:approvers]
+        return unless result[:status] == :success
+
+        API::Entities::UserBasic.represent(result[:users]) + API::Entities::PublicGroupDetails.represent(result[:groups])
       end
     end
   end
diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
index 94bc03a79559ab..eeb93c28affdff 100644
--- a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
@@ -12,30 +12,28 @@ def initialize(policy:, project:, current_user:)
       end
 
       def execute
-        success({ approvers: approvers })
+        action = policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
+
+        success({ users: user_approvers(action), groups: group_approvers(action) })
       end
 
       private
 
       attr_reader :policy, :project, :current_user
 
-      def approvers
-        action = policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
-
-        return [] unless action
-
-        API::Entities::UserBasic.represent(user_approvers(action)) + API::Entities::PublicGroupDetails.represent(group_approvers(action))
-      end
-
       def user_approvers(action)
-        user_names = action[:user_approvers] || []
-        user_ids = action[:user_approvers_ids] || []
+        return [] unless action && (action[:user_approvers] || action[:user_approvers_ids])
+
+        user_names = action[:user_approvers]
+        user_ids = action[:user_approvers_ids]
         project.team.users.by_ids_or_usernames(user_ids, user_names)
       end
 
       def group_approvers(action)
-        group_paths = action[:group_approvers] || []
-        group_ids = action[:group_approvers_ids] || []
+        return [] unless action && (action[:group_approvers] || action[:group_approvers_ids])
+
+        group_paths = action[:group_approvers]
+        group_ids = action[:group_approvers_ids]
         Group.unscoped.public_to_user(current_user).by_ids_or_paths(group_ids, group_paths)
       end
     end
diff --git a/ee/spec/requests/projects/security/policies_controller_spec.rb b/ee/spec/requests/projects/security/policies_controller_spec.rb
index ff4ec618213b00..c4081bad31e26a 100644
--- a/ee/spec/requests/projects/security/policies_controller_spec.rb
+++ b/ee/spec/requests/projects/security/policies_controller_spec.rb
@@ -104,9 +104,10 @@
         context 'with scan result policy type' do
           let_it_be(:type) {'scan_result_policy'}
           let_it_be(:policy) {build(:scan_result_policy)}
-          let_it_be(:approvers) { [{ id: 1, name: 'one' }, { id: 2, name: "two" }] }
+          let_it_be(:group) { create(:group) }
+          let_it_be(:service_result) { { users: [user], groups: [group], status: :success } }
 
-          let(:service) { instance_double('::Security::SecurityOrchestrationPolicies::FetchPolicyApproversService', execute: { approvers: approvers }) }
+          let(:service) { instance_double('::Security::SecurityOrchestrationPolicies::FetchPolicyApproversService', execute: service_result) }
 
           before do
             stub_feature_flags(scan_result_policy: true)
@@ -126,7 +127,7 @@
 
             expect(app['data-policy']).to eq(policy.to_json)
             expect(app['data-policy-type']).to eq(type)
-            expect(app['data-scan-result-approvers']).to eq(approvers.to_json)
+            expect(app['data-scan-result-approvers']).to include(user.name, user.id.to_s, group.full_path, group.id.to_s)
           end
 
           context 'with feature flag disabled' do
diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
index d2d45effdc6f7f..1b4d13f3a4dfbe 100644
--- a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -25,8 +25,8 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to all(be_a(API::Entities::UserBasic))
-        expect(response[:approvers].as_json.first).to include(id: user.id, username: user.username, state: user.state)
+        expect(response[:users]).to eq([user])
+        expect(response[:groups]).to be_empty
       end
     end
 
@@ -37,8 +37,8 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to all(be_a(API::Entities::PublicGroupDetails))
-        expect(response[:approvers].as_json.first).to include(id: group.id, full_name: group.full_name, full_path: group.full_path, name: group.name)
+        expect(response[:groups]).to eq([group])
+        expect(response[:users]).to be_empty
       end
     end
 
@@ -49,42 +49,44 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers].map(&:class)).to match_array([API::Entities::UserBasic, API::Entities::PublicGroupDetails])
-        expect(response[:approvers].as_json.first).to include(id: user.id, username: user.username, state: user.state)
-        expect(response[:approvers].as_json.last).to include(id: group.id, full_name: group.full_name, full_path: group.full_path, name: group.name)
+        expect(response[:users]).to eq([user])
+        expect(response[:groups]).to eq([group])
       end
     end
 
     context 'with policy equals to nil' do
       let(:policy) { nil }
 
-      it 'returns an empty array' do
+      it 'returns no approver' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_empty
+        expect(response[:users]).to be_empty
+        expect(response[:groups]).to be_empty
       end
     end
 
     context 'with action equals to nil' do
       let(:action) { nil }
 
-      it 'returns an empty array' do
+      it 'returns no approver' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_empty
+        expect(response[:users]).to be_empty
+        expect(response[:groups]).to be_empty
       end
     end
 
     context 'with action of an unknown type' do
       let(:action) { { type: "random_type", approvals_required: 1, group_approvers_ids: [group.id] } }
 
-      it 'returns sn empty array' do
+      it 'returns no approver' do
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:approvers]).to be_empty
+        expect(response[:users]).to be_empty
+        expect(response[:groups]).to be_empty
       end
     end
   end
-- 
GitLab


From 1ec2803db7eeddd961ca4b4053580b05d48e110d Mon Sep 17 00:00:00 2001
From: Danger bot <group9970_bot@noreply.gitlab.com>
Date: Wed, 9 Feb 2022 17:20:05 +0000
Subject: [PATCH 5/7] Apply 4 suggestion(s) to 1 file(s)

---
 .../fetch_policy_approvers_service_spec.rb                | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
index 1b4d13f3a4dfbe..075a8ca3bd20c5 100644
--- a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -25,7 +25,7 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:users]).to eq([user])
+        expect(response[:users]).to match_array([user])
         expect(response[:groups]).to be_empty
       end
     end
@@ -37,7 +37,7 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:groups]).to eq([group])
+        expect(response[:groups]).to match_array([group])
         expect(response[:users]).to be_empty
       end
     end
@@ -49,8 +49,8 @@
         response = service.execute
 
         expect(response[:status]).to eq(:success)
-        expect(response[:users]).to eq([user])
-        expect(response[:groups]).to eq([group])
+        expect(response[:users]).to match_array([user])
+        expect(response[:groups]).to match_array([group])
       end
     end
 
-- 
GitLab


From fa235a7af5e68d6a56b5e3ad72f09270716e65bb Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Thu, 10 Feb 2022 20:59:56 -0500
Subject: [PATCH 6/7] Replace how Group was being queried. Add limits

the total amount of user approvers and group
approvers and update specs.
---
 .../concerns/security/scan_result_policy.rb   |  3 +
 .../fetch_policy_approvers_service.rb         | 34 ++++++---
 .../fetch_policy_approvers_service_spec.rb    | 69 +++++++++++++++++--
 3 files changed, 94 insertions(+), 12 deletions(-)

diff --git a/ee/app/models/concerns/security/scan_result_policy.rb b/ee/app/models/concerns/security/scan_result_policy.rb
index e06f7d7a1d1fd0..6104e118c67743 100644
--- a/ee/app/models/concerns/security/scan_result_policy.rb
+++ b/ee/app/models/concerns/security/scan_result_policy.rb
@@ -4,8 +4,11 @@ module Security
   module ScanResultPolicy
     extend ActiveSupport::Concern
 
+    # Used for both policies and rules
     LIMIT = 5
 
+    APPROVERS_LIMIT = 300
+
     SCAN_FINDING = 'scan_finding'
 
     REQUIRE_APPROVAL = 'require_approval'
diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
index eeb93c28affdff..894693327e2074 100644
--- a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
@@ -5,6 +5,8 @@ module SecurityOrchestrationPolicies
     class FetchPolicyApproversService
       include BaseServiceUtility
 
+      GROUP_FINDER_PARAMS = { with_shared: true, shared_visible_only: true, shared_min_access_level: 30 }.freeze
+
       def initialize(policy:, project:, current_user:)
         @policy = policy
         @project = project
@@ -12,7 +14,9 @@ def initialize(policy:, project:, current_user:)
       end
 
       def execute
-        action = policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
+        action = required_approval(policy)
+
+        return success({ users: [], groups: [] }) unless action
 
         success({ users: user_approvers(action), groups: group_approvers(action) })
       end
@@ -21,20 +25,34 @@ def execute
 
       attr_reader :policy, :project, :current_user
 
+      def required_approval(policy)
+        policy&.fetch(:actions)&.find { |action| action&.fetch(:type) == Security::ScanResultPolicy::REQUIRE_APPROVAL }
+      end
+
       def user_approvers(action)
-        return [] unless action && (action[:user_approvers] || action[:user_approvers_ids])
+        return [] unless action[:user_approvers] || action[:user_approvers_ids]
 
-        user_names = action[:user_approvers]
-        user_ids = action[:user_approvers_ids]
+        user_names, user_ids = approvers_within_limit(action[:user_approvers], action[:user_approvers_ids])
         project.team.users.by_ids_or_usernames(user_ids, user_names)
       end
 
       def group_approvers(action)
-        return [] unless action && (action[:group_approvers] || action[:group_approvers_ids])
+        return [] unless action[:group_approvers] || action[:group_approvers_ids]
+
+        group_paths, group_ids = approvers_within_limit(action[:group_approvers], action[:group_approvers_ids])
+
+        Projects::GroupsFinder.new(project: project, current_user: current_user, params: GROUP_FINDER_PARAMS).execute.by_ids_or_paths(group_ids, group_paths)
+      end
+
+      def approvers_within_limit(names, ids)
+        filtered_names = names&.first(Security::ScanResultPolicy::APPROVERS_LIMIT) || []
+        filtered_ids = []
+
+        if filtered_names.count < Security::ScanResultPolicy::APPROVERS_LIMIT
+          filtered_ids = ids&.first(Security::ScanResultPolicy::APPROVERS_LIMIT - filtered_names.count)
+        end
 
-        group_paths = action[:group_approvers]
-        group_ids = action[:group_approvers_ids]
-        Group.unscoped.public_to_user(current_user).by_ids_or_paths(group_ids, group_paths)
+        [filtered_names, filtered_ids]
       end
     end
   end
diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
index 075a8ca3bd20c5..69982183c6312d 100644
--- a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -4,11 +4,12 @@
 
 RSpec.describe Security::SecurityOrchestrationPolicies::FetchPolicyApproversService do
   describe '#execute' do
-    let(:group) { create(:group) }
-    let(:project) { create(:project, :public, namespace: group) }
-    let(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, :public, namespace: group) }
+    let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+    let_it_be(:user) { create(:user) }
+
     let(:policy) { build(:scan_result_policy, actions: [action]) }
-    let(:user) { create(:user) }
 
     subject(:service) do
       described_class.new(policy: policy, current_user: user, project: project)
@@ -89,5 +90,65 @@
         expect(response[:groups]).to be_empty
       end
     end
+
+    context 'with more users than the limit' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:user_ids) { [user.id] }
+      let(:user_names) { [user.username] }
+
+      where(:ids_multiplier, :names_multiplier, :ids_expected, :names_expected) do
+        150 | 150 | 150 | 150
+        300 | 300 | 0   | 300
+        300 | 200 | 100 | 200
+        600 | 600 | 0   | 300
+      end
+
+      with_them do
+        let(:user_ids_multiplied) { user_ids * ids_multiplier }
+        let(:user_name_multiplied) { user_names * names_multiplier }
+        let(:user_ids_expected) { user_ids * ids_expected }
+        let(:user_name_expected) { user_names * names_expected }
+        let(:action) { { type: "require_approval", approvals_required: 1, user_approvers: user_name_multiplied, user_approvers_ids: user_ids_multiplied } }
+
+        it 'considers only the first within the limit' do
+          expect(project).to receive_message_chain(:team, :users, :by_ids_or_usernames).with(user_ids_expected, user_name_expected)
+
+          service.execute
+
+          expect((user_ids_expected + user_name_expected).count).not_to be > Security::ScanResultPolicy::APPROVERS_LIMIT
+        end
+      end
+    end
+
+    context 'with more groups than the limit' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:group_ids) { [group.id] }
+      let(:group_paths) { [group.path] }
+
+      where(:ids_multiplier, :paths_multiplier, :ids_expected, :paths_expected) do
+        150 | 150 | 150 | 150
+        300 | 300 | 0   | 300
+        300 | 200 | 100 | 200
+        600 | 600 | 0   | 300
+      end
+
+      with_them do
+        let(:group_ids_multiplied) { group_ids * ids_multiplier }
+        let(:group_path_multiplied) { group_paths * paths_multiplier }
+        let(:group_ids_expected) { group_ids * ids_expected }
+        let(:group_path_expected) { group_paths * paths_expected }
+        let(:action) { { type: "require_approval", approvals_required: 1, group_approvers: group_path_multiplied, group_approvers_ids: group_ids_multiplied } }
+
+        it 'considers only the first within the limit' do
+          expect(Projects::GroupsFinder).to receive_message_chain(:new, :execute, :by_ids_or_paths).with(group_ids_expected, group_path_expected)
+
+          service.execute
+
+          expect((group_ids_expected + group_path_expected).count).not_to be > Security::ScanResultPolicy::APPROVERS_LIMIT
+        end
+      end
+    end
   end
 end
-- 
GitLab


From 11da3216a3ff7bcf1f53d5fcb23b1aa490764940 Mon Sep 17 00:00:00 2001
From: Zamir Martins Filho <zfilho@gitlab.com>
Date: Mon, 14 Feb 2022 11:50:13 -0500
Subject: [PATCH 7/7] Address maintainer reviews

by adding spec to check if the scope of groups is
being considered.
---
 .../fetch_policy_approvers_service_spec.rb            | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
index 69982183c6312d..947b556073a054 100644
--- a/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
+++ b/ee/spec/services/security/security_orchestration_policies/fetch_policy_approvers_service_spec.rb
@@ -19,6 +19,17 @@
       group.add_user(user, :owner)
     end
 
+    context 'with group outside of the scope' do
+      let(:unrelated_group) { create(:group) }
+      let(:action) { { type: "require_approval", approvals_required: 1, group_approvers_ids: [unrelated_group.id, group.id] } }
+
+      it 'does not return the unrelated group' do
+        response = service.execute
+
+        expect(response[:groups]).to contain_exactly(group)
+      end
+    end
+
     context 'with user approver' do
       let(:action) { { type: "require_approval", approvals_required: 1, user_approvers: [user.username] } }
 
-- 
GitLab