Merge branch 'revert-b3e21ec0' into 'master'

Revert "Merge branch 'khanchi-designs-patch2' into 'master'"

See merge request gitlab-org/gitlab!53939

app/services/design_management/save_designs_service.rb

@@ -18,7 +18,6 @@ module DesignManagement
       return error("Only #{MAX_FILES} files are allowed simultaneously") if files.size > MAX_FILES
       return error("Duplicate filenames are not allowed!") if files.map(&:original_filename).uniq.length != files.length
       return error("Design copy is in progress") if design_collection.copy_in_progress?
-      return error("Filenames contained invalid characters and could not be saved") if files.any?(&:filename_sanitized?)
 
       uploaded_designs, version = upload_designs!
       skipped_designs = designs - uploaded_designs

changelogs/unreleased/khanchi-designs-patch2.yml

----
-title: 'Designs: return error if uploading filenames with special chars'
-merge_request: 44136
-author: Sushil Khanchi @khanchi97
-type: fixed

ee/spec/services/ee/design_management/save_designs_service_spec.rb

@@ -3,12 +3,11 @@ require 'spec_helper'
 
 RSpec.describe DesignManagement::SaveDesignsService do
   include DesignManagementTestHelpers
-  using FixtureFileRefinements
 
   let_it_be(:project) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:design_file) { fixture_file_upload('spec/fixtures/rails_sample.jpg').to_gitlab_uploaded_file }
+  let_it_be(:design_file) { fixture_file_upload('spec/fixtures/rails_sample.jpg') }
   let_it_be(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project)}
 
   subject { described_class.new(project, user, issue: issue, files: [design_file]) }

lib/uploaded_file.rb

@@ -78,15 +78,9 @@ class UploadedFile
   def sanitize_filename(name)
     name = name.tr("\\", "/") # work-around for IE
     name = ::File.basename(name)
-
-    pre_sanitized_name = name
-
     name = name.gsub(CarrierWave::SanitizedFile.sanitize_regexp, "_")
     name = "_#{name}" if name =~ /\A\.+\z/
     name = "unnamed" if name.empty?
-
-    @filename_sanitized = name != pre_sanitized_name
-
     name.mb_chars.to_s
   end
 
@@ -98,10 +92,6 @@ class UploadedFile
     @tempfile&.close
   end
 
-  def filename_sanitized?
-    @filename_sanitized
-  end
-
   alias_method :local_path, :path
 
   def method_missing(method_name, *args, &block) #:nodoc:

spec/graphql/mutations/design_management/upload_spec.rb

@@ -4,7 +4,6 @@ require 'spec_helper'
 RSpec.describe Mutations::DesignManagement::Upload do
   include DesignManagementTestHelpers
   include ConcurrentHelpers
-  using FixtureFileRefinements
 
   let(:issue) { create(:issue) }
   let(:user) { issue.author }
@@ -19,12 +18,8 @@ RSpec.describe Mutations::DesignManagement::Upload do
     mutation.resolve(project_path: project_path, iid: iid, files: files_to_upload)
   end
 
-  def uploaded_file(filename)
-    fixture_file_upload(expand_fixture_path(filename))
-  end
-
   describe "#resolve" do
-    let(:files) { [uploaded_file('dk.png').to_gitlab_uploaded_file] }
+    let(:files) { [fixture_file_upload('spec/fixtures/dk.png')] }
 
     subject(:resolve) do
       mutation.resolve(project_path: project.full_path, iid: issue.iid, files: files)
@@ -54,7 +49,7 @@ RSpec.describe Mutations::DesignManagement::Upload do
           ['dk.png', 'rails_sample.jpg', 'banana_sample.gif']
            .cycle
            .take(Concurrent.processor_count * 2)
-           .map { |f| uploaded_file(f).uniquely_named.to_gitlab_uploaded_file }
+           .map { |f| RenameableUpload.unique_file(f) }
         end
 
         def creates_designs

spec/lib/uploaded_file_spec.rb

@@ -218,20 +218,6 @@ RSpec.describe UploadedFile do
     end
   end
 
