Merge branch...

Merge branch '12417-add-messages-to-warn-and-stop-users-when-attempting-to-change-the-path-of-projects-with-npm-packages' into 'master'

Resolve "Add messages to warn and stop users when attempting to change the path of projects with NPM packages"

See merge request gitlab-org/gitlab!18515

app/services/groups/transfer_service.rb

@@ -13,7 +13,7 @@ module Groups
 
     TransferError = Class.new(StandardError)
 
-    attr_reader :error
+    attr_reader :error, :new_parent_group
 
     def initialize(group, user, params = {})
       super
@@ -115,3 +115,5 @@ module Groups
     end
   end
 end
+
+Groups::TransferService.prepend_if_ee('EE::Groups::TransferService')

app/services/projects/transfer_service.rb

@@ -13,6 +13,8 @@ module Projects
     include Gitlab::ShellAdapter
     TransferError = Class.new(StandardError)
 
+    attr_reader :new_namespace
+
     def execute(new_namespace)
       @new_namespace = new_namespace
 

doc/user/packages/npm_registry/index.md

@@ -42,6 +42,9 @@ it is not possible due to a naming collision. For example:
 | `gitlab-org/gitlab`    | `@gitlab-org/gitlab`    | Yes       |
 | `gitlab-org/gitlab`    | `@foo/bar`              | No        |
 
+CAUTION: **When updating the path of a user/group or transferring a (sub)group/project:**
+If you update the root namespace of a project with NPM packages, your changes will be rejected. To be allowed to do that, make sure to remove any NPM package first. Don't forget to update your `.npmrc` files to follow the above naming convention and run `npm publish` if necessary.
+
 Now, you can configure your project to authenticate with the GitLab NPM
 Registry.
 

ee/app/finders/packages/group_packages_finder.rb

@@ -4,9 +4,10 @@ module Packages
   class GroupPackagesFinder
     attr_reader :current_user, :group
 
-    def initialize(current_user, group)
+    def initialize(current_user, group, params = {})
       @current_user = current_user
       @group = group
+      @params = params
     end
 
     def execute
@@ -18,7 +19,11 @@ module Packages
     private
 
     def packages_for_group_projects
-      ::Packages::Package.for_projects(group_projects_visible_to_current_user)
+      packages = ::Packages::Package.for_projects(group_projects_visible_to_current_user)
+
+      return packages unless package_type
+
+      packages.with_package_type(package_type)
     end
 
     def group_projects_visible_to_current_user
@@ -26,5 +31,9 @@ module Packages
         .in_namespace(group.self_and_descendants.select(:id))
         .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER)
     end
+
+    def package_type
+      @params[:package_type].presence
+    end
   end
 end

ee/app/models/ee/project.rb

@@ -668,6 +668,12 @@ module EE
       marked_for_deletion_at.present?
     end
 
+    def has_packages?(package_type)
+      return false unless feature_available?(:packages)
+
+      packages.where(package_type: package_type).exists?
+    end
+
     private
 
     def set_override_pull_mirror_available

ee/app/models/packages/package.rb

@@ -23,6 +23,7 @@ class Packages::Package < ApplicationRecord
   scope :with_name, ->(name) { where(name: name) }
   scope :with_name_like, ->(name) { where(arel_table[:name].matches(name)) }
   scope :with_version, ->(version) { where(version: version) }
+  scope :with_package_type, ->(package_type) { where(package_type: package_type) }
   scope :has_version, -> { where.not(version: nil) }
   scope :preload_files, -> { preload(:package_files) }
   scope :last_of_each_version, -> { where(id: all.select('MAX(id) AS id').group(:version)) }

ee/app/services/ee/groups/transfer_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Groups
+    module TransferService
+      extend ::Gitlab::Utils::Override
+
+      EE_ERROR_MESSAGES = {
+        group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
+      }.freeze
+
+      private
+
+      override :ensure_allowed_transfer
+      def ensure_allowed_transfer
+        super
+        return unless group.packages_feature_available?
+
+        npm_packages = Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
+        if !new_parent_group_has_same_root_ancestor? && npm_packages.exists?
+          raise_ee_transfer_error(:group_contains_npm_packages)
+        end
+      end
+
+      def raise_ee_transfer_error(message)
+        raise ::Groups::TransferService::TransferError, EE_ERROR_MESSAGES[message]
+      end
+
+      def new_parent_group_has_same_root_ancestor?
+        group.root_ancestor == new_parent_group.root_ancestor
+      end
+    end
+  end
+end

ee/app/services/ee/groups/update_service.rb

