Merge branch '226007-refuse-pypi-duplicate-packages' into 'master'

Rejects duplicated pypi files

Closes #226007

See merge request gitlab-org/gitlab!38006

app/models/packages/package_file.rb

@@ -15,6 +15,8 @@ class Packages::PackageFile < ApplicationRecord
   validates :file, presence: true
   validates :file_name, presence: true
 
+  validates :file_name, uniqueness: { scope: :package }, if: -> { package&.pypi? }
+
   scope :recent, -> { order(id: :desc) }
   scope :with_file_name, ->(file_name) { where(file_name: file_name) }
   scope :with_file_name_like, ->(file_name) { where(arel_table[:file_name].matches(file_name)) }

changelogs/unreleased/226007-refuse-pypi-duplicate-packages.yml

+---
+title: Rejects duplicated pypi files
+merge_request: 38006
+author:
+type: changed

lib/api/pypi_packages.rb

@@ -22,10 +22,6 @@ module API
       render_api_error!(e.message, 400)
     end
 
-    rescue_from ActiveRecord::RecordInvalid do |e|
-      render_api_error!(e.message, 400)
-    end
-
     helpers do
       def packages_finder(project = authorized_user_project)
         project

spec/services/packages/pypi/create_package_service_spec.rb

@@ -49,18 +49,11 @@ RSpec.describe Packages::Pypi::CreatePackageService do
           params[:md5_digest] = 'def'
         end
 
-        it 'replaces the file' do
+        it 'throws an error' do
           expect { subject }
             .to change { Packages::Package.pypi.count }.by(0)
-            .and change { Packages::PackageFile.count }.by(1)
-
-          expect(created_package.package_files.size).to eq 2
-          expect(created_package.package_files.first.file_name).to eq 'foo.tgz'
-          expect(created_package.package_files.first.file_sha256).to eq '123'
-          expect(created_package.package_files.first.file_md5).to eq '567'
-          expect(created_package.package_files.last.file_name).to eq 'foo.tgz'
-          expect(created_package.package_files.last.file_sha256).to eq 'abc'
-          expect(created_package.package_files.last.file_md5).to eq 'def'
+            .and change { Packages::PackageFile.count }.by(0)
+            .and raise_error(/File name has already been taken/)
         end
       end
 

spec/support/shared_examples/requests/api/pypi_packages_shared_examples.rb

@@ -24,6 +24,20 @@ RSpec.shared_examples 'PyPi package creation' do |user_type, status, add_member
 
     it_behaves_like 'creating pypi package files'
 
+    context 'with a pre-existing file' do
+      it 'rejects the duplicated file' do
+        existing_package = create(:pypi_package, name: base_params[:name], version: base_params[:version], project: project)
+        create(:package_file, :pypi, package: existing_package, file_name: params[:content].original_filename)
+
+        expect { subject }
+            .to change { project.packages.pypi.count }.by(0)
+            .and change { Packages::PackageFile.count }.by(0)
+            .and change { Packages::Pypi::Metadatum.count }.by(0)
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+
     context 'with object storage disabled' do
       before do
         stub_package_file_object_storage(enabled: false)