Merge branch 'rd-gitaly-client-for-list-new-objects-rpc' into 'master'

Fix bug related to size check of repositories and files

Closes #6459 and #6780

See merge request gitlab-org/gitlab-ee!6767

GITALY_SERVER_VERSION

-0.115.0
+0.116.0

Gemfile

@@ -438,7 +438,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.109.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.112.0', require: 'gitaly'
 gem 'grpc', '~> 1.11.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed

Gemfile.lock

@@ -308,7 +308,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.109.0)
+    gitaly-proto (0.112.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1082,7 +1082,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.109.0)
+  gitaly-proto (~> 0.112.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

Gemfile.rails5.lock

@@ -311,7 +311,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.109.0)
+    gitaly-proto (0.112.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1092,7 +1092,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.109.0)
+  gitaly-proto (~> 0.112.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

ee/changelogs/unreleased/rd-gitaly-client-for-list-new-objects-rpc.yml

+---
+title: Ensure that push size checks only count the size of newly-pushed files
+merge_request: 6767
+author:
+type: fixed

ee/lib/ee/gitlab/checks/change_access.rb

@@ -34,16 +34,14 @@ module EE
           return if push_rule.nil? || push_rule.max_file_size.zero?
 
           max_file_size = push_rule.max_file_size
-          raw_changes = project.repository.raw_changes_between(oldrev, newrev)
+          blobs = project.repository.new_blobs(newrev)
 
-          large_file = raw_changes.find do |c|
-            size_in_mb = ::Gitlab::Utils.bytes_to_megabytes(c.blob_size)
-
-            c.operation != :deleted && size_in_mb > max_file_size
+          large_blob = blobs.find do |blob|
+            ::Gitlab::Utils.bytes_to_megabytes(blob.size) > max_file_size
           end
 
-          if large_file
-            raise ::Gitlab::GitAccess::UnauthorizedError, %Q{File "#{large_file.new_path}" is larger than the allowed size of #{max_file_size} MB}
+          if large_blob
+            raise ::Gitlab::GitAccess::UnauthorizedError, %Q{File "#{large_blob.path}" is larger than the allowed size of #{max_file_size} MB}
           end
         end
 

ee/spec/lib/gitlab/checks/change_access_spec.rb

@@ -262,15 +262,19 @@ describe Gitlab::Checks::ChangeAccess do
 
       context 'max file size rules' do
         let(:push_rule) { create(:push_rule, max_file_size: 1) }
+        # SHA of the 2-mb-file branch
+        let(:newrev)    { 'bf12d2567099e26f59692896f73ac819bae45b00' }
+        let(:ref)       { 'my-branch' }
 
         before do
-          allow_any_instance_of(::Gitlab::Git::RawDiffChange).to receive(:blob_size).and_return(2.megabytes)
+          # Delete branch so Repository#new_blobs can return results
+          project.repository.delete_branch('2-mb-file')
         end
 
         it_behaves_like 'check ignored when push rule unlicensed'
 
         it 'returns an error if file exceeds the maximum file size' do
-          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File \"README\" is larger than the allowed size of 1 MB")
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File \"file.bin\" is larger than the allowed size of 1 MB")
         end
       end
 

ee/spec/lib/gitlab/git_access_spec.rb

@@ -5,6 +5,7 @@ describe Gitlab::GitAccess do
 
   let(:actor) { user }
   let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
   let(:protocol) { 'web' }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
@@ -154,42 +155,44 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
     end
 
     describe "max file size check" do
-      let(:start_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
-      let(:end_sha)   { 'c84ff944ff4529a70788a5e9003c2b7feae29047' }
-      let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
+      let(:start_sha) { ::Gitlab::Git::BLANK_SHA }
+      # SHA of the 2-mb-file branch
+      let(:end_sha)   { 'bf12d2567099e26f59692896f73ac819bae45b00' }
+      let(:changes) { "#{start_sha} #{end_sha} refs/heads/my-branch" }
 
       before do
         project.add_developer(user)
+        # Delete branch so Repository#new_blobs can return results
+        repository.delete_branch('2-mb-file')
       end
 
       it "returns false when size is too large" do
         project.create_push_rule(max_file_size: 1)
 
+        expect(repository.new_blobs(end_sha)).to be_present
         expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
       end
 
       it "returns true when size is allowed" do
-        project.create_push_rule(max_file_size: 2)
-
-        expect { push_changes(changes) }.not_to raise_error
-      end
-
-      it "returns true when size is nil" do
-        allow_any_instance_of(Gitlab::Git::Blob).to receive(:size).and_return(nil)
-        project.create_push_rule(max_file_size: 2)
+        project.create_push_rule(max_file_size: 3)
 
+        expect(repository.new_blobs(end_sha)).to be_present
         expect { push_changes(changes) }.not_to raise_error
       end
     end
   end
 
   describe 'repository size restrictions' do
-    let(:start_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
-    let(:sha_with_2_mb_file) { 'c84ff944ff4529a70788a5e9003c2b7feae29047' }
-    let(:sha_with_smallest_changes) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
+    # SHA for the 2-mb-file branch
+    let(:sha_with_2_mb_file) { 'bf12d2567099e26f59692896f73ac819bae45b00' }
+    # SHA for the wip branch
+    let(:sha_with_smallest_changes) { 'b9238ee5bf1d7359dd3b8c89fd76c1c7f8b75aba' }
 
     before do
       project.add_developer(user)
+      # Delete branch so Repository#new_blobs can return results
+      repository.delete_branch('2-mb-file')
+      repository.delete_branch('wip')
     end
 
     context 'when repository size is over limit' do
@@ -200,17 +203,17 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
       end
 
       it 'rejects the push' do
+        expect(repository.new_blobs(sha_with_smallest_changes)).to be_present
+
         expect do
-          push_changes("#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
+          push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/master")
         end.to raise_error(described_class::UnauthorizedError, /Your push has been rejected/)
       end
 
       context 'when deleting a branch' do
         it 'accepts the operation' do
-          feature_branch_sha = project.commit('feature').id
-
           expect do
-            push_changes("#{feature_branch_sha} #{::Gitlab::Git::BLANK_SHA} refs/heads/feature")
+            push_changes("#{sha_with_smallest_changes} #{::Gitlab::Git::BLANK_SHA} refs/heads/feature")
           end.not_to raise_error
         end
       end
@@ -223,62 +226,33 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         project.update_attribute(:repository_size_limit, 2.megabytes)
       end
 
-      context 'when new change exceeds the limit' do
-        it 'rejects the push' do
-          expect do
-            push_changes("#{start_sha} #{sha_with_2_mb_file} refs/heads/master")
-          end.to raise_error(described_class::UnauthorizedError, /Your push to this repository would cause it to exceed the size limit/)
-        end
-      end
-
-      context 'when new change does not exceeds the limit' do
+      context 'when pushing a new branch' do
         it 'accepts the push' do
+          master_sha = project.commit('master').id
+
           expect do
-            push_changes("#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
+            push_changes("#{Gitlab::Git::BLANK_SHA} #{master_sha} refs/heads/my_branch")
           end.not_to raise_error
         end
       end
 
-      context 'when a file is modified' do
-        let(:start_sha) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } # file created
-        let(:end_sha) { 'c347ca2e140aa667b968e51ed0ffe055501fe4f4' } # file modified
-        let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
-
-        before do
-          # Substract 10_000 bytes in order to demostrate that the 23 KB are not added to the total
-          allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes - 10000)
-        end
-
-        it 'just add the difference between the two versions to the total size' do
-          expect { push_changes(changes) }.not_to raise_error
-        end
-      end
-
-      context 'when a file is renamed' do
-        let(:start_sha) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } # file deleted
-        let(:end_sha) { 'c347ca2e140aa667b968e51ed0ffe055501fe4f4' } # file added with different name
-        let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
-
-        before do
-          allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes)
-        end
+      context 'when new change exceeds the limit' do
+        it 'rejects the push' do
+          expect(repository.new_blobs(sha_with_2_mb_file)).to be_present
 
-        it 'does not modify the total size given the content is the same' do
-          expect { push_changes(changes) }.not_to raise_error
+          expect do
+            push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_2_mb_file} refs/heads/my_branch_2")
+          end.to raise_error(described_class::UnauthorizedError, /Your push to this repository would cause it to exceed the size limit/)
         end
       end
 
