Merge branch 'secure-files-unique-filename' into 'master'

Adds a project scoped unique file name constraint for Secure Files

See merge request gitlab-org/gitlab!83628

app/models/ci/secure_file.rb

@@ -15,6 +15,7 @@ module Ci
 
     validates :file, presence: true, file_size: { maximum: FILE_SIZE_LIMIT }
     validates :checksum, :file_store, :name, :permissions, :project_id, presence: true
+    validates :name, uniqueness: { scope: :project }
 
     after_initialize :generate_key_data
     before_validation :assign_checksum

doc/api/secure_files.md

@@ -103,7 +103,7 @@ Supported attributes:
 | Attribute       | Type           | Required               | Description |
 |-----------------|----------------|------------------------|-------------|
 | `project_id`    | integer/string | **{check-circle}** Yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user. |
-| `name`          | string         | **{check-circle}** Yes | The `name` of the file being uploaded. |
+| `name`          | string         | **{check-circle}** Yes | The `name` of the file being uploaded. The file name must be unique within the project. |
 | `file`          | file           | **{check-circle}** Yes | The `file` being uploaded (5 MB limit). |
 | `permissions`   | string         | **{dotted-circle}** No | The file is created with the specified permissions when created in the CI/CD job. Available types are: `read_only` (default), `read_write`, and `execute`. |
 

spec/models/ci/secure_file_spec.rb

@@ -3,14 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Ci::SecureFile do
-  let(:sample_file) { fixture_file('ci_secure_files/upload-keystore.jks') }
-
-  subject { create(:ci_secure_file) }
-
   before do
     stub_ci_secure_file_object_storage
   end
 
+  let(:sample_file) { fixture_file('ci_secure_files/upload-keystore.jks') }
+
+  subject { create(:ci_secure_file, file: CarrierWaveStringFile.new(sample_file)) }
+
   it { is_expected.to be_a FileStoreMounter }
 
   it { is_expected.to belong_to(:project).required }
@@ -27,6 +27,26 @@ RSpec.describe Ci::SecureFile do
     it { is_expected.to validate_presence_of(:name) }
     it { is_expected.to validate_presence_of(:permissions) }
     it { is_expected.to validate_presence_of(:project_id) }
+    context 'unique filename' do
+      let_it_be(:project1) { create(:project) }
+
+      it 'ensures the file name is unique within a given project' do
+        file1 = create(:ci_secure_file, project: project1, name: 'file1')
+        expect do
+          create(:ci_secure_file, project: project1, name: 'file1')
+        end.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Name has already been taken')
+
+        expect(project1.secure_files.where(name: 'file1').count).to be 1
+        expect(project1.secure_files.find_by(name: 'file1').id).to eq(file1.id)
+      end
+
+      it 'allows duplicate file names in different projects' do
+        create(:ci_secure_file, project: project1)
+        expect do
+          create(:ci_secure_file, project: create(:project))
+        end.not_to raise_error
+      end
+    end
   end
 
   describe '#permissions' do
@@ -37,8 +57,6 @@ RSpec.describe Ci::SecureFile do
 
   describe '#checksum' do
     it 'computes SHA256 checksum on the file before encrypted' do
-      subject.file = CarrierWaveStringFile.new(sample_file)
-      subject.save!
       expect(subject.checksum).to eq(Digest::SHA256.hexdigest(sample_file))
     end
   end
@@ -51,8 +69,6 @@ RSpec.describe Ci::SecureFile do
 
   describe '#file' do
     it 'returns the saved file' do
-      subject.file = CarrierWaveStringFile.new(sample_file)
-      subject.save!
       expect(Base64.encode64(subject.file.read)).to eq(Base64.encode64(sample_file))
     end
   end

spec/requests/api/ci/secure_files_spec.rb

@@ -293,6 +293,20 @@ RSpec.describe API::Ci::SecureFiles do
         expect(json_response['error']).to eq('name is missing')
       end
 
+      it 'returns an error when the file name has already been used' do
+        post_params = {
+          name: secure_file.name,
+          file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks')
+        }
+
+        expect do
+          post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
+        end.not_to change { project.secure_files.count }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']['name']).to include('has already been taken')
+      end
+
       it 'returns an error when an unexpected permission is supplied' do
         post_params = {
           file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks'),