From 025265ce6915bcc2114bb92aa6ca2f264bfff435 Mon Sep 17 00:00:00 2001
From: Radamanthus Batnag <rbatnag@gitlab.com>
Date: Mon, 17 Feb 2025 13:19:42 +0000
Subject: [PATCH] Package registry: Optimize query for selecting namespace
projects
---
.../packages/npm/packages_for_user_finder.rb | 14 ++++
.../concerns/packages/finder_helper_spec.rb | 69 +++++++++++++------
.../npm/packages_for_user_finder_spec.rb | 50 +++++++++++++-
3 files changed, 108 insertions(+), 25 deletions(-)
diff --git a/app/finders/packages/npm/packages_for_user_finder.rb b/app/finders/packages/npm/packages_for_user_finder.rb
index e9b069340b2e..7979e9e1dd42 100644
--- a/app/finders/packages/npm/packages_for_user_finder.rb
+++ b/app/finders/packages/npm/packages_for_user_finder.rb
@@ -20,6 +20,20 @@ def group_packages
packages_visible_to_user(@current_user, within_group: @project_or_group, with_package_registry_enabled: true)
end
+ # This overrides projects_visible_to_reporters in app/finders/concerns/packages/finder_helper.rb
+ # to implement npm-specific optimizations
+ def projects_visible_to_reporters(user, within_group:, _within_public_package_registry: false)
+ return user.accessible_projects if user.is_a?(DeployToken)
+
+ access = if Feature.enabled?(:allow_guest_plus_roles_to_pull_packages, within_group.root_ancestor)
+ ::Gitlab::Access::GUEST
+ else
+ ::Gitlab::Access::REPORTER
+ end
+
+ ::Project.public_or_visible_to_user(user, access).by_any_overlap_with_traversal_ids(within_group.id)
+ end
+
override :packages_class
def packages_class
::Packages::Npm::Package
diff --git a/spec/finders/concerns/packages/finder_helper_spec.rb b/spec/finders/concerns/packages/finder_helper_spec.rb
index e0729cff5ac8..b8696cbe500e 100644
--- a/spec/finders/concerns/packages/finder_helper_spec.rb
+++ b/spec/finders/concerns/packages/finder_helper_spec.rb
@@ -323,6 +323,20 @@ def packages_class
it { is_expected.to be_empty }
end
+ shared_examples 'handles a group deploy token' do
+ let_it_be(:user) { create(:deploy_token, :group, read_package_registry: true) }
+ let_it_be(:group_deploy_token) { create(:group_deploy_token, deploy_token: user, group: group) }
+
+ before do
+ project2.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+ subgroup.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+ project1.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+ group.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+ end
+
+ it_behaves_like 'returning both projects'
+ end
+
describe '#projects_visible_to_user' do
subject { finder.projects_visible_to_user(user, within_group: group) }
@@ -365,28 +379,7 @@ def packages_class
end
end
- context 'with a group deploy token' do
- let_it_be(:user) { create(:deploy_token, :group, read_package_registry: true) }
- let_it_be(:group_deploy_token) { create(:group_deploy_token, deploy_token: user, group: group) }
-
- where(:group_visibility, :subgroup_visibility, :project2_visibility, :shared_example_name) do
- 'PUBLIC' | 'PUBLIC' | 'PUBLIC' | 'returning both projects'
- 'PUBLIC' | 'PUBLIC' | 'PRIVATE' | 'returning both projects'
- 'PUBLIC' | 'PRIVATE' | 'PRIVATE' | 'returning both projects'
- 'PRIVATE' | 'PRIVATE' | 'PRIVATE' | 'returning both projects'
- end
-
- with_them do
- before do
- project2.update!(visibility_level: Gitlab::VisibilityLevel.const_get(project2_visibility, false))
- subgroup.update!(visibility_level: Gitlab::VisibilityLevel.const_get(subgroup_visibility, false))
- project1.update!(visibility_level: Gitlab::VisibilityLevel.const_get(group_visibility, false))
- group.update!(visibility_level: Gitlab::VisibilityLevel.const_get(group_visibility, false))
- end
-
- it_behaves_like params[:shared_example_name]
- end
- end
+ it_behaves_like 'handles a group deploy token'
end
describe '#projects_visible_to_user_including_public_registries' do
@@ -415,5 +408,37 @@ def packages_class
it_behaves_like params[:shared_example_name]
end
end
+
+ describe '#projects_visible_to_reporters' do
+ before_all do
+ project1.add_reporter(user)
+ end
+
+ let(:within_public_package_registry) { false }
+
+ subject do
+ finder.projects_visible_to_reporters(user,
+ within_group: group,
+ within_public_package_registry: within_public_package_registry)
+ end
+
+ it_behaves_like 'handles a group deploy token'
+
+ it_behaves_like 'returning project1'
+
+ context 'when within_public_package_registry set to true' do
+ let_it_be(:project_with_public_package_registry) { create(:project, group: group) }
+
+ let(:within_public_package_registry) { true }
+
+ before do
+ project_with_public_package_registry
+ .project_feature
+ .update_attribute(:package_registry_access_level, ::ProjectFeature::PUBLIC)
+ end
+
+ it { is_expected.to match_array [project1, project_with_public_package_registry] }
+ end
+ end
end
end
diff --git a/spec/finders/packages/npm/packages_for_user_finder_spec.rb b/spec/finders/packages/npm/packages_for_user_finder_spec.rb
index 473c3463e7dd..81c9b1fb1d05 100644
--- a/spec/finders/packages/npm/packages_for_user_finder_spec.rb
+++ b/spec/finders/packages/npm/packages_for_user_finder_spec.rb
@@ -33,15 +33,32 @@
let(:project_or_group) { group }
before_all do
- project.add_reporter(user)
+ project.add_guest(user)
end
it_behaves_like 'searches for packages'
it_behaves_like 'avoids N+1 database queries in the package registry'
- context 'when an user is a reporter of both projects' do
+ context 'with a project inaccessible to user, but with a public registry' do
+ let_it_be(:project_with_public_package_registry) { create(:project, group: group) }
+ let_it_be(:other_package) do
+ create(:npm_package, project: project_with_public_package_registry, name: package.name)
+ end
+
+ before do
+ project_with_public_package_registry
+ .project_feature
+ .update_attribute(:package_registry_access_level, ::ProjectFeature::PUBLIC)
+ end
+
+ it 'excludes the packages from the public registry' do
+ is_expected.to contain_exactly(package)
+ end
+ end
+
+ context 'when an user is a guest of both projects' do
before_all do
- project2.add_reporter(user)
+ project2.add_guest(user)
end
it { is_expected.to contain_exactly(package, package_with_diff_project) }
@@ -56,6 +73,33 @@
it_behaves_like 'searches for packages'
end
end
+
+ context 'when allow_guest_plus_roles_to_pull_packages is disabled' do
+ before_all do
+ stub_feature_flags(allow_guest_plus_roles_to_pull_packages: false)
+ project.add_reporter(user)
+ end
+
+ it_behaves_like 'searches for packages'
+
+ context 'when an user is a reporter of both projects' do
+ before_all do
+ project2.add_reporter(user)
+ end
+
+ it { is_expected.to contain_exactly(package, package_with_diff_project) }
+
+ context 'when the second project has the package registry disabled' do
+ before_all do
+ project.reload.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project2.reload.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC,
+ package_registry_access_level: 'disabled', packages_enabled: false)
+ end
+
+ it_behaves_like 'searches for packages'
+ end
+ end
+ end
end
end
end
--
GitLab