Merge branch '9960-publish-npm-package-from-sub-group-project' into 'master'

Resolve "Publish npm package from sub group/project"

Closes #9960

See merge request gitlab-org/gitlab-ee!13986

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