-      context 'when a file is deleted' do
-        let(:start_sha) { 'c1acaa58bbcbc3eafe538cb8274ba387047b69f8' } # file deleted
-        let(:end_sha) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' } # New changes introduced
-        let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
-
-        before do
-          allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes)
-        end
+      context 'when new change does not exceeds the limit' do
+        it 'accepts the push' do
+          expect(repository.new_blobs(sha_with_smallest_changes)).to be_present
 
-        it 'subtracts the size of the deleted file before calculate the new total' do
-          expect { push_changes(changes) }.not_to raise_error
+          expect do
+            push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/my_branch_3")
+          end.not_to raise_error
         end
       end
     end

lib/gitlab/checks/lfs_integrity.rb

 module Gitlab
   module Checks
     class LfsIntegrity
-      REV_LIST_OBJECT_LIMIT = 2_000
-
       def initialize(project, newrev)
         @project = project
         @newrev = newrev
@@ -11,7 +9,8 @@ module Gitlab
       def objects_missing?
         return false unless @newrev && @project.lfs_enabled?
 
-        new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev).new_pointers(object_limit: REV_LIST_OBJECT_LIMIT)
+        new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev)
+                                                  .new_pointers(object_limit: ::Gitlab::Git::Repository::REV_LIST_COMMIT_LIMIT)
 
         return false unless new_lfs_pointers.present?
 

