From 183f78d09639b5a71d1d90cded42c4b00de31470 Mon Sep 17 00:00:00 2001 From: Jay Swain Date: Fri, 17 Oct 2025 17:18:28 -0700 Subject: [PATCH 1/3] Refactor role assignment methods Moving logic from EE::Member to Authz::Role. Separating the concerns here a little bit and trying to make member only responsible for knowing things like source, and what role a member has, and moving all the logic focused on member role and ability comparison to Authz::Role part of: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209232 --- ee/app/models/ee/member.rb | 56 +++++++++-------------------- ee/lib/ee/authz/role.rb | 49 ++++++++++++++++++++++++++ ee/spec/lib/ee/authz/role_spec.rb | 58 +++++++++++++++++++++++++++++++ lib/authz/role.rb | 2 ++ 4 files changed, 125 insertions(+), 40 deletions(-) create mode 100644 ee/lib/ee/authz/role.rb create mode 100644 ee/spec/lib/ee/authz/role_spec.rb diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index e6010d8087cd36..2806de23f00153 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -225,55 +225,31 @@ def update_user_group_member_roles(old_values_map: nil) end override :prevent_role_assignement? - def prevent_role_assignement?(current_user, params) - return false if current_user.can_admin_all_resources? + def prevent_role_assignement?(assigning_user, params) + member_role_id_to_assign = params[:member_role_id] || member_role_id - member_role_id = params[:member_role_id] || member_role_id - - # first we need to check if there are possibly more custom abilities than current user has - return true if custom_role_abilities_too_high?(current_user, member_role_id) + return true if custom_role_abilities_too_high?(assigning_user:, member_role_id_to_assign:) super end private - def custom_role_abilities_too_high?(current_user, member_role_id) - return false unless member_role_id - - current_user_access_level = source.max_member_access_for_user(current_user) - return false if ::Gitlab::Access::OWNER == current_user_access_level - - current_member_role = ::Member.highest_role(current_user, source)&.member_role - current_member_role_abilities = member_role_abilities( - current_member_role) + custom_abilities_included_with_base_access_level(current_user_access_level) - - new_member_role = MemberRole.find_by_id(member_role_id) - new_member_role_abilities = member_role_abilities(new_member_role) - - (new_member_role_abilities - current_member_role_abilities).present? - end - - def custom_abilities_included_with_base_access_level(current_access_level) - abilities = [] - - customizable_permissions = MemberRole.all_customizable_permissions + def custom_role_abilities_too_high?(assigning_user:, member_role_id_to_assign:) + return false if assigning_user.can_admin_all_resources? + return false unless member_role_id_to_assign - enabled_for_key = :"enabled_for_#{source.class.name.demodulize.downcase}_access_levels" + assigning_user_access_level = source.max_member_access_for_user(assigning_user) + assigning_user_member_role = ::Member.highest_role(assigning_user, source)&.member_role + member_role_to_assign = MemberRole.find_by_id(member_role_id_to_assign) + source_klass_name = source.class.name.demodulize - customizable_permissions.each do |name, definition| - next unless definition.fetch(enabled_for_key, []).include?(current_access_level) - - abilities << name - end - - abilities - end - - def member_role_abilities(member_role) - return [] unless member_role - - member_role.enabled_permissions.keys + Authz::Role.custom_role_abilities_too_high?( + assigning_user_access_level:, + assigning_user_member_role:, + member_role_to_assign:, + source_klass_name: + ) end def group_allowed_email_domains diff --git a/ee/lib/ee/authz/role.rb b/ee/lib/ee/authz/role.rb new file mode 100644 index 00000000000000..71f154cdf8e288 --- /dev/null +++ b/ee/lib/ee/authz/role.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module EE + module Authz + module Role + extend ActiveSupport::Concern + + class_methods do + def custom_role_abilities_too_high?( + assigning_user_access_level:, + assigning_user_member_role:, + member_role_to_assign:, + source_klass_name: + ) + return false if ::Gitlab::Access::OWNER == assigning_user_access_level + + assigning_user_role_abilities = member_role_abilities(assigning_user_member_role) + + custom_abilities_included_with_base_access_level(assigning_user_access_level, source_klass_name) + + member_role_to_assign_abilities = member_role_abilities(member_role_to_assign) + + (member_role_to_assign_abilities - assigning_user_role_abilities).present? + end + + private + + def custom_abilities_included_with_base_access_level(access_level, source_klass_name) + abilities = [] + customizable_permissions = MemberRole.all_customizable_permissions + enabled_for_key = :"enabled_for_#{source_klass_name.downcase}_access_levels" + + customizable_permissions.each do |name, definition| + next unless definition.fetch(enabled_for_key, []).include?(access_level) + + abilities << name + end + + abilities + end + + def member_role_abilities(member_role) + return [] unless member_role + + member_role.enabled_permissions.keys + end + end + end + end +end diff --git a/ee/spec/lib/ee/authz/role_spec.rb b/ee/spec/lib/ee/authz/role_spec.rb new file mode 100644 index 00000000000000..cc371ad8d3466c --- /dev/null +++ b/ee/spec/lib/ee/authz/role_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Authz::Role, feature_category: :permissions do + describe '.custom_role_abilities_too_high?' do + subject(:custom_role_abilities_too_high?) do + described_class.custom_role_abilities_too_high?( + assigning_user_access_level: assigning_user_access_level, + assigning_user_member_role: assigning_user_member_role, + member_role_to_assign: member_role_to_assign, + source_klass_name: source_klass_name + ) + end + + let(:assigning_user_access_level) { 30 } # Developer + let(:source_klass_name) { 'project' } + let(:weak_member_role) { create(:member_role, :developer, :read_runners) } + let(:power_member_role) { create(:member_role, :developer, :read_runners, :manage_security_policy_link) } + + context 'when assigned role and assigning role is equal' do + let(:assigning_user_member_role) { weak_member_role } + let(:member_role_to_assign) { weak_member_role } + + it { is_expected.to be_falsey } + end + + context 'when assigned role has more permissions than assigning role' do + let(:assigning_user_member_role) { power_member_role } + let(:member_role_to_assign) { weak_member_role } + + it { is_expected.to be_falsey } + end + + context 'when assigned role has less permissions than assigning role' do + let(:assigning_user_member_role) { weak_member_role } + let(:member_role_to_assign) { power_member_role } + + it { is_expected.to be_truthy } + end + + context 'when no assigned role and has all permissions of assigning role' do + let(:assigning_user_access_level) { 40 } # Maintainer + let(:assigning_user_member_role) { nil } + let(:member_role_to_assign) { weak_member_role } + + it { is_expected.to be_falsey } + end + + context 'when no assigned role and does not have all permissions of assigning role' do + let(:assigning_user_access_level) { 40 } # Maintainer + let(:assigning_user_member_role) { nil } + let(:member_role_to_assign) { power_member_role } + + it { is_expected.to be_truthy } + end + end +end diff --git a/lib/authz/role.rb b/lib/authz/role.rb index 86d8a55e7580ac..8a6f46d3071af0 100644 --- a/lib/authz/role.rb +++ b/lib/authz/role.rb @@ -15,3 +15,5 @@ def self.roles_user_can_assign(current_access_level, roles = nil) end end end + +Authz::Role.prepend_mod -- GitLab From be3620dea85ac9cc3365864f40b070b8532ca406 Mon Sep 17 00:00:00 2001 From: Jay Swain Date: Tue, 21 Oct 2025 12:36:17 -0700 Subject: [PATCH 2/3] woops, reference global namespace --- ee/app/models/ee/member.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index 2806de23f00153..803787b19a3185 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -244,7 +244,7 @@ def custom_role_abilities_too_high?(assigning_user:, member_role_id_to_assign:) member_role_to_assign = MemberRole.find_by_id(member_role_id_to_assign) source_klass_name = source.class.name.demodulize - Authz::Role.custom_role_abilities_too_high?( + ::Authz::Role.custom_role_abilities_too_high?( assigning_user_access_level:, assigning_user_member_role:, member_role_to_assign:, -- GitLab From 05345a0bd9151814f45c70dabc18c646edf898b1 Mon Sep 17 00:00:00 2001 From: Jay Swain Date: Tue, 21 Oct 2025 12:58:18 -0700 Subject: [PATCH 3/3] use current_user and target for variables names --- ee/app/models/ee/member.rb | 24 +++++++++++------------ ee/lib/ee/authz/role.rb | 16 ++++++++-------- ee/spec/lib/ee/authz/role_spec.rb | 32 +++++++++++++++---------------- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index 803787b19a3185..db379b52396cbf 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -225,29 +225,29 @@ def update_user_group_member_roles(old_values_map: nil) end override :prevent_role_assignement? - def prevent_role_assignement?(assigning_user, params) - member_role_id_to_assign = params[:member_role_id] || member_role_id + def prevent_role_assignement?(current_user, params) + target_member_role_id = params[:member_role_id] || member_role_id - return true if custom_role_abilities_too_high?(assigning_user:, member_role_id_to_assign:) + return true if custom_role_abilities_too_high?(current_user:, target_member_role_id:) super end private - def custom_role_abilities_too_high?(assigning_user:, member_role_id_to_assign:) - return false if assigning_user.can_admin_all_resources? - return false unless member_role_id_to_assign + def custom_role_abilities_too_high?(current_user:, target_member_role_id:) + return false if current_user.can_admin_all_resources? + return false unless target_member_role_id - assigning_user_access_level = source.max_member_access_for_user(assigning_user) - assigning_user_member_role = ::Member.highest_role(assigning_user, source)&.member_role - member_role_to_assign = MemberRole.find_by_id(member_role_id_to_assign) + current_user_access_level = source.max_member_access_for_user(current_user) + current_user_member_role = ::Member.highest_role(current_user, source)&.member_role + target_member_role = MemberRole.find_by_id(target_member_role_id) source_klass_name = source.class.name.demodulize ::Authz::Role.custom_role_abilities_too_high?( - assigning_user_access_level:, - assigning_user_member_role:, - member_role_to_assign:, + current_user_access_level:, + current_user_member_role:, + target_member_role:, source_klass_name: ) end diff --git a/ee/lib/ee/authz/role.rb b/ee/lib/ee/authz/role.rb index 71f154cdf8e288..14c235c105c87c 100644 --- a/ee/lib/ee/authz/role.rb +++ b/ee/lib/ee/authz/role.rb @@ -7,19 +7,19 @@ module Role class_methods do def custom_role_abilities_too_high?( - assigning_user_access_level:, - assigning_user_member_role:, - member_role_to_assign:, + current_user_access_level:, + current_user_member_role:, + target_member_role:, source_klass_name: ) - return false if ::Gitlab::Access::OWNER == assigning_user_access_level + return false if ::Gitlab::Access::OWNER == current_user_access_level - assigning_user_role_abilities = member_role_abilities(assigning_user_member_role) + - custom_abilities_included_with_base_access_level(assigning_user_access_level, source_klass_name) + current_user_role_abilities = member_role_abilities(current_user_member_role) + + custom_abilities_included_with_base_access_level(current_user_access_level, source_klass_name) - member_role_to_assign_abilities = member_role_abilities(member_role_to_assign) + target_member_role_abilities = member_role_abilities(target_member_role) - (member_role_to_assign_abilities - assigning_user_role_abilities).present? + (target_member_role_abilities - current_user_role_abilities).present? end private diff --git a/ee/spec/lib/ee/authz/role_spec.rb b/ee/spec/lib/ee/authz/role_spec.rb index cc371ad8d3466c..880a9e596c93be 100644 --- a/ee/spec/lib/ee/authz/role_spec.rb +++ b/ee/spec/lib/ee/authz/role_spec.rb @@ -6,51 +6,51 @@ describe '.custom_role_abilities_too_high?' do subject(:custom_role_abilities_too_high?) do described_class.custom_role_abilities_too_high?( - assigning_user_access_level: assigning_user_access_level, - assigning_user_member_role: assigning_user_member_role, - member_role_to_assign: member_role_to_assign, + current_user_access_level: current_user_access_level, + current_user_member_role: current_user_member_role, + target_member_role: target_member_role, source_klass_name: source_klass_name ) end - let(:assigning_user_access_level) { 30 } # Developer + let(:current_user_access_level) { 30 } # Developer let(:source_klass_name) { 'project' } let(:weak_member_role) { create(:member_role, :developer, :read_runners) } let(:power_member_role) { create(:member_role, :developer, :read_runners, :manage_security_policy_link) } context 'when assigned role and assigning role is equal' do - let(:assigning_user_member_role) { weak_member_role } - let(:member_role_to_assign) { weak_member_role } + let(:current_user_member_role) { weak_member_role } + let(:target_member_role) { weak_member_role } it { is_expected.to be_falsey } end context 'when assigned role has more permissions than assigning role' do - let(:assigning_user_member_role) { power_member_role } - let(:member_role_to_assign) { weak_member_role } + let(:current_user_member_role) { power_member_role } + let(:target_member_role) { weak_member_role } it { is_expected.to be_falsey } end context 'when assigned role has less permissions than assigning role' do - let(:assigning_user_member_role) { weak_member_role } - let(:member_role_to_assign) { power_member_role } + let(:current_user_member_role) { weak_member_role } + let(:target_member_role) { power_member_role } it { is_expected.to be_truthy } end context 'when no assigned role and has all permissions of assigning role' do - let(:assigning_user_access_level) { 40 } # Maintainer - let(:assigning_user_member_role) { nil } - let(:member_role_to_assign) { weak_member_role } + let(:current_user_access_level) { 40 } # Maintainer + let(:current_user_member_role) { nil } + let(:target_member_role) { weak_member_role } it { is_expected.to be_falsey } end context 'when no assigned role and does not have all permissions of assigning role' do - let(:assigning_user_access_level) { 40 } # Maintainer - let(:assigning_user_member_role) { nil } - let(:member_role_to_assign) { power_member_role } + let(:current_user_access_level) { 40 } # Maintainer + let(:current_user_member_role) { nil } + let(:target_member_role) { power_member_role } it { is_expected.to be_truthy } end -- GitLab