Merge branch 'brodock/geo-self-service-framework-uploader' into 'master'

Geo: Self-Service Framework (Uploader / API)

See merge request gitlab-org/gitlab!25679

ee/app/models/concerns/geo/blob_replicator_strategy.rb

@@ -27,6 +27,10 @@ module Geo
       download
     end
 
+    # Return the carrierwave uploader instance scoped to current model
+    #
+    # @abstract
+    # @return [Carrierwave::Uploader]
     def carrierwave_uploader
       raise NotImplementedError
     end
@@ -39,6 +43,14 @@ module Geo
       update_verification_state!(failure: e.message)
     end
 
+    # Check if given checksum matches known one
+    #
+    # @param [String] checksum
+    # @return [Boolean] whether checksum matches
+    def matches_checksum?(checksum)
+      model_record.verification_checksum == checksum
+    end
+
     private
 
     def update_verification_state!(checksum: nil, failure: nil)

ee/app/services/geo/blob_upload_service.rb

+# frozen_string_literal: true
+
+module Geo
+  class BlobUploadService
+    include ExclusiveLeaseGuard
+    include ::Gitlab::Geo::LogHelpers
+
+    attr_reader :replicable_name, :blob_id, :checksum, :replicator
+
+    def initialize(replicable_name:, blob_id:, decoded_params:)
+      @replicable_name = replicable_name
+      @blob_id = blob_id
+      @checksum = decoded_params.delete(:checksum)
+
+      replicator_klass = Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
+      @replicator = replicator_klass.new(model_record_id: blob_id)
+
+      fail_unimplemented!(replicable_name) unless @replicator
+    end
+
+    def execute
+      retriever.execute
+    end
+
+    def retriever
+      Gitlab::Geo::Replication::BlobRetriever.new(replicator: replicator, checksum: checksum)
+    end
+
+    private
+
+    def fail_unimplemented!(replicable_name)
+      error_message = "Cannot find a Geo::Replicator for #{replicable_name}"
+
+      log_error(error_message)
+
+      raise NotImplementedError, error_message
+    end
+
+    # This is called by LogHelpers to build json log with context info
+    #
+    # @see ::Gitlab::Geo::LogHelpers
+    def extra_log_data
+      {
+        replicable_name: replicable_name,
+        blob_id: blob_id
+      }.compact
+    end
+  end
+end

ee/app/services/geo/file_upload_service.rb

@@ -5,37 +5,25 @@ module Geo
   #   * Handling file requests from the secondary over the API
   #   * Returning the necessary response data to send the file back
   class FileUploadService < BaseFileService
-    attr_reader :auth_header
-    include ::Gitlab::Utils::StrongMemoize
+    attr_reader :decoded_params
 
-    def initialize(params, auth_header)
+    def initialize(params, decoded_params)
       super(params[:type], params[:id])
-      @auth_header = auth_header
+
+      @decoded_params = decoded_params
     end
 
     # Returns { code: :ok, file: CarrierWave File object } upon success
     def execute
-      return unless decoded_authorization.present? && jwt_scope_valid?
-
       retriever.execute
     end
 
     def retriever
-      retriever_klass.new(object_db_id, decoded_authorization)
+      retriever_klass.new(object_db_id, decoded_params)
     end
 
     private
 
-    def jwt_scope_valid?
-      (decoded_authorization[:file_type] == object_type.to_s) && (decoded_authorization[:file_id] == object_db_id)
-    end
-
-    def decoded_authorization
-      strong_memoize(:decoded_authorization) do
-        ::Gitlab::Geo::JwtRequestDecoder.new(auth_header).decode
-      end
-    end
-
     def retriever_klass
       return Gitlab::Geo::Replication::FileRetriever if user_upload?
       return Gitlab::Geo::Replication::JobArtifactRetriever if job_artifact?

ee/lib/api/geo.rb

@@ -11,6 +11,32 @@ module API
           valid_attributes = params.keys & allowed_attributes
           params.slice(*valid_attributes)
         end
