Merge branch '37364-fix-npm-validator' into 'master'

Fix the npm package already taken validator

See merge request gitlab-org/gitlab!67107

app/models/packages/npm.rb

+# frozen_string_literal: true
+module Packages
+  module Npm
+    # from "@scope/package-name" return "scope" or nil
+    def self.scope_of(package_name)
+      return unless package_name
+      return unless package_name.starts_with?('@')
+      return unless package_name.include?('/')
+
+      package_name.match(Gitlab::Regex.npm_package_name_regex)&.captures&.first
+    end
+  end
+end

app/models/packages/package.rb

@@ -62,7 +62,7 @@ class Packages::Package < ApplicationRecord
 
   validate :valid_conan_package_recipe, if: :conan?
   validate :valid_composer_global_name, if: :composer?
-  validate :package_already_taken, if: :npm?
+  validate :npm_package_already_taken, if: :npm?
   validates :name, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
   validates :name, format: { with: Gitlab::Regex.generic_package_name_regex }, if: :generic?
   validates :name, format: { with: Gitlab::Regex.helm_package_regex }, if: :helm?
@@ -320,14 +320,22 @@ class Packages::Package < ApplicationRecord
     end
   end
 
-  def package_already_taken
+  def npm_package_already_taken
     return unless project
+    return unless follows_npm_naming_convention?
 
-    if project.package_already_taken?(name)
+    if project.package_already_taken?(name, package_type: :npm)
       errors.add(:base, _('Package already exists'))
     end
   end
 
+  # https://docs.gitlab.com/ee/user/packages/npm_registry/#package-naming-convention
+  def follows_npm_naming_convention?
+    return false unless project&.root_namespace&.path
+
+    project.root_namespace.path == ::Packages::Npm.scope_of(name)
+  end
+
   def unique_debian_package_name
     return unless debian_publication&.distribution
 

app/models/project.rb

@@ -2566,12 +2566,15 @@ class Project < ApplicationRecord
     [project&.id, root_group&.id]
   end
 
-  def package_already_taken?(package_name)
+  def package_already_taken?(package_name, package_type:)
     namespace.root_ancestor.all_projects
       .joins(:packages)
       .where.not(id: id)
-      .merge(Packages::Package.default_scoped.with_name(package_name))
-      .exists?
+      .merge(
+        Packages::Package.default_scoped
+          .with_name(package_name)
+          .with_package_type(package_type)
+      ).exists?
   end
 
   def default_branch_or_main

lib/api/helpers/packages/npm.rb

@@ -49,28 +49,20 @@ module API
             when :project
               params[:id]
             when :instance
-              namespace_path = namespace_path_from_package_name
+              package_name = params[:package_name]
+              namespace_path = ::Packages::Npm.scope_of(package_name)
               next unless namespace_path
 
               namespace = Namespace.top_most
                                    .by_path(namespace_path)
               next unless namespace
 
-              finder = ::Packages::Npm::PackageFinder.new(params[:package_name], namespace: namespace)
+              finder = ::Packages::Npm::PackageFinder.new(package_name, namespace: namespace)
 
               finder.last&.project_id
             end
           end
         end
-
-        # from "@scope/package-name" return "scope" or nil
-        def namespace_path_from_package_name
-          package_name = params[:package_name]
-          return unless package_name.starts_with?('@')
-          return unless package_name.include?('/')
-
-          package_name.match(Gitlab::Regex.npm_package_name_regex)&.captures&.first
-        end
       end
     end
   end

spec/models/packages/npm_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Packages::Npm do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '.scope_of' do
+    subject { described_class.scope_of(package_name) }
+
+    where(:package_name, :expected_result) do
+      nil             | nil
+      'test'          | nil
+      '@test'         | nil
+      'test/package'  | nil
+      '@/package'     | nil
+      '@test/package' | 'test'
+      '@test/'        | nil
+    end
+
+    with_them do
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+end

