Merge branch '37364-fix-npm-validator-with-name+version-duplicates' into 'master'

Update the npm package validator

See merge request gitlab-org/gitlab!67427

app/models/packages/package.rb

@@ -324,7 +324,7 @@ class Packages::Package < ApplicationRecord
     return unless project
     return unless follows_npm_naming_convention?
 
-    if project.package_already_taken?(name, package_type: :npm)
+    if project.package_already_taken?(name, version, package_type: :npm)
       errors.add(:base, _('Package already exists'))
     end
   end

app/models/project.rb

@@ -2566,14 +2566,14 @@ class Project < ApplicationRecord
     [project&.id, root_group&.id]
   end
 
-  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)
-          .with_package_type(package_type)
+  def package_already_taken?(package_name, package_version, package_type:)
+    Packages::Package.with_name(package_name)
+      .with_version(package_version)
+      .with_package_type(package_type)
+      .for_projects(
+        root_ancestor.all_projects
+          .id_not_in(id)
+          .select(:id)
       ).exists?
   end
 

doc/user/packages/npm_registry/index.md

@@ -353,6 +353,13 @@ In this configuration:
 You cannot publish a package if a package of the same name and version already exists.
 You must delete the existing package first.
 
+This rule has a different impact depending on the package name:
+
+- For packages following the [naming convention](#package-naming-convention), you can't publish a
+  package with a duplicate name and version to the root namespace.
+- For packages not following the [naming convention](#package-naming-convention), you can't publish
+  a package with a duplicate name and version to the project you target with the upload.
+
 This aligns with npmjs.org's behavior. However, npmjs.org does not ever let you publish
 the same version more than once, even if it has been deleted.
 

spec/models/packages/package_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 
 RSpec.describe Packages::Package, type: :model do
   include SortingHelper
+  using RSpec::Parameterized::TableSyntax
 
   it_behaves_like 'having unique enum values'
 
@@ -435,33 +436,154 @@ RSpec.describe Packages::Package, type: :model do
         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
+        shared_examples 'validating the first package' do
+          it 'validates the first package' do
             expect(package).to be_valid
           end
+        end
 
-          it 'will not allow npm package with duplicate name' do
+        shared_examples 'validating the second package' do
+          it 'validates the second package' do
+            package.save!
+
+            expect(second_package).to be_valid
+          end
+        end
+
+        shared_examples 'not validating the second package' do |field_with_error:|
+          it 'does not validate the second package' do
             package.save!
 
             expect(second_package).not_to be_valid
+            case field_with_error
+            when :base
+              expect(second_package.errors.messages[:base]).to eq ['Package already exists']
+            when :name
+              expect(second_package.errors.messages[:name]).to eq ['has already been taken']
+            else
+              raise ArgumentError, "field #{field_with_error} not expected"
+            end
+          end
+        end
+
+        context 'following the naming convention' do
+          let(:name) { "@#{group.path}/test" }
+
+          context 'with the second package in the project of the first package' do
+            let(:second_package) { build(:npm_package, project: project, name: second_package_name, version: second_package_version) }
+
+            context 'with no duplicated name' do
+              let(:second_package_name) { "@#{group.path}/test2" }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicated name' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicate name and duplicated version' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { package.version }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'not validating the second package', field_with_error: :name
+            end
+          end
+
+          context 'with the second package in a different project than the first package' do
+            let(:second_package) { build(:npm_package, project: second_project, name: second_package_name, version: second_package_version) }
+
+            context 'with no duplicated name' do
+              let(:second_package_name) { "@#{group.path}/test2" }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicated name' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicate name and duplicated version' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { package.version }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'not validating the second package', field_with_error: :base
+            end
           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
+          context 'with the second package in the project of the first package' do
+            let(:second_package) { build(:npm_package, project: project, name: second_package_name, version: second_package_version) }
+
+            context 'with no duplicated name' do
+              let(:second_package_name) { "@foobar/test2" }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicated name' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicate name and duplicated version' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { package.version }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'not validating the second package', field_with_error: :name
+            end
           end
 
-          it 'will allow npm package with duplicate name' do
-            package.save!
+          context 'with the second package in a different project than the first package' do
+            let(:second_package) { build(:npm_package, project: second_project, name: second_package_name, version: second_package_version) }
 
-            expect(second_package).to be_valid
+            context 'with no duplicated name' do
+              let(:second_package_name) { "@foobar/test2" }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicated name' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { '5.0.0' }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
+
+            context 'with duplicate name and duplicated version' do
+              let(:second_package_name) { package.name }
+              let(:second_package_version) { package.version }
+
+              it_behaves_like 'validating the first package'
+              it_behaves_like 'validating the second package'
+            end
           end
         end
       end

spec/models/project_spec.rb

@@ -826,8 +826,6 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#merge_method' do
-    using RSpec::Parameterized::TableSyntax
-
     where(:ff, :rebase, :method) do
       true  | true  | :ff
       true  | false | :ff
@@ -1951,8 +1949,6 @@ RSpec.describe Project, factory_default: :keep do
     end
 
     context 'when set to INTERNAL in application settings' do
-      using RSpec::Parameterized::TableSyntax
-
       before do
         stub_application_setting(default_project_visibility: Gitlab::VisibilityLevel::INTERNAL)
       end
@@ -2013,8 +2009,6 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#default_branch_protected?' do
-    using RSpec::Parameterized::TableSyntax
-
     let_it_be(:namespace) { create(:namespace) }
     let_it_be(:project) { create(:project, namespace: namespace) }
 
@@ -6839,33 +6833,44 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#package_already_taken?' do
-    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:namespace) { create(:namespace, path: 'test') }
     let_it_be(:project) { create(:project, :public, namespace: namespace) }
-    let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
+    let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo", version: '1.2.3') }
 
-    context 'no package exists with the same name' do
-      it 'returns false' do
-        result = project.package_already_taken?("@#{namespace.path}/bar", package_type: :npm)
-        expect(result).to be false
+    subject { project.package_already_taken?(package_name, package_version, package_type: :npm) }
+
+    context 'within the package project' do
+      where(:package_name, :package_version, :expected_result) do
+        '@test/bar' | '1.2.3' | false
+        '@test/bar' | '5.5.5' | false
+        '@test/foo' | '1.2.3' | false
+        '@test/foo' | '5.5.5' | false
       end
 
-      it 'returns false if it is the project that the package belongs to' do
-        result = project.package_already_taken?("@#{namespace.path}/foo", package_type: :npm)
-        expect(result).to be false
+      with_them do
+        it { is_expected.to eq expected_result}
       end
     end
 
-    context 'a package already exists with the same name' do
+    context 'within a different project' do
       let_it_be(:alt_project) { create(:project, :public, namespace: namespace) }
 
-      it 'returns true' do
-        result = alt_project.package_already_taken?(package.name, package_type: :npm)
-        expect(result).to be true
+      subject { alt_project.package_already_taken?(package_name, package_version, package_type: :npm) }
+
+      where(:package_name, :package_version, :expected_result) do
+        '@test/bar' | '1.2.3' | false
+        '@test/bar' | '5.5.5' | false
+        '@test/foo' | '1.2.3' | true
+        '@test/foo' | '5.5.5' | false
+      end
+
+      with_them do
+        it { is_expected.to eq expected_result}
       end
 
       context 'for a different package type' do
         it 'returns false' do
-          result = alt_project.package_already_taken?(package.name, package_type: :nuget)
+          result = alt_project.package_already_taken?(package.name, package.version, package_type: :nuget)
           expect(result).to be false
         end
       end

spec/requests/api/npm_project_packages_spec.rb

@@ -161,8 +161,10 @@ RSpec.describe API::NpmProjectPackages do
         end
       end
 
-      context 'valid package record' do
-        let(:params) { upload_params(package_name: package_name) }
+      context 'valid package params' do
+        let_it_be(:version) { '1.2.3' }
+
+        let(:params) { upload_params(package_name: package_name, package_version: version) }
         let(:snowplow_gitlab_standard_context) { { project: project, namespace: project.namespace, user: user } }
 
         shared_examples 'handling upload with different authentications' do
@@ -211,6 +213,15 @@ RSpec.describe API::NpmProjectPackages do
           end
         end
 
+        shared_examples 'uploading the package' do
+          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
+
         context 'with a scoped name' do
           let(:package_name) { "@#{group.path}/my_package_name" }
 
@@ -233,24 +244,25 @@ RSpec.describe API::NpmProjectPackages 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_it_be(:second_package) { create(:npm_package, project: second_project, name: "@#{group.path}/test", version: version) }
 
             let(:package_name) { "@#{group.path}/test" }
 
             it_behaves_like 'handling invalid record with 400 error'
+
+            context 'with a new version' do
+              let_it_be(:version) { '4.5.6' }
+
+              it_behaves_like 'uploading the package'
+            end
           end
 
           context 'not following the naming convention' do
-            let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@any_scope/test") }
+            let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@any_scope/test", version: version) }
 
             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
+            it_behaves_like 'uploading the package'
           end
         end
       end