+
+        def jwt_decoder
+          ::Gitlab::Geo::JwtRequestDecoder.new(headers['Authorization'])
+        end
+
+        # Check if a Geo request is legit or fail the flow
+        #
+        # @param [Hash] attributes to be matched against JWT
+        def authorize_geo_transfer!(**attributes)
+          unauthorized! unless jwt_decoder.valid_attributes?(**attributes)
+        end
+      end
+
+      params do
+        requires :replicable_name, type: String, desc: 'Replicable name (eg. package_file)'
+        requires :id, type: Integer, desc: 'The model ID that needs to be transferred'
+      end
+      get 'retrieve/:replicable_name/:id' do
+        check_gitlab_geo_request_ip!
+        authorize_geo_transfer!(replicable_name: params[:replicable_name], id: params[:id])
+
+        decoded_params = jwt_decoder.decode
+        service = Geo::BlobUploadService.new(replicable_name: params[:replicable_name],
+                                             blob_id: params[:id],
+                                             decoded_params: decoded_params)
+        service.execute
       end
 
       # Verify the GitLab Geo transfer request is valid
@@ -27,12 +53,12 @@ module API
       end
       get 'transfers/:type/:id' do
         check_gitlab_geo_request_ip!
+        authorize_geo_transfer!(file_type: params[:type], file_id: params[:id])
 
-        service = ::Geo::FileUploadService.new(params, headers['Authorization'])
+        decoded_params = jwt_decoder.decode
+        service = ::Geo::FileUploadService.new(params, decoded_params)
         response = service.execute
 
-        unauthorized! unless response.present?
-
         if response[:code] == :ok
           file = response[:file]
           present_carrierwave_file!(file)

ee/lib/gitlab/geo/jwt_request_decoder.rb

@@ -4,6 +4,7 @@ module Gitlab
   module Geo
     class JwtRequestDecoder
       include LogHelpers
+      include ::Gitlab::Utils::StrongMemoize
 
       IAT_LEEWAY = 60.seconds.to_i
 
@@ -18,8 +19,25 @@ module Gitlab
         @auth_header = auth_header
       end
 
+      # Return decoded attributes from given header
+      #
+      # @return [Hash] decoded attributes
       def decode
-        decode_geo_request
+        strong_memoize(:decoded_authorization) do
+          decode_geo_request
+        end
+      end
+
+      # Check if set of attributes match against attributes decoded from JWT
+      #
+      # @param [Hash] attributes to be matched against JWT
+      # @return bool true
+      def valid_attributes?(**attributes)
+        decoded_attributes = decode
+
+        return false if decoded_attributes.nil?
+
+        attributes.all? { |attribute, value| decoded_attributes[attribute] == value }
       end
 
       private
@@ -72,6 +90,7 @@ module Gitlab
                            geo_node&.secret_access_key
                          end
       end
+
       # rubocop: enable CodeReuse/ActiveRecord
 
       def decode_auth_header

ee/lib/gitlab/geo/replication/base_retriever.rb

@@ -7,11 +7,11 @@ module Gitlab
         include LogHelpers
         include Gitlab::Utils::StrongMemoize
 
-        attr_reader :object_db_id, :message
+        attr_reader :object_db_id, :extra_params
 
-        def initialize(object_db_id, message)
+        def initialize(object_db_id, extra_params)
           @object_db_id = object_db_id
-          @message = message
+          @extra_params = extra_params
         end
 
         private

ee/lib/gitlab/geo/replication/blob_retriever.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    module Replication
+      class BlobRetriever < BaseRetriever
+        attr_reader :replicator, :checksum
+
+        # @param [Gitlab::Geo::Replicator] replicator
+        # @param [String] checksum
+        def initialize(replicator:, checksum:)
+          raise ArgumentError, 'Invalid replicator given' unless replicator.is_a?(Gitlab::Geo::Replicator)
+
+          @replicator = replicator
+          @checksum = checksum
+        end
+
+        def execute
+          return error("#{replicator.replicable_name} not found") unless recorded_file
+          return file_not_found(recorded_file) unless recorded_file.file_exist?
+          return error('Checksum mismatch') unless matches_checksum?
+
+          success(replicator.carrierwave_uploader.file)
+        end
+
+        private
+
+        def recorded_file
+          strong_memoize(:recorded_file) do
+            replicator.model_record
+          rescue ActiveRecord::RecordNotFound
+            nil
+          end
+        end
+
+        # Check if given checksum matches known good one
+        #
+        # We skip the check if no checksum is given to the retriever
+        #
+        # @return [Boolean]
+        def matches_checksum?
+          return true unless checksum
+
+          replicator.matches_checksum?(checksum)
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/geo/replication/file_retriever.rb

