Merge branch 'gitlab-workhorse-multipart' into 'master'

Replace Rack::Multipart with gitlab-workhorse based system

See merge request !5867

GITLAB_WORKHORSE_VERSION

-1.1.1
+1.2.0

app/uploaders/artifact_uploader.rb

-class ArtifactUploader < CarrierWave::Uploader::Base
+class ArtifactUploader < GitlabUploader
   storage :file
 
   attr_accessor :build, :field
@@ -38,12 +38,4 @@ class ArtifactUploader < CarrierWave::Uploader::Base
   def exists?
     file.try(:exists?)
   end
-
-  def move_to_cache
-    true
-  end
-
-  def move_to_store
-    true
-  end
 end

app/uploaders/attachment_uploader.rb

-class AttachmentUploader < CarrierWave::Uploader::Base
+class AttachmentUploader < GitlabUploader
   include UploaderHelper
 
   storage :file

app/uploaders/avatar_uploader.rb

-class AvatarUploader < CarrierWave::Uploader::Base
+class AvatarUploader < GitlabUploader
   include UploaderHelper
 
   storage :file

app/uploaders/file_uploader.rb

-class FileUploader < CarrierWave::Uploader::Base
+class FileUploader < GitlabUploader
   include UploaderHelper
   MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
 

app/uploaders/gitlab_uploader.rb

+class GitlabUploader < CarrierWave::Uploader::Base
+  # Reduce disk IO
+  def move_to_cache
+    true
+  end
+
+  # Reduce disk IO
+  def move_to_store
+    true
+  end
+end

app/uploaders/lfs_object_uploader.rb

-class LfsObjectUploader < CarrierWave::Uploader::Base
+class LfsObjectUploader < GitlabUploader
   storage :file
 
   def store_dir
@@ -9,14 +9,6 @@ class LfsObjectUploader < CarrierWave::Uploader::Base
     "#{Gitlab.config.lfs.storage_path}/tmp/cache"
   end
 
-  def move_to_cache
-    true
-  end
-
-  def move_to_store
-    true
-  end
-
   def exists?
     file.try(:exists?)
   end

changelogs/unreleased/gitlab-workhorse-multipart.yml

+---
+title: Replace Rack::Multipart with GitLab-Workhorse based solution
+merge_request: 5867
+author: 

config/initializers/workhorse_multipart.rb

+Rails.application.configure do |config|
+  config.middleware.use(Gitlab::Middleware::Multipart)
+end

lib/gitlab/gfm/uploads_rewriter.rb

+require 'fileutils'
+
 module Gitlab
   module Gfm
     ##
@@ -22,7 +24,9 @@ module Gitlab
           return markdown unless file.try(:exists?)
 
           new_uploader = FileUploader.new(target_project)
-          new_uploader.store!(file)
+          with_link_in_tmp_dir(file.file) do |open_tmp_file|
+            new_uploader.store!(open_tmp_file)
+          end
           new_uploader.to_markdown
         end
       end
@@ -46,6 +50,19 @@ module Gitlab
         uploader.retrieve_from_store!(file)
         uploader.file
       end
+
+      # Because the uploaders use 'move_to_store' we must have a temporary
+      # file that is allowed to be (re)moved.
+      def with_link_in_tmp_dir(file)
+        dir = Dir.mktmpdir('UploadsRewriter', File.dirname(file))
+        # The filename matters to Carrierwave so we make sure to preserve it
+        tmp_file = File.join(dir, File.basename(file))
+        File.link(file, tmp_file)
+        # Open the file to placate Carrierwave
+        File.open(tmp_file) { |open_file| yield open_file }
+      ensure
+        FileUtils.rm_rf(dir)
+      end
     end
   end
 end

lib/gitlab/middleware/multipart.rb

