Merge branch 'jprovazn-direct-upload' into 'master'

Add workhorse authorize method for project/group uploads

Closes #44663

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

app/controllers/concerns/uploads_actions.rb

@@ -45,6 +45,16 @@ module UploadsActions
     send_upload(uploader, attachment: uploader.filename, disposition: disposition)
   end
 
+  def authorize
+    set_workhorse_internal_api_content_type
+
+    authorized = uploader_class.workhorse_authorize(
+      has_length: false,
+      maximum_size: Gitlab::CurrentSettings.max_attachment_size.megabytes.to_i)
+
+    render json: authorized
+  end
+
   private
 
   # Explicitly set the format.

app/controllers/groups/uploads_controller.rb

 class Groups::UploadsController < Groups::ApplicationController
   include UploadsActions
+  include WorkhorseRequest
 
   skip_before_action :group, if: -> { action_name == 'show' && image_or_video? }
 
-  before_action :authorize_upload_file!, only: [:create]
+  before_action :authorize_upload_file!, only: [:create, :authorize]
+  before_action :verify_workhorse_api!, only: [:authorize]
 
   private
 

app/controllers/projects/uploads_controller.rb

 class Projects::UploadsController < Projects::ApplicationController
   include UploadsActions
+  include WorkhorseRequest
 
   # These will kick you out if you don't have access.
   skip_before_action :project, :repository,
     if: -> { action_name == 'show' && image_or_video? }
 
-  before_action :authorize_upload_file!, only: [:create]
+  before_action :authorize_upload_file!, only: [:create, :authorize]
+  before_action :verify_workhorse_api!, only: [:authorize]
 
   private
 

changelogs/unreleased/jprovazn-direct-upload.yml

+---
+title: Support direct_upload for generic uploads
+merge_request:
+author:
+type: added

config/routes/group.rb

@@ -55,6 +55,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     resources :uploads, only: [:create] do
       collection do
         get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+        post :authorize
       end
     end
 

config/routes/project.rb

@@ -406,6 +406,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       resources :uploads, only: [:create] do
         collection do
           get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+          post :authorize
         end
       end
 

doc/administration/uploads.md

@@ -52,6 +52,7 @@ _The uploads are stored by default in
 
 >**Notes:**
 - [Introduced][ee-3867] in [GitLab Enterprise Edition Premium][eep] 10.5.
+- Since version 11.1, we support direct_upload to S3.
 
 If you don't want to use the local disk where GitLab is installed to store the
 uploads, you can use an object storage provider like AWS S3 instead.
@@ -65,7 +66,7 @@ For source installations the following settings are nested under `uploads:` and
 |---------|-------------|---------|
 | `enabled` | Enable/disable object storage | `false` |
 | `remote_directory` | The bucket name where Uploads will be stored| |
-| `direct_upload` | Set to true to enable direct upload of Uploads without the need of local shared storage. Option may be removed once we decide to support only single storage for all files. This is beta option as it uses inefficient way of uploading data (via Unicorn). The accelerated uploads gonna be implemented in future releases | `false` |
+| `direct_upload` | Set to true to enable direct upload of Uploads without the need of local shared storage. Option may be removed once we decide to support only single storage for all files. | `false` |
 | `background_upload` | Set to false to disable automatic upload. Option may be removed once upload is direct to S3 | `true` |
 | `proxy_download` | Set to true to enable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
 | `connection` | Various connection options described below | |

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
 

spec/controllers/groups/uploads_controller_spec.rb

 require 'spec_helper'
 
 describe Groups::UploadsController do
+  include WorkhorseHelpers
+
   let(:model) { create(:group, :public) }
   let(:params) do
     { group_id: model }
@@ -9,4 +11,10 @@ describe Groups::UploadsController do
   it_behaves_like 'handle uploads' do
     let(:uploader_class) { NamespaceFileUploader }
   end
+
+  def post_authorize(verified: true)
+    request.headers.merge!(workhorse_internal_api_request_header) if verified
+
+    post :authorize, group_id: model.full_path, format: :json
+  end
 end

spec/controllers/projects/uploads_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::UploadsController do
+  include WorkhorseHelpers
+
   let(:model) { create(:project, :public) }
   let(:params) do
     { namespace_id: model.namespace.to_param, project_id: model }
@@ -15,4 +17,10 @@ describe Projects::UploadsController do
       expect(response).to redirect_to(new_user_session_path)
     end
   end
+
+  def post_authorize(verified: true)
+    request.headers.merge!(workhorse_internal_api_request_header) if verified
+
+    post :authorize, namespace_id: model.namespace, project_id: model.path, format: :json
+  end
 end

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)

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -260,4 +260,83 @@ shared_examples 'handle uploads' do
       end
     end
   end
+
+  describe "POST #authorize" do
+    context 'when a user is not authorized to upload a file' do
+      it 'returns 404 status' do
+        post_authorize
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context 'when a user can upload a file' do
+      before do
+        sign_in(user)
+        model.add_developer(user)
+      end
+
+      context 'and the request bypassed workhorse' do
+        it 'raises an exception' do
+          expect { post_authorize(verified: false) }.to raise_error JWT::DecodeError
+        end
+      end
+
+      context 'and request is sent by gitlab-workhorse to authorize the request' do
+        shared_examples 'a valid response' do
+          before do
+            post_authorize
+          end
+
+          it 'responds with status 200' do
+            expect(response).to have_gitlab_http_status(200)
+          end
+
+          it 'uses the gitlab-workhorse content type' do
+            expect(response.headers["Content-Type"]).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+          end
+        end
+
+        shared_examples 'a local file' do
+          it_behaves_like 'a valid response' do
+            it 'responds with status 200, location of uploads store and object details' do
+              expect(json_response['TempPath']).to eq(uploader_class.workhorse_local_upload_path)
+              expect(json_response['RemoteObject']).to be_nil
+            end
+          end
+        end
+
+        context 'when using local storage' do
+          it_behaves_like 'a local file'
+        end
+
+        context 'when using remote storage' do
+          context 'when direct upload is enabled' do
+            before do
+              stub_uploads_object_storage(uploader_class, direct_upload: true)
+            end
+
+            it_behaves_like 'a valid response' do
+              it 'responds with status 200, location of uploads remote store and object details' do
+                expect(json_response['TempPath']).to eq(uploader_class.workhorse_local_upload_path)
+                expect(json_response['RemoteObject']).to have_key('ID')
+                expect(json_response['RemoteObject']).to have_key('GetURL')
+                expect(json_response['RemoteObject']).to have_key('StoreURL')
+                expect(json_response['RemoteObject']).to have_key('DeleteURL')
+                expect(json_response['RemoteObject']).to have_key('MultipartUpload')
+              end
+            end
+          end
+
+          context 'when direct upload is disabled' do
+            before do
+              stub_uploads_object_storage(uploader_class, direct_upload: false)
+            end
+
+            it_behaves_like 'a local file'
+          end
+        end
+      end
+    end
+  end
 end