@@ -11,7 +11,8 @@ module Gitlab
         def execute
           return error('Upload not found') unless recorded_file
           return file_not_found(recorded_file) unless recorded_file.exist?
-          return error('Upload not found') unless valid?
+          return error('Invalid request') unless valid?
+          return error('Checksum mismatch') unless matches_checksum?
 
           success(recorded_file.retrieve_uploader)
         end
@@ -25,19 +26,17 @@ module Gitlab
         end
 
         def valid?
-          matches_requested_model? && matches_checksum?
-        end
+          return false if extra_params.nil?
 
-        def matches_requested_model?
-          message[:id] == recorded_file.model_id &&
-            message[:type] == recorded_file.model_type
+          extra_params[:id] == recorded_file.model_id &&
+            extra_params[:type] == recorded_file.model_type
         end
 
         def matches_checksum?
           # Remove this when we implement checksums for files on the Object Storage
           return true unless recorded_file.local?
 
-          message[:checksum] == Upload.hexdigest(recorded_file.absolute_path)
+          extra_params[:checksum] == Upload.hexdigest(recorded_file.absolute_path)
         end
       end
     end

ee/lib/gitlab/geo/replication/job_artifact_retriever.rb

@@ -9,9 +9,7 @@ module Gitlab
       #
       class JobArtifactRetriever < BaseRetriever
         def execute
-          unless job_artifact.present?
-            return error('Job artifact not found')
-          end
+          return error('Job artifact not found') unless job_artifact.present?
 
           unless job_artifact.file.present? && job_artifact.file.exists?
             log_error("Could not upload job artifact because it does not have a file", id: job_artifact.id)

ee/lib/gitlab/geo/replication/lfs_retriever.rb

@@ -9,6 +9,7 @@ module Gitlab
       #
       class LfsRetriever < BaseRetriever
         def execute
+          return error('Invalid request') unless valid?
           return error('LFS object not found') unless lfs_object
           return error('LFS object not found') unless matches_checksum?
 
@@ -29,8 +30,12 @@ module Gitlab
           end
         end
 
+        def valid?
+          !extra_params.nil?
+        end
+
         def matches_checksum?
-          message[:checksum] == lfs_object.oid
+          extra_params[:checksum] == lfs_object.oid
         end
       end
     end

ee/spec/factories/packages.rb

@@ -175,6 +175,8 @@ FactoryBot.define do
       file { fixture_file_upload('ee/spec/fixtures/npm/foo-1.0.1.tgz') }
       file_name { 'foo-1.0.1.tgz' }
       file_sha1 { 'be93151dc23ac34a82752444556fe79b32c7a1ad' }
+      verified_at { Date.current }
+      verification_checksum { '4437b5775e61455588a7e5187a2e5c58c680694260bbe5501c235ec690d17f83' }
       size { 400.kilobytes }
     end
 

ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Geo::JwtRequestDecoder do
   include EE::GeoHelpers
 
   let!(:primary_node) { FactoryBot.create(:geo_node, :primary) }
-  let(:data) { { input: 123 } }
+  let(:data) { { input: 123, other_input: 'string value' } }
   let(:request) { Gitlab::Geo::TransferRequest.new(data) }
 
   subject { described_class.new(request.headers['Authorization']) }
@@ -60,4 +60,18 @@ describe Gitlab::Geo::JwtRequestDecoder do
       expect { subject.decode }.to raise_error(Gitlab::Geo::InvalidDecryptionKeyError)
     end
   end
+
+  describe '#valid_attributes?' do
+    it 'returns true when all given attributes and decoded data are all the same' do
+      expect(subject.valid_attributes?(input: 123, other_input: 'string value')).to be_truthy
+    end
+
+    it 'returns true when given attributes is a slice of decoded data' do
+      expect(subject.valid_attributes?(input: 123)).to be_truthy
+    end
+
+    it 'returns false when one given data doesnt match its corresponding decoded one' do
+      expect(subject.valid_attributes?(input: 123, other_input: 'wrong value')).to be_falsey
+    end
+  end
 end

