Merge branch '3232-lfs-files-pull-from-secondary' into 'master'

Geo - Whitelist LFS requests to download objects on a secondary node

Closes #3232

See merge request !2758

app/controllers/projects/lfs_api_controller.rb

 class Projects::LfsApiController < Projects::GitHttpClientController
+  include ApplicationSettingsHelper
+  include ApplicationHelper
+  include GitlabRoutingHelper
   include LfsRequest
 
   skip_before_action :lfs_check_access!, only: [:deprecated]
+  before_action :lfs_check_batch_operation!, only: [:batch]
 
   def batch
     unless objects.present?
@@ -90,4 +94,16 @@ class Projects::LfsApiController < Projects::GitHttpClientController
       }
     }
   end
+
+  def lfs_check_batch_operation!
+    if upload_request? && Gitlab::Geo.secondary?
+      render(
+        json: {
+          message: "You cannot write to a secondary GitLab Geo instance. Please use #{geo_primary_default_url_to_repo(project)} instead."
+        },
+        content_type: "application/vnd.git-lfs+json",
+        status: 403
+      )
+    end
+  end
 end

changelogs/unreleased-ee/3232-lfs-files-pull-from-secondary.yml

+---
+title: Geo - Whitelist LFS requests to download objects on a secondary node
+merge_request: 2758
+author:
+type: fixed

lib/gitlab/middleware/readonly_geo.rb

@@ -65,7 +65,7 @@ module Gitlab
       end
 
       def whitelisted_routes
-        logout_route || grack_route || @whitelisted.any? { |path| request.path.include?(path) } || sidekiq_route
+        logout_route || grack_route || @whitelisted.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route
       end
 
       def logout_route
@@ -79,6 +79,10 @@ module Gitlab
       def grack_route
         request.path.end_with?('.git/git-upload-pack')
       end
+
+      def lfs_route
+        request.path.end_with?('/info/lfs/objects/batch')
+      end
     end
   end
 end

spec/lib/gitlab/middleware/readonly_geo_spec.rb

@@ -35,6 +35,7 @@ describe Gitlab::Middleware::ReadonlyGeo do
   end
 
   subject { described_class.new(fake_app) }
+
   let(:request) { Rack::MockRequest.new(rack_stack) }
 
   context 'normal requests to a secondary Gitlab Geo' do
@@ -103,6 +104,13 @@ describe Gitlab::Middleware::ReadonlyGeo do
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
       end
+
+      it 'expects a POST LFS request to batch URL to be allowed' do
+        response = request.post('/root/rouge.git/info/lfs/objects/batch')
+
+        expect(response).not_to be_a_redirect
+        expect(subject).not_to disallow_request
+      end
     end
   end
 

spec/requests/lfs_http_spec.rb

@@ -40,7 +40,7 @@ describe 'Git LFS API and storage' do
     end
 
     it 'responds with 501' do
-      expect(response).to have_http_status(501)
+      expect(response).to have_gitlab_http_status(501)
       expect(json_response).to include('message' => 'Git LFS is not enabled on this GitLab server, contact your admin.')
     end
   end
@@ -74,13 +74,13 @@ describe 'Git LFS API and storage' do
         it 'responds with a 501 message on upload' do
           post_lfs_json "#{project.http_url_to_repo}/info/lfs/objects/batch", body, headers
 
-          expect(response).to have_http_status(501)
+          expect(response).to have_gitlab_http_status(501)
         end
 
         it 'responds with a 501 message on download' do
           get "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}", nil, headers
 
-          expect(response).to have_http_status(501)
+          expect(response).to have_gitlab_http_status(501)
         end
       end
 
@@ -92,13 +92,13 @@ describe 'Git LFS API and storage' do
         it 'responds with a 501 message on upload' do
           post_lfs_json "#{project.http_url_to_repo}/info/lfs/objects/batch", body, headers
 
-          expect(response).to have_http_status(501)
+          expect(response).to have_gitlab_http_status(501)
         end
 
         it 'responds with a 501 message on download' do
           get "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}", nil, headers
 
-          expect(response).to have_http_status(501)
+          expect(response).to have_gitlab_http_status(501)
         end
       end
     end
@@ -117,14 +117,14 @@ describe 'Git LFS API and storage' do
         it 'responds with a 403 message on upload' do
           post_lfs_json "#{project.http_url_to_repo}/info/lfs/objects/batch", body, headers
 
-          expect(response).to have_http_status(403)
+          expect(response).to have_gitlab_http_status(403)
           expect(json_response).to include('message' => 'Access forbidden. Check your access level.')
         end
 
         it 'responds with a 403 message on download' do
           get "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}", nil, headers
 
-          expect(response).to have_http_status(403)
+          expect(response).to have_gitlab_http_status(403)
           expect(json_response).to include('message' => 'Access forbidden. Check your access level.')
         end
       end
@@ -137,14 +137,14 @@ describe 'Git LFS API and storage' do
         it 'responds with a 200 message on upload' do
           post_lfs_json "#{project.http_url_to_repo}/info/lfs/objects/batch", body, headers
 
