Allow NPM packages to have any scoped name

To allow for the publishing of NPM packages from groups and subgroups,
packages must be able to be named without the forward slash character.
This change removes requiring the package name match the full project
path and name, and updates the query for finding a package by its
name.

ee/app/models/ee/project.rb

@@ -586,6 +586,14 @@ module EE
       @design_repository ||= DesignManagement::Repository.new(self)
     end
 
+    def package_already_taken?(package_name)
+      namespace.root_ancestor.all_projects
+        .joins(:packages)
+        .where.not(id: id)
+        .merge(Packages::Package.with_name(package_name))
+        .exists?
+    end
+
     private
 
     def set_override_pull_mirror_available

ee/app/models/packages/package.rb

@@ -12,9 +12,13 @@ class Packages::Package < ApplicationRecord
     presence: true,
     format: { with: Gitlab::Regex.package_name_regex }
 
+  validate :valid_npm_package_name, if: :npm?
+  validate :package_already_taken
+
   enum package_type: { maven: 1, npm: 2 }
 
   scope :with_name, ->(name) { where(name: name) }
+  scope :with_version, ->(version) { where(version: version) }
   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)) }
@@ -34,4 +38,22 @@ class Packages::Package < ApplicationRecord
       .joins(:package_files)
       .where(packages_package_files: { file_name: file_name }).last!
   end
+
+  private
+
+  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
+
+    if project.package_already_taken?(name)
+      errors.add(:base, 'Package already exists')
+    end
+  end
 end

ee/app/services/packages/create_npm_package_service.rb

@@ -6,6 +6,10 @@ module Packages
       version = params[:versions].keys.first
       version_data = params[:versions][version]
 
+      existing_package = project.packages.npm.with_name(name).with_version(version)
+
+      return error('Package already exists.', 403) if existing_package.exists?
+
       package = project.packages.create!(
         name: name,
         version: version,

ee/changelogs/unreleased/9960-publish-npm-package-from-sub-group-project.yml

+---
+title: Add the ability to publish and install NPM packages from groups and subgroups
+merge_request: 13986
+author:
+type: added

ee/lib/api/npm_packages.rb

@@ -5,6 +5,10 @@ module API
       package_name: API::NO_SLASH_URL_PART_REGEX
     }.freeze
 
+    rescue_from ActiveRecord::RecordInvalid do |e|
+      render_api_error!(e.message, 400)
+    end
+
     before do
       require_packages_enabled!
       authenticate_non_get!
@@ -14,15 +18,7 @@ module API
 
     helpers do
       def find_project_by_package_name(name)
-        Project.find_by_full_path(name.sub('@', ''))
-      end
-
-      def project_package_name_match?
-        "@#{user_project.full_path}" == params[:package_name]
-      end
-
-      def ensure_project_package_match!
-        bad_request!(:package_name) unless project_package_name_match?
+        ::Packages::Package.npm.with_name(name).first&.project
       end
     end
 
@@ -35,7 +31,6 @@ module API
     get 'packages/npm/*package_name', format: false, requirements: NPM_ENDPOINT_REQUIREMENTS do
       package_name = params[:package_name]
 
-      # To avoid name collision we require project path and project package be the same.
       project = find_project_by_package_name(package_name)
 
       authorize!(:read_package, project)
@@ -54,7 +49,6 @@ module API
     resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       before do
         authorize_packages_feature!
-        ensure_project_package_match!
       end
 
       desc 'Download the NPM tarball' do
@@ -86,8 +80,14 @@ module API
       put ':id/packages/npm/:package_name', requirements: NPM_ENDPOINT_REQUIREMENTS do
         authorize_create_package!
 
-        ::Packages::CreateNpmPackageService
+        created_package = ::Packages::CreateNpmPackageService
           .new(user_project, current_user, params).execute
+
+        if created_package[:status] == :error
+          render_api_error!(created_package[:message], created_package[:http_status])
+        else
+          created_package
+        end
       end
     end
   end