ee/spec/lib/gitlab/geo/replication/blob_retriever_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::Geo::Replication::BlobRetriever, :aggregate_failures do
+  let(:package_file) { create(:package_file, :npm) }
+  let(:package_checksum) { package_file.class.hexdigest(package_file.file.path) }
+  let(:replicator_class) { Geo::PackageFileReplicator }
+  let(:replicator) { replicator_class.new(model_record_id: package_file.id) }
+
+  describe '#initialize' do
+    it 'errors out with an invalid replicator' do
+      expect { described_class.new(replicator: Object.new, checksum: nil) }.to raise_error(ArgumentError)
+    end
+
+    it 'accepts valid attributes' do
+      expect { described_class.new(replicator: replicator, checksum: nil) }.not_to raise_error
+      expect { described_class.new(replicator: replicator, checksum: package_checksum) }.not_to raise_error
+    end
+  end
+
+  describe '#execute' do
+    subject { described_class.new(replicator: replicator, checksum: package_checksum) }
+
+    it 'returns model not found error if record cant be found' do
+      subject = described_class.new(replicator: replicator_class.new(model_record_id: 1234567890), checksum: nil)
+      response = subject.execute
+
+      expect(response).to include(code: :not_found)
+      expect(response).to include(message: /package_file not found/)
+    end
+
+    it 'returns file not found if file cant be found' do
+      subject
+      File.unlink(package_file.file.path)
+
+      response = subject.execute
+
+      expect(response).to include(code: :not_found)
+      expect(response).to include(message: /file not found/)
+    end
+
+    it 'returns checksum mismatch if sending an invalid checksum' do
+      subject = described_class.new(replicator: replicator, checksum: 'invalid')
+      response = subject.execute
+
+      expect(response).to include(code: :not_found)
+      expect(response).to include(message: 'Checksum mismatch')
+    end
+
+    it 'works with valid attributes' do
+      response = subject.execute
+
+      expect(response).to include(code: :ok)
+      expect(response).to include(message: 'Success')
+      expect(response[:file].path).to eq(package_file.file.path)
+    end
+  end
+end

ee/spec/lib/gitlab/geo/replication/file_retriever_spec.rb

@@ -7,7 +7,7 @@ describe Gitlab::Geo::Replication::FileRetriever, :geo do
     subject { @subject ||= retriever.execute }
 
     context 'when the upload exists' do
-      let(:retriever) { described_class.new(upload.id, message) }
+      let(:retriever) { described_class.new(upload.id, extra_params) }
 
       context 'when the upload has a file' do
         before do
@@ -15,8 +15,8 @@ describe Gitlab::Geo::Replication::FileRetriever, :geo do
           FileUtils.touch(upload.absolute_path) unless File.exist?(upload.absolute_path)
         end
 
-        context 'when the message parameters match the upload' do
-          let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+        context 'when the extra_params parameters match the upload' do
+          let(:extra_params) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
 
           it 'returns the file in a success hash' do
             expect(subject).to include(code: :ok, message: 'Success')
@@ -24,33 +24,33 @@ describe Gitlab::Geo::Replication::FileRetriever, :geo do
           end
         end
 
