Merge branch '208744-fix-nuget-uploads-without-object-storage' into 'master'

Allow packages multipart uploads

See merge request gitlab-org/gitlab!26387

ee/changelogs/unreleased/208744-fix-nuget-uploads-without-object-storage.yml

+---
+title: Allow multipart uploads for packages
+merge_request: 26387
+author:
+type: fixed

ee/lib/ee/gitlab/middleware/multipart/handler.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Middleware
+      module Multipart
+        module Handler
+          extend ::Gitlab::Utils::Override
+
+          private
+
+          override :allowed_paths
+          def allowed_paths
+            paths = super
+            packages_config = ::Gitlab.config.packages
+            if allow_packages_storage_path?(packages_config)
+              paths << ::Packages::PackageFileUploader.workhorse_upload_path
+            end
+
+            paths
+          end
+
+          def allow_packages_storage_path?(packages_config)
+            return unless packages_config.enabled
+            return unless packages_config['storage_path']
+            return if packages_config.object_store.enabled && packages_config.object_store.direct_upload
+
+            true
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/middleware/multipart_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'tempfile'
+
+describe Gitlab::Middleware::Multipart do
+  include_context 'multipart middleware context'
+
+  describe '#call' do
+    context 'with packages storage' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:storage_path) { 'shared/packages' }
+
+      RSpec.shared_examples 'allowing the multipart upload' do
+        it 'allows files to be uploaded' do
+          with_tmp_dir('tmp/uploads', storage_path) do |dir, env|
+            allow(Packages::PackageFileUploader).to receive(:root).and_return(File.join(dir, storage_path))
+
+            expect(app).to receive(:call) do |env|
+              expect(get_params(env)['file']).to be_a(::UploadedFile)
+            end
+
+            middleware.call(env)
+          end
+        end
+      end
+
+      RSpec.shared_examples 'not allowing the multipart upload' do
+        it 'does not allow files to be uploaded' do
+          with_tmp_dir('tmp/uploads', storage_path) do |dir, env|
+            allow(Packages::PackageFileUploader).to receive(:root).and_return(File.join(dir, storage_path))
+
+            expect { middleware.call(env) }.to raise_error(UploadedFile::InvalidPathError)
+          end
+        end
+      end
+
+      RSpec.shared_examples 'adding package storage to multipart allowed paths' do
+        before do
+          expect(::Packages::PackageFileUploader).to receive(:workhorse_upload_path).and_call_original
+        end
+
+        it_behaves_like 'allowing the multipart upload'
+      end
+
+      RSpec.shared_examples 'not adding package storage to multipart allowed paths' do
+        before do
+          expect(::Packages::PackageFileUploader).not_to receive(:workhorse_upload_path)
+        end
+
+        it_behaves_like 'not allowing the multipart upload'
+      end
+
+      where(:object_storage_enabled, :direct_upload_enabled, :example_name) do
+        false | true  | 'adding package storage to multipart allowed paths'
+        false | false | 'adding package storage to multipart allowed paths'
+        true  | true  | 'not adding package storage to multipart allowed paths'
+        true  | false | 'adding package storage to multipart allowed paths'
+      end
+
+      with_them do
+        before do
+          stub_config(packages: {
+            enabled: true,
+            object_store: {
+              enabled: object_storage_enabled,
+              direct_upload: direct_upload_enabled
+            },
+            storage_path: storage_path
+          })
+        end
+
+        it_behaves_like params[:example_name]
+      end
+    end
+  end
+end

ee/spec/support/shared_examples/requests/api/nuget_packages_shared_examples.rb

@@ -164,6 +164,10 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
     end
 
     context 'with object storage disabled' do
+      before do
+        stub_package_file_object_storage(enabled: false)
+      end
+
       context 'without a file from workhorse' do
         let(:params) { { package: nil } }
 
@@ -178,18 +182,19 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
     end
 
     context 'with object storage enabled' do
+      let(:tmp_object) do
+        fog_connection.directories.new(key: 'packages').files.create(
+          key: "tmp/uploads/#{file_name}",
+          body: 'content'
+        )
+      end
+      let(:fog_file) { fog_to_uploaded_file(tmp_object) }
+      let(:params) { { package: fog_file, 'package.remote_id' => file_name } }
+
       context 'and direct upload enabled' do
-        let!(:fog_connection) do
+        let(:fog_connection) do
           stub_package_file_object_storage(direct_upload: true)
         end