-          expect(response).to have_http_status(200)
+          expect(response).to have_gitlab_http_status(200)
           expect(json_response['objects'].first['size']).to eq(1575078)
         end
 
         it 'responds with a 200 message on download' do
           get "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}", nil, headers
 
-          expect(response).to have_http_status(200)
+          expect(response).to have_gitlab_http_status(200)
         end
       end
     end
@@ -159,7 +159,7 @@ describe 'Git LFS API and storage' do
 
     shared_examples 'a deprecated' do
       it 'responds with 501' do
-        expect(response).to have_http_status(501)
+        expect(response).to have_gitlab_http_status(501)
       end
 
       it 'returns deprecated message' do
@@ -200,7 +200,7 @@ describe 'Git LFS API and storage' do
     context 'and request comes from gitlab-workhorse' do
       context 'without user being authorized' do
         it 'responds with status 401' do
-          expect(response).to have_http_status(401)
+          expect(response).to have_gitlab_http_status(401)
         end
       end
 
@@ -209,7 +209,7 @@ describe 'Git LFS API and storage' do
           let(:sendfile) { 'X-Sendfile' }
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'responds with the file location' do
@@ -227,7 +227,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 404' do
-              expect(response).to have_http_status(404)
+              expect(response).to have_gitlab_http_status(404)
             end
           end
 
@@ -271,7 +271,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 404' do
-              expect(response).to have_http_status(404)
+              expect(response).to have_gitlab_http_status(404)
             end
           end
         end
@@ -310,7 +310,7 @@ describe 'Git LFS API and storage' do
               end
 
               it 'rejects downloading code' do
-                expect(response).to have_http_status(other_project_status)
+                expect(response).to have_gitlab_http_status(other_project_status)
               end
             end
           end
@@ -350,7 +350,7 @@ describe 'Git LFS API and storage' do
         let(:authorization) { authorize_user }
 
         it 'responds with status 404' do
-          expect(response).to have_http_status(404)
+          expect(response).to have_gitlab_http_status(404)
         end
       end
     end
@@ -386,7 +386,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'with href to download' do
@@ -414,7 +414,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'with href to download' do
@@ -445,7 +445,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'with an 404 for specific object' do
@@ -482,7 +482,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'responds with upload hypermedia link for the new object' do
@@ -527,7 +527,7 @@ describe 'Git LFS API and storage' do
           let(:update_user_permissions) { nil }
 
           it 'responds with 404' do
-            expect(response).to have_http_status(404)
+            expect(response).to have_gitlab_http_status(404)
           end
         end
 
@@ -535,7 +535,7 @@ describe 'Git LFS API and storage' do
           let(:role) { :guest }
 
           it 'responds with 403' do
-            expect(response).to have_http_status(403)
+            expect(response).to have_gitlab_http_status(403)
           end
         end
       end
@@ -563,7 +563,7 @@ describe 'Git LFS API and storage' do
             let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
 
             it 'rejects downloading code' do
-              expect(response).to have_http_status(other_project_status)
+              expect(response).to have_gitlab_http_status(other_project_status)
             end
           end
         end
@@ -607,7 +607,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200 and href to download' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'responds with status 200 and href to download' do
@@ -635,7 +635,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with authorization required' do
-            expect(response).to have_http_status(401)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
       end
@@ -668,7 +668,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'responds with links the object to the project' do
@@ -697,7 +697,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'responds with upload hypermedia link' do
@@ -716,7 +716,7 @@ describe 'Git LFS API and storage' do
               end
 
               it 'responds with status 406' do
-                expect(response).to have_http_status(406)
+                expect(response).to have_gitlab_http_status(406)
               end
 
               it 'show correct error message' do
@@ -732,7 +732,7 @@ describe 'Git LFS API and storage' do
               end
 
               it 'responds with status 406' do
-                expect(response).to have_http_status(406)
+                expect(response).to have_gitlab_http_status(406)
                 expect(json_response['documentation_url']).to include('/help')
               end
 
@@ -760,7 +760,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'responds with upload hypermedia link for the new object' do
@@ -782,7 +782,7 @@ describe 'Git LFS API and storage' do
           let(:authorization) { authorize_user }
 
           it 'responds with 403' do
-            expect(response).to have_http_status(403)
+            expect(response).to have_gitlab_http_status(403)
           end
         end
 
@@ -796,7 +796,7 @@ describe 'Git LFS API and storage' do
               let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
               it 'responds with 403 (not 404 because project is public)' do
-                expect(response).to have_http_status(403)
+                expect(response).to have_gitlab_http_status(403)
               end
             end
 
@@ -807,7 +807,7 @@ describe 'Git LFS API and storage' do
 
               # I'm not sure what this tests that is different from the previous test
               it 'responds with 403 (not 404 because project is public)' do
-                expect(response).to have_http_status(403)
+                expect(response).to have_gitlab_http_status(403)
               end
             end
           end
@@ -816,7 +816,7 @@ describe 'Git LFS API and storage' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
             it 'responds with 403 (not 404 because project is public)' do
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
           end
         end
