Merge branch '213289-remove-uploaded-file-call-from-lfs-storage-controller' into 'master'

Remove UploadedFile.from_params in Repositories::LfsStorageController

See merge request gitlab-org/gitlab!38167

app/controllers/repositories/lfs_storage_controller.rb

@@ -73,9 +73,8 @@ module Repositories
     # rubocop: enable CodeReuse/ActiveRecord
 
     def create_file!(oid, size)
-      uploaded_file = UploadedFile.from_params(
-        params, :file, LfsObjectUploader.workhorse_local_upload_path)
-      return unless uploaded_file
+      uploaded_file = params[:file]
+      return unless uploaded_file.is_a?(UploadedFile)
 
       LfsObject.create!(oid: oid, size: size, file: uploaded_file)
     end

changelogs/unreleased/213289-remove-uploaded-file-call-from-lfs-storage-controller.yml

+---
+title: Use the uploaded file set by middleware in Repositories::LfsStorageController
+merge_request: 38167
+author:
+type: changed

spec/controllers/repositories/lfs_storage_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Repositories::LfsStorageController do
+  using RSpec::Parameterized::TableSyntax
+  include GitHttpHelpers
+
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:pat) { create(:personal_access_token, user: user, scopes: ['write_repository']) }
+
+  let(:lfs_enabled) { true }
+
+  before do
+    stub_config(lfs: { enabled: lfs_enabled })
+  end
+
+  describe 'PUT #upload_finalize' do
+    let(:headers) { workhorse_internal_api_request_header }
+    let(:extra_headers) { {} }
+    let(:uploaded_file) { temp_file }
+
+    let(:params) do
+      {
+        namespace_id: project.namespace.path,
+        repository_id: "#{project.path}.git",
+        oid: '6b9765d3888aaec789e8c309eb05b05c3a87895d6ad70d2264bd7270fff665ac',
+        size: '6725030'
+      }
+    end
+
+    before do
+      request.headers.merge!(extra_headers)
+      request.headers.merge!(headers)
+
+      if uploaded_file
+        allow_next_instance_of(ActionController::Parameters) do |params|
+          allow(params).to receive(:[]).and_call_original
+          allow(params).to receive(:[]).with(:file).and_return(uploaded_file)
+        end
+      end
+    end
+
+    after do
+      FileUtils.rm_r(temp_file) if temp_file
+    end
+
+    subject do
+      put :upload_finalize, params: params
+    end
+
+    context 'with lfs enabled' do
+      context 'with unauthorized roles' do
+        where(:user_role, :expected_status) do
+          :guest     | :forbidden
+          :anonymous | :unauthorized
+        end
+
+        with_them do
+          let(:extra_headers) do
+            if user_role == :anonymous
+              {}
+            else
+              { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user.username, pat.token) }
+            end
+          end
+
+          before do
+            project.send("add_#{user_role}", user) unless user_role == :anonymous
+          end
+
+          it_behaves_like 'returning response status', params[:expected_status]
+        end
+      end
+
+      context 'with at least developer role' do
+        let(:extra_headers) { { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user.username, pat.token) } }
+
+        before do
+          project.add_developer(user)
+        end
+
+        it 'creates the objects' do
+          expect { subject }
+            .to change { LfsObject.count }.by(1)
+            .and change { LfsObjectsProject.count }.by(1)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        context 'without the workhorse header' do
+          let(:headers) { {} }
+
+          it { expect { subject }.to raise_error(JWT::DecodeError) }
+        end
+
+        context 'without file' do
+          let(:uploaded_file) { nil }
+
+          it_behaves_like 'returning response status', :unprocessable_entity
+        end
+
+        context 'with an invalid file' do
+          let(:uploaded_file) { 'test' }
+
+          it_behaves_like 'returning response status', :unprocessable_entity
+        end
+
+        context 'when an expected error' do
+          [
+            ActiveRecord::RecordInvalid,
+            UploadedFile::InvalidPathError,
+            ObjectStorage::RemoteStoreError
+          ].each do |exception_class|
+            context "#{exception_class} raised" do
+              it 'renders lfs forbidden' do
+                expect(LfsObjectsProject).to receive(:safe_find_or_create_by!).and_raise(exception_class)
+
+                subject
+
+                expect(response).to have_gitlab_http_status(:forbidden)
+                expect(json_response['documentation_url']).to be_present
+                expect(json_response['message']).to eq('Access forbidden. Check your access level.')
+              end
+            end
+          end
+        end
+
+        context 'when file is not stored' do
+          it 'renders unprocessable entity' do
+            expect(controller).to receive(:store_file!).and_return(nil)
+
+            subject
+
+            expect(response).to have_gitlab_http_status(:unprocessable_entity)
+            expect(response.body).to eq('Unprocessable entity')
+          end
+        end
+      end
+    end
+
+    context 'with lfs disabled' do
+      let(:lfs_enabled) { false }
+      let(:extra_headers) { { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user.username, pat.token) } }
+
+      it_behaves_like 'returning response status', :not_implemented
+    end
+
+    def temp_file
+      upload_path = LfsObjectUploader.workhorse_local_upload_path
+      file_path = "#{upload_path}/lfs"
+
+      FileUtils.mkdir_p(upload_path)
+      File.write(file_path, 'test')
+
+      UploadedFile.new(file_path, filename: File.basename(file_path))
+    end
+  end
+end

spec/requests/lfs_http_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe 'Git LFS API and storage' do
       'X-Sendfile-Type' => sendfile
     }.compact
   end
+  let(:include_workhorse_jwt_header) { true }
   let(:authorization) { }
   let(:sendfile) { }
   let(:pipeline) { create(:ci_empty_pipeline, project: project) }
@@ -1075,14 +1076,24 @@ RSpec.describe 'Git LFS API and storage' do
             end
           end
 
-          context 'invalid tempfiles' do
+          context 'without the lfs object' do
             before do
               lfs_object.destroy
             end
 
             it 'rejects slashes in the tempfile name (path traversal)' do
               put_finalize('../bar', with_tempfile: true)
-              expect(response).to have_gitlab_http_status(:forbidden)
+              expect(response).to have_gitlab_http_status(:bad_request)
+            end
+
+            context 'not sending the workhorse jwt header' do
+              let(:include_workhorse_jwt_header) { false }
+
+              it 'rejects the request' do
+                put_finalize(with_tempfile: true)
+
+                expect(response).to have_gitlab_http_status(:unprocessable_entity)
+              end
             end
           end
         end
@@ -1308,7 +1319,8 @@ RSpec.describe 'Git LFS API and storage' do
         method: :put,
         file_key: :file,
         params: args.merge(file: uploaded_file),
-        headers: finalize_headers
+        headers: finalize_headers,
+        send_rewritten_field: include_workhorse_jwt_header
       )
     end