spec/models/packages/package_spec.rb

@@ -418,7 +418,7 @@ RSpec.describe Packages::Package, type: :model do
       end
     end
 
-    describe '#package_already_taken' do
+    describe '#npm_package_already_taken' do
       context 'maven package' do
         let!(:package) { create(:maven_package) }
 
@@ -428,6 +428,43 @@ RSpec.describe Packages::Package, type: :model do
           expect(new_package).to be_valid
         end
       end
+
+      context 'npm package' do
+        let_it_be(:group) { create(:group) }
+        let_it_be(:project) { create(:project, namespace: group) }
+        let_it_be(:second_project) { create(:project, namespace: group)}
+
+        let(:package) { build(:npm_package, project: project, name: name) }
+        let(:second_package) { build(:npm_package, project: second_project, name: name, version: '5.0.0') }
+
+        context 'following the naming convention' do
+          let(:name) { "@#{group.path}/test" }
+
+          it 'will allow the first package' do
+            expect(package).to be_valid
+          end
+
+          it 'will not allow npm package with duplicate name' do
+            package.save!
+
+            expect(second_package).not_to be_valid
+          end
+        end
+
+        context 'not following the naming convention' do
+          let(:name) { '@foobar/test' }
+
+          it 'will allow the first package' do
+            expect(package).to be_valid
+          end
+
+          it 'will allow npm package with duplicate name' do
+            package.save!
+
+            expect(second_package).to be_valid
+          end
+        end
+      end
     end
 
     context "recipe uniqueness for conan packages" do

spec/models/project_spec.rb

@@ -6839,29 +6839,36 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#package_already_taken?' do
-    let(:namespace) { create(:namespace) }
-    let(:project) { create(:project, :public, namespace: namespace) }
-    let!(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
+    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:project) { create(:project, :public, namespace: namespace) }
+    let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
 
     context 'no package exists with the same name' do
       it 'returns false' do
-        result = project.package_already_taken?("@#{namespace.path}/bar")
+        result = project.package_already_taken?("@#{namespace.path}/bar", package_type: :npm)
         expect(result).to be false
       end
 
       it 'returns false if it is the project that the package belongs to' do
-        result = project.package_already_taken?("@#{namespace.path}/foo")
+        result = project.package_already_taken?("@#{namespace.path}/foo", package_type: :npm)
         expect(result).to be false
       end
     end
 
     context 'a package already exists with the same name' do
-      let(:alt_project) { create(:project, :public, namespace: namespace) }
+      let_it_be(:alt_project) { create(:project, :public, namespace: namespace) }
 
       it 'returns true' do
-        result = alt_project.package_already_taken?("@#{namespace.path}/foo")
+        result = alt_project.package_already_taken?(package.name, package_type: :npm)
         expect(result).to be true
       end
+
+      context 'for a different package type' do
+        it 'returns false' do
+          result = alt_project.package_already_taken?(package.name, package_type: :nuget)
+          expect(result).to be false
+        end
+      end
     end
   end
 

spec/requests/api/npm_project_packages_spec.rb

@@ -228,6 +228,31 @@ RSpec.describe API::NpmProjectPackages do
 
           it_behaves_like 'handling upload with different authentications'
         end
+
+        context 'with an existing package' do
+          let_it_be(:second_project) { create(:project, namespace: namespace) }
+
+          context 'following the naming convention' do
+            let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@#{group.path}/test") }
+
+            let(:package_name) { "@#{group.path}/test" }
+
+            it_behaves_like 'handling invalid record with 400 error'
+          end
+
+          context 'not following the naming convention' do
+            let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@any_scope/test") }
+
+            let(:package_name) { "@any_scope/test" }
+
+            it "uploads the package" do
+              expect { upload_package_with_token(package_name, params) }
+                .to change { project.packages.count }.by(1)
+
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+        end
       end
 
       context 'package creation fails' do