-        context 'when the message id does not match the upload model_id' do
-          let(:message) { { id: 10000, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+        context 'when the extra_params id does not match the upload model_id' do
+          let(:extra_params) { { id: 10000, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
 
           it 'returns an error hash' do
-            expect(subject).to include(code: :not_found, message: "Upload not found")
+            expect(subject).to include(code: :not_found, message: 'Invalid request')
           end
         end
 
-        context 'when the message type does not match the upload model_type' do
-          let(:message) { { id: upload.model_id, type: 'bad_type', checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+        context 'when the extra_params type does not match the upload model_type' do
+          let(:extra_params) { { id: upload.model_id, type: 'bad_type', checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
 
           it 'returns an error hash' do
-            expect(subject).to include(code: :not_found, message: "Upload not found")
+            expect(subject).to include(code: :not_found, message: 'Invalid request')
           end
         end
 
-        context 'when the message checksum does not match the upload checksum' do
-          let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'doesnotmatch' } }
+        context 'when the extra_params checksum does not match the upload checksum' do
+          let(:extra_params) { { id: upload.model_id, type: upload.model_type, checksum: 'doesnotmatch' } }
 
           it 'returns an error hash' do
-            expect(subject).to include(code: :not_found, message: "Upload not found")
+            expect(subject).to include(code: :not_found, message: 'Checksum mismatch')
           end
         end
       end
 
       context 'when the upload does not have a file' do
-        let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+        let(:extra_params) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
 
         it 'returns an error hash' do
           expect(subject).to include(code: :not_found, geo_code: 'FILE_NOT_FOUND', message: match(/Upload #\d+ file not found/))
@@ -62,7 +62,7 @@ describe Gitlab::Geo::Replication::FileRetriever, :geo do
       it 'returns an error hash' do
         result = described_class.new(10000, {}).execute
 
-        expect(result).to eq(code: :not_found, message: "Upload not found")
+        expect(result).to eq(code: :not_found, message: 'Upload not found')
       end
     end
   end

ee/spec/lib/gitlab/geo/replication/lfs_retriever_spec.rb

@@ -7,7 +7,7 @@ describe Gitlab::Geo::Replication::LfsRetriever, :geo do
     subject { retriever.execute }
 
     context 'when the LFS object exists' do
-      let(:retriever) { described_class.new(lfs_object.id, message) }
+      let(:retriever) { described_class.new(lfs_object.id, extra_params) }
 
       before do
         expect(LfsObject).to receive(:find_by).with(id: lfs_object.id).and_return(lfs_object)
@@ -15,33 +15,33 @@ describe Gitlab::Geo::Replication::LfsRetriever, :geo do
 
       context 'when the LFS object has a file' do
         let(:lfs_object) { create(:lfs_object, :with_file) }
-        let(:message) { { checksum: lfs_object.oid } }
+        let(:extra_params) { { checksum: lfs_object.oid } }
 
-        context 'when the message checksum matches the LFS object oid' do
+        context 'when the extra_params checksum matches the LFS object oid' do
           it 'returns the file in a success hash' do
             expect(subject).to eq(code: :ok, message: 'Success', file: lfs_object.file)
           end
         end
 
-        context 'when the message checksum does not match the LFS object oid' do
-          let(:message) { { checksum: 'foo' } }
+        context 'when the extra_params checksum does not match the LFS object oid' do
+          let(:extra_params) { { checksum: 'foo' } }
 
           it 'returns an error hash' do
-            expect(subject).to include(code: :not_found, message: "LFS object not found")
+            expect(subject).to include(code: :not_found, message: 'LFS object not found')
           end
         end
       end
 
       context 'when the LFS object does not have a file' do
         let(:lfs_object) { create(:lfs_object) }
-        let(:message) { { checksum: lfs_object.oid } }
+        let(:extra_params) { { checksum: lfs_object.oid } }
 
         it 'returns an error hash' do
           expect(subject).to include(code: :not_found, geo_code: 'FILE_NOT_FOUND', message: match(/LfsObject #\d+ file not found/))
         end
 
         it 'logs the missing file' do
-          expect(retriever).to receive(:log_error).with("Could not upload LFS object because it does not have a file", id: lfs_object.id)
+          expect(retriever).to receive(:log_error).with('Could not upload LFS object because it does not have a file', id: lfs_object.id)
 
           subject
         end

ee/spec/requests/api/geo_spec.rb

@@ -42,14 +42,14 @@ describe API::Geo do
 
     describe 'allowed IPs' do
       let(:note) { create(:note, :with_attachment) }
-      let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
-      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, upload) }
+      let(:resource) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
+      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, resource) }
       let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
 
       it 'responds with 401 when IP is not allowed' do
         stub_application_setting(geo_node_allowed_ips: '192.34.34.34')
 
-        get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+        get api("/geo/transfers/attachment/#{resource.id}"), headers: req_header
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
@@ -57,18 +57,13 @@ describe API::Geo do
       it 'responds with 200 when IP is allowed' do
         stub_application_setting(geo_node_allowed_ips: '127.0.0.1')
 
-        get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+        get api("/geo/transfers/attachment/#{resource.id}"), headers: req_header
 
         expect(response).to have_gitlab_http_status(:ok)
       end
     end
 
-    describe 'GET /geo/transfers/attachment/:id' do
-      let(:note) { create(:note, :with_attachment) }
-      let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
-      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, upload) }
-      let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
-
+    shared_examples 'validate geo transfer requests' do |api_path|
       before do
         allow_next_instance_of(Gitlab::Geo::TransferRequest) do |instance|
           allow(instance).to receive(:requesting_node).and_return(secondary_node)
@@ -76,13 +71,43 @@ describe API::Geo do
       end
 
       it 'responds with 401 with invalid auth header' do
-        get api("/geo/transfers/attachment/#{upload.id}"), headers: { Authorization: 'Test' }
+        path = File.join(api_path, resource.id.to_s)
+        get api(path), headers: { Authorization: 'Test' }
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
 
+      context 'with mismatched params in auth headers' do
+        let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:wrong, resource) }
+
+        it 'responds with 401' do
+          path = File.join(api_path, resource.id.to_s)
+          get api(path), headers: { Authorization: 'Test' }
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context 'resource does not exist' do
+        it 'responds with 404' do
+          path = File.join(api_path, '100000')
+          get api(path), headers: not_found_req_header
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    describe 'GET /geo/transfers/attachment/:id' do
+      let(:note) { create(:note, :with_attachment) }
+      let(:resource) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
+      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, resource) }
+      let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
+
+      it_behaves_like 'validate geo transfer requests', '/geo/transfers/attachment/'
+
       context 'when attachment file exists' do
