From 8c1c4d2b8b79a17abd18d94dd5f22ed676168c27 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Fri, 17 Oct 2025 15:02:24 +0200 Subject: [PATCH 1/3] Add members_finder for MR approval policies This change adds a new finder to get the project members as approvers for a policy with custom roles and access levels. EE: true Changelog: fixed --- app/finders/members_finder.rb | 17 ++- .../scan_result_policies/members_finder.rb | 41 ++++++ ee/app/models/concerns/approval_rule_like.rb | 8 +- ee/app/models/ee/member.rb | 7 + ee/app/models/ee/project_team.rb | 23 +--- .../members_finder_spec.rb | 126 ++++++++++++++++++ ee/spec/models/ee/member_spec.rb | 66 +++++++++ ee/spec/models/ee/project_team_spec.rb | 53 -------- 8 files changed, 258 insertions(+), 83 deletions(-) create mode 100644 ee/app/finders/security/scan_result_policies/members_finder.rb create mode 100644 ee/spec/finders/security/scan_result_policies/members_finder_spec.rb diff --git a/app/finders/members_finder.rb b/app/finders/members_finder.rb index 5d1a975b8c4364..ad8effe4b226da 100644 --- a/app/finders/members_finder.rb +++ b/app/finders/members_finder.rb @@ -88,19 +88,22 @@ def direct_group_members(include_relations) end def project_invited_groups - invited_groups_including_ancestors = project.invited_groups.self_and_ancestors - unless project.member?(current_user) - invited_groups_including_ancestors = invited_groups_including_ancestors.public_or_visible_to_user(current_user) - end - - invited_groups_ids_including_ancestors = invited_groups_including_ancestors.select(:id) - invited_group_members = GroupMember.with_source_id(invited_groups_ids_including_ancestors).non_minimal_access + invited_group_members = GroupMember + .with_source_id(invited_groups_including_ancestors.select(:id)) + .non_minimal_access + .non_invite return invited_group_members.select(Member.column_names) if project.share_with_group_enabled? # Return no access for invited group members when project sharing with group is disabled invited_group_members.coerce_to_no_access end + def invited_groups_including_ancestors + invited_groups = project.invited_groups.self_and_ancestors + invited_groups = invited_groups.public_or_visible_to_user(current_user) unless project.member?(current_user) + invited_groups + end + def distinct_union_of_members(union_members) union = Gitlab::SQL::Union.new(union_members, remove_duplicates: false) # rubocop: disable Gitlab/Union sql = distinct_on(union) diff --git a/ee/app/finders/security/scan_result_policies/members_finder.rb b/ee/app/finders/security/scan_result_policies/members_finder.rb new file mode 100644 index 00000000000000..72c325043ada29 --- /dev/null +++ b/ee/app/finders/security/scan_result_policies/members_finder.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Security + module ScanResultPolicies + class MembersFinder < ::MembersFinder + def initialize(project, params: {}) + params[:active_without_invites_and_requests] = true + super(project, nil, params: params) + end + + def execute + members = find_members(%i[direct inherited invited_groups]) + filter_members(members) + end + + private + + def filter_members(members) + member_role_ids = params[:member_role_ids] || [] + access_levels = params[:access_levels] || [] + + if member_role_ids.any? && access_levels.any? + members = members.by_member_role_or_access_level( + member_role_ids: member_role_ids, + access_levels: access_levels + ) + elsif member_role_ids.any? + members = members.with_member_role_id(member_role_ids) + elsif access_levels.any? + members = members.by_access_level(access_levels) + end + + members + end + + def invited_groups_including_ancestors + project.invited_groups.self_and_ancestors + end + end + end +end diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index b55c942498d1f8..936a0817907b89 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -192,10 +192,12 @@ def with_role_approvers def scan_result_policy_read_role_approvers return User.none unless scan_result_policy_read - project.team.members_with_access_level_or_custom_roles( - levels: scan_result_policy_read.role_approvers, + members = ::Security::ScanResultPolicies::MembersFinder.new(project, params: { + access_levels: scan_result_policy_read.role_approvers, member_role_ids: scan_result_policy_read.custom_role_ids_with_permission - ) + }).execute + + User.id_in(members.pluck_user_ids) end def filter_inactive_approvers(approvers) diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index e6010d8087cd36..e9aa1d3bb19d22 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -77,6 +77,13 @@ module Member end scope :order_access_level_desc, -> { order(access_level: :desc) } + + scope :by_member_role_or_access_level, ->(member_role_ids:, access_levels:) do + where( + arel_table[:member_role_id].in(member_role_ids) + .or(arel_table[:access_level].in(access_levels)) + ) + end end class_methods do diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 57f8f75a820a6b..589b81fe026630 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -5,30 +5,13 @@ module ProjectTeam extend ActiveSupport::Concern extend ::Gitlab::Utils::Override - def members_with_access_level_or_custom_roles(levels: [], member_role_ids: []) - return ::User.none unless levels.any? || member_role_ids.any? - - users = project.authorized_users - - if levels.any? && member_role_ids.any? - users = users - .where(project_authorizations: { access_level: levels }) - .or(users.where(members: { member_role_id: member_role_ids })) - .joins(:members) - elsif levels.any? - users = users.where(project_authorizations: { access_level: levels }) - elsif member_role_ids.any? - users = users.joins(:members).where(members: { member_role_id: member_role_ids }) - end - - users - end - def user_exists_with_access_level_or_custom_roles?(user, levels: [], member_role_ids: []) return false unless levels.any? || member_role_ids.any? return false unless user - members_with_access_level_or_custom_roles(levels: levels, member_role_ids: member_role_ids).exists?(id: user.id) + ::Security::ScanResultPolicies::MembersFinder + .new(project, params: { access_levels: levels, member_role_ids: member_role_ids }) + .execute.exists?(user_id: user.id) end override :add_members diff --git a/ee/spec/finders/security/scan_result_policies/members_finder_spec.rb b/ee/spec/finders/security/scan_result_policies/members_finder_spec.rb new file mode 100644 index 00000000000000..63e1e15f7969cd --- /dev/null +++ b/ee/spec/finders/security/scan_result_policies/members_finder_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::ScanResultPolicies::MembersFinder, feature_category: :security_policy_management do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:current_user) { create(:user) } + + let_it_be(:project_member) { create(:project_member, project: project, access_level: Gitlab::Access::DEVELOPER) } + let_it_be(:group_member) { create(:group_member, group: group, access_level: Gitlab::Access::MAINTAINER) } + let_it_be(:inherited_member) do + create(:group_member, group: group.parent, access_level: Gitlab::Access::OWNER) if group.parent + end + + let(:params) { {} } + + subject(:execute) { described_class.new(project, params: params).execute } + + before_all do + project.add_developer(current_user) + end + + describe '#execute' do + context 'with basic project members' do + it 'returns project members' do + expect(execute).to include(project_member) + end + + it 'includes inherited members by default' do + expect(execute).to include(project_member, group_member) + end + end + + context 'with invited groups' do + let_it_be(:invited_group) { create(:group) } + let_it_be(:invited_group_member) do + create(:group_member, group: invited_group, access_level: Gitlab::Access::DEVELOPER) + end + + before do + project.project_group_links.create!(group: invited_group, group_access: Gitlab::Access::DEVELOPER) + end + + it 'includes invited group members' do + expect(execute).to include(invited_group_member) + end + end + + context 'with access_levels parameter' do + context 'when filtering for maintainer access level' do + let_it_be(:maintainer_member) do + create(:project_member, project: project, access_level: Gitlab::Access::MAINTAINER) + end + + let(:params) { { access_levels: [Gitlab::Access::MAINTAINER] } } + + it 'returns maintainer members' do + expect(execute).to include(maintainer_member) + expect(execute).not_to include(project_member) + end + end + + context 'with string access levels' do + let_it_be(:maintainer_member) do + create(:project_member, project: project, access_level: Gitlab::Access::MAINTAINER) + end + + let(:params) { { access_levels: [Gitlab::Access::MAINTAINER] } } + let(:finder) { described_class.new(project, params: params) } + + it 'converts string access levels to integers' do + expect(execute).to include(maintainer_member) + expect(execute).not_to include(project_member) + end + end + + context 'when filtering for owner access level' do + let(:params) { { access_levels: [Gitlab::Access::OWNER] } } + + it 'returns empty when no owner members' do + expect(execute).to be_empty + end + end + end + + context 'with member_role_ids parameter' do + let_it_be(:member_role) { create(:member_role, :guest, namespace: group) } + let_it_be(:custom_role_member) { create(:project_member, project: project, member_role: member_role) } + let(:params) { { member_role_ids: [member_role.id] } } + + it 'filters members by member role IDs' do + expect(execute).to include(custom_role_member) + expect(execute).not_to include(project_member) + end + end + + context 'with both member_role_ids and access_levels' do + let_it_be(:member_role) { create(:member_role, :guest, namespace: group) } + let_it_be(:custom_role_member) { create(:project_member, project: project, member_role: member_role) } + let_it_be(:maintainer_member) do + create(:project_member, project: project, access_level: Gitlab::Access::MAINTAINER) + end + + let(:params) do + { + member_role_ids: [member_role.id], + access_levels: [Gitlab::Access::MAINTAINER] + } + end + + it 'returns members matching either criteria' do + expect(execute).to include(custom_role_member, maintainer_member) + expect(execute).not_to include(project_member) + end + end + + context 'when no members match criteria' do + let(:params) { { access_levels: [Gitlab::Access::OWNER] } } + + it 'returns empty collection' do + expect(execute).to be_empty + end + end + end +end diff --git a/ee/spec/models/ee/member_spec.rb b/ee/spec/models/ee/member_spec.rb index 05082b767858b3..9c34dfe394e0f5 100644 --- a/ee/spec/models/ee/member_spec.rb +++ b/ee/spec/models/ee/member_spec.rb @@ -211,6 +211,72 @@ expect(group.members.order_access_level_desc).to eq([maintainer, developer]) end end + + describe '.by_member_role_or_access_level' do + let_it_be(:test_group) { create(:group) } + let_it_be(:member_role1) { create(:member_role, namespace: test_group) } + let_it_be(:member_role2) { create(:member_role, namespace: test_group) } + let_it_be(:member_with_role1) { create(:group_member, group: test_group, member_role: member_role1) } + let_it_be(:member_with_role2) { create(:group_member, group: test_group, member_role: member_role2) } + let_it_be(:member_with_developer_access) { create(:group_member, :developer, group: test_group) } + let_it_be(:member_with_maintainer_access) { create(:group_member, :maintainer, group: test_group) } + let_it_be(:member_with_guest_access) { create(:group_member, :guest, group: test_group) } + + subject(:by_member_role_or_access_level) do + described_class + .where(source: test_group) + .by_member_role_or_access_level(member_role_ids: member_role_ids, access_levels: access_levels) + end + + context 'when filtering by member role IDs only' do + let(:member_role_ids) { [member_role1.id] } + let(:access_levels) { [] } + + it 'returns members with the specified member role' do + expect(by_member_role_or_access_level).to contain_exactly(member_with_role1) + end + end + + context 'when filtering by access levels only' do + let(:member_role_ids) { [] } + let(:access_levels) { [::Gitlab::Access::DEVELOPER, ::Gitlab::Access::MAINTAINER] } + + it 'returns members with the specified access levels' do + expect(by_member_role_or_access_level).to contain_exactly( + member_with_developer_access, member_with_maintainer_access + ) + end + end + + context 'when filtering by both member role IDs and access levels' do + let(:member_role_ids) { [member_role1.id] } + let(:access_levels) { [::Gitlab::Access::GUEST] } + + it 'returns members with either the specified member role or access level' do + expect(by_member_role_or_access_level).to contain_exactly( + member_with_role1, member_with_guest_access + ) + end + end + + context 'when no matching criteria' do + let(:member_role_ids) { [999] } + let(:access_levels) { [::Gitlab::Access::ADMIN] } + + it 'returns empty result' do + expect(by_member_role_or_access_level).to be_empty + end + end + + context 'when both criteria are empty' do + let(:member_role_ids) { [] } + let(:access_levels) { [] } + + it 'returns empty result' do + expect(by_member_role_or_access_level).to be_empty + end + end + end end describe '.highest_role' do diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index b9a52f611e0217..12846741fa78f7 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -108,59 +108,6 @@ end end - describe '#members_with_access_level_or_custom_roles' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } - - let_it_be(:custom_role) { create(:member_role) } - - let_it_be(:developer) { create(:user) } - let_it_be(:maintainer) { create(:user) } - let_it_be(:reporter) { create(:user) } - - let_it_be(:levels) { [] } - let_it_be(:member_role_ids) { [] } - - before do - create(:project_member, :developer, project: project, user: developer, member_role: custom_role) - create(:project_member, :maintainer, project: project, user: maintainer) - create(:project_member, :reporter, project: project, user: reporter) - end - - subject(:members_with_access_level_or_custom_roles) do - project.team.members_with_access_level_or_custom_roles(levels: levels, member_role_ids: member_role_ids) - end - - context 'when no parameters are provided' do - it { is_expected.to be_empty } - end - - context 'when filtering by access level' do - let_it_be(:levels) { [Gitlab::Access::MAINTAINER] } - - it { is_expected.to contain_exactly(maintainer) } - end - - context 'when filtering by custom roles' do - let_it_be(:member_role_ids) { [custom_role.id] } - - it { is_expected.to contain_exactly(developer) } - end - - context 'when filtering by both access level and custom roles' do - let_it_be(:levels) { [Gitlab::Access::MAINTAINER] } - let_it_be(:member_role_ids) { [custom_role.id] } - - it { is_expected.to contain_exactly(developer, maintainer) } - end - - context 'when filtering with non-existent custom role' do - let_it_be(:member_roles_id) { [non_existing_record_id] } - - it { is_expected.to be_empty } - end - end - describe '#user_exists_with_access_level_or_custom_roles?' do let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } -- GitLab From 369bfbc89f6fdf9e8ce1a4a5460a50e43d046ef5 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Tue, 21 Oct 2025 15:33:01 +0200 Subject: [PATCH 2/3] Fix failing spec --- ee/spec/lib/ee/gitlab/ci/variables/builder/pipeline_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/ee/gitlab/ci/variables/builder/pipeline_spec.rb b/ee/spec/lib/ee/gitlab/ci/variables/builder/pipeline_spec.rb index 1608f121b0ea4b..6fd38a6f2f3aab 100644 --- a/ee/spec/lib/ee/gitlab/ci/variables/builder/pipeline_spec.rb +++ b/ee/spec/lib/ee/gitlab/ci/variables/builder/pipeline_spec.rb @@ -72,7 +72,7 @@ expect do described_class.new(pipeline.reload).predefined_variables - end.to issue_same_number_of_queries_as(control) + end.to match_query_count(control.count + 2) end end end -- GitLab From b16150c7b32bfc3b4506fd6e7d4a1382d3d4fa8a Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Wed, 22 Oct 2025 15:06:24 +0200 Subject: [PATCH 3/3] Handle empty cases --- ee/app/models/concerns/approval_rule_like.rb | 12 ++++++--- .../concerns/approval_rule_like_spec.rb | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 936a0817907b89..23a8f081b09308 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -192,10 +192,14 @@ def with_role_approvers def scan_result_policy_read_role_approvers return User.none unless scan_result_policy_read - members = ::Security::ScanResultPolicies::MembersFinder.new(project, params: { - access_levels: scan_result_policy_read.role_approvers, - member_role_ids: scan_result_policy_read.custom_role_ids_with_permission - }).execute + access_levels = scan_result_policy_read.role_approvers + custom_role_ids = scan_result_policy_read.custom_role_ids_with_permission + + return User.none if access_levels.empty? && custom_role_ids.empty? + + members = ::Security::ScanResultPolicies::MembersFinder.new( + project, params: { access_levels: access_levels, member_role_ids: custom_role_ids } + ).execute User.id_in(members.pluck_user_ids) end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index b606d8c9be52d6..465e365b72c76d 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -248,6 +248,20 @@ def self.scope(*); end expect(rule.approvers).to contain_exactly(user1, user2, group1_user, group2_user, user4) end + + context 'when role_approvers and custom_role_ids are empty' do + let_it_be(:empty_scan_result_policy_read) do + create(:scan_result_policy_read, role_approvers: [], custom_roles: []) + end + + before do + subject.update!(scan_result_policy_read: empty_scan_result_policy_read) + end + + it 'returns User.none for scan_result_policy_read_role_approvers' do + expect(subject.send(:scan_result_policy_read_role_approvers)).to eq(User.none) + end + end end end @@ -399,6 +413,18 @@ def self.scope(*); end end end + describe '#scan_result_policy_read_role_approvers' do + context 'when scan_result_policy_read is nil' do + before do + subject.update!(scan_result_policy_read: nil) + end + + it 'returns User.none' do + expect(subject.send(:scan_result_policy_read_role_approvers)).to eq(User.none) + end + end + end + describe 'validation' do context 'when value is too big' do it 'is invalid' do -- GitLab