Merge branch 'add_support_for_group_and_user_id' into 'master'

Add support for user_id, group_id and group_path

See merge request gitlab-org/gitlab!76595

app/models/group.rb

@@ -123,6 +123,8 @@ class Group < Namespace
 
   scope :by_id, ->(groups) { where(id: groups) }
 
+  scope :by_ids_or_paths, -> (ids, paths) { by_id(ids).or(where(path: paths)) }
+
   scope :for_authorized_group_members, -> (user_ids) do
     joins(:group_members)
       .where(members: { user_id: user_ids })
@@ -214,6 +216,10 @@ class Group < Namespace
       Set.new(group_ids)
     end
 
+    def get_ids_by_ids_or_paths(ids, paths)
+      by_ids_or_paths(ids, paths).pluck(:id)
+    end
+
     private
 
     def public_to_user_arel(user)

app/models/user.rb

@@ -466,7 +466,7 @@ class User < ApplicationRecord
   scope :dormant, -> { with_state(:active).human_or_service_user.where('last_activity_on <= ?', MINIMUM_INACTIVE_DAYS.day.ago.to_date) }
   scope :with_no_activity, -> { with_state(:active).human_or_service_user.where(last_activity_on: nil) }
   scope :by_provider_and_extern_uid, ->(provider, extern_uid) { joins(:identities).merge(Identity.with_extern_uid(provider, extern_uid)) }
-  scope :get_ids_by_username, -> (username) { where(username: username).pluck(:id) }
+  scope :by_ids_or_usernames, -> (ids, usernames) { where(username: usernames).or(where(id: ids)) }
 
   strip_attributes! :name
 
@@ -836,6 +836,10 @@ class User < ApplicationRecord
     def single_user
       User.non_internal.first if single_user?
     end
+
+    def get_ids_by_ids_or_usernames(ids, usernames)
+      by_ids_or_usernames(ids, usernames).pluck(:id)
+    end
   end
 
   #

ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb

@@ -42,11 +42,12 @@ module Security
           scanners: rule[:scanners],
           rule_type: :report_approver,
           severity_levels: rule[:severity_levels],
-          user_ids: project.users.get_ids_by_username(action_info[:approvers]),
+          user_ids: users_ids(action_info[:user_approvers_ids], action_info[:user_approvers]),
           vulnerabilities_allowed: rule[:vulnerabilities_allowed],
           report_type: :scan_finding,
           orchestration_policy_idx: policy_index,
-          vulnerability_states: rule[:vulnerability_states]
+          vulnerability_states: rule[:vulnerability_states],
+          group_ids: groups_ids(action_info[:group_approvers_ids], action_info[:group_approvers])
         }
       end
 
@@ -56,6 +57,14 @@ module Security
 
         "#{truncated} #{rule_index + 1}"
       end
+
+      def users_ids(user_ids, user_names)
+        project.team.users.get_ids_by_ids_or_usernames(user_ids, user_names)
+      end
+
+      def groups_ids(group_ids, group_paths)
+        Group.unscoped.public_to_user(author).get_ids_by_ids_or_paths(group_ids, group_paths)
+      end
     end
   end
 end

ee/app/validators/json_schemas/security_orchestration_policy.json

@@ -315,10 +315,11 @@
             "type": "array",
             "additionalItems": false,
             "items": {
-              "required": [
-                "type",
-                "approvals_required",
-                "approvers"
+              "anyOf":[
+                {"required": ["type","approvals_required","user_approvers"]},
+                {"required": ["type","approvals_required","user_approvers_ids"]},
+                {"required": ["type","approvals_required","group_approvers"]},
+                {"required": ["type","approvals_required","group_approvers_ids"]}
               ],
               "type": "object",
               "properties": {
@@ -331,13 +332,37 @@
                 "approvals_required": {
                   "type": "integer"
                 },
-                "approvers": {
+                "user_approvers": {
+                  "type": "array",
+                  "additionalItems": false,
+                  "items": {
+                    "minLength": 1,
+                    "type": "string"
+                  }
+                },
+                "user_approvers_ids": {
+                  "type": "array",
+                  "additionalItems": false,
+                  "items": {
+                    "minLength": 1,
+                    "type": "integer"
+                  }
+                },
+                "group_approvers": {
                   "type": "array",
                   "additionalItems": false,
                   "items": {
                     "minLength": 1,
                     "type": "string"
                   }
+                },
+                "group_approvers_ids": {
+                  "type": "array",
+                  "additionalItems": false,
+                  "items": {
+                    "minLength": 1,
+                    "type": "integer"
+                  }
                 }
               }
             }

ee/spec/factories/security/policies.rb

@@ -54,7 +54,7 @@ FactoryBot.define do
       ]
     end
 
-    actions { [{ type: 'require_approval', approvals_required: 1, approvers: %w[admin] }] }
+    actions { [{ type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] }] }
   end
 
   factory :orchestration_policy_yaml, class: Struct.new(:scan_execution_policy, :scan_result_policy) do

ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb

@@ -6,13 +6,15 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyS
   describe '#execute' do
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration) }
 
-    let(:approver) { create(:user) }
+    let(:group) { create(:group, :public) }
     let(:policy) { build(:scan_result_policy, name: 'Test Policy') }
     let(:policy_yaml) { Gitlab::Config::Loader::Yaml.new(policy.to_yaml).load! }
     let(:project) { policy_configuration.project }
+    let(:approver) { project.owner }
     let(:service) { described_class.new(policy_configuration: policy_configuration, policy: policy, policy_index: 0) }
 
     before do
+      group.add_maintainer(approver)
       allow(policy_configuration).to receive(:policy_last_updated_by).and_return(project.owner)
     end
 
@@ -44,8 +46,35 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyS
       end
     end
 
-    it 'creates a new project approval rule' do
-      expect { subject }.to change { project.approval_rules.count }.by(1)
+    shared_examples 'create approval rule with specific approver' do
+      it 'succeeds creating approval rules with specific approver' do
+        expect { subject }.to change { project.approval_rules.count }.by(1)
+        expect(project.approval_rules.first.approvers).to contain_exactly(approver)
+      end
+    end
+
+    context 'with only user id' do
+      let(:policy) { build(:scan_result_policy, name: 'Test Policy', actions: [{ type: 'require_approval', approvals_required: 1, user_approvers_ids: [approver.id] }]) }
+
+      it_behaves_like 'create approval rule with specific approver'
+    end
+
+    context 'with only username' do
+      let(:policy) { build(:scan_result_policy, name: 'Test Policy', actions: [{ type: 'require_approval', approvals_required: 1, user_approvers: [approver.username] }]) }
+
+      it_behaves_like 'create approval rule with specific approver'
+    end
+
+    context 'with only group id' do
+      let(:policy) { build(:scan_result_policy, name: 'Test Policy', actions: [{ type: 'require_approval', approvals_required: 1, group_approvers_ids: [group.id] }]) }
+
+      it_behaves_like 'create approval rule with specific approver'
+    end
+
+    context 'with only group path' do
+      let(:policy) { build(:scan_result_policy, name: 'Test Policy', actions: [{ type: 'require_approval', approvals_required: 1, group_approvers: [group.path] }]) }
+
+      it_behaves_like 'create approval rule with specific approver'
     end
 
     it 'sets project approval rules names based on policy name', :aggregate_failures do

ee/spec/workers/security/create_orchestration_policy_worker_spec.rb

@@ -48,7 +48,7 @@ RSpec.describe Security::CreateOrchestrationPolicyWorker do
               enabled: true,
               rules: [{ type: 'scan_finding', branches: %w[production], vulnerabilities_allowed: 0, severity_levels: %w[critical], scanners: %w[container_scanning], vulnerability_states: %w[newly_detected] }],
               actions: [
-                { type: 'require_approval', approvals_required: 1, approvers: %w[admin] }
+                { type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] }
               ]
             }
           ]