+# Gitlab::Middleware::Multipart - a Rack::Multipart replacement
+#
+# Rack::Multipart leaves behind tempfiles in /tmp and uses valuable Ruby
+# process time to copy files around. This alternative solution uses
+# gitlab-workhorse to clean up the tempfiles and puts the tempfiles in a
+# location where copying should not be needed.
+#
+# When gitlab-workhorse finds files in a multipart MIME body it sends
+# a signed message via a request header. This message lists the names of
+# the multipart entries that gitlab-workhorse filtered out of the
+# multipart structure and saved to tempfiles. Workhorse adds new entries
+# in the multipart structure with paths to the tempfiles.
+#
+# The job of this Rack middleware is to detect and decode the message
+# from workhorse. If present, it walks the Rack 'params' hash for the
+# current request, opens the respective tempfiles, and inserts the open
+# Ruby File objects in the params hash where Rack::Multipart would have
+# put them. The goal is that application code deeper down can keep
+# working the way it did with Rack::Multipart without changes.
+#
+# CAVEAT: the code that modifies the params hash is a bit complex. It is
+# conceivable that certain Rack params structures will not be modified
+# correctly. We are not aware of such bugs at this time though.
+#
+
+module Gitlab
+  module Middleware
+    class Multipart
+      RACK_ENV_KEY = 'HTTP_GITLAB_WORKHORSE_MULTIPART_FIELDS'
+
+      class Handler
+        def initialize(env, message)
+          @request = Rack::Request.new(env)
+          @rewritten_fields = message['rewritten_fields']
+          @open_files = []
+        end
+
+        def with_open_files
+          @rewritten_fields.each do |field, tmp_path|
+            parsed_field = Rack::Utils.parse_nested_query(field)
+            raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
+
+            key, value = parsed_field.first
+            if value.nil?
+              value = File.open(tmp_path)
+              @open_files << value
+            else
+              value = decorate_params_value(value, @request.params[key], tmp_path)
+            end
+            @request.update_param(key, value)
+          end
+
+          yield
+        ensure
+          @open_files.each(&:close)
+        end
+
+        # This function calls itself recursively
+        def decorate_params_value(path_hash, value_hash, tmp_path)
+          unless path_hash.is_a?(Hash) && path_hash.count == 1
+            raise "invalid path: #{path_hash.inspect}"
+          end
+          path_key, path_value = path_hash.first
+
+          unless value_hash.is_a?(Hash) && value_hash[path_key]
+            raise "invalid value hash: #{value_hash.inspect}"
+          end
+
+          case path_value
+          when nil
+            value_hash[path_key] = File.open(tmp_path)
+            @open_files << value_hash[path_key]
+            value_hash
+          when Hash
+            decorate_params_value(path_value, value_hash[path_key], tmp_path)
+            value_hash
+          else
+            raise "unexpected path value: #{path_value.inspect}"
+          end
+        end
+      end
+
+      def initialize(app)
+        @app = app
+      end
+
+      def call(env)
+        encoded_message = env.delete(RACK_ENV_KEY)
+        return @app.call(env) if encoded_message.blank?
+
+        message = Gitlab::Workhorse.decode_jwt(encoded_message)[0]
+
+        Handler.new(env, message).with_open_files do
+          @app.call(env)
+        end
+      end
+    end
+  end
+end

lib/gitlab/workhorse.rb

@@ -117,8 +117,12 @@ module Gitlab
       end
 
       def verify_api_request!(request_headers)
+        decode_jwt(request_headers[INTERNAL_API_REQUEST_HEADER])
+      end
+
+      def decode_jwt(encoded_message)
         JWT.decode(
-          request_headers[INTERNAL_API_REQUEST_HEADER],
+          encoded_message,
           secret,
           true,
           { iss: 'gitlab-workhorse', verify_iss: true, algorithm: 'HS256' },

spec/helpers/application_helper_spec.rb

 require 'spec_helper'
 
 describe ApplicationHelper do
+  include UploadHelpers
+
   describe 'current_controller?' do
     it 'returns true when controller matches argument' do
       stub_controller_name('foo')
@@ -52,10 +54,8 @@ describe ApplicationHelper do
   end
 
   describe 'project_icon' do
-    let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') }
-
     it 'returns an url for the avatar' do
-      project = create(:project, avatar: File.open(avatar_file_path))
+      project = create(:project, avatar: File.open(uploaded_image_temp_path))
 
       avatar_url = "http://#{Gitlab.config.gitlab.host}/uploads/project/avatar/#{project.id}/banana_sample.gif"
       expect(helper.project_icon("#{project.namespace.to_param}/#{project.to_param}").to_s).
@@ -74,10 +74,8 @@ describe ApplicationHelper do
   end
 
   describe 'avatar_icon' do
-    let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') }
-
     it 'returns an url for the avatar' do
-      user = create(:user, avatar: File.open(avatar_file_path))
+      user = create(:user, avatar: File.open(uploaded_image_temp_path))
 
       expect(helper.avatar_icon(user.email).to_s).
         to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")
@@ -88,7 +86,7 @@ describe ApplicationHelper do
       # Must be stubbed after the stub above, and separately
       stub_config_setting(url: Settings.send(:build_gitlab_url))
 
-      user = create(:user, avatar: File.open(avatar_file_path))
+      user = create(:user, avatar: File.open(uploaded_image_temp_path))
 
       expect(helper.avatar_icon(user.email).to_s).
         to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif")
@@ -102,7 +100,7 @@ describe ApplicationHelper do
 
     describe 'using a User' do
       it 'returns an URL for the avatar' do
-        user = create(:user, avatar: File.open(avatar_file_path))
+        user = create(:user, avatar: File.open(uploaded_image_temp_path))
 
         expect(helper.avatar_icon(user).to_s).
           to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")

spec/lib/gitlab/import_export/avatar_restorer_spec.rb

 require 'spec_helper'
 
 describe Gitlab::ImportExport::AvatarRestorer, lib: true do
+  include UploadHelpers
+
   let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: 'test') }
   let(:project) { create(:empty_project) }
 
   before do
     allow_any_instance_of(described_class).to receive(:avatar_export_file)