@@ -829,13 +829,13 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 401' do
-            expect(response).to have_http_status(401)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
 
         context 'when user does not have push access' do
           it 'responds with status 401' do
-            expect(response).to have_http_status(401)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
       end
@@ -855,11 +855,40 @@ describe 'Git LFS API and storage' do
       end
 
       it 'responds with status 404' do
-        expect(response).to have_http_status(404)
+        expect(response).to have_gitlab_http_status(404)
       end
     end
   end
 
+  describe 'when handling lfs batch request on a secondary Geo node' do
+    let!(:primary) { create(:geo_node, :primary) }
+    let(:authorization) { authorize_user }
+    let(:project) { create(:project) }
+    let(:path) { "#{project.http_url_to_repo}/info/lfs/objects/batch" }
+    let(:body) do
+      { 'objects' => [{ 'oid' => sample_oid, 'size' => sample_size }] }
+    end
+
+    before do
+      allow(Gitlab::Geo).to receive(:secondary?) { true }
+      project.team << [user, :master]
+      enable_lfs
+    end
+
+    it 'responds with a 200 message on download' do
+      post_lfs_json path, body.merge('operation' => 'download'), headers
+
+      expect(response).to have_gitlab_http_status(200)
+    end
+
+    it 'responds with a 403 message on upload' do
+      post_lfs_json path, body.merge('operation' => 'upload'), headers
+
+      expect(response).to have_gitlab_http_status(403)
+      expect(json_response).to include('message' => "You cannot write to a secondary GitLab Geo instance. Please use #{project.http_url_to_repo} instead.")
+    end
+  end
+
   describe 'when pushing a lfs object' do
     before do
       enable_lfs
@@ -872,7 +901,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'responds with status 401' do
-          expect(response).to have_http_status(401)
+          expect(response).to have_gitlab_http_status(401)
         end
       end
 
@@ -882,7 +911,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'responds with status 401' do
-          expect(response).to have_http_status(401)
+          expect(response).to have_gitlab_http_status(401)
         end
       end
 
@@ -892,7 +921,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'does not recognize it as a valid lfs command' do
-          expect(response).to have_http_status(401)
+          expect(response).to have_gitlab_http_status(401)
         end
       end
     end
@@ -904,7 +933,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'responds with 403' do
-          expect(response).to have_http_status(403)
+          expect(response).to have_gitlab_http_status(403)
         end
       end
 
@@ -914,7 +943,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'responds with 403' do
-          expect(response).to have_http_status(403)
+          expect(response).to have_gitlab_http_status(403)
         end
       end
 
@@ -924,7 +953,7 @@ describe 'Git LFS API and storage' do
         end
 
         it 'does not recognize it as a valid lfs command' do
-          expect(response).to have_http_status(403)
+          expect(response).to have_gitlab_http_status(403)
         end
       end
     end
@@ -952,7 +981,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'uses the gitlab-workhorse content type' do
@@ -972,7 +1001,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'lfs object is linked to the project' do
@@ -990,19 +1019,19 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
           end
 
           context 'invalid tempfiles' do
             it 'rejects slashes in the tempfile name (path traversal' do
               put_finalize('foo/bar')
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
 
             it 'rejects tempfile names that do not start with the oid' do
               put_finalize("foo#{sample_oid}")
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
           end
         end
@@ -1031,7 +1060,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with 403 (not 404 because the build user can read the project)' do
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
           end
 
@@ -1045,7 +1074,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with 404 (do not leak non-public project existence)' do
-              expect(response).to have_http_status(404)
+              expect(response).to have_gitlab_http_status(404)
             end
           end
         end
@@ -1058,7 +1087,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with 404 (do not leak non-public project existence)' do
-            expect(response).to have_http_status(404)
+            expect(response).to have_gitlab_http_status(404)
           end
         end
       end
@@ -1087,7 +1116,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'with location of lfs store and object details' do
@@ -1103,7 +1132,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'responds with status 200' do
-              expect(response).to have_http_status(200)
+              expect(response).to have_gitlab_http_status(200)
             end
 
             it 'lfs object is linked to the source project' do
@@ -1131,7 +1160,7 @@ describe 'Git LFS API and storage' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
             it 'responds with 403 (not 404 because project is public)' do
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
           end
 
@@ -1142,7 +1171,7 @@ describe 'Git LFS API and storage' do
 
             # I'm not sure what this tests that is different from the previous test
             it 'responds with 403 (not 404 because project is public)' do
-              expect(response).to have_http_status(403)
+              expect(response).to have_gitlab_http_status(403)
             end
           end
         end
@@ -1151,7 +1180,7 @@ describe 'Git LFS API and storage' do
           let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
           it 'responds with 403 (not 404 because project is public)' do
-            expect(response).to have_http_status(403)
+            expect(response).to have_gitlab_http_status(403)
           end
         end
       end
@@ -1176,7 +1205,7 @@ describe 'Git LFS API and storage' do
           end
 
           it 'responds with status 200' do
-            expect(response).to have_http_status(200)
+            expect(response).to have_gitlab_http_status(200)
           end
 
           it 'links the lfs object to the project' do