Merge branch 'sh-fix-attachments-inline' into 'master'

Fix attachments not displaying inline with Google Cloud Storage

Closes #49957

See merge request gitlab-org/gitlab-ce!21265

GITLAB_WORKHORSE_VERSION

-6.0.0
+6.1.0

Gemfile

@@ -107,7 +107,9 @@ gem 'kaminari', '~> 1.0'
 gem 'hamlit', '~> 2.8.8'
 
 # Files attachments
-gem 'carrierwave', '~> 1.2'
+# Locked until https://github.com/carrierwaveuploader/carrierwave/pull/2332/files is merged.
+# config/initializers/carrierwave_patch.rb can be removed once that change is released.
+gem 'carrierwave', '= 1.2.3'
 gem 'mini_magick'
 
 # Drag and Drop UI

Gemfile.lock

@@ -996,7 +996,7 @@ DEPENDENCIES
   bundler-audit (~> 0.5.0)
   capybara (~> 2.15)
   capybara-screenshot (~> 1.0.0)
-  carrierwave (~> 1.2)
+  carrierwave (= 1.2.3)
   charlock_holmes (~> 0.7.5)
   chronic (~> 0.10.2)
   chronic_duration (~> 0.10.6)

Gemfile.rails5.lock

@@ -1005,7 +1005,7 @@ DEPENDENCIES
   bundler-audit (~> 0.5.0)
   capybara (~> 2.15)
   capybara-screenshot (~> 1.0.0)
-  carrierwave (~> 1.2)
+  carrierwave (= 1.2.3)
   charlock_holmes (~> 0.7.5)
   chronic (~> 0.10.2)
   chronic_duration (~> 0.10.6)

app/controllers/concerns/send_file_upload.rb

 module SendFileUpload
   def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, disposition: 'attachment')
     if attachment
-      redirect_params[:query] = { "response-content-disposition" => "#{disposition};filename=#{attachment.inspect}" }
+      # Response-Content-Type will not override an existing Content-Type in
+      # Google Cloud Storage, so the metadata needs to be cleared on GCS for
+      # this to work. However, this override works with AWS.
+      redirect_params[:query] = { "response-content-disposition" => "#{disposition};filename=#{attachment.inspect}",
+                                  "response-content-type" => guess_content_type(attachment) }
       # By default, Rails will send uploads with an extension of .js with a
       # content-type of text/javascript, which will trigger Rails'
       # cross-origin JavaScript protection.
@@ -18,4 +22,14 @@ module SendFileUpload
       redirect_to file_upload.url(**redirect_params)
     end
   end
+
+  def guess_content_type(filename)
+    types = MIME::Types.type_for(filename)
+
+    if types.present?
+      types.first.content_type
+    else
+      "application/octet-stream"
+    end
+  end
 end

changelogs/unreleased/sh-fix-attachments-inline.yml

+---
+title: Fix attachments not displaying inline with Google Cloud Storage
+merge_request: 21265
+author:
+type: fixed

config/initializers/carrierwave_patch.rb

+# This monkey patches CarrierWave 1.2.3 to make Google Cloud Storage work with
+# extra query parameters:
+# https://github.com/carrierwaveuploader/carrierwave/pull/2332/files
+module CarrierWave
+  module Storage
+    class Fog < Abstract
+      class File
+        def authenticated_url(options = {})
+          if %w(AWS Google Rackspace OpenStack).include?(@uploader.fog_credentials[:provider])
+            # avoid a get by using local references
+            local_directory = connection.directories.new(key: @uploader.fog_directory)
+            local_file = local_directory.files.new(key: path)
+            expire_at = ::Fog::Time.now + @uploader.fog_authenticated_url_expiration
+            case @uploader.fog_credentials[:provider]
+            when 'AWS', 'Google'
+              local_file.url(expire_at, options)
+            when 'Rackspace'
+              connection.get_object_https_url(@uploader.fog_directory, path, expire_at, options)
+            when 'OpenStack'
+              connection.get_object_https_url(@uploader.fog_directory, path, expire_at)
+            else
+              local_file.url(expire_at)
+            end
+          end
+        end
+      end
+    end
+  end
+end

config/initializers/fog_google_https_private_urls.rb

@@ -9,7 +9,7 @@ module Fog
         module MonkeyPatch
           def url(expires, options = {})
             requires :key
-            collection.get_https_url(key, expires)
+            collection.get_https_url(key, expires, options)
           end
         end
 

lib/object_storage/direct_upload.rb

@@ -158,7 +158,7 @@ module ObjectStorage
     end
 
     def upload_options
-      { 'Content-Type' => 'application/octet-stream' }
+      {}
     end
 
     def connection

spec/controllers/concerns/send_file_upload_spec.rb

@@ -52,7 +52,7 @@ describe SendFileUpload do
     end
 
     context 'with attachment' do
-      subject { controller.send_upload(uploader, attachment: 'test.js') }
+      let(:send_attachment) { controller.send_upload(uploader, attachment: 'test.js') }
 
       it 'sends a file with content-type of text/plain' do
         expected_params = {
@@ -62,7 +62,29 @@ describe SendFileUpload do
         }
         expect(controller).to receive(:send_file).with(uploader.path, expected_params)
 
-        subject
+        send_attachment
+      end
+
+      context 'with a proxied file in object storage' do
+        before do
+          stub_uploads_object_storage(uploader: uploader_class)
+          uploader.object_store = ObjectStorage::Store::REMOTE
+          uploader.store!(temp_file)
+          allow(Gitlab.config.uploads.object_store).to receive(:proxy_download) { true }
+        end
+
+        it 'sends a file with a custom type' do
+          headers = double
+          expected_headers = %r(response-content-disposition=attachment%3Bfilename%3D%22test.js%22&response-content-type=application/javascript)
+          expect(Gitlab::Workhorse).to receive(:send_url).with(expected_headers).and_call_original
+          expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
+
+          expect(controller).not_to receive(:send_file)
+          expect(controller).to receive(:headers) { headers }
+          expect(controller).to receive(:head).with(:ok)
+
+          send_attachment
+        end
       end
     end
 
@@ -80,7 +102,12 @@ describe SendFileUpload do
 
         it 'sends a file' do
           headers = double
+          expect(Gitlab::Workhorse).not_to receive(:send_url).with(/response-content-disposition/)
+          expect(Gitlab::Workhorse).not_to receive(:send_url).with(/response-content-type/)
+          expect(Gitlab::Workhorse).to receive(:send_url).and_call_original
+
           expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
+          expect(controller).not_to receive(:send_file)
           expect(controller).to receive(:headers) { headers }
           expect(controller).to receive(:head).with(:ok)
 

spec/lib/object_storage/direct_upload_spec.rb

@@ -62,7 +62,7 @@ describe ObjectStorage::DirectUpload do
         expect(subject[:StoreURL]).to start_with(storage_url)
         expect(subject[:DeleteURL]).to start_with(storage_url)
         expect(subject[:CustomPutHeaders]).to be_truthy
-        expect(subject[:PutHeaders]).to eq({ 'Content-Type' => 'application/octet-stream' })
+        expect(subject[:PutHeaders]).to eq({})
       end
     end