diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index e6010d8087cd3601e41b1c9c80c3862fdfdf8aeb..db379b52396cbf719e3a4e79c52205541c8c6794 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -226,54 +226,30 @@ def update_user_group_member_roles(old_values_map: nil) override :prevent_role_assignement? def prevent_role_assignement?(current_user, params) - return false if current_user.can_admin_all_resources? - - member_role_id = params[:member_role_id] || member_role_id + target_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?(current_user:, target_member_role_id:) super end private - def custom_role_abilities_too_high?(current_user, member_role_id) - return false unless member_role_id + 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 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 - - enabled_for_key = :"enabled_for_#{source.class.name.demodulize.downcase}_access_levels" - - 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 + 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?( + current_user_access_level:, + current_user_member_role:, + target_member_role:, + 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 0000000000000000000000000000000000000000..14c235c105c87c86f00484dcd80c18892e09e970 --- /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?( + current_user_access_level:, + current_user_member_role:, + target_member_role:, + source_klass_name: + ) + return false if ::Gitlab::Access::OWNER == current_user_access_level + + 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) + + target_member_role_abilities = member_role_abilities(target_member_role) + + (target_member_role_abilities - current_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 0000000000000000000000000000000000000000..880a9e596c93be8fbe0663ef4da952c73fad6584 --- /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?( + 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(: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(: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(: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(: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(: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(: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 + end +end diff --git a/lib/authz/role.rb b/lib/authz/role.rb index 86d8a55e7580acd0997c68674fcead2a4771aa24..8a6f46d3071af03705223dba55d91d83c72cb516 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