Merge branch 'fix-transfer-projects-issue-370834' into 'master'

Fail TransferService only if there are namespaced npm packages

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/95852



Merged-by: David Fernandez <dfernandez@gitlab.com>
Approved-by: David Fernandez <dfernandez@gitlab.com>
Approved-by: Marcel Amirault <mamirault@gitlab.com>
Reviewed-by: David Fernandez <dfernandez@gitlab.com>
Reviewed-by: Marcel Amirault <mamirault@gitlab.com>
Co-authored-by: Kate Grechishkina <khrechyshkina@gitlab.com>

app/models/packages/package.rb

@@ -180,6 +180,7 @@ class Packages::Package < ApplicationRecord
   scope :order_project_name, -> { joins(:project).reorder('projects.name ASC') }
   scope :order_project_name_desc, -> { joins(:project).reorder('projects.name DESC') }
   scope :order_by_package_file, -> { joins(:package_files).order('packages_package_files.created_at ASC') }
+  scope :with_npm_scope, ->(scope) { npm.where("name ILIKE :package_name", package_name: "@#{sanitize_sql_like(scope)}/%") }
   scope :order_project_path, -> do
     keyset_order = keyset_pagination_order(join_class: Project, column_name: :path, direction: :asc)

app/models/project.rb

@@ -1177,10 +1177,6 @@ def has_auto_devops_implicitly_disabled?
     auto_devops_config[:scope] != :project && !auto_devops_config[:status]
   end
-  def has_packages?(package_type)
-    packages.where(package_type: package_type).exists?
-  end
-
   def packages_cleanup_policy
     super || build_packages_cleanup_policy
   end
@@ -2957,6 +2953,12 @@ def package_already_taken?(package_name, package_version, package_type:)
       ).exists?
   end
+  def has_namespaced_npm_packages?
+    packages.with_npm_scope(root_namespace.path)
+            .not_pending_destruction
+            .exists?
+  end
+
   def default_branch_or_main
     return default_branch if default_branch

app/services/groups/transfer_service.rb

@@ -60,7 +60,7 @@ def ensure_allowed_transfer
       raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path?
       raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images?
       raise_transfer_error(:cannot_transfer_to_subgroup) if transfer_to_subgroup?
-      raise_transfer_error(:group_contains_npm_packages) if group_with_npm_packages?
+      raise_transfer_error(:group_contains_namespaced_npm_packages) if group_with_namespaced_npm_packages?
       raise_transfer_error(:no_permissions_to_migrate_crm) if no_permissions_to_migrate_crm?
     end
@@ -74,10 +74,11 @@ def no_permissions_to_migrate_crm?
       false
     end
-    def group_with_npm_packages?
+    def group_with_namespaced_npm_packages?
       return false unless group.packages_feature_enabled?
-      npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
+      npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm, preload_pipelines: false).execute
+      npm_packages = npm_packages.with_npm_scope(group.root_ancestor.path)
       different_root_ancestor? && npm_packages.exists?
     end
@@ -219,7 +220,7 @@ def localized_error_messages
         invalid_policies: s_("TransferGroup|You don't have enough permissions."),
         group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.'),
         cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.'),
-        group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.'),
+        group_contains_namespaced_npm_packages: s_('TransferGroup|Group contains projects with NPM packages scoped to the current root level group.'),
         no_permissions_to_migrate_crm: s_("TransferGroup|Group contains contacts/organizations and you don't have enough permissions to move them to the new root group.")
       }.freeze
     end

app/services/projects/transfer_service.rb

@@ -63,8 +63,8 @@ def transfer(project)
         raise TransferError, s_('TransferProject|Project cannot be transferred, because tags are present in its container registry')
       end
-      if project.has_packages?(:npm) && !new_namespace_has_same_root?(project)
-        raise TransferError, s_("TransferProject|Root namespace can't be updated if project has NPM packages")
+      if !new_namespace_has_same_root?(project) && project.has_namespaced_npm_packages?
+        raise TransferError, s_("TransferProject|Root namespace can't be updated if the project has NPM packages scoped to the current root level namespace.")
       end
       proceed_to_transfer

locale/gitlab.pot

