summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2018-08-13 15:36:15 -0700
committerStan Hu <stanhu@gmail.com>2018-09-05 17:01:54 -0700
commit262b974123d22b5d6b662b232ca4792d7998a166 (patch)
tree223b600596a24fa4a4cd8436f8317d2874fb98ce
parent9dd34eac14a94e58999c335a175e06067f1092fa (diff)
downloadgitlab-ce-262b974123d22b5d6b662b232ca4792d7998a166.tar.gz
Fix attachments not displaying inline with Google Cloud Storage
There were several issues: 1. With Google Cloud Storage, we can't override the Content-Type with Response-Content-Type once it is set. Setting the value to `application/octet-stream` doesn't buy us anything. GCS defaults to `application/octet-stream`, and AWS uses `binary/octet-stream`. Just remove this `Content-Type` when we upload new files. 2. CarrierWave and fog-google need to support query parameters: https://github.com/fog/fog-google/pull/409/files, https://github.com/carrierwaveuploader/carrierwave/pull/2332/files. CarrierWave has been monkey-patched until an official release. 3. Workhorse also needs to remove the Content-Type header in the request (https://gitlab.com/gitlab-org/gitlab-workhorse/blob/ef80978ff89e628c8eeb66556720e30587d3deb6/internal/objectstore/object.go#L66), or we'll get a 403 error when uploading due to signed URLs not matching the headers. Upgrading to Workhorse 6.1.0 for https://gitlab.com/gitlab-org/gitlab-workhorse/merge_requests/297 will make Workhorse use the headers that are used by Rails. Closes #49957
-rw-r--r--GITLAB_WORKHORSE_VERSION2
-rw-r--r--Gemfile4
-rw-r--r--Gemfile.lock2
-rw-r--r--Gemfile.rails5.lock2
-rw-r--r--app/controllers/concerns/send_file_upload.rb16
-rw-r--r--changelogs/unreleased/sh-fix-attachments-inline.yml5
-rw-r--r--config/initializers/carrierwave_patch.rb29
-rw-r--r--config/initializers/fog_google_https_private_urls.rb2
-rw-r--r--lib/object_storage/direct_upload.rb2
-rw-r--r--spec/controllers/concerns/send_file_upload_spec.rb31
-rw-r--r--spec/lib/object_storage/direct_upload_spec.rb2
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
diff --git a/Gemfile b/Gemfile
index 7b83c6d1178..746a152a59b 100644
--- a/Gemfile
+++ b/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
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