Merge branch '345376-workhorse-not-allowing-multiple-files-for-upload' into 'master'

Workhorse: Allow uploading up to 10 files

See merge request gitlab-org/gitlab!74377

spec/features/projects/issues/design_management/user_uploads_designs_spec.rb

@@ -27,10 +27,10 @@ RSpec.describe 'User uploads new design', :js do
         expect(page).to have_content('dk.png')
       end
 
-      upload_design(gif_fixture, count: 2)
+      upload_design([gif_fixture, logo_svg_fixture, big_image_fixture], count: 4)
 
-      expect(page).to have_selector('.js-design-list-item', count: 2)
-      expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif'])
+      expect(page).to have_selector('.js-design-list-item', count: 4)
+      expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif', 'logo_sample.svg', 'big-image.png'])
     end
   end
 
@@ -50,8 +50,16 @@ RSpec.describe 'User uploads new design', :js do
     Rails.root.join('spec', 'fixtures', 'banana_sample.gif')
   end
 
-  def upload_design(fixture, count:)
-    attach_file(:upload_file, fixture, match: :first, make_visible: true)
+  def logo_svg_fixture
+    Rails.root.join('spec', 'fixtures', 'logo_sample.svg')
+  end
+
+  def big_image_fixture
+    Rails.root.join('spec', 'fixtures', 'big-image.png')
+  end
+
+  def upload_design(fixtures, count:)
+    attach_file(:upload_file, fixtures, multiple: true, match: :first, make_visible: true)
 
     wait_for('designs uploaded') do
       issue.reload.designs.count == count

workhorse/internal/artifacts/artifacts_upload_test.go

@@ -270,7 +270,7 @@ func TestUploadHandlerForMultipleFiles(t *testing.T) {
 	require.NoError(t, s.writer.Close())
 
 	response := testUploadArtifacts(t, s.writer.FormDataContentType(), s.url, s.buffer)
-	require.Equal(t, http.StatusBadRequest, response.Code)
+	require.Equal(t, http.StatusInternalServerError, response.Code)
 }
 
 func TestUploadFormProcessing(t *testing.T) {

workhorse/internal/upload/rewrite.go

@@ -24,10 +24,12 @@ import (
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/upload/exif"
 )
 
+const maxFilesAllowed = 10
+
 // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields
 var (
-	ErrInjectedClientParam   = errors.New("injected client parameter")
-	ErrMultipleFilesUploaded = errors.New("upload request contains more than one file")
+	ErrInjectedClientParam  = errors.New("injected client parameter")
+	ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed)
 )
 
 var (
@@ -117,8 +119,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 }
 
 func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error {
-	if rew.filter.Count() > 0 {
-		return ErrMultipleFilesUploaded
+	if rew.filter.Count() >= maxFilesAllowed {
+		return ErrTooManyFilesUploaded
 	}
 
 	multipartFiles.WithLabelValues(rew.filter.Name()).Inc()

workhorse/internal/upload/saved_file_tracker.go

@@ -27,6 +27,10 @@ func (s *SavedFileTracker) Count() int {
 }
 
 func (s *SavedFileTracker) ProcessFile(_ context.Context, fieldName string, file *filestore.FileHandler, _ *multipart.Writer) error {
+	if _, ok := s.rewrittenFields[fieldName]; ok {
+		return fmt.Errorf("the %v field has already been processed", fieldName)
+	}
+
 	s.Track(fieldName, file.LocalPath)
 	return nil
 }

workhorse/internal/upload/saved_file_tracker_test.go

@@ -37,3 +37,14 @@ func TestSavedFileTracking(t *testing.T) {
 
 	require.Contains(t, rewrittenFields, "test")
 }
+
+func TestDuplicatedFileProcessing(t *testing.T) {
+	tracker := SavedFileTracker{}
+	file := &filestore.FileHandler{}
+
+	require.NoError(t, tracker.ProcessFile(context.Background(), "file", file, nil))
+
+	err := tracker.ProcessFile(context.Background(), "file", file, nil)
+	require.Error(t, err)
+	require.Equal(t, "the file field has already been processed", err.Error())
+}

workhorse/internal/upload/uploads.go

@@ -35,8 +35,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p
 		switch err {
 		case ErrInjectedClientParam:
 			helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest)
-		case ErrMultipleFilesUploaded:
-			helper.CaptureAndFail(w, r, err, "Uploading multiple files is not allowed", http.StatusBadRequest)
+		case ErrTooManyFilesUploaded:
+			helper.CaptureAndFail(w, r, err, err.Error(), http.StatusBadRequest)
 		case http.ErrNotMultipart:
 			h.ServeHTTP(w, r)
 		case filestore.ErrEntityTooLarge:

workhorse/internal/upload/uploads_test.go

@@ -260,10 +260,10 @@ func TestUploadingMultipleFiles(t *testing.T) {
 	var buffer bytes.Buffer
 
 	writer := multipart.NewWriter(&buffer)
-	_, err = writer.CreateFormFile("file", "my.file")
-	require.NoError(t, err)
-	_, err = writer.CreateFormFile("file", "my.file")
-	require.NoError(t, err)
+	for i := 0; i < 11; i++ {
+		_, err = writer.CreateFormFile(fmt.Sprintf("file %v", i), "my.file")
+		require.NoError(t, err)
+	}
 	require.NoError(t, writer.Close())
 
 	httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer)
@@ -279,7 +279,7 @@ func TestUploadingMultipleFiles(t *testing.T) {
 	HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
 
 	require.Equal(t, 400, response.Code)
-	require.Equal(t, "Uploading multiple files is not allowed\n", response.Body.String())
+	require.Equal(t, "upload request contains more than 10 files\n", response.Body.String())
 }
 
 func TestUploadProcessingFile(t *testing.T) {