Merge branch '33685-lift-npm-package-naming-convention-for-project-level-api' into 'master'

Lift the NPM package naming convention for the project level API

See merge request gitlab-org/gitlab!53266

app/finders/packages/npm/package_finder.rb

@@ -2,29 +2,41 @@
 module Packages
   module Npm
     class PackageFinder
-      attr_reader :project, :package_name
-
       delegate :find_by_version, to: :execute
+      delegate :last, to: :execute
 
-      def initialize(project, package_name)
-        @project = project
+      def initialize(package_name, project: nil, namespace: nil)
         @package_name = package_name
+        @project = project
+        @namespace = namespace
       end
 
       def execute
-        return Packages::Package.none unless project
-
-        packages
+        base.npm
+            .with_name(@package_name)
+            .last_of_each_version
+            .preload_files
       end
 
       private
 
-      def packages
-        project.packages
-          .npm
-          .with_name(package_name)
-          .last_of_each_version
-          .preload_files
+      def base
+        if @project
+          packages_for_project
+        elsif @namespace
+          packages_for_namespace
+        else
+          ::Packages::Package.none
+        end
+      end
+
+      def packages_for_project
+        @project.packages
+      end
+
+      def packages_for_namespace
+        projects = ::Project.in_namespace(@namespace.self_and_descendants.select(:id))
+        ::Packages::Package.for_projects(projects.select(:id))
       end
     end
   end

app/models/packages/package.rb

@@ -40,11 +40,11 @@ class Packages::Package < ApplicationRecord
   validate :unique_debian_package_name, if: :debian_package?
 
   validate :valid_conan_package_recipe, if: :conan?
-  validate :valid_npm_package_name, if: :npm?
   validate :valid_composer_global_name, if: :composer?
   validate :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.npm_package_name_regex }, if: :npm?
   validates :name, format: { with: Gitlab::Regex.nuget_package_name_regex }, if: :nuget?
   validates :name, format: { with: Gitlab::Regex.debian_package_name_regex }, if: :debian_package?
   validates :name, inclusion: { in: %w[incoming] }, if: :debian_incoming?
@@ -247,14 +247,6 @@ class Packages::Package < ApplicationRecord
     end
   end
 