-                                                .and_return(Rails.root + "spec/fixtures/dk.png")
+                                                .and_return(uploaded_image_temp_path)
   end
 
   after do

spec/lib/gitlab/middleware/multipart_spec.rb

+require 'spec_helper'
+
+require 'tempfile'
+
+describe Gitlab::Middleware::Multipart do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+
+  it 'opens top-level files' do
+    Tempfile.open do |tempfile|
+      env = post_env({ 'file' => tempfile.path }, { 'file.name' => 'filename' }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+      expect(app).to receive(:call) do |env|
+        file = Rack::Request.new(env).params['file']
+        expect(file).to be_a(File)
+        expect(file.path).to eq(tempfile.path)
+      end
+
+      middleware.call(env)
+    end
+  end
+
+  it 'rejects headers signed with the wrong secret' do
+    env = post_env({ 'file' => '/var/empty/nonesuch' }, {}, 'x' * 32, 'gitlab-workhorse')
+
+    expect { middleware.call(env) }.to raise_error(JWT::VerificationError)
+  end
+
+  it 'rejects headers signed with the wrong issuer' do
+    env = post_env({ 'file' => '/var/empty/nonesuch' }, {}, Gitlab::Workhorse.secret, 'acme-inc')
+
+    expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError)
+  end
+
+  it 'opens files one level deep' do
+    Tempfile.open do |tempfile|
+      in_params = { 'user' => { 'avatar' => { '.name' => 'filename' } } }
+      env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+      expect(app).to receive(:call) do |env|
+        file = Rack::Request.new(env).params['user']['avatar']
+        expect(file).to be_a(File)
+        expect(file.path).to eq(tempfile.path)
+      end
+
+      middleware.call(env)
+    end
+  end
+
+  it 'opens files two levels deep' do
+    Tempfile.open do |tempfile|
+      in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => 'filename' } } } }
+      env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+      expect(app).to receive(:call) do |env|
+        file = Rack::Request.new(env).params['project']['milestone']['themesong']
+        expect(file).to be_a(File)
+        expect(file.path).to eq(tempfile.path)
+      end
+
+      middleware.call(env)
+    end
+  end
+
+  def post_env(rewritten_fields, params, secret, issuer)
+    token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256')
+    Rack::MockRequest.env_for(
+      '/',
+      method: 'post',
+      params: params,
+      described_class::RACK_ENV_KEY => token
+    )
+  end
+end

spec/requests/api/groups_spec.rb

@@ -2,13 +2,13 @@ require 'spec_helper'
 
 describe API::Groups, api: true  do
   include ApiHelpers
+  include UploadHelpers
 
   let(:user1) { create(:user, can_create_group: false) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
   let(:admin) { create(:admin) }
-  let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') }
-  let!(:group1) { create(:group, avatar: File.open(avatar_file_path)) }
+  let!(:group1) { create(:group, avatar: File.open(uploaded_image_temp_path)) }
   let!(:group2) { create(:group, :private) }
   let!(:project1) { create(:project, namespace: group1) }
   let!(:project2) { create(:project, namespace: group2) }

spec/support/upload_helpers.rb

+require 'fileutils'
+
+module UploadHelpers
+  extend self
+
+  def uploaded_image_temp_path
+    basename = 'banana_sample.gif'
+    orig_path = File.join(Rails.root, 'spec', 'fixtures', basename)
+    tmp_path = File.join(Rails.root, 'tmp', 'tests', basename)
+    # Because we use 'move_to_store' on all uploaders, we create a new
+    # tempfile on each call: the file we return here will be renamed in most
+    # cases.
+    FileUtils.copy(orig_path, tmp_path)
+    tmp_path
+  end
+end

spec/uploaders/attachment_uploader_spec.rb

+require 'spec_helper'
+
+describe AttachmentUploader do
+  let(:issue) { build(:issue) }
+  subject { described_class.new(issue) }
+
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(subject.move_to_cache).to eq(true)
+    end
+  end
+
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(subject.move_to_store).to eq(true)
+    end
+  end
+end

spec/uploaders/avatar_uploader_spec.rb

+require 'spec_helper'
+
+describe AvatarUploader do
+  let(:user) { build(:user) }
+  subject { described_class.new(user) }
+
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(subject.move_to_cache).to eq(true)
+    end
+  end
+
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(subject.move_to_store).to eq(true)
+    end
+  end
+end

spec/uploaders/file_uploader_spec.rb

@@ -42,4 +42,16 @@ describe FileUploader do
       expect(@uploader.image_or_video?).to be false
     end
   end
+
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(@uploader.move_to_cache).to eq(true)
+    end
+  end
+
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(@uploader.move_to_store).to eq(true)
+    end
+  end
 end