diff options
| -rw-r--r-- | GITLAB_WORKHORSE_VERSION | 2 | ||||
| -rw-r--r-- | Gemfile | 4 | ||||
| -rw-r--r-- | Gemfile.lock | 2 | ||||
| -rw-r--r-- | Gemfile.rails5.lock | 2 | ||||
| -rw-r--r-- | app/controllers/concerns/send_file_upload.rb | 16 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-fix-attachments-inline.yml | 5 | ||||
| -rw-r--r-- | config/initializers/carrierwave_patch.rb | 29 | ||||
| -rw-r--r-- | config/initializers/fog_google_https_private_urls.rb | 2 | ||||
| -rw-r--r-- | lib/object_storage/direct_upload.rb | 2 | ||||
| -rw-r--r-- | spec/controllers/concerns/send_file_upload_spec.rb | 31 | ||||
| -rw-r--r-- | spec/lib/object_storage/direct_upload_spec.rb | 2 |
11 files changed, 87 insertions, 10 deletions
diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 09b254e90c6..dfda3e0b4f0 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -6.0.0 +6.1.0 @@ -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 diff --git a/Gemfile.lock b/Gemfile.lock index 91cd360e708..cf2634a26d3 100644 --- a/Gemfile.lock +++ b/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) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index ba9b06a08cb..9daf710ece6 100644 --- a/Gemfile.rails5.lock +++ b/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) diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 237c93daee8..382ec91f771 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/app/controllers/concerns/send_file_upload.rb @@ -1,7 +1,11 @@ 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 diff --git a/changelogs/unreleased/sh-fix-attachments-inline.yml b/changelogs/unreleased/sh-fix-attachments-inline.yml new file mode 100644 index 00000000000..2926edca97a --- /dev/null +++ b/changelogs/unreleased/sh-fix-attachments-inline.yml @@ -0,0 +1,5 @@ +--- +title: Fix attachments not displaying inline with Google Cloud Storage +merge_request: 21265 +author: +type: fixed diff --git a/config/initializers/carrierwave_patch.rb b/config/initializers/carrierwave_patch.rb new file mode 100644 index 00000000000..35ffff03abe --- /dev/null +++ b/config/initializers/carrierwave_patch.rb @@ -0,0 +1,29 @@ +# 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 diff --git a/config/initializers/fog_google_https_private_urls.rb b/config/initializers/fog_google_https_private_urls.rb index c65a534b536..682b1050c68 100644 --- a/config/initializers/fog_google_https_private_urls.rb +++ b/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 diff --git a/lib/object_storage/direct_upload.rb b/lib/object_storage/direct_upload.rb index b372b4af090..ab43910c8bd 100644 --- a/lib/object_storage/direct_upload.rb +++ b/lib/object_storage/direct_upload.rb @@ -158,7 +158,7 @@ module ObjectStorage end def upload_options - { 'Content-Type' => 'application/octet-stream' } + {} end def connection diff --git a/spec/controllers/concerns/send_file_upload_spec.rb b/spec/controllers/concerns/send_file_upload_spec.rb index 58bb91a0c80..767fba7fd58 100644 --- a/spec/controllers/concerns/send_file_upload_spec.rb +++ b/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) diff --git a/spec/lib/object_storage/direct_upload_spec.rb b/spec/lib/object_storage/direct_upload_spec.rb index 632acd6eb46..9c308cc1be9 100644 --- a/spec/lib/object_storage/direct_upload_spec.rb +++ b/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 |