-  def valid_npm_package_name
-    return unless project&.root_namespace
-
-    unless name =~ %r{\A@#{project.root_namespace.path}/[^/]+\z}
-      errors.add(:name, 'is not valid')
-    end
-  end
-
   def package_already_taken
     return unless project
 

changelogs/unreleased/33685-lift-npm-package-naming-convention-for-project-level-api.yml

+---
+title: Lift the NPM package naming convention for the project level API
+merge_request: 53266
+author:
+type: changed

doc/user/packages/npm_registry/index.md

@@ -86,7 +86,9 @@ A `package.json` file is created.
 To use the GitLab endpoint for npm packages, choose an option:
 
 - **Project-level**: Use when you have few npm packages and they are not in
-  the same GitLab group.
+  the same GitLab group. The [package naming convention](#package-naming-convention) is not enforced at this level. 
+  Instead, you should use a [scope](https://docs.npmjs.com/cli/v6/using-npm/scope) for your package. 
+  When you use a scope, the registry URL is [updated](#authenticate-to-the-package-registry) only for that scope.
 - **Instance-level**: Use when you have many npm packages in different
   GitLab groups or in their own namespace. Be sure to comply with the [package naming convention](#package-naming-convention).
 
@@ -204,7 +206,7 @@ Then, you can run `npm publish` either locally or by using GitLab CI/CD.
 
 ## Package naming convention
 
-Your npm package name must be in the format of `@scope/package-name`.
+When you use the [instance-level endpoint](#use-the-gitlab-endpoint-for-npm-packages), only the packages with names in the format of `@scope/package-name` are available.
 
 - The `@scope` is the root namespace of the GitLab project. It must match exactly, including the case.
 - The `package-name` can be whatever you want.
@@ -302,8 +304,9 @@ the same version more than once, even if it has been deleted.
 ## Install a package
 
 npm packages are commonly-installed by using the `npm` or `yarn` commands
-in a JavaScript project. You can install a package from the scope of a project, group,
-or instance.
+in a JavaScript project. You can install a package from the scope of a project or instance.
+
+If multiple packages have the same name and version, when you install a package, the most recently-published package is retrieved.
 
 1. Set the URL for scoped packages by running:
 

lib/api/concerns/packages/npm_endpoints.rb

@@ -41,7 +41,7 @@ module API
 
               authorize_read_package!(project)
 
-              packages = ::Packages::Npm::PackageFinder.new(project, package_name)
+              packages = ::Packages::Npm::PackageFinder.new(package_name, project: project)
                                                        .execute
 
               not_found! if packages.empty?
@@ -68,8 +68,7 @@ module API
 
                 authorize_create_package!(project)
 
-                package = ::Packages::Npm::PackageFinder
-                  .new(project, package_name)
+                package = ::Packages::Npm::PackageFinder.new(package_name, project: project)
                                                         .find_by_version(version)
                 not_found!('Package') unless package
 
@@ -112,8 +111,7 @@ module API
           route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true
           get '*package_name', format: false, requirements: ::API::Helpers::Packages::Npm::NPM_ENDPOINT_REQUIREMENTS do
             package_name = params[:package_name]
-
-            packages = ::Packages::Npm::PackageFinder.new(project_or_nil, package_name)
+            packages = ::Packages::Npm::PackageFinder.new(package_name, project: project_or_nil)
                                                      .execute
 
             redirect_request = project_or_nil.blank? || packages.empty?

lib/api/helpers/packages/npm.rb

@@ -49,13 +49,34 @@ module API
             when :project
               params[:id]
             when :instance
-              ::Packages::Package.npm
-                                .with_name(params[:package_name])
-                                .first
-                                &.project_id
+              namespace_path = namespace_path_from_package_name
+              next unless namespace_path
+
+              namespace = namespace_from_path(namespace_path)
+              next unless namespace
+
+              finder = ::Packages::Npm::PackageFinder.new(params[: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
+
+        def namespace_from_path(path)
+          group = Group.by_path(path)
+          return group if group
+
+          Namespace.for_user.by_path(path)
+        end
       end
     end
   end

lib/gitlab/regex.rb

@@ -61,6 +61,10 @@ module Gitlab
         maven_app_name_regex
       end
 
+      def npm_package_name_regex
+        @npm_package_name_regex ||= %r{\A(?:@(#{Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX})/)?[-+\.\_a-zA-Z0-9]+\z}
+      end
+
       def nuget_package_name_regex
         @nuget_package_name_regex ||= %r{\A[-+\.\_a-zA-Z0-9]+\z}.freeze
       end

spec/finders/packages/npm/package_finder_spec.rb

@@ -2,13 +2,43 @@
 require 'spec_helper'
 
 RSpec.describe ::Packages::Npm::PackageFinder do
-  let(:package) { create(:npm_package) }
+  let_it_be_with_reload(:project) { create(:project)}
+  let_it_be(:package) { create(:npm_package, project: project) }
+
   let(:project) { package.project }
   let(:package_name) { package.name }
 
-  describe '#execute!' do
-    subject { described_class.new(project, package_name).execute }
+  shared_examples 'accepting a namespace for' do |example_name|
+    before do
+      project.update!(namespace: namespace)
+    end
+
+    context 'that is a group' do
+      let_it_be(:namespace) { create(:group) }
+
+      it_behaves_like example_name
+
+      context 'within another group' do
+        let_it_be(:subgroup) { create(:group, parent: namespace) }
+
+        before do
+          project.update!(namespace: subgroup)
+        end
 
+        it_behaves_like example_name
+      end
+    end
+
+    context 'that is a user namespace' do
+      let_it_be(:user) { create(:user) }
+      let_it_be(:namespace) { user.namespace }
+
+      it_behaves_like example_name
+    end
+  end
+
+  describe '#execute' do
+    shared_examples 'finding packages by name' do
       it { is_expected.to eq([package]) }
 
       context 'with unknown package name' do
@@ -16,19 +46,41 @@ RSpec.describe ::Packages::Npm::PackageFinder do
 
         it { is_expected.to be_empty }
       end
+    end
+
+    subject { finder.execute }
 
-    context 'with nil project' do
+    context 'with a project' do
+      let(:finder) { described_class.new(package_name, project: project) }
+
+      it_behaves_like 'finding packages by name'
+
+      context 'set to nil' do
         let(:project) { nil }
 
         it { is_expected.to be_empty }
       end
     end
 
+    context 'with a namespace' do
+      let(:finder) { described_class.new(package_name, namespace: namespace) }
+
+      it_behaves_like 'accepting a namespace for', 'finding packages by name'
+
+      context 'set to nil' do
+        let_it_be(:namespace) { nil }
+
+        it { is_expected.to be_empty }
+      end
+    end
+  end
+
   describe '#find_by_version' do
     let(:version) { package.version }
 
-    subject { described_class.new(project, package.name).find_by_version(version) }
+    subject { finder.find_by_version(version) }
 
+    shared_examples 'finding packages by version' do
       it { is_expected.to eq(package) }
 
       context 'with unknown version' do
@@ -37,4 +89,52 @@ RSpec.describe ::Packages::Npm::PackageFinder do
         it { is_expected.to be_nil }
       end
     end
+
+    context 'with a project' do
+      let(:finder) { described_class.new(package_name, project: project) }
+
+      it_behaves_like 'finding packages by version'
+    end
+
+    context 'with a namespace' do
+      let(:finder) { described_class.new(package_name, namespace: namespace) }
+
+      it_behaves_like 'accepting a namespace for', 'finding packages by version'
+    end
+  end
+
+  describe '#last' do
+    subject { finder.last }
+
+    shared_examples 'finding package by last' do
+      it { is_expected.to eq(package) }
+    end
+
+    context 'with a project' do
+      let(:finder) { described_class.new(package_name, project: project) }
+
+      it_behaves_like 'finding package by last'
+    end
+
+    context 'with a namespace' do
+      let(:finder) { described_class.new(package_name, namespace: namespace) }
+
+      it_behaves_like 'accepting a namespace for', 'finding package by last'
+
+      context 'with duplicate packages' do
+        let_it_be(:namespace) { create(:group) }
+        let_it_be(:subgroup1) { create(:group, parent: namespace) }
+        let_it_be(:subgroup2) { create(:group, parent: namespace) }
+        let_it_be(:project2) { create(:project, namespace: subgroup2) }
+        let_it_be(:package2) { create(:npm_package, name: package.name, project: project2) }
+
+        before do
+          project.update!(namespace: subgroup1)
+        end
+
+        # the most recent one is returned
+        it { is_expected.to eq(package2) }
+      end
+    end
+  end
 end

spec/lib/gitlab/regex_spec.rb

@@ -367,6 +367,35 @@ RSpec.describe Gitlab::Regex do
     it { is_expected.not_to match('%2e%2e%2f1.2.3') }
   end
 
+  describe '.npm_package_name_regex' do
+    subject { described_class.npm_package_name_regex }
+
+    it { is_expected.to match('@scope/package') }
+    it { is_expected.to match('unscoped-package') }
+    it { is_expected.not_to match('@first-scope@second-scope/package') }
+    it { is_expected.not_to match('scope-without-at-symbol/package') }
+    it { is_expected.not_to match('@not-a-scoped-package') }
+    it { is_expected.not_to match('@scope/sub/package') }
+    it { is_expected.not_to match('@scope/../../package') }
+    it { is_expected.not_to match('@scope%2e%2e%2fpackage') }
+    it { is_expected.not_to match('@%2e%2e%2f/package') }
+
+    context 'capturing group' do
+      [
+        ['@scope/package', 'scope'],
+        ['unscoped-package', nil],
+        ['@not-a-scoped-package', nil],
+        ['@scope/sub/package', nil],
+        ['@inv@lid-scope/package', nil]
+      ].each do |package_name, extracted_scope_name|
+        it "extracts the scope name for #{package_name}" do
+          match = package_name.match(described_class.npm_package_name_regex)
+          expect(match&.captures&.first).to eq(extracted_scope_name)
+        end
+      end
+    end
+  end
+
   describe '.nuget_version_regex' do
     subject { described_class.nuget_version_regex }
 

spec/models/packages/package_spec.rb

@@ -162,6 +162,18 @@ RSpec.describe Packages::Package, type: :model do
         it { is_expected.not_to allow_value('../../../my_package').for(:name) }
         it { is_expected.not_to allow_value('%2e%2e%2fmy_package').for(:name) }
       end
+
+      context 'npm package' do
+        subject { build_stubbed(:npm_package) }
+
+        it { is_expected.to allow_value("@group-1/package").for(:name) }
+        it { is_expected.to allow_value("@any-scope/package").for(:name) }
+        it { is_expected.to allow_value("unscoped-package").for(:name) }
+        it { is_expected.not_to allow_value("@inv@lid-scope/package").for(:name) }
+        it { is_expected.not_to allow_value("@scope/../../package").for(:name) }
+        it { is_expected.not_to allow_value("@scope%2e%2e%fpackage").for(:name) }
+        it { is_expected.not_to allow_value("@scope/sub/package").for(:name) }
+      end
     end
 
     describe '#version' do
@@ -342,16 +354,6 @@ RSpec.describe Packages::Package, type: :model do
     end
 
     describe '#package_already_taken' do
-      context 'npm package' do
-        let!(:package) { create(:npm_package) }
-
-        it 'will not allow a package of the same name' do
-          new_package = build(:npm_package, project: create(:project), name: package.name)
-
-          expect(new_package).not_to be_valid
-        end
-      end
-
       context 'maven package' do
         let!(:package) { create(:maven_package) }
 

spec/requests/api/npm_project_packages_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe API::NpmProjectPackages do
   end
 
   describe 'GET /api/v4/projects/:id/packages/npm/*package_name/-/*file_name' do
-    let_it_be(:package_file) { package.package_files.first }
+    let(:package_file) { package.package_files.first }
 
     let(:headers) { {} }
     let(:url) { api("/projects/#{project.id}/packages/npm/#{package.name}/-/#{package_file.file_name}") }
@@ -127,24 +127,6 @@ RSpec.describe API::NpmProjectPackages do
 
     context 'when params are correct' do
       context 'invalid package record' do
-        context 'unscoped package' do
-          let(:package_name) { 'my_unscoped_package' }
-          let(:params) { upload_params(package_name: package_name) }
-
-          it_behaves_like 'handling invalid record with 400 error'
-
-          context 'with empty versions' do
-            let(:params) { upload_params(package_name: package_name).merge!(versions: {}) }
-
-            it 'throws a 400 error' do
-              expect { upload_package_with_token(package_name, params) }
-              .not_to change { project.packages.count }
-
-              expect(response).to have_gitlab_http_status(:bad_request)
-            end
-          end
-        end
-
         context 'invalid package name' do
           let(:package_name) { "@#{group.path}/my_inv@@lid_package_name" }
           let(:params) { upload_params(package_name: package_name) }
@@ -175,10 +157,10 @@ RSpec.describe API::NpmProjectPackages do
         end
       end
 
-      context 'scoped package' do
-        let(:package_name) { "@#{group.path}/my_package_name" }
+      context 'valid package record' do
         let(:params) { upload_params(package_name: package_name) }
 
+        shared_examples 'handling upload with different authentications' do
           context 'with access token' do
             subject { upload_package_with_token(package_name, params) }
 
@@ -224,6 +206,25 @@ RSpec.describe API::NpmProjectPackages do
           end
         end
 
+        context 'with a scoped name' do
+          let(:package_name) { "@#{group.path}/my_package_name" }
+
+          it_behaves_like 'handling upload with different authentications'
+        end
+
+        context 'with any scoped name' do
+          let(:package_name) { "@any_scope/my_package_name" }
+
+          it_behaves_like 'handling upload with different authentications'
+        end
+
+        context 'with an unscoped name' do
+          let(:package_name) { "my_unscoped_package_name" }
+
+          it_behaves_like 'handling upload with different authentications'
+        end
+      end
+
       context 'package creation fails' do
         let(:package_name) { "@#{group.path}/my_package_name" }
         let(:params) { upload_params(package_name: package_name) }

spec/services/packages/npm/create_package_service_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Packages::Npm::CreatePackageService do
   end
 
   let(:override) { {} }
-  let(:package_name) { "@#{namespace.path}/my-app".freeze }
+  let(:package_name) { "@#{namespace.path}/my-app" }
 
   subject { described_class.new(project, user, params).execute }
 
@@ -42,11 +42,6 @@ RSpec.describe Packages::Npm::CreatePackageService do
 
     it { expect(subject.name).to eq(package_name) }
     it { expect(subject.version).to eq(version) }
-  end
-
-  describe '#execute' do
-    context 'scoped package' do
-      it_behaves_like 'valid package'
 
     context 'with build info' do
       let(:job) { create(:ci_build, user: user) }
@@ -61,10 +56,21 @@ RSpec.describe Packages::Npm::CreatePackageService do
     end
   end
 
-    context 'invalid package name' do
-      let(:package_name) { "@#{namespace.path}/my-group/my-app".freeze }
+  describe '#execute' do
+    context 'scoped package' do
+      it_behaves_like 'valid package'
+    end
 
-      it { expect { subject }.to raise_error(ActiveRecord::RecordInvalid) }
+    context 'scoped package not following the naming convention' do
+      let(:package_name) { '@any-scope/package' }
+
+      it_behaves_like 'valid package'
+    end
+
+    context 'unscoped package' do
+      let(:package_name) { 'unscoped-package' }
+
+      it_behaves_like 'valid package'
     end
 
     context 'package already exists' do
@@ -84,13 +90,20 @@ RSpec.describe Packages::Npm::CreatePackageService do
       it { expect(subject[:message]).to be 'File is too large.' }
     end
 
-    context 'with incorrect namespace' do
-      let(:package_name) { '@my_other_namespace/my-app' }
+    [
+      '@inv@lid_scope/package',
+      '@scope/sub/group',
+      '@scope/../../package',
+      '@scope%2e%2e%2fpackage'
+    ].each do |invalid_package_name|
+      context "with invalid name #{invalid_package_name}" do
+        let(:package_name) { invalid_package_name }
 
         it 'raises a RecordInvalid error' do
           expect { subject }.to raise_error(ActiveRecord::RecordInvalid)
         end
       end
+    end
 
     context 'with empty versions' do
       let(:override) { { versions: {} } }

spec/support/shared_contexts/requests/api/npm_packages_shared_context.rb

@@ -22,6 +22,10 @@ RSpec.shared_context 'set package name from package name type' do
     case package_name_type
     when :scoped_naming_convention
       "@#{group.path}/scoped-package"
+    when :scoped_no_naming_convention
+      '@any-scope/scoped-package'
+    when :unscoped
+      'unscoped-package'
     when :non_existing
       'non-existing-package'
     end