spec/models/group_spec.rb

@@ -680,6 +680,26 @@ RSpec.describe Group do
         expect(result).to match_array([internal_group])
       end
     end
+
+    describe 'by_ids_or_paths' do
+      let(:group_path) { 'group_path' }
+      let!(:group) { create(:group, path: group_path) }
+      let(:group_id) { group.id }
+
+      it 'returns matching records based on paths' do
+        expect(described_class.by_ids_or_paths(nil, [group_path])).to match_array([group])
+      end
+
+      it 'returns matching records based on ids' do
+        expect(described_class.by_ids_or_paths([group_id], nil)).to match_array([group])
+      end
+
+      it 'returns matching records based on both paths and ids' do
+        new_group = create(:group)
+
+        expect(described_class.by_ids_or_paths([new_group.id], [group_path])).to match_array([group, new_group])
+      end
+    end
   end
 
   describe '#to_reference' do
@@ -2803,4 +2823,23 @@ RSpec.describe Group do
       expect(group.crm_enabled?).to be_truthy
     end
   end
+  describe '.get_ids_by_ids_or_paths' do
+    let(:group_path) { 'group_path' }
+    let!(:group) { create(:group, path: group_path) }
+    let(:group_id) { group.id }
+
+    it 'returns ids matching records based on paths' do
+      expect(described_class.get_ids_by_ids_or_paths(nil, [group_path])).to match_array([group_id])
+    end
+
+    it 'returns ids matching records based on ids' do
+      expect(described_class.get_ids_by_ids_or_paths([group_id], nil)).to match_array([group_id])
+    end
+
+    it 'returns ids matching records based on both paths and ids' do
+      new_group_id = create(:group).id
+
+      expect(described_class.get_ids_by_ids_or_paths([new_group_id], [group_path])).to match_array([group_id, new_group_id])
+    end
+  end
 end

spec/models/user_spec.rb

@@ -6458,13 +6458,43 @@ RSpec.describe User do
     specify { is_expected.to contain_exactly(developer_group2) }
   end
 
-  describe '.get_ids_by_username' do
+  describe '.get_ids_by_ids_or_usernames' do
     let(:user_name) { 'user_name' }
     let!(:user) { create(:user, username: user_name) }
     let(:user_id) { user.id }
 
     it 'returns the id of each record matching username' do
-      expect(described_class.get_ids_by_username([user_name])).to match_array([user_id])
+      expect(described_class.get_ids_by_ids_or_usernames(nil, [user_name])).to match_array([user_id])
+    end
+
+    it 'returns the id of each record matching user id' do
+      expect(described_class.get_ids_by_ids_or_usernames([user_id], nil)).to match_array([user_id])
+    end
+
+    it 'return the id for all records matching either user id or user name' do
+      new_user_id = create(:user).id
+
+      expect(described_class.get_ids_by_ids_or_usernames([new_user_id], [user_name])).to match_array([user_id, new_user_id])
+    end
+  end
+
+  describe '.by_ids_or_usernames' do
+    let(:user_name) { 'user_name' }
+    let!(:user) { create(:user, username: user_name) }
+    let(:user_id) { user.id }
+
+    it 'returns matching records based on username' do
+      expect(described_class.by_ids_or_usernames(nil, [user_name])).to match_array([user])
+    end
+
+    it 'returns matching records based on id' do
+      expect(described_class.by_ids_or_usernames([user_id], nil)).to match_array([user])
+    end
+
+    it 'returns matching records based on both username and id' do
+      new_user = create(:user)
+
+      expect(described_class.by_ids_or_usernames([new_user.id], [user_name])).to match_array([user, new_user])
     end
   end