diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 6ad04f64f1abe1d7776dc6ccac20fa7b90b129d2..1365d4dc2e8834ab37d2797d3394951995a140a5 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_or_custom_roles( + levels: scan_result_policy_read.role_approvers, + 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) + end end def filter_inactive_approvers(approvers) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 68ccf0f9e624ca35fdaf81fd708843e539a72c2f..af61084968264e5f08c2797a6feb9c11ed79ea71 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -5,6 +5,25 @@ 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 + override :add_members def add_members( users, diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb index f6aae7e3df027cdbfb50a92f57406785f5835c7f..2401b8d9a5773b84741f4e7b2650cd176b4425de 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' @@ -108,5 +109,15 @@ def real_policy_index real_index end strong_memoize_attr :real_policy_index + + 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).pluck_primary_key + end end end diff --git a/ee/spec/factories/security/scan_result_policy_reads.rb b/ee/spec/factories/security/scan_result_policy_reads.rb index 54e470eae570c45d2ec6aee5aae5617394904090..c7157c79e618163c94a1ce81052bbfb870a3f39a 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 c64f14c931362fc122f6214b9bccee81e91880ea..83079010d458a372cc50349d4fd3ef3b1b3ff605 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -62,6 +62,100 @@ expect(rule.approvers_include_user?(user3)).to be_falsey end end + + 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, :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 + create(:scan_result_policy_read, custom_roles: [custom_role.id]) + end + + before do + project = rule.project + project.update!(group: group) + + 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 '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 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 + + 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 + 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 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 + + 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 + + expect(rule.approvers_include_user?(team_member_with_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 + + 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/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index 4b44c430555e05d4475b1aa870d0a8aa68770f9d..c76c39740ea0366b770932ad75924f7f0737f6f1 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_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 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 509d5efd4b2283d0825bcae1da3346e1773d2dfc..436cf84bedc086ccc55793448b5e36e1344b2630 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_role_ids_with_permission' do + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + + 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) } + 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 + + 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.id) } + end + + 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_read).to receive(:gitlab_com_subscription?).and_return(false) + end + + it { is_expected.to contain_exactly(role_with_permission.id) } + 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) }