-        subject(:request) { get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header }
+        subject(:request) { get api("/geo/transfers/attachment/#{resource.id}"), headers: req_header }
 
         it 'responds with 200 with X-Sendfile' do
           request
@@ -95,19 +120,11 @@ describe API::Geo do
         it_behaves_like 'with terms enforced'
       end
 
-      context 'when attachment does not exist' do
-        it 'responds with 404' do
-          get api("/geo/transfers/attachment/100000"), headers: not_found_req_header
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
-
       context 'when attachment has mount_point nil' do
         it 'responds with 200 with X-Sendfile' do
-          upload.update(mount_point: nil)
+          resource.update(mount_point: nil)
 
-          get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+          get api("/geo/transfers/attachment/#{resource.id}"), headers: req_header
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.headers['Content-Type']).to eq('application/octet-stream')
@@ -118,24 +135,14 @@ describe API::Geo do
 
     describe 'GET /geo/transfers/avatar/1' do
       let(:user) { create(:user, avatar: fixture_file_upload('spec/fixtures/dk.png', 'image/png')) }
-      let(:upload) { Upload.find_by(model: user, uploader: 'AvatarUploader') }
-      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:avatar, upload) }
+      let(:resource) { Upload.find_by(model: user, uploader: 'AvatarUploader') }
+      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:avatar, resource) }
       let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
 
-      before do
-        allow_next_instance_of(Gitlab::Geo::TransferRequest) do |instance|
-          allow(instance).to receive(:requesting_node).and_return(secondary_node)
-        end
-      end
-
-      it 'responds with 401 with invalid auth header' do
-        get api("/geo/transfers/avatar/#{upload.id}"), headers: { Authorization: 'Test' }
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-      end
+      it_behaves_like 'validate geo transfer requests', '/geo/transfers/avatar/'
 
       context 'avatar file exists' do
-        subject(:request) { get api("/geo/transfers/avatar/#{upload.id}"), headers: req_header }
+        subject(:request) { get api("/geo/transfers/avatar/#{resource.id}"), headers: req_header }
 
         it 'responds with 200 with X-Sendfile' do
           request
@@ -148,19 +155,11 @@ describe API::Geo do
         it_behaves_like 'with terms enforced'
       end
 
-      context 'avatar does not exist' do
-        it 'responds with 404' do
-          get api("/geo/transfers/avatar/100000"), headers: not_found_req_header
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
-
       context 'avatar has mount_point nil' do
         it 'responds with 200 with X-Sendfile' do
-          upload.update(mount_point: nil)
+          resource.update(mount_point: nil)
 
-          get api("/geo/transfers/avatar/#{upload.id}"), headers: req_header
+          get api("/geo/transfers/avatar/#{resource.id}"), headers: req_header
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.headers['Content-Type']).to eq('application/octet-stream')
@@ -171,26 +170,19 @@ describe API::Geo do
 
     describe 'GET /geo/transfers/file/1' do
       let(:project) { create(:project) }
-      let(:upload) { Upload.find_by(model: project, uploader: 'FileUploader') }
-      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:file, upload) }
+      let(:resource) { Upload.find_by(model: project, uploader: 'FileUploader') }
+      let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:file, resource) }
       let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
 