@@ -12,6 +12,8 @@ module EE
           return false if group.errors.present?
         end
 
+        return false unless valid_path_change_with_npm_packages?
+
         handle_changes
 
         remove_insight_if_insight_project_absent
@@ -108,6 +110,19 @@ module EE
         end
       end
 
+      def valid_path_change_with_npm_packages?
+        return true unless group.packages_feature_available?
+        return true if !group.has_parent? && group.path == params[:path]
+
+        npm_packages = Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
+        if npm_packages.exists?
+          group.errors.add(:path, s_('GroupSettings|cannot change when group contains projects with NPM packages'))
+          return
+        end
+
+        true
+      end
+
       def allowed_domain_params
         @allowed_domain_params ||= params[:allowed_email_domain_attributes]
       end

ee/app/services/ee/projects/transfer_service.rb

@@ -19,6 +19,19 @@ module EE
           old_path_with_namespace: @old_path # rubocop:disable Gitlab/ModuleWithInstanceVariables
         ).create!
       end
+
+      override :transfer
+      def transfer(project)
+        if project.feature_available?(:packages) && project.has_packages?(:npm) && !new_namespace_has_same_root?(project)
+          raise ::Projects::TransferService::TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages"))
+        end
+
+        super
+      end
+
+      def new_namespace_has_same_root?(project)
+        new_namespace.root_ancestor == project.namespace.root_ancestor
+      end
     end
   end
 end

ee/changelogs/unreleased/12417-add-messages-to-warn-and-stop-users-when-attempting-to-change-th.yml

+---
+title: Add messages to warn and stop users when attempting to change the path of projects with NPM packages
+merge_request: 18515
+author:
+type: fixed

ee/spec/finders/packages/group_packages_finder_spec.rb

@@ -2,41 +2,59 @@
 require 'spec_helper'
 
 describe Packages::GroupPackagesFinder do
-  set(:user)     { create(:user) }
-  set(:group)    { create(:group) }
-  set(:project)  { create(:project, namespace: group) }
-  set(:package1) { create(:maven_package, project: project) }
-  set(:package2) { create(:maven_package, project: project) }
-  set(:package3) { create(:maven_package) }
-  set(:another_group) { create(:group) }
+  let_it_be(:user)     { create(:user) }
+  let_it_be(:group)    { create(:group) }
+  let_it_be(:project)  { create(:project, namespace: group) }
 
   before do
     group.add_developer(user)
   end
 
   describe '#execute' do
+    subject { described_class.new(user, group).execute }
+
+    shared_examples 'with package type' do |package_type|
+      subject { described_class.new(user, group, package_type: package_type).execute }
+
+      it { is_expected.to match_array([send("package_#{package_type}")]) }
+    end
+
+    def self.package_types
+      @package_types ||= Packages::Package.package_types.keys
+    end
+
     context 'group has packages' do
-      it 'returns group packages' do
-        finder = described_class.new(user, group)
+      let(:package1) { create(:maven_package, project: project) }
+      let(:package2) { create(:maven_package, project: project) }
+      let_it_be(:package3) { create(:maven_package) }
 
-        expect(finder.execute).to match_array([package1, package2])
-      end
+      it { is_expected.to match_array([package1, package2]) }
     end
 
-    context 'group has no packages' do
-      it 'returns an empty collection' do
-        finder = described_class.new(user, another_group)
+    context 'group has package of all types' do
+      package_types.each { |pt| let!("package_#{pt}") { create("#{pt}_package", project: project) } }
 
-        expect(finder.execute).to be_empty
+      package_types.each do |package_type|
+        it_behaves_like 'with package type', package_type
       end
     end
 
+    context 'group has no packages' do
+      it { is_expected.to be_empty }
+    end
+
     context 'group is nil' do
-      it 'returns an empty collection' do
-        finder = described_class.new(user, nil)
+      subject { described_class.new(user, nil).execute }
 
-        expect(finder.execute).to be_empty
-      end
+      it { is_expected.to be_empty}
+    end
+
+    context 'package type is nil' do
+      let_it_be(:package1) { create(:maven_package, project: project) }
+
+      subject { described_class.new(user, group, package_type: nil).execute }
+
+      it { is_expected.to match_array([package1])}
     end
   end
 end

ee/spec/models/project_spec.rb

@@ -2263,4 +2263,61 @@ describe Project do
       end
     end
   end