ee/spec/factories/packages.rb

@@ -21,7 +21,7 @@ FactoryBot.define do
     end
 
     factory :npm_package do
-      name 'foo'
+      sequence(:name) { |n| "@#{project.root_namespace.path}/package-#{n}"}
       version '1.0.0'
       package_type 'npm'
 

ee/spec/models/packages/package_spec.rb

@@ -32,28 +32,36 @@ RSpec.describe Packages::Package, type: :model do
     end
   end
 
-  describe '.last_of_each_version' do
+  context 'version scopes' do
     let!(:package1) { create(:npm_package, version: '1.0.0') }
     let!(:package2) { create(:npm_package, version: '1.0.1') }
     let!(:package3) { create(:npm_package, version: '1.0.1') }
 
-    subject { described_class.last_of_each_version }
+    describe '.last_of_each_version' do
+      subject { described_class.last_of_each_version }
 
-    it 'includes only latest package per version' do
-      is_expected.to include(package1, package3)
-      is_expected.not_to include(package2)
+      it 'includes only latest package per version' do
+        is_expected.to include(package1, package3)
+        is_expected.not_to include(package2)
+      end
     end
-  end
 
-  describe '.has_version' do
-    let!(:package1) { create(:npm_package, version: '1.0.0') }
-    let!(:package2) { create(:npm_package, version: nil) }
-    let!(:package3) { create(:npm_package, version: '1.0.1') }
+    describe '.has_version' do
+      let!(:package4) { create(:npm_package, version: nil) }
+
+      subject { described_class.has_version }
+
+      it 'includes only packages with version attribute' do
+        is_expected.to match_array([package1, package2, package3])
+      end
+    end
 
-    subject { described_class.has_version }
+    describe '.with_version' do
+      subject { described_class.with_version('1.0.1') }
 
-    it 'includes only packages with version attribute' do
-      is_expected.to match_array([package1, package3])
+      it 'includes only packages with specified version' do
+        is_expected.to match_array([package2, package3])
+      end
     end
   end
 end

ee/spec/models/project_spec.rb

@@ -2143,4 +2143,31 @@ describe Project do
       .to receive(:default_url_options)
       .and_return(host: host)
   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") }
+
+    context 'no package exists with the same name' do
+      it 'returns false' do
+        result = project.package_already_taken?("@#{namespace.path}/bar")
+        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")
+        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) }
+
+      it 'returns true' do
+        result = alt_project.package_already_taken?("@#{namespace.path}/foo")
+        expect(result).to be true
+      end
+    end
+  end
 end

ee/spec/requests/api/npm_packages_spec.rb

@@ -27,7 +27,7 @@ describe API::NpmPackages do
   end
 
   describe 'GET /api/v4/packages/npm/*package_name' do
-    let(:package) { create(:npm_package, project: project, name: "@#{project.full_path}") }
+    let(:package) { create(:npm_package, project: project) }
 
     context 'a public project' do
       it 'returns the package info without oauth token' do
@@ -80,16 +80,6 @@ describe API::NpmPackages do
       expect(response).to have_gitlab_http_status(403)
     end
 
-    context 'project name is different from a package name' do
-      let(:package) { create(:npm_package, project: project) }
-
-      it 'rejects request' do
-        get_package(package)
-
-        expect(response).to have_gitlab_http_status(403)
-      end
-    end
-
     def get_package(package, params = {})
       get api("/packages/npm/#{package.name}"), params: params
     end
@@ -100,7 +90,7 @@ describe API::NpmPackages do
   end
 
   describe 'GET /api/v4/projects/:id/packages/npm/*package_name/-/*file_name' do
-    let(:package) { create(:npm_package, project: project, name: "@#{project.full_path}") }
+    let(:package) { create(:npm_package, project: project) }
     let(:package_file) { package.package_files.first }
 
     context 'a public project' do