+      it_behaves_like 'validate geo transfer requests', '/geo/transfers/file/'
+
       before do
-        allow_next_instance_of(Gitlab::Geo::TransferRequest) do |instance|
-          allow(instance).to receive(:requesting_node).and_return(secondary_node)
-        end
         FileUploader.new(project).store!(fixture_file_upload('spec/fixtures/dk.png', 'image/png'))
       end
 
-      it 'responds with 401 with invalid auth header' do
-        get api("/geo/transfers/file/#{upload.id}"), headers: { Authorization: 'Test' }
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-      end
-
       context 'when the Upload record exists' do
         context 'when the file exists' do
-          subject(:request) { get api("/geo/transfers/file/#{upload.id}"), headers: req_header }
+          subject(:request) { get api("/geo/transfers/file/#{resource.id}"), headers: req_header }
 
           it 'responds with 200 with X-Sendfile' do
             request
@@ -205,52 +197,34 @@ describe API::Geo do
 
         context 'file does not exist' do
           it 'responds with 404 and a specific geo code' do
-            File.unlink(upload.absolute_path)
+            File.unlink(resource.absolute_path)
 
-            get api("/geo/transfers/file/#{upload.id}"), headers: req_header
+            get api("/geo/transfers/file/#{resource.id}"), headers: req_header
 
             expect(response).to have_gitlab_http_status(:not_found)
             expect(json_response['geo_code']).to eq(Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE)
           end
         end
       end
-
-      context 'when the Upload record does not exist' do
-        it 'responds with 404' do
-          get api("/geo/transfers/file/100000"), headers: not_found_req_header
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
     end
 
     describe 'GET /geo/transfers/lfs/1' do
-      let(:lfs_object) { create(:lfs_object, :with_file) }
-      let(:transfer) { Gitlab::Geo::Replication::LfsTransfer.new(lfs_object) }
+      let(:resource) { create(:lfs_object, :with_file) }
+      let(:transfer) { Gitlab::Geo::Replication::LfsTransfer.new(resource) }
       let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
 
-      before do
-        allow_next_instance_of(Gitlab::Geo::TransferRequest) do |instance|
-          allow(instance).to receive(:requesting_node).and_return(secondary_node)
-        end
-      end
-
-      it 'responds with 401 with invalid auth header' do
-        get api("/geo/transfers/lfs/#{lfs_object.id}"), headers: { Authorization: 'Test' }
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-      end
+      it_behaves_like 'validate geo transfer requests', '/geo/transfers/lfs/'
 
       context 'LFS object exists' do
         context 'file exists' do
-          subject(:request) { get api("/geo/transfers/lfs/#{lfs_object.id}"), headers: req_header }
+          subject(:request) { get api("/geo/transfers/lfs/#{resource.id}"), headers: req_header }
 
           it 'responds with 200 with X-Sendfile' do
             request
 
             expect(response).to have_gitlab_http_status(:ok)
             expect(response.headers['Content-Type']).to eq('application/octet-stream')
-            expect(response.headers['X-Sendfile']).to eq(lfs_object.file.path)
+            expect(response.headers['X-Sendfile']).to eq(resource.file.path)
           end
 
           it_behaves_like 'with terms enforced'
@@ -258,23 +232,15 @@ describe API::Geo do
 
         context 'file does not exist' do
           it 'responds with 404 and a specific geo code' do
-            File.unlink(lfs_object.file.path)
+            File.unlink(resource.file.path)
 
-            get api("/geo/transfers/lfs/#{lfs_object.id}"), headers: req_header
+            get api("/geo/transfers/lfs/#{resource.id}"), headers: req_header
 
             expect(response).to have_gitlab_http_status(:not_found)
             expect(json_response['geo_code']).to eq(Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE)
           end
         end
       end
-
-      context 'LFS object does not exist' do
-        it 'responds with 404' do
-          get api("/geo/transfers/lfs/100000"), headers: not_found_req_header
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
     end
   end
 