+
+  describe '#has_packages?' do
+    let(: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 packages disabled' do
+      before do
+        stub_licensed_features(packages: false)
+      end
+
+      it_behaves_like 'returning false examples' do
+        let!(:package) { create(:maven_package, project: project) }
+        let(:package_type) { :maven }
+      end
+    end
+
+    context 'with packages enabled' do
+      before do
+        stub_licensed_features(packages: true)
+      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
+  end
 end

ee/spec/services/groups/transfer_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::TransferService, '#execute' do
+  let(:user) { create(:user) }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :public, namespace: group) }
+  let(:new_group) { create(:group, :public) }
+  let(:transfer_service) { described_class.new(group, user) }
+
+  before do
+    stub_licensed_features(packages: true)
+    group.add_owner(user)
+    new_group.add_owner(user)
+  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
+        transfer_service.execute(new_group)
+
+        expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.')
+      end
+    end
+
+    it_behaves_like 'transfer not allowed'
+
+    context 'with a project within subgroup' do
+      let(:root_group) { create(:group) }
+      let(:group) { create(:group, parent: root_group) }
+
+      before do
+        root_group.add_owner(user)
+      end
+
+      it_behaves_like 'transfer not allowed'
+
+      context 'without a root namespace change' do
+        let(:new_group) { create(:group, parent: root_group) }
+
+        it 'allows transfer' do
+          transfer_service.execute(new_group)
+
+          expect(transfer_service.error).not_to be
+          expect(group.parent).to eq(new_group)
+        end
+      end
+    end
+  end
+end

ee/spec/services/groups/update_service_spec.rb

@@ -132,6 +132,44 @@ describe Groups::UpdateService, '#execute' do
     end
   end
 
+  context 'with project' do
+    let(:project) { create(:project, namespace: group) }
+
+    shared_examples 'transfer not allowed' do
+      before do
+        stub_licensed_features(packages: true)
+        group.add_owner(user)
+      end
+
+      context 'with npm packages' do
+        let!(:package) { create(:npm_package, project: project) }
+
+        it 'does not allow a path update' do
+          expect(update_group(group, user, path: 'updated')).to be false
+          expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages')
+        end
+      end
+    end
+
+    it_behaves_like 'transfer not allowed'
+
+    context 'located in a subgroup' do
+      let(:subgroup) { create(:group, parent: group) }
+      let!(:project) { create(:project, namespace: subgroup) }
+
+      before do
+        subgroup.add_owner(user)
+      end
+
+      it_behaves_like 'transfer not allowed'
+
+      it 'does allow a path update if there is not a root namespace change' do
+        expect(update_group(subgroup, user, path: 'updated')).to be true
+        expect(subgroup.errors[:path]).to be_empty
+      end
+    end
+  end
+
   context 'repository_size_limit assignment as Bytes' do
     let(:group) { create(:group, :public, repository_size_limit: 0) }
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -51,4 +51,35 @@ describe Projects::TransferService do
       end
     end
   end
+
+  context 'with npm packages' do
+    let!(:package) { create(:npm_package, project: project) }
+
+    before do
+      stub_licensed_features(packages: true)
+    end
+
+    context 'with a root namespace change' do
+      it 'does not allow the transfer' do
+        expect(subject.execute(group)).to be false
+        expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages")
+      end
+    end
+
+    context 'without a root namespace change' do
+      let(:root) { create(:group) }
+      let(:group) { create(:group, parent: root) }
+      let(:other_group) { create(:group, parent: root) }
+      let(:project) { create(:project, :repository, namespace: group) }
+
+      before do
+        other_group.add_owner(user)
+      end
+
+      it 'does allow the transfer' do
+        expect(subject.execute(other_group)).to be true
+        expect(project.errors[:new_namespace]).to be_empty
+      end
+    end
+  end
 end

locale/gitlab.pot

@@ -8498,6 +8498,9 @@ msgstr ""
 msgid "GroupSettings|cannot be disabled when the parent group \"Share with group lock\" is enabled, except by the owner of the parent group"
 msgstr ""
 
+msgid "GroupSettings|cannot change when group contains projects with NPM packages"
+msgstr ""
+
 msgid "GroupSettings|remove the share with group lock from %{ancestor_group_name}"
 msgstr ""
 
@@ -17645,6 +17648,9 @@ msgstr ""
 msgid "TransferGroup|Database is not supported."
 msgstr ""
 
+msgid "TransferGroup|Group contains projects with NPM packages."
+msgstr ""
+
 msgid "TransferGroup|Group is already a root group."
 msgstr ""
 
@@ -17672,6 +17678,9 @@ 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"
+msgstr ""
+
 msgid "TransferProject|Transfer failed, please contact an admin."
 msgstr ""