@@ -159,20 +149,34 @@ describe API::NpmPackages do
 
   describe 'PUT /api/v4/projects/:id/packages/npm/:package_name' do
     context 'when params are correct' do
-      context 'unscoped package' do
-        let(:package_name) { project.path }
-        let(:params) { upload_params(package_name) }
+      context 'invalid package record' do
+        context 'unscoped package' do
+          let(:package_name) { 'my_unscoped_package' }
+          let(:params) { upload_params(package_name) }
 
-        it 'denies the request with 400 error' do
-          expect { upload_package_with_token(package_name, params) }
-            .not_to change { project.packages.count }
+          it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
+            expect { upload_package_with_token(package_name, params) }
+              .not_to change { project.packages.count }
+
+            expect(response).to have_gitlab_http_status(400)
+          end
+        end
+
+        context 'invalid package name' do
+          let(:package_name) { "@#{group.path}/my_inv@lid_package_name" }
+          let(:params) { upload_params(package_name) }
+
+          it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
+            expect { upload_package_with_token(package_name, params) }
+              .not_to change { project.packages.count }
 
-          expect(response).to have_gitlab_http_status(400)
+            expect(response).to have_gitlab_http_status(400)
+          end
         end
       end
 
       context 'scoped package' do
-        let(:package_name) { "@#{project.full_path}" }
+        let(:package_name) { "@#{group.path}/my_package_name" }
         let(:params) { upload_params(package_name) }
 
         it 'creates npm package with file' do
@@ -183,6 +187,19 @@ describe API::NpmPackages do
           expect(response).to have_gitlab_http_status(200)
         end
       end
+
+      context 'package creation fails' do
+        let(:package_name) { "@#{group.path}/my_package_name" }
+        let(:params) { upload_params(package_name) }
+
+        it 'returns an error if the package already exists' do
+          create(:npm_package, project: project, version: '1.0.1', name: "@#{group.path}/my_package_name")
+          expect { upload_package_with_token(package_name, params) }
+            .not_to change { project.packages.count }
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
     end
 
     def upload_package(package_name, params = {})

ee/spec/services/packages/create_npm_package_service_spec.rb

@@ -2,7 +2,8 @@
 require 'spec_helper'
 
 describe Packages::CreateNpmPackageService do
-  let(:project) { create(:project) }
+  let(:namespace) {create(:namespace)}
+  let(:project) { create(:project, namespace: namespace) }
   let(:user) { create(:user) }
   let(:version) { '1.0.1'.freeze }
 
@@ -26,15 +27,41 @@ describe Packages::CreateNpmPackageService do
 
   describe '#execute' do
     context 'scoped package' do
-      let(:package_name) { '@gitlab/my-app'.freeze }
+      let(:package_name) { "@#{namespace.path}/my-app".freeze }
 
       it_behaves_like 'valid package'
     end
 
-    context 'normal package' do
-      let(:package_name) { 'my-app'.freeze }
+    context 'invalid package name' do
+      let(:package_name) { "@#{namespace.path}/my-group/my-app".freeze }
 
-      it_behaves_like 'valid package'
+      it 'raises a RecordInvalid error' do
+        service = described_class.new(project, user, params)
+
+        expect { service.execute }.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+
+    context 'package already exists' do
+      let(:package_name) { "@#{namespace.path}/my_package" }
+
+      it 'returns a 403 error' do
+        create(:npm_package, project: project, name: package_name, version: '1.0.1')
+        response = described_class.new(project, user, params).execute
+
+        expect(response[:http_status]).to eq 403
+        expect(response[:message]).to be 'Package already exists.'
+      end
+    end
+
+    context 'with incorrect namespace' do
+      let(:package_name) { '@my_other_namespace/my-app' }
+
+      it 'raises a RecordInvalid error' do
+        service = described_class.new(project, user, params)
+
+        expect { service.execute }.to raise_error(ActiveRecord::RecordInvalid)
+      end
     end
   end
 end