ee/spec/services/geo/blob_upload_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Geo::BlobUploadService do
+  let(:package_file) { create(:package_file, :npm) }
+
+  subject { described_class.new(replicable_name: 'package_file', blob_id: package_file.id, decoded_params: {}) }
+
+  describe '#initialize' do
+    it 'initializes with valid attributes' do
+      expect { subject }.not_to raise_error
+    end
+  end
+
+  describe '#execute' do
+    it 'works with valid attributes' do
+      expect { subject.execute }.not_to raise_error
+    end
+
+    it 'errors with an invalid attributes' do
+      service = described_class.new(replicable_name: 'package_file', blob_id: 1234567890, decoded_params: {})
+
+      response = service.execute
+
+      expect(response).to include(code: :not_found)
+    end
+
+    it 'returns a file with valid attributes' do
+      service = described_class.new(replicable_name: 'package_file', blob_id: package_file.id,
+                                    decoded_params: { checksum: package_file.verification_checksum })
+
+      response = service.execute
+
+      expect(response).to include(code: :ok)
+      expect(response[:file].path).to eq(package_file.file.path)
+    end
+  end
+end

ee/spec/services/geo/file_upload_service_spec.rb

@@ -7,9 +7,6 @@ describe Geo::FileUploadService do
 
   let_it_be(:node) { create(:geo_node, :primary) }
 
-  let(:transfer_request) { Gitlab::Geo::TransferRequest.new(request_data) }
-  let(:req_header) { transfer_request.headers['Authorization'] }
-
   before do
     stub_current_geo_node(node)
   end
@@ -36,23 +33,13 @@ describe Geo::FileUploadService do
     end
   end
 
-  shared_examples 'no authorization header' do
-    it 'returns nil' do
+  shared_examples 'no decoded params' do
+    it 'returns invalid request error' do
       service = described_class.new(params, nil)
 
-      expect(service.execute).to be_nil
-    end
-  end
-
-  shared_examples 'wrong scope' do
-    context 'at least one scope parameter is wrong' do
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(request_data.merge(file_type: 'wrong')) }
-
-      it 'returns nil' do
-        service = described_class.new(params, req_header)
-
-        expect(service.execute).to be_nil
-      end
+      response = service.execute
+      expect(response[:code]).to eq(:not_found)
+      expect(response[:message]).to eq('Invalid request')
     end
   end
 
@@ -64,7 +51,7 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -72,8 +59,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(user.avatar.path)
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'group avatar' do
@@ -83,7 +69,7 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -91,8 +77,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(group.avatar.path)
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'project avatar' do
@@ -102,7 +87,7 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -110,8 +95,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(project.avatar.path)
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'attachment' do
@@ -121,7 +105,7 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, upload).request_data }
 
       it 'sends attachment file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -129,8 +113,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(note.attachment.path)
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'file upload' do
@@ -145,7 +128,7 @@ describe Geo::FileUploadService do
       end
 
       it 'sends the file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -153,8 +136,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('dk.png')
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'namespace file upload' do
@@ -169,7 +151,7 @@ describe Geo::FileUploadService do
       end
 
       it 'sends the file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -177,8 +159,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('dk.png')
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'LFS Object' do
@@ -187,7 +168,7 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::LfsTransfer.new(lfs_object).request_data }
 
       it 'sends LFS file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -195,8 +176,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(lfs_object.file.path)
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
 
     context 'job artifact' do
@@ -205,16 +185,13 @@ describe Geo::FileUploadService do
       let(:request_data) { Gitlab::Geo::Replication::JobArtifactTransfer.new(job_artifact).request_data }
 
       it 'sends job artifact file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
         expect(response[:code]).to eq(:ok)
         expect(response[:file].path).to eq(job_artifact.file.path)
       end
-
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
     end
 
     context 'import export archive' do
@@ -229,7 +206,7 @@ describe Geo::FileUploadService do
       end
 
       it 'sends the file' do
-        service = described_class.new(params, req_header)
+        service = described_class.new(params, request_data)
 
         response = service.execute
 
@@ -237,8 +214,7 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('tar.gz')
       end
 
-      include_examples 'no authorization header'
-      include_examples 'wrong scope'
+      include_examples 'no decoded params'
     end
   end
 end

spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb

@@ -52,6 +52,7 @@ RSpec.shared_examples 'a blob replicator' do
       replicator.calculate_checksum!
 
       expect(model_record.reload.verification_checksum).not_to be_nil
+      expect(model_record.reload.verified_at).not_to be_nil
     end
 
     it 'saves the error message and increments retry counter' do