From 24dc954fa994ccb7ad5c9e75d3cec0da097a3202 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Tue, 31 Dec 2024 12:07:22 +0530 Subject: [PATCH 01/11] Include users from custom roles as MR approvers This change updates the logic of querying the approvers of MR with security policy from the custom roles configured in the policy. EE: true Changelog: changed --- app/models/project_team.rb | 6 ++++++ ee/app/models/concerns/approval_rule_like.rb | 9 ++++++++- ee/app/models/security/scan_result_policy_read.rb | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 2ef643e7e188df..7d50eec24f999f 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -120,6 +120,12 @@ def owners end end + def members_with_access_level_and_custom_roles(level: nil, member_role_ids: []) + members = project.authorized_users + members = members.where(project_authorizations: { access_level: level }) if level + members.joins(:members).where(members: { member_role_id: member_role_ids }) + end + def owner?(user) owners.include?(user) end diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 6ad04f64f1abe1..e15b72773c8574 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -174,7 +174,14 @@ def with_role_approvers def scan_result_policy_read_role_approvers return User.none unless scan_result_policy_read - project.team.members_with_access_levels(scan_result_policy_read.role_approvers) + if Feature.enabled?(:security_policy_custom_roles, project) + project.team.members_with_access_level_and_custom_roles( + level: scan_result_policy_read.role_approvers, + member_role_ids: scan_result_policy_read.custom_roles_with_permission + ) + else + project.team.members_with_access_levels(scan_result_policy_read.role_approvers) + end end def filter_inactive_approvers(approvers) diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb index f6aae7e3df027c..92352b3338c583 100644 --- a/ee/app/models/security/scan_result_policy_read.rb +++ b/ee/app/models/security/scan_result_policy_read.rb @@ -108,5 +108,9 @@ def real_policy_index real_index end strong_memoize_attr :real_policy_index + + def custom_roles_with_permission + MemberRole.id_in(custom_roles).permissions_where(admin_merge_request: true) + end end end -- GitLab From 20ff3d2a72e68f06ef8a27c8883d86a4e9b2d0db Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Tue, 21 Jan 2025 15:04:41 +0100 Subject: [PATCH 02/11] Add missing specs --- .../security/scan_result_policy_reads.rb | 1 + .../concerns/approval_rule_like_spec.rb | 80 +++++++++++++++++++ .../security/scan_result_policy_read_spec.rb | 38 +++++++++ 3 files changed, 119 insertions(+) diff --git a/ee/spec/factories/security/scan_result_policy_reads.rb b/ee/spec/factories/security/scan_result_policy_reads.rb index 54e470eae570c4..c7157c79e61816 100644 --- a/ee/spec/factories/security/scan_result_policy_reads.rb +++ b/ee/spec/factories/security/scan_result_policy_reads.rb @@ -8,6 +8,7 @@ orchestration_policy_idx { 0 } match_on_inclusion_license { true } sequence :rule_idx + custom_roles { [] } trait :prevent_pushing_and_force_pushing do project_approval_settings { { prevent_pushing_and_force_pushing: true } } diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index c64f14c931362f..3cff3a714121d1 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -62,6 +62,86 @@ expect(rule.approvers_include_user?(user3)).to be_falsey end end + + context 'when dealing with team members and custom roles' do + let(:group_project) { create(:project, group: group1) } + let(:custom_role) { create(:member_role, :developer, :instance) } + let(:team_member_with_role) { create(:user) } + let(:team_member_without_role) { create(:user) } + + before do + create(:project_member, :developer, user: team_member_with_role, project: rule.project, + member_role: custom_role) + create(:project_member, :developer, user: team_member_without_role, project: rule.project) + + rule.member_roles << custom_role + end + + context 'for a team member with the custom role' do + context 'when security_policy_custom_roles feature flag is enabled' do + it 'returns true' do + expect(rule.approvers_include_user?(team_member_with_role)).to be_truthy + end + end + + context 'when security_policy_custom_roles feature flag is disabled' do + before do + stub_feature_flags(security_policy_custom_roles: false) + end + + it 'returns false regardless of custom role assignment' do + expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey + end + end + end + + it 'returns false for a team member without the custom role' do + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey + end + + it 'returns false for a user who is not a team member' do + non_team_member = create(:user) + expect(rule.approvers_include_user?(non_team_member)).to be_falsey + end + + context 'when the user relations are already loaded' do + context 'for a team member with the custom role' do + context 'when security_policy_custom_roles feature flag is enabled' do + it 'returns true' do + rule.users.to_a + rule.group_members.to_a + rule.member_roles.to_a + + expect(rule.approvers_include_user?(team_member_with_role)).to be_truthy + end + end + + context 'when security_policy_custom_roles feature flag is disabled' do + before do + stub_feature_flags(security_policy_custom_roles: false) + end + + it 'returns false regardless of custom role assignment' do + rule.users.to_a + rule.group_members.to_a + rule.member_roles.to_a + + expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey + end + end + end + + it 'returns false for a team member without the custom role' do + rule.users.to_a + rule.group_members.to_a + rule.member_roles.to_a + + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey + end + end + end end describe '#approvers' do diff --git a/ee/spec/models/security/scan_result_policy_read_spec.rb b/ee/spec/models/security/scan_result_policy_read_spec.rb index 509d5efd4b2283..6790e2d99bf1ae 100644 --- a/ee/spec/models/security/scan_result_policy_read_spec.rb +++ b/ee/spec/models/security/scan_result_policy_read_spec.rb @@ -302,6 +302,44 @@ end end + describe '#custom_roles_with_permission' do + context 'when custom roles exist' do + let!(:role_with_permission) { create(:member_role, :admin_merge_request) } + let!(:role_without_permission) { create(:member_role) } + + let(:scan_result_policy) do + create(:scan_result_policy_read, custom_roles: [role_with_permission, role_without_permission]) + end + + before do + allow(scan_result_policy).to receive(:custom_roles) + .and_return([role_with_permission.id, role_without_permission.id]) + end + + it 'returns only roles with merge request admin permission' do + expect(scan_result_policy.custom_roles_with_permission) + .to contain_exactly(role_with_permission) + end + + it 'does not return roles without merge request admin permission' do + expect(scan_result_policy.custom_roles_with_permission) + .not_to include(role_without_permission) + end + end + + context 'when no custom roles exist' do + let(:scan_result_policy) { create(:scan_result_policy_read) } + + before do + allow(scan_result_policy).to receive(:custom_roles).and_return([]) + end + + it 'returns an empty collection' do + expect(scan_result_policy.custom_roles_with_permission).to be_empty + end + end + end + describe '.for_project' do let_it_be(:project) { create(:project) } let_it_be(:scan_result_policy_read_1) { create(:scan_result_policy_read, project: project) } -- GitLab From f081679c500fb8a234bc1e0e307bb2ccfa79a4a3 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Sun, 26 Jan 2025 18:33:19 +0100 Subject: [PATCH 03/11] Fix failing spec and refactor query --- app/models/project_team.rb | 6 -- ee/app/models/concerns/approval_rule_like.rb | 2 +- ee/app/models/ee/project_team.rb | 17 ++++++ .../concerns/approval_rule_like_spec.rb | 61 +++++++++++++++---- ee/spec/models/ee/project_team_spec.rb | 53 ++++++++++++++++ 5 files changed, 121 insertions(+), 18 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 7d50eec24f999f..2ef643e7e188df 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -120,12 +120,6 @@ def owners end end - def members_with_access_level_and_custom_roles(level: nil, member_role_ids: []) - members = project.authorized_users - members = members.where(project_authorizations: { access_level: level }) if level - members.joins(:members).where(members: { member_role_id: member_role_ids }) - end - def owner?(user) owners.include?(user) end diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index e15b72773c8574..85191afbd389be 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -176,7 +176,7 @@ def scan_result_policy_read_role_approvers if Feature.enabled?(:security_policy_custom_roles, project) project.team.members_with_access_level_and_custom_roles( - level: scan_result_policy_read.role_approvers, + levels: scan_result_policy_read.role_approvers, member_role_ids: scan_result_policy_read.custom_roles_with_permission ) else diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 68ccf0f9e624ca..614f18b9f5cd32 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -5,6 +5,23 @@ module ProjectTeam extend ActiveSupport::Concern extend ::Gitlab::Utils::Override + def members_with_access_level_and_custom_roles(levels: [], member_role_ids: []) + members = project.authorized_users + + if levels.any? && member_role_ids.any? + members = members + .where(project_authorizations: { access_level: levels }) + .or(members.where(members: { member_role_id: member_role_ids })) + .joins(:members) + elsif levels.any? + members = members.where(project_authorizations: { access_level: levels }) if levels.any? + elsif member_role_ids.any? + members = members.joins(:members).where(members: { member_role_id: member_role_ids }) + end + + members + end + override :add_members def add_members( users, diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 3cff3a714121d1..1e6e76f0ae12c3 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -64,17 +64,23 @@ end context 'when dealing with team members and custom roles' do - let(:group_project) { create(:project, group: group1) } - let(:custom_role) { create(:member_role, :developer, :instance) } - let(:team_member_with_role) { create(:user) } - let(:team_member_without_role) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:group_project) { create(:project, group: group) } + let_it_be(:custom_role) { create(:member_role, permissions: { admin_merge_request: true }) } + let_it_be(:team_member_with_role) { create(:user) } + let_it_be(:team_member_without_role) { create(:user) } + let_it_be(:scan_result_policy_read) do + create(:scan_result_policy_read, custom_roles: [custom_role.id]) + end before do - create(:project_member, :developer, user: team_member_with_role, project: rule.project, - member_role: custom_role) - create(:project_member, :developer, user: team_member_without_role, project: rule.project) + project = rule.project + project.update!(group: group) - rule.member_roles << custom_role + rule.update!(scan_result_policy_read: scan_result_policy_read) + + create(:group_member, member_role: custom_role, user: team_member_with_role, group: group) + create(:group_member, :developer, user: team_member_without_role, group: group) end context 'for a team member with the custom role' do @@ -111,7 +117,6 @@ it 'returns true' do rule.users.to_a rule.group_members.to_a - rule.member_roles.to_a expect(rule.approvers_include_user?(team_member_with_role)).to be_truthy end @@ -125,7 +130,6 @@ it 'returns false regardless of custom role assignment' do rule.users.to_a rule.group_members.to_a - rule.member_roles.to_a expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey @@ -136,7 +140,6 @@ it 'returns false for a team member without the custom role' do rule.users.to_a rule.group_members.to_a - rule.member_roles.to_a expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey end @@ -211,6 +214,42 @@ expect(rule.approvers).to contain_exactly(user1, user2, group1_user, group2_user, user4) end + + # context 'with custom role approvers' do + # let_it_be(:group) { create(:group) } + # let_it_be(:custom_role) { create(:member_role, :developer) } + # let_it_be(:team_member_with_role) { create(:user) } + # let_it_be(:scan_result_policy_read) do + # create(:scan_result_policy_read, role_approvers: [Gitlab::Access::MAINTAINER], + # custom_roles: [custom_role.id]) + # end + + # let(:rule) { subject.class.find(subject.id) } + + # before do + # project = rule.project + # project.update!(group: group) + # create(:group_member, member_role: custom_role, user: team_member_with_role, group: group) + # end + + # it 'contains users as direct members and group members and role members' do + # rule = subject.class.find(subject.id) + + # expect(rule.approvers).to contain_exactly( + # user1, user2, group1_user, group2_user, user4, team_member_with_role + # ) + # end + + # context 'when security_policy_custom_roles feature flag is disabled' do + # before do + # stub_feature_flags(security_policy_custom_roles: false) + # end + + # it 'does not contain custom role users as approvers' do + # expect(rule.approvers).not_to include(team_member_with_role) + # end + # end + # end end end diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index 4b44c430555e05..c8acc316196e22 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -69,4 +69,57 @@ end end end + + describe '#members_with_access_level_and_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_and_custom_roles) do + project.team.members_with_access_level_and_custom_roles(levels: levels, member_role_ids: member_role_ids) + end + + context 'when no parameters are provided' do + it { is_expected.to contain_exactly(developer, maintainer, reporter) } + 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_role_ids) { [non_existing_record_id] } + + it { is_expected.to be_empty } + end + end end -- GitLab From b30ee1e94faeb5774db739410a27f0f7bdebb7b6 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Sun, 26 Jan 2025 18:41:40 +0100 Subject: [PATCH 04/11] Remove unwanted changes --- .../concerns/approval_rule_like_spec.rb | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 1e6e76f0ae12c3..7ba503a00acb56 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -214,42 +214,6 @@ expect(rule.approvers).to contain_exactly(user1, user2, group1_user, group2_user, user4) end - - # context 'with custom role approvers' do - # let_it_be(:group) { create(:group) } - # let_it_be(:custom_role) { create(:member_role, :developer) } - # let_it_be(:team_member_with_role) { create(:user) } - # let_it_be(:scan_result_policy_read) do - # create(:scan_result_policy_read, role_approvers: [Gitlab::Access::MAINTAINER], - # custom_roles: [custom_role.id]) - # end - - # let(:rule) { subject.class.find(subject.id) } - - # before do - # project = rule.project - # project.update!(group: group) - # create(:group_member, member_role: custom_role, user: team_member_with_role, group: group) - # end - - # it 'contains users as direct members and group members and role members' do - # rule = subject.class.find(subject.id) - - # expect(rule.approvers).to contain_exactly( - # user1, user2, group1_user, group2_user, user4, team_member_with_role - # ) - # end - - # context 'when security_policy_custom_roles feature flag is disabled' do - # before do - # stub_feature_flags(security_policy_custom_roles: false) - # end - - # it 'does not contain custom role users as approvers' do - # expect(rule.approvers).not_to include(team_member_with_role) - # end - # end - # end end end -- GitLab From 5de6f53ce169120abe7dbd7ab71c3d49516f445b Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Sun, 26 Jan 2025 22:35:49 +0100 Subject: [PATCH 05/11] Fix failing spec --- ee/spec/models/security/scan_result_policy_read_spec.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/ee/spec/models/security/scan_result_policy_read_spec.rb b/ee/spec/models/security/scan_result_policy_read_spec.rb index 6790e2d99bf1ae..c0be9ad6d08565 100644 --- a/ee/spec/models/security/scan_result_policy_read_spec.rb +++ b/ee/spec/models/security/scan_result_policy_read_spec.rb @@ -308,12 +308,7 @@ let!(:role_without_permission) { create(:member_role) } let(:scan_result_policy) do - create(:scan_result_policy_read, custom_roles: [role_with_permission, role_without_permission]) - end - - before do - allow(scan_result_policy).to receive(:custom_roles) - .and_return([role_with_permission.id, role_without_permission.id]) + create(:scan_result_policy_read, custom_roles: [role_with_permission.id, role_without_permission.id]) end it 'returns only roles with merge request admin permission' do -- GitLab From bdffd3d252ef0af732d5de7b20d2ae5712ef61b1 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Fri, 31 Jan 2025 09:36:08 +0100 Subject: [PATCH 06/11] Refactor and update specs --- .../security/scan_result_policy_read.rb | 9 +++- .../concerns/approval_rule_like_spec.rb | 2 +- .../security/scan_result_policy_read_spec.rb | 41 +++++++++++-------- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb index 92352b3338c583..030fef6e321fb1 100644 --- a/ee/app/models/security/scan_result_policy_read.rb +++ b/ee/app/models/security/scan_result_policy_read.rb @@ -4,6 +4,7 @@ module Security class ScanResultPolicyRead < ApplicationRecord include EachBatch include ::Gitlab::Utils::StrongMemoize + include ::GitlabSubscriptions::SubscriptionHelper self.table_name = 'scan_result_policies' @@ -110,7 +111,13 @@ def real_policy_index strong_memoize_attr :real_policy_index def custom_roles_with_permission - MemberRole.id_in(custom_roles).permissions_where(admin_merge_request: true) + member_roles = if gitlab_com_subscription? + project.root_ancestor.member_roles.id_in(custom_roles) + else + MemberRole.for_instance.id_in(custom_roles) + end + + member_roles.permissions_where(admin_merge_request: true) end end end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 7ba503a00acb56..bbcd48b6707c70 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -66,7 +66,7 @@ context 'when dealing with team members and custom roles' do let_it_be(:group) { create(:group) } let_it_be(:group_project) { create(:project, group: group) } - let_it_be(:custom_role) { create(:member_role, permissions: { admin_merge_request: true }) } + let_it_be(:custom_role) { create(:member_role, :instance, :admin_merge_request) } let_it_be(:team_member_with_role) { create(:user) } let_it_be(:team_member_without_role) { create(:user) } let_it_be(:scan_result_policy_read) do diff --git a/ee/spec/models/security/scan_result_policy_read_spec.rb b/ee/spec/models/security/scan_result_policy_read_spec.rb index c0be9ad6d08565..28c9565dc5dede 100644 --- a/ee/spec/models/security/scan_result_policy_read_spec.rb +++ b/ee/spec/models/security/scan_result_policy_read_spec.rb @@ -303,35 +303,40 @@ end describe '#custom_roles_with_permission' do - context 'when custom roles exist' do - let!(:role_with_permission) { create(:member_role, :admin_merge_request) } - let!(:role_without_permission) { create(:member_role) } + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } - let(:scan_result_policy) do - create(:scan_result_policy_read, custom_roles: [role_with_permission.id, role_without_permission.id]) - end + subject(:custom_roles_with_permission) { scan_result_policy_read.custom_roles_with_permission } - it 'returns only roles with merge request admin permission' do - expect(scan_result_policy.custom_roles_with_permission) - .to contain_exactly(role_with_permission) + context 'when on gitlab.com' do + let_it_be(:role_with_permission) { create(:member_role, :admin_merge_request, namespace: group) } + let_it_be(:role_without_permission) { create(:member_role, namespace: group) } + let_it_be(:scan_result_policy_read) do + create(:scan_result_policy_read, project: project, + custom_roles: [role_with_permission.id, role_without_permission.id]) end - it 'does not return roles without merge request admin permission' do - expect(scan_result_policy.custom_roles_with_permission) - .not_to include(role_without_permission) + before do + allow(scan_result_policy_read).to receive(:gitlab_com_subscription?).and_return(true) + allow(project).to receive(:root_ancestor).and_return(group) end + + it { is_expected.to contain_exactly(role_with_permission) } end - context 'when no custom roles exist' do - let(:scan_result_policy) { create(:scan_result_policy_read) } + context 'when not on gitlab.com' do + let_it_be(:role_with_permission) { create(:member_role, :admin_merge_request, :instance) } + let_it_be(:role_without_permission) { create(:member_role, :instance) } + let_it_be(:scan_result_policy_read) do + create(:scan_result_policy_read, project: project, + custom_roles: [role_with_permission.id, role_without_permission.id]) + end before do - allow(scan_result_policy).to receive(:custom_roles).and_return([]) + allow(scan_result_policy_read).to receive(:gitlab_com_subscription?).and_return(false) end - it 'returns an empty collection' do - expect(scan_result_policy.custom_roles_with_permission).to be_empty - end + it { is_expected.to contain_exactly(role_with_permission) } end end -- GitLab From e2480fada03641eed1827459d78b7f4b3074099f Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Sat, 1 Feb 2025 17:53:36 +0100 Subject: [PATCH 07/11] Fix failing spec --- ee/app/models/ee/project_team.rb | 2 ++ ee/spec/models/ee/project_team_spec.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 614f18b9f5cd32..ce757719e1d869 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -6,6 +6,8 @@ module ProjectTeam extend ::Gitlab::Utils::Override def members_with_access_level_and_custom_roles(levels: [], member_role_ids: []) + return ::User.none unless levels.any? || member_role_ids.any? + members = project.authorized_users if levels.any? && member_role_ids.any? diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index c8acc316196e22..a2140c0e9f726c 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -94,7 +94,7 @@ end context 'when no parameters are provided' do - it { is_expected.to contain_exactly(developer, maintainer, reporter) } + it { is_expected.to be_empty } end context 'when filtering by access level' do -- GitLab From 947ec1e5da2f6d2709a733f6aebff5588109a0c1 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Mon, 3 Feb 2025 18:36:05 +0100 Subject: [PATCH 08/11] Rename function and paramters --- ee/app/models/concerns/approval_rule_like.rb | 4 ++-- ee/app/models/ee/project_team.rb | 20 ++++++++++---------- ee/spec/models/ee/project_team_spec.rb | 14 +++++++------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 85191afbd389be..c7d7ef82cd4252 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -175,9 +175,9 @@ def scan_result_policy_read_role_approvers return User.none unless scan_result_policy_read if Feature.enabled?(:security_policy_custom_roles, project) - project.team.members_with_access_level_and_custom_roles( + project.team.members_with_access_level_or_custom_roles( levels: scan_result_policy_read.role_approvers, - member_role_ids: scan_result_policy_read.custom_roles_with_permission + member_roles: scan_result_policy_read.custom_roles_with_permission ) else project.team.members_with_access_levels(scan_result_policy_read.role_approvers) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index ce757719e1d869..0a9ceb62a8b343 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -5,23 +5,23 @@ module ProjectTeam extend ActiveSupport::Concern extend ::Gitlab::Utils::Override - def members_with_access_level_and_custom_roles(levels: [], member_role_ids: []) - return ::User.none unless levels.any? || member_role_ids.any? + def members_with_access_level_or_custom_roles(levels: [], member_roles: []) + return ::User.none unless levels.any? || member_roles.any? - members = project.authorized_users + users = project.authorized_users - if levels.any? && member_role_ids.any? - members = members + if levels.any? && member_roles.any? + users = users .where(project_authorizations: { access_level: levels }) - .or(members.where(members: { member_role_id: member_role_ids })) + .or(users.where(members: { member_role_id: member_roles })) .joins(:members) elsif levels.any? - members = members.where(project_authorizations: { access_level: levels }) if levels.any? - elsif member_role_ids.any? - members = members.joins(:members).where(members: { member_role_id: member_role_ids }) + users = users.where(project_authorizations: { access_level: levels }) if levels.any? + elsif member_roles.any? + users = users.joins(:members).where(members: { member_role_id: member_roles }) end - members + users end override :add_members diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index a2140c0e9f726c..655de570313647 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -70,7 +70,7 @@ end end - describe '#members_with_access_level_and_custom_roles' do + describe '#members_with_access_level_or_custom_roles' do let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } @@ -81,7 +81,7 @@ let_it_be(:reporter) { create(:user) } let_it_be(:levels) { [] } - let_it_be(:member_role_ids) { [] } + let_it_be(:member_roles) { [] } before do create(:project_member, :developer, project: project, user: developer, member_role: custom_role) @@ -89,8 +89,8 @@ create(:project_member, :reporter, project: project, user: reporter) end - subject(:members_with_access_level_and_custom_roles) do - project.team.members_with_access_level_and_custom_roles(levels: levels, member_role_ids: member_role_ids) + subject(:members_with_access_level_or_custom_roles) do + project.team.members_with_access_level_or_custom_roles(levels: levels, member_roles: member_roles) end context 'when no parameters are provided' do @@ -104,20 +104,20 @@ end context 'when filtering by custom roles' do - let_it_be(:member_role_ids) { [custom_role.id] } + let_it_be(:member_roles) { [custom_role] } 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] } + let_it_be(:member_roles) { [custom_role] } it { is_expected.to contain_exactly(developer, maintainer) } end context 'when filtering with non-existent custom role' do - let_it_be(:member_role_ids) { [non_existing_record_id] } + let_it_be(:member_roles) { [non_existing_record_id] } it { is_expected.to be_empty } end -- GitLab From 8182de8f10eee23896ba1751dd0ea4bb49a41a20 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Tue, 4 Feb 2025 10:35:33 +0100 Subject: [PATCH 09/11] Address review comments --- ee/app/models/ee/project_team.rb | 2 +- .../concerns/approval_rule_like_spec.rb | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 0a9ceb62a8b343..f8a8ca14762fa9 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -16,7 +16,7 @@ def members_with_access_level_or_custom_roles(levels: [], member_roles: []) .or(users.where(members: { member_role_id: member_roles })) .joins(:members) elsif levels.any? - users = users.where(project_authorizations: { access_level: levels }) if levels.any? + users = users.where(project_authorizations: { access_level: levels }) elsif member_roles.any? users = users.joins(:members).where(members: { member_role_id: member_roles }) end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index bbcd48b6707c70..83079010d458a3 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -83,10 +83,22 @@ create(:group_member, :developer, user: team_member_without_role, group: group) end + context 'when scan_result_policy_read is nil' do + before do + rule.update!(scan_result_policy_read: nil) + end + + it 'returns false' do + expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey + end + end + context 'for a team member with the custom role' do context 'when security_policy_custom_roles feature flag is enabled' do - it 'returns true' do + it 'returns true when user belongs to custom role and false otherwise' do expect(rule.approvers_include_user?(team_member_with_role)).to be_truthy + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey end end @@ -97,7 +109,6 @@ it 'returns false regardless of custom role assignment' do expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey - expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey end end end @@ -114,11 +125,12 @@ context 'when the user relations are already loaded' do context 'for a team member with the custom role' do context 'when security_policy_custom_roles feature flag is enabled' do - it 'returns true' do + it 'returns true when user belongs to custom role and false otherwise' do rule.users.to_a rule.group_members.to_a expect(rule.approvers_include_user?(team_member_with_role)).to be_truthy + expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey end end @@ -132,7 +144,6 @@ rule.group_members.to_a expect(rule.approvers_include_user?(team_member_with_role)).to be_falsey - expect(rule.approvers_include_user?(team_member_without_role)).to be_falsey end end end -- GitLab From f18b6aad5c3fbbcafeb7b9d0bad70bbcf86ab4ff Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Wed, 5 Feb 2025 14:53:38 +0100 Subject: [PATCH 10/11] Use member_role_ids directly instead of subquery --- ee/app/models/ee/project_team.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index f8a8ca14762fa9..838f27aee71bee 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -9,16 +9,17 @@ def members_with_access_level_or_custom_roles(levels: [], member_roles: []) return ::User.none unless levels.any? || member_roles.any? users = project.authorized_users + member_role_ids = member_roles.map(&:id) if levels.any? && member_roles.any? users = users .where(project_authorizations: { access_level: levels }) - .or(users.where(members: { member_role_id: member_roles })) + .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_roles.any? - users = users.joins(:members).where(members: { member_role_id: member_roles }) + users = users.joins(:members).where(members: { member_role_id: member_role_ids }) end users -- GitLab From 7eee542845f9f23db27acb9ea07b00f5ad119c8c Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Wed, 5 Feb 2025 15:03:39 +0100 Subject: [PATCH 11/11] Refactor to pass ids instead of AR objects --- ee/app/models/concerns/approval_rule_like.rb | 2 +- ee/app/models/ee/project_team.rb | 9 ++++----- ee/app/models/security/scan_result_policy_read.rb | 4 ++-- ee/spec/models/ee/project_team_spec.rb | 10 +++++----- .../models/security/scan_result_policy_read_spec.rb | 8 ++++---- 5 files changed, 16 insertions(+), 17 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index c7d7ef82cd4252..1365d4dc2e8834 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -177,7 +177,7 @@ def scan_result_policy_read_role_approvers if Feature.enabled?(:security_policy_custom_roles, project) project.team.members_with_access_level_or_custom_roles( levels: scan_result_policy_read.role_approvers, - member_roles: scan_result_policy_read.custom_roles_with_permission + member_role_ids: scan_result_policy_read.custom_role_ids_with_permission ) else project.team.members_with_access_levels(scan_result_policy_read.role_approvers) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 838f27aee71bee..af61084968264e 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -5,20 +5,19 @@ module ProjectTeam extend ActiveSupport::Concern extend ::Gitlab::Utils::Override - def members_with_access_level_or_custom_roles(levels: [], member_roles: []) - return ::User.none unless levels.any? || member_roles.any? + 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 - member_role_ids = member_roles.map(&:id) - if levels.any? && member_roles.any? + 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_roles.any? + elsif member_role_ids.any? users = users.joins(:members).where(members: { member_role_id: member_role_ids }) end diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb index 030fef6e321fb1..2401b8d9a5773b 100644 --- a/ee/app/models/security/scan_result_policy_read.rb +++ b/ee/app/models/security/scan_result_policy_read.rb @@ -110,14 +110,14 @@ def real_policy_index end strong_memoize_attr :real_policy_index - def custom_roles_with_permission + def custom_role_ids_with_permission member_roles = if gitlab_com_subscription? project.root_ancestor.member_roles.id_in(custom_roles) else MemberRole.for_instance.id_in(custom_roles) end - member_roles.permissions_where(admin_merge_request: true) + member_roles.permissions_where(admin_merge_request: true).pluck_primary_key end end end diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index 655de570313647..c76c39740ea036 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -81,7 +81,7 @@ let_it_be(:reporter) { create(:user) } let_it_be(:levels) { [] } - let_it_be(:member_roles) { [] } + let_it_be(:member_role_ids) { [] } before do create(:project_member, :developer, project: project, user: developer, member_role: custom_role) @@ -90,7 +90,7 @@ end subject(:members_with_access_level_or_custom_roles) do - project.team.members_with_access_level_or_custom_roles(levels: levels, member_roles: member_roles) + 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 @@ -104,20 +104,20 @@ end context 'when filtering by custom roles' do - let_it_be(:member_roles) { [custom_role] } + 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_roles) { [custom_role] } + 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) { [non_existing_record_id] } + let_it_be(:member_roles_id) { [non_existing_record_id] } it { is_expected.to be_empty } end diff --git a/ee/spec/models/security/scan_result_policy_read_spec.rb b/ee/spec/models/security/scan_result_policy_read_spec.rb index 28c9565dc5dede..436cf84bedc086 100644 --- a/ee/spec/models/security/scan_result_policy_read_spec.rb +++ b/ee/spec/models/security/scan_result_policy_read_spec.rb @@ -302,11 +302,11 @@ end end - describe '#custom_roles_with_permission' do + describe '#custom_role_ids_with_permission' do let_it_be(:project) { create(:project) } let_it_be(:group) { create(:group) } - subject(:custom_roles_with_permission) { scan_result_policy_read.custom_roles_with_permission } + subject(:custom_role_ids_with_permission) { scan_result_policy_read.custom_role_ids_with_permission } context 'when on gitlab.com' do let_it_be(:role_with_permission) { create(:member_role, :admin_merge_request, namespace: group) } @@ -321,7 +321,7 @@ allow(project).to receive(:root_ancestor).and_return(group) end - it { is_expected.to contain_exactly(role_with_permission) } + it { is_expected.to contain_exactly(role_with_permission.id) } end context 'when not on gitlab.com' do @@ -336,7 +336,7 @@ allow(scan_result_policy_read).to receive(:gitlab_com_subscription?).and_return(false) end - it { is_expected.to contain_exactly(role_with_permission) } + it { is_expected.to contain_exactly(role_with_permission.id) } end end -- GitLab