-        let(:tmp_object) do
-          fog_connection.directories.new(key: 'packages').files.create(
-            key: "tmp/uploads/#{file_name}",
-            body: 'content'
-          )
-        end
-        let(:fog_file) { fog_to_uploaded_file(tmp_object) }
-        let(:params) { { package: fog_file, 'package.remote_id' => file_name } }
 
         it_behaves_like 'creates nuget package files'
 
@@ -207,8 +212,26 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
         end
       end
 
-      it_behaves_like 'background upload schedules a file migration'
+      context 'and direct upload disabled' do
+        context 'and background upload disabled' do
+          let(:fog_connection) do
+            stub_package_file_object_storage(direct_upload: false, background_upload: false)
+          end
+
+          it_behaves_like 'creates nuget package files'
+        end
+
+        context 'and background upload enabled' do
+          let(:fog_connection) do
+            stub_package_file_object_storage(direct_upload: false, background_upload: true)
+          end
+
+          it_behaves_like 'creates nuget package files'
+        end
+      end
     end
+
+    it_behaves_like 'background upload schedules a file migration'
   end
 end
 

lib/gitlab/middleware/multipart.rb

@@ -84,12 +84,6 @@ module Gitlab
         end
 
         def open_file(params, key)
-          allowed_paths = [
-            ::FileUploader.root,
-            Gitlab.config.uploads.storage_path,
-            File.join(Rails.root, 'public/uploads/tmp')
-          ]
-
           ::UploadedFile.from_params(params, key, allowed_paths)
         end
 
@@ -106,6 +100,16 @@ module Gitlab
           # inside other env keys, here we ensure everything is updated correctly
           ActionDispatch::Request.new(@request.env).update_param(key, value)
         end
+
+        private
+
+        def allowed_paths
+          [
+            ::FileUploader.root,
+            Gitlab.config.uploads.storage_path,
+            File.join(Rails.root, 'public/uploads/tmp')
+          ]
+        end
       end
 
       def initialize(app)
@@ -125,3 +129,5 @@ module Gitlab
     end
   end
 end
+
+::Gitlab::Middleware::Multipart::Handler.prepend_if_ee('EE::Gitlab::Middleware::Multipart::Handler')

spec/lib/gitlab/middleware/multipart_spec.rb

@@ -5,9 +5,7 @@ require 'spec_helper'
 require 'tempfile'
 
 describe Gitlab::Middleware::Multipart do
-  let(:app) { double(:app) }
-  let(:middleware) { described_class.new(app) }
-  let(:original_filename) { 'filename' }
+  include_context 'multipart middleware context'
 
   shared_examples_for 'multipart upload files' do
     it 'opens top-level files' do
@@ -82,22 +80,12 @@ describe Gitlab::Middleware::Multipart do
   end
 
   it 'allows files in uploads/tmp directory' do
-    Dir.mktmpdir do |dir|
-      uploads_dir = File.join(dir, 'public/uploads/tmp')
-      FileUtils.mkdir_p(uploads_dir)
-
-      allow(Rails).to receive(:root).and_return(dir)
-      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
-
-      Tempfile.open('top-level', uploads_dir) do |tempfile|
-        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
-
-        expect(app).to receive(:call) do |env|
-          expect(get_params(env)['file']).to be_a(::UploadedFile)
-        end
-
-        middleware.call(env)
+    with_tmp_dir('public/uploads/tmp') do |dir, env|
+      expect(app).to receive(:call) do |env|
+        expect(get_params(env)['file']).to be_a(::UploadedFile)
       end
+
+      middleware.call(env)
     end
   end
 
@@ -127,22 +115,4 @@ describe Gitlab::Middleware::Multipart do
       middleware.call(env)
     end
   end
-
-  # Rails 5 doesn't combine the GET/POST parameters in
-  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
-  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
-  def get_params(env)
-    req = ActionDispatch::Request.new(env)
-    req.GET.merge(req.POST)
-  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/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'multipart middleware context' do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:original_filename) { 'filename' }
+
+  # Rails 5 doesn't combine the GET/POST parameters in
+  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
+  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
+  def get_params(env)
+    req = ActionDispatch::Request.new(env)
+    req.GET.merge(req.POST)
+  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
+
+  def with_tmp_dir(uploads_sub_dir, storage_path = '')
+    Dir.mktmpdir do |dir|
+      upload_dir = File.join(dir, storage_path, uploads_sub_dir)
+      FileUtils.mkdir_p(upload_dir)
+
+      allow(Rails).to receive(:root).and_return(dir)
+      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
+      allow(GitlabUploader).to receive(:root).and_return(File.join(dir, storage_path))
+
+      Tempfile.open('top-level', upload_dir) do |tempfile|
+        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+        yield dir, env
+      end
+    end
+  end
+end