@@ -46609,7 +46609,7 @@ msgstr ""
 msgid "TransferGroup|Group contains contacts/organizations and you don't have enough permissions to move them to the new root group."
 msgstr ""
-msgid "TransferGroup|Group contains projects with NPM packages."
+msgid "TransferGroup|Group contains projects with NPM packages scoped to the current root level group."
 msgstr ""
 msgid "TransferGroup|Group is already a root group."
@@ -46645,7 +46645,7 @@ msgstr ""
 msgid "TransferProject|Project with same name or path in target namespace already exists"
 msgstr ""
-msgid "TransferProject|Root namespace can't be updated if project has NPM packages"
+msgid "TransferProject|Root namespace can't be updated if the project has NPM packages scoped to the current root level namespace."
 msgstr ""
 msgid "TransferProject|You don't have permission to transfer projects into that namespace."

spec/models/project_spec.rb

@@ -882,46 +882,6 @@
     end
   end
-  describe '#has_packages?' do
-    let_it_be(:project) { create(:project, :public) }
-
-    subject { project.has_packages?(package_type) }
-
-    shared_examples 'returning true examples' do
-      let!(:package) { create("#{package_type}_package", project: project) }
-
-      it { is_expected.to be true }
-    end
-
-    shared_examples 'returning false examples' do
-      it { is_expected.to be false }
-    end
-
-    context 'with maven packages' do
-      it_behaves_like 'returning true examples' do
-        let(:package_type) { :maven }
-      end
-    end
-
-    context 'with npm packages' do
-      it_behaves_like 'returning true examples' do
-        let(:package_type) { :npm }
-      end
-    end
-
-    context 'with conan packages' do
-      it_behaves_like 'returning true examples' do
-        let(:package_type) { :conan }
-      end
-    end
-
-    context 'with no package type' do
-      it_behaves_like 'returning false examples' do
-        let(:package_type) { nil }
-      end
-    end
-  end
-
   describe '#ci_pipelines' do
     let_it_be(:project) { create(:project) }
@@ -7660,48 +7620,6 @@ def has_external_wiki
     end
   end
-  describe '#has_packages?' do
-    let(:project) { create(:project, :public) }
-
-    subject { project.has_packages?(package_type) }
-
-    shared_examples 'has_package' do
-      context 'package of package_type exists' do
-        let!(:package) { create("#{package_type}_package", project: project) }
-
-        it { is_expected.to be true }
-      end
-
-      context 'package of package_type does not exist' do
-        it { is_expected.to be false }
-      end
-    end
-
-    context 'with maven packages' do
-      it_behaves_like 'has_package' do
-        let(:package_type) { :maven }
-      end
-    end
-
-    context 'with npm packages' do
-      it_behaves_like 'has_package' do
-        let(:package_type) { :npm }
-      end
-    end
-
-    context 'with conan packages' do
-      it_behaves_like 'has_package' do
-        let(:package_type) { :conan }
-      end
-    end
-
-    context 'calling has_package? with nil' do
-      let(:package_type) { nil }
-
-      it { is_expected.to be false }
-    end
-  end
-
   describe 'with Debian Distributions' do
     subject { create(:project) }
@@ -7822,6 +7740,29 @@ def has_external_wiki
     end
   end
+  describe '#has_namespaced_npm_packages?' do
+    let_it_be(:namespace) { create(:namespace, path: 'test') }
+    let_it_be(:project) { create(:project, :public, namespace: namespace) }
+
+    subject { project.has_namespaced_npm_packages? }
+
+    context 'with scope of the namespace path' do
+      let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
+
+      it { is_expected.to be true }
+    end
+
+    context 'without scope of the namespace path' do
+      let_it_be(:package) { create(:npm_package, project: project, name: "@someotherscope/foo") }
+
+      it { is_expected.to be false }
+    end
+
+    context 'without packages' do
+      it { is_expected.to be false }
+    end
+  end
+
   describe '#package_already_taken?' do
     let_it_be(:namespace) { create(:namespace, path: 'test') }
     let_it_be(:project) { create(:project, :public, namespace: namespace) }

spec/services/groups/transfer_service_spec.rb