-  describe '#filename_sanitized?' do
-    it 'is true when filename has been sanitized' do
-      file = described_class.new(temp_file.path, filename: 'foo①.png')
-
-      expect(file).to be_filename_sanitized
-    end
-
-    it 'is false when filename has not been sanitized' do
-      file = described_class.new(temp_file.path, filename: 'foo.png')
-
-      expect(file).not_to be_filename_sanitized
-    end
-  end
-
   describe '#sanitize_filename' do
     it { expect(described_class.new(temp_file.path).sanitize_filename('spaced name')).to eq('spaced_name') }
     it { expect(described_class.new(temp_file.path).sanitize_filename('#$%^&')).to eq('_____') }

spec/services/design_management/save_designs_service_spec.rb

@@ -4,7 +4,6 @@ require 'spec_helper'
 RSpec.describe DesignManagement::SaveDesignsService do
   include DesignManagementTestHelpers
   include ConcurrentHelpers
-  using FixtureFileRefinements
 
   let_it_be_with_reload(:issue) { create(:issue) }
   let_it_be(:developer) { create(:user, developer_projects: [issue.project]) }
@@ -13,11 +12,11 @@ RSpec.describe DesignManagement::SaveDesignsService do
   let(:files) { [rails_sample] }
   let(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) }
   let(:rails_sample_name) { 'rails_sample.jpg' }
-  let(:rails_sample) { uploaded_file(rails_sample_name).to_gitlab_uploaded_file }
-  let(:dk_png) { uploaded_file('dk.png').to_gitlab_uploaded_file }
+  let(:rails_sample) { sample_image(rails_sample_name) }
+  let(:dk_png) { sample_image('dk.png') }
 
-  def uploaded_file(filename)
-    fixture_file_upload(expand_fixture_path(filename))
+  def sample_image(filename)
+    fixture_file_upload("spec/fixtures/#{filename}")
   end
 
   def commit_count
@@ -123,8 +122,7 @@ RSpec.describe DesignManagement::SaveDesignsService do
         parellism = 4
 
         blocks = Array.new(parellism).map do
-          unique_file = uploaded_file('dk.png').uniquely_named.to_gitlab_uploaded_file
-          unique_files = [unique_file]
+          unique_files = [RenameableUpload.unique_file('rails_sample.jpg')]
 
           -> { run_service(unique_files) }
         end
@@ -308,14 +306,6 @@ RSpec.describe DesignManagement::SaveDesignsService do
             expect(response[:message]).to match('Duplicate filenames are not allowed!')
           end
         end
-
-        context 'when uploading files with special characters in filenames' do
-          let(:files) { [uploaded_file('dk.png').renamed_as('special_char①.png').to_gitlab_uploaded_file] }
-
-          it 'returns the correct error' do
-            expect(response[:message]).to match('Filenames contained invalid characters and could not be saved')
-          end
-        end
       end
 
       context 'when the user is not allowed to upload designs' do

spec/support/refinements/fixture_file_refinements.rb

-# frozen_string_literal: true
-
-module FixtureFileRefinements
-  refine Rack::Test::UploadedFile do
-    # Recast this instance of `Rack::Test::UploadedFile` to an `::UploadedFile`.
-    def to_gitlab_uploaded_file
-      ::UploadedFile.new(path, filename: original_filename, content_type: content_type || 'application/octet-stream').tap do |file|
-        # `UploadedFile#tempfile` is read-only, so replace this with the writeable fixture file
-        file.instance_variable_set(:@tempfile, self)
-      end
-    end
-
-    # Renames `original_filename` to something guaranteed to be unique.
-    def uniquely_named
-      name = File.basename(FactoryBot.generate(:filename), '.*')
-      extension = File.extname(original_filename)
-      unique_filename = name + extension
-
-      renamed_as(unique_filename)
-    end
-
-    def renamed_as(new_filename)
-      tap { @original_filename = new_filename }
-    end
-  end
-end

spec/support/renameable_upload.rb

+# frozen_string_literal: true
+
+class RenameableUpload < SimpleDelegator
+  attr_accessor :original_filename
+
+  # Get a fixture file with a new unique name, and the same extension
+  def self.unique_file(name)
+    upload = new(fixture_file_upload("spec/fixtures/#{name}"))
+    ext = File.extname(name)
+    new_name = File.basename(FactoryBot.generate(:filename), '.*')
+    upload.original_filename = new_name + ext
+
+    upload
+  end
+end