From 5114c3526a8d8a1f69cc663e3f5ea43234990691 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Fri, 1 Aug 2025 10:41:16 +0800 Subject: [PATCH 1/4] Return all groups if user can read_admin_groups --- app/finders/groups_finder.rb | 7 ++++++- ee/app/finders/ee/groups_finder.rb | 5 +++++ ee/spec/finders/groups_finder_spec.rb | 22 +++++++++++++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb index 406e297c4219d4..12f6da41b03cf2 100644 --- a/app/finders/groups_finder.rb +++ b/app/finders/groups_finder.rb @@ -61,7 +61,7 @@ def filtered_groups def all_groups return [owned_groups] if params[:owned] return [groups_with_min_access_level] if min_access_level? - return [Group.all] if current_user&.can_read_all_resources? && all_available? + return [Group.all] if can_read_all_groups? && all_available? groups = [ authorized_groups, @@ -192,6 +192,11 @@ def include_public_groups? current_user.nil? || all_available? end + # Overridden in EE + def can_read_all_groups? + current_user&.can_read_all_resources? + end + def all_available? params.fetch(:all_available, true) end diff --git a/ee/app/finders/ee/groups_finder.rb b/ee/app/finders/ee/groups_finder.rb index b1db44718b6980..56ce3f53a9c439 100644 --- a/ee/app/finders/ee/groups_finder.rb +++ b/ee/app/finders/ee/groups_finder.rb @@ -23,5 +23,10 @@ def by_repository_storage(groups) groups.by_repository_storage(params[:repository_storage]) end + + override :can_read_all_groups? + def can_read_all_groups? + super || current_user&.can?(:read_admin_groups) + end end end diff --git a/ee/spec/finders/groups_finder_spec.rb b/ee/spec/finders/groups_finder_spec.rb index 004b887f80c8f4..ebeacef4cae488 100644 --- a/ee/spec/finders/groups_finder_spec.rb +++ b/ee/spec/finders/groups_finder_spec.rb @@ -10,7 +10,7 @@ let_it_be(:group_with_wiki) { create(:group, :wiki_repo) } let_it_be(:group_without_wiki) { create(:group) } - subject { described_class.new(user, params).execute } + subject(:execute) { described_class.new(user, params).execute } before_all do group_with_wiki.add_developer(user) @@ -36,5 +36,25 @@ it_behaves_like 'groups finder with SAML session filtering' do let(:finder) { described_class.new(current_user, params) } end + + context 'when user has custom admin role with read_admin_groups permission' do + let_it_be(:role) { create(:admin_member_role, :read_admin_groups, user: user) } + + let_it_be(:authorized_groups) { [group_with_wiki, group_without_wiki] } + let_it_be(:unauthorized_group) { create(:group, :private) } # group the user is not a member of + let_it_be(:internal_group) { create(:group, :internal) } + let_it_be(:public_group) { create(:group, :public) } + + let(:params) { {} } + + before do + stub_licensed_features(custom_roles: true) + enable_admin_mode!(user) + end + + it 'returns all groups' do + expect(execute).to match_array([*authorized_groups, unauthorized_group, internal_group, public_group]) + end + end end end -- GitLab From 56d340a7718d167805a1dfd3db555cb59a15d2a6 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Fri, 1 Aug 2025 10:42:29 +0800 Subject: [PATCH 2/4] Return all projects if user can read_admin_projects --- app/models/project.rb | 7 ++++++- ee/app/models/ee/project.rb | 7 +++++++ ee/spec/models/ee/project_spec.rb | 24 ++++++++++++++++++++++++ lib/gitlab/visibility_level.rb | 3 ++- 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 0de83b4466b1cc..ca989eb321c04e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1035,11 +1035,16 @@ def self.public_or_visible_to_user(user = nil, min_access_level = nil) where( 'EXISTS (?) OR projects.visibility_level IN (?)', user.authorizations_for_projects(min_access_level: min_access_level), - Gitlab::VisibilityLevel.levels_for_user(user) + self.visibility_levels_for_user(user) ) end end + # Overridden in EE + def self.visibility_levels_for_user(user) + Gitlab::VisibilityLevel.levels_for_user(user) + end + # Define two instance methods: # # - [attribute]?(inherit_group_setting) Returns the final value after inheriting the parent group diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a0562726352759..c7bef30443d2bd 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -744,6 +744,13 @@ def project_features_defaults def verification_state_model_key :project_id end + + override :visibility_levels_for_user + def visibility_levels_for_user(user) + return ::Gitlab::VisibilityLevel::LEVELS_FOR_ADMINS if user.can?(:read_admin_projects) + + super + end end def project_state diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index c830d5eb2636fb..7b4dcb95898c11 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Project, feature_category: :groups_and_projects do + include AdminModeHelper include ProjectForksHelper include ::EE::GeoHelpers include ::ProjectHelpers @@ -5512,4 +5513,27 @@ def stub_default_url_options(host) expect { update_name }.to change { inventory_filter.reload.project_name }.from(old_name).to(new_name) end end + + describe '.public_or_visible_to_user' do + let_it_be(:user) { create(:user) } + let_it_be(:role) { create(:admin_member_role, :read_admin_projects, user: user) } + + let_it_be(:private_authorized_project) { create(:project, :private, creator: user) } + let_it_be(:private_unauthorized_project) { create(:project, :private) } # project the user is not a member of + let_it_be(:internal_project) { create(:project, :internal) } + let_it_be(:public_project) { create(:project, :public) } + + subject(:projects) { described_class.all.public_or_visible_to_user(user) } + + before do + stub_licensed_features(custom_roles: true) + enable_admin_mode!(user) + end + + it 'returns all projects' do + expect(projects).to match_array( + [private_authorized_project, private_unauthorized_project, internal_project, public_project] + ) + end + end end diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 9ae19ce0271822..254f576f18364f 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -25,6 +25,7 @@ module VisibilityLevel PRIVATE = 0 unless const_defined?(:PRIVATE) INTERNAL = 10 unless const_defined?(:INTERNAL) PUBLIC = 20 unless const_defined?(:PUBLIC) + LEVELS_FOR_ADMINS = [PRIVATE, INTERNAL, PUBLIC].freeze class << self delegate :values, to: :options @@ -33,7 +34,7 @@ def levels_for_user(user = nil) return [PUBLIC] unless user if user.can_read_all_resources? - [PRIVATE, INTERNAL, PUBLIC] + LEVELS_FOR_ADMINS elsif user.external? [PUBLIC] else -- GitLab From 806425fb6ed53d66906cd640f9a20131f6c9d37f Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Mon, 4 Aug 2025 09:34:27 +0800 Subject: [PATCH 3/4] Return super if user is nil --- ee/app/models/ee/project.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index c7bef30443d2bd..ab5ef7a659c237 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -747,7 +747,7 @@ def verification_state_model_key override :visibility_levels_for_user def visibility_levels_for_user(user) - return ::Gitlab::VisibilityLevel::LEVELS_FOR_ADMINS if user.can?(:read_admin_projects) + return ::Gitlab::VisibilityLevel::LEVELS_FOR_ADMINS if user&.can?(:read_admin_projects) super end -- GitLab From 98bd1d8a2cfeec09600d05c88abc9f0b182a6c65 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Thu, 7 Aug 2025 16:28:11 +0800 Subject: [PATCH 4/4] Check for read_all_resources and read_admin_groups|projects in CE --- app/finders/groups_finder.rb | 8 ++++++-- app/models/project.rb | 8 +++++++- ee/app/finders/ee/groups_finder.rb | 5 ----- ee/app/models/ee/project.rb | 7 ------- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb index 12f6da41b03cf2..84142875a26b65 100644 --- a/app/finders/groups_finder.rb +++ b/app/finders/groups_finder.rb @@ -192,9 +192,13 @@ def include_public_groups? current_user.nil? || all_available? end - # Overridden in EE def can_read_all_groups? - current_user&.can_read_all_resources? + return false unless current_user + + # Auditors can :read_all_resources while admins can :read_all_resources and + # read_admin_groups. In EE, a regular user can read_admin_groups through + # custom admin roles. + current_user.can_read_all_resources? || current_user.can?(:read_admin_groups) end def all_available? diff --git a/app/models/project.rb b/app/models/project.rb index ca989eb321c04e..60ece7a17bae6e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1040,8 +1040,14 @@ def self.public_or_visible_to_user(user = nil, min_access_level = nil) end end - # Overridden in EE + # Auditors can :read_all_resources while admins can :read_all_resources and + # read_admin_projects. In EE, a regular user can read_admin_projects through + # custom admin roles. def self.visibility_levels_for_user(user) + can_read_all_projects = user&.can_read_all_resources? || user&.can?(:read_admin_projects) + + return ::Gitlab::VisibilityLevel::LEVELS_FOR_ADMINS if can_read_all_projects + Gitlab::VisibilityLevel.levels_for_user(user) end diff --git a/ee/app/finders/ee/groups_finder.rb b/ee/app/finders/ee/groups_finder.rb index 56ce3f53a9c439..b1db44718b6980 100644 --- a/ee/app/finders/ee/groups_finder.rb +++ b/ee/app/finders/ee/groups_finder.rb @@ -23,10 +23,5 @@ def by_repository_storage(groups) groups.by_repository_storage(params[:repository_storage]) end - - override :can_read_all_groups? - def can_read_all_groups? - super || current_user&.can?(:read_admin_groups) - end end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index ab5ef7a659c237..a0562726352759 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -744,13 +744,6 @@ def project_features_defaults def verification_state_model_key :project_id end - - override :visibility_levels_for_user - def visibility_levels_for_user(user) - return ::Gitlab::VisibilityLevel::LEVELS_FOR_ADMINS if user&.can?(:read_admin_projects) - - super - end end def project_state -- GitLab