@@ -35,10 +35,10 @@
   end
   context 'handling packages' do
-    let_it_be(:group) { create(:group, :public) }
-    let_it_be(:new_group) { create(:group, :public) }
-    let(:project) { create(:project, :public, namespace: group) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:new_group) { create(:group) }
+    let_it_be(:project) { create(:project, namespace: group) }
     before do
       group.add_owner(user)
@@ -46,46 +46,63 @@
     end
     context 'with an npm package' do
-      before do
-        create(:npm_package, project: project)
-      end
-      shared_examples 'transfer not allowed' do
-        it 'does not allow transfer when there is a root namespace change' do
+      let_it_be(:npm_package) { create(:npm_package, project: project, name: "@testscope/test") }
+      shared_examples 'transfer allowed' do
+        it 'allows transfer' do
           transfer_service.execute(new_group)
-          expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.')
-          expect(group.parent).not_to eq(new_group)
+          expect(transfer_service.error).to be nil
+          expect(group.parent).to eq(new_group)
         end
       end
-      it_behaves_like 'transfer not allowed'
+      it_behaves_like 'transfer allowed'
       context 'with a project within subgroup' do
         let_it_be(:root_group) { create(:group) }
         let_it_be(:group) { create(:group, parent: root_group) }
+        let_it_be(:project) { create(:project, namespace: group) }
         before do
           root_group.add_owner(user)
         end
-        it_behaves_like 'transfer not allowed'
+        it_behaves_like 'transfer allowed'
         context 'without a root namespace change' do
-          let(:new_group) { create(:group, parent: root_group) }
-          it 'allows transfer' do
+          let_it_be(:new_group) { create(:group, parent: root_group) }
+
+          it_behaves_like 'transfer allowed'
+        end
+
+        context 'with namespaced packages present' do
+          let_it_be(:package) { create(:npm_package, project: project, name: "@#{project.root_namespace.path}/test") }
+          it 'does not allow transfer' do
             transfer_service.execute(new_group)
-            expect(transfer_service.error).to be nil
-            expect(group.parent).to eq(new_group)
+            expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages scoped to the current root level group.')
+            expect(group.parent).not_to eq(new_group)
+          end
+
+          context 'namespaced package is pending destruction' do
+            let!(:group) { create(:group) }
+
+            before do
+              package.pending_destruction!
+            end
+
+            it_behaves_like 'transfer allowed'
           end
         end
         context 'when transferring a group into a root group' do
-          let(:new_group) { nil }
-          it_behaves_like 'transfer not allowed'
+          let_it_be(:root_group) { create(:group) }
+          let_it_be(:group) { create(:group, parent: root_group) }
+          let_it_be(:new_group) { nil }
+          it_behaves_like 'transfer allowed'
         end
       end
     end

spec/services/projects/transfer_service_spec.rb

@@ -20,12 +20,32 @@
     subject(:transfer_service) { described_class.new(project, user) }
-    let!(:package) { create(:npm_package, project: project) }
+    let!(:package) { create(:npm_package, project: project, name: "@testscope/test") }
     context 'with a root namespace change' do
+      it 'allow the transfer' do
+        expect(transfer_service.execute(group)).to be true
+        expect(project.errors[:new_namespace]).to be_empty
+      end
+    end
+
+    context 'with pending destruction package' do
+      before do
+        package.pending_destruction!
+      end
+
+      it 'allow the transfer' do
+        expect(transfer_service.execute(group)).to be true
+        expect(project.errors[:new_namespace]).to be_empty
+      end
+    end
+
+    context 'with namespaced packages present' do
+      let!(:package) { create(:npm_package, project: project, name: "@#{project.root_namespace.path}/test") }
+
       it 'does not allow the transfer' do
         expect(transfer_service.execute(group)).to be false
-        expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages")
+        expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if the project has NPM packages scoped to the current root level namespace.")
       end
     end
@@ -39,7 +59,7 @@
         other_group.add_owner(user)
       end
-      it 'does allow the transfer' do
+      it 'allow the transfer' do
         expect(transfer_service.execute(other_group)).to be true
         expect(project.errors[:new_namespace]).to be_empty
       end