lib/gitlab/git/repository.rb

@@ -19,6 +19,7 @@ module Gitlab
         GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
       ].freeze
       SEARCH_CONTEXT_LINES = 3
+      REV_LIST_COMMIT_LIMIT = 2_000
       # In https://gitlab.com/gitlab-org/gitaly/merge_requests/698
       # We copied these two prefixes into gitaly-go, so don't change these
       # or things will break! (REBASE_WORKTREE_PREFIX and SQUASH_WORKTREE_PREFIX)
@@ -380,6 +381,16 @@ module Gitlab
         end
       end
 
+      def new_blobs(newrev)
+        return [] if newrev == ::Gitlab::Git::BLANK_SHA
+
+        strong_memoize("new_blobs_#{newrev}") do
+          wrapped_gitaly_errors do
+            gitaly_ref_client.list_new_blobs(newrev, REV_LIST_COMMIT_LIMIT)
+          end
+        end
+      end
+
       def count_commits(options)
         options = process_count_commits_options(options.dup)
 

lib/gitlab/git_access.rb

@@ -272,7 +272,7 @@ module Gitlab
         check_single_change_access(change, skip_lfs_integrity_check: !first_change)
 
         if project.size_limit_enabled?
-          push_size_in_bytes += push_size(change)
+          push_size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size)
         end
       end
 
@@ -281,29 +281,6 @@ module Gitlab
       end
     end
 
-    def push_size(change)
-      old_rev, new_rev = change.values_at(:oldrev, :newrev)
-      current_changes_size = 0
-      old_paths = []
-
-      repository.raw_changes_between(old_rev, new_rev).each do |c|
-        case c.operation
-        when :deleted
-          current_changes_size -= c.blob_size
-        when :added
-          current_changes_size += c.blob_size
-        when :copied, :modified, :renamed, :type_changed
-          current_changes_size += c.blob_size
-
-          old_paths << [old_rev, c.old_path]
-        end
-      end
-
-      old_changes_size = Gitlab::Git::Blob.batch_metadata(repository, old_paths).sum(&:size)
-
-      current_changes_size - old_changes_size
-    end
-
     def check_single_change_access(change, skip_lfs_integrity_check: false)
       Checks::ChangeAccess.new(
         change,

lib/gitlab/gitaly_client/ref_service.rb

@@ -82,6 +82,23 @@ module Gitlab
         commits
       end
 
+      def list_new_blobs(newrev, limit = 0)
+        request = Gitaly::ListNewBlobsRequest.new(
+          repository: @gitaly_repo,
+          commit_id: newrev,
+          limit: limit
+        )
+
+        response = GitalyClient
+          .call(@storage, :ref_service, :list_new_blobs, request, timeout: GitalyClient.medium_timeout)
+
+        response.flat_map do |msg|
+          # Returns an Array of Gitaly::NewBlobObject objects
+          # Available methods are: #size, #oid and #path
+          msg.new_blob_objects
+        end
+      end
+
       def count_tag_names
         tag_names.count
       end

spec/support/helpers/test_env.rb

@@ -51,7 +51,8 @@ module TestEnv
     'add-pdf-text-binary'                => '79faa7b',
     'add_images_and_changes'             => '010d106',
     'update-gitlab-shell-v-6-0-1'        => '2f61d70',
-    'update-gitlab-shell-v-6-0-3'        => 'de78448'
+    'update-gitlab-shell-v-6-0-3'        => 'de78448',
+    '2-mb-file'                          => 'bf12d25'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily