From 249c24891a3a54d2fd6b9355244cad4e35d722f7 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 27 Jun 2018 12:54:46 +0200 Subject: [PATCH] Updated multipart to support workhorse direct uploads --- lib/gitlab/middleware/multipart.rb | 16 +++-- spec/lib/gitlab/middleware/multipart_spec.rb | 73 +++++++++++--------- 2 files changed, 50 insertions(+), 39 deletions(-) diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index a5f5d719cc1..9753be6d5c3 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -42,10 +42,10 @@ module Gitlab key, value = parsed_field.first if value.nil? - value = open_file(tmp_path, @request.params["#{key}.name"]) + value = open_file(@request.params, key) @open_files << value else - value = decorate_params_value(value, @request.params[key], tmp_path) + value = decorate_params_value(value, @request.params[key]) end @request.update_param(key, value) @@ -57,7 +57,7 @@ module Gitlab end # This function calls itself recursively - def decorate_params_value(path_hash, value_hash, tmp_path) + def decorate_params_value(path_hash, value_hash) unless path_hash.is_a?(Hash) && path_hash.count == 1 raise "invalid path: #{path_hash.inspect}" end @@ -70,19 +70,21 @@ module Gitlab case path_value when nil - value_hash[path_key] = open_file(tmp_path, value_hash.dig(path_key, '.name')) + value_hash[path_key] = open_file(value_hash.dig(path_key), '') @open_files << value_hash[path_key] value_hash when Hash - decorate_params_value(path_value, value_hash[path_key], tmp_path) + decorate_params_value(path_value, value_hash[path_key]) value_hash else raise "unexpected path value: #{path_value.inspect}" end end - def open_file(path, name) - ::UploadedFile.new(path, filename: name || File.basename(path), content_type: 'application/octet-stream') + def open_file(params, key) + ::UploadedFile.from_params( + params, key, + Gitlab.config.uploads.storage_path) end end diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb index a2ba91dae80..b4837a1689a 100644 --- a/spec/lib/gitlab/middleware/multipart_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_spec.rb @@ -7,18 +7,47 @@ describe Gitlab::Middleware::Multipart do let(:middleware) { described_class.new(app) } let(:original_filename) { 'filename' } - it 'opens top-level files' do - Tempfile.open('top-level') do |tempfile| - env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename }, Gitlab::Workhorse.secret, 'gitlab-workhorse') + shared_examples_for 'multipart upload files' do + it 'opens top-level files' do + Tempfile.open('top-level') do |tempfile| + env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path, 'file.remote_id' => remote_id }, Gitlab::Workhorse.secret, 'gitlab-workhorse') + expect_uploaded_file(tempfile, %w(file)) + + middleware.call(env) + end + end + + it 'opens files one level deep' do + Tempfile.open('one-level') do |tempfile| + in_params = { 'user' => { 'avatar' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } } + env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + expect_uploaded_file(tempfile, %w(user avatar)) + + middleware.call(env) + end + end + + it 'opens files two levels deep' do + Tempfile.open('two-levels') do |tempfile| + in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } } } + env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + expect_uploaded_file(tempfile, %w(project milestone themesong)) + + middleware.call(env) + end + end + + def expect_uploaded_file(tempfile, path, remote: false) expect(app).to receive(:call) do |env| - file = Rack::Request.new(env).params['file'] + file = Rack::Request.new(env).params.dig(*path) expect(file).to be_a(::UploadedFile) expect(file.path).to eq(tempfile.path) expect(file.original_filename).to eq(original_filename) + expect(file.remote_id).to eq(remote_id) end - - middleware.call(env) end end @@ -34,36 +63,16 @@ describe Gitlab::Middleware::Multipart do expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError) end - it 'opens files one level deep' do - Tempfile.open('one-level') do |tempfile| - in_params = { 'user' => { 'avatar' => { '.name' => original_filename } } } - env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + context 'with remote file' do + let(:remote_id) { 'someid' } - expect(app).to receive(:call) do |env| - file = Rack::Request.new(env).params['user']['avatar'] - expect(file).to be_a(::UploadedFile) - expect(file.path).to eq(tempfile.path) - expect(file.original_filename).to eq(original_filename) - end - - middleware.call(env) - end + it_behaves_like 'multipart upload files' end - it 'opens files two levels deep' do - Tempfile.open('two-levels') do |tempfile| - in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename } } } } - env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') + context 'with local file' do + let(:remote_id) { nil } - expect(app).to receive(:call) do |env| - file = Rack::Request.new(env).params['project']['milestone']['themesong'] - expect(file).to be_a(::UploadedFile) - expect(file.path).to eq(tempfile.path) - expect(file.original_filename).to eq(original_filename) - end - - middleware.call(env) - end + it_behaves_like 'multipart upload files' end def post_env(rewritten_fields, params, secret, issuer) -- GitLab