Merge branch 'rd-6804-bug-unable-to-delete-branches-when-repo-reaches-the-size-limit' into 'master'

Fix a bug where user was unable to delete a branch when repo size was above the limit

Closes #6804

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

ee/changelogs/unreleased/rd-6804-bug-unable-to-delete-branches-when-repo-reaches-the-size-limit.yml

+---
+title: Fix a bug where user was unable to delete a branch when repo size was above the limit
+merge_request: 6373
+author:
+type: fixed

ee/spec/lib/gitlab/git_access_spec.rb

@@ -10,7 +10,6 @@ describe Gitlab::GitAccess do
   let(:redirected_path) { nil }
 
   let(:access) { described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) }
-  subject { access.check('git-receive-pack', '_any') }
 
   context "when in a read-only GitLab instance" do
     before do
@@ -21,7 +20,7 @@ describe Gitlab::GitAccess do
     it 'denies push access' do
       project.add_master(user)
 
-      expect { subject }.to raise_unauthorized("You can't push code to a read-only GitLab instance.")
+      expect { push_changes }.to raise_unauthorized("You can't push code to a read-only GitLab instance.")
     end
 
     it 'denies push access with primary present' do
@@ -35,13 +34,14 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
 
       project.add_master(user)
 
-      expect { subject }.to raise_unauthorized(error_message)
+      expect { push_changes }.to raise_unauthorized(error_message)
     end
   end
 
   describe "push_rule_check" do
     let(:start_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
     let(:end_sha)   { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
+    let(:changes)   { "#{start_sha} #{end_sha} refs/heads/master" }
 
     before do
       project.add_developer(user)
@@ -52,25 +52,25 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
 
     describe "author email check" do
       it 'returns true' do
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        expect { push_changes(changes) }.not_to raise_error
       end
 
       it 'returns false when a commit message is missing required matches (positive regex match)' do
         project.create_push_rule(commit_message_regex: "@only.com")
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
       end
 
       it 'returns false when a commit message contains forbidden characters (negative regex match)' do
         project.create_push_rule(commit_message_negative_regex: "@gmail.com")
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
       end
 
       it 'returns true for tags' do
         project.create_push_rule(commit_message_regex: "@only.com")
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/tags/v1") }.not_to raise_error
+        expect { push_changes("#{start_sha} #{end_sha} refs/tags/v1") }.not_to raise_error
       end
 
       it 'allows githook for new branch with an old bad commit' do
@@ -82,7 +82,7 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         project.create_push_rule(commit_message_regex: "Change some files")
 
         # push to new branch, so use a blank old rev and new ref
-        expect { access.send(:check_push_access!, "#{Gitlab::Git::BLANK_SHA} #{end_sha} refs/heads/new-branch") }.not_to raise_error
+        expect { push_changes("#{Gitlab::Git::BLANK_SHA} #{end_sha} refs/heads/new-branch") }.not_to raise_error
       end
 
       it 'allows githook for any change with an old bad commit' do
@@ -94,7 +94,7 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         project.create_push_rule(commit_message_regex: "Change some files")
 
         # push to new branch, so use a blank old rev and new ref
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        expect { push_changes("#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
       end
 
       it 'does not allow any change from Web UI with bad commit' do
@@ -108,29 +108,32 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         project.create_push_rule(commit_message_regex: "Change some files")
 
         # push to new branch, so use a blank old rev and new ref
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        expect { push_changes("#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
       end
     end
 
     describe "member_check" do
+      let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
+
       before do
         project.create_push_rule(member_check: true)
       end
 
       it 'returns false for non-member user' do
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
       end
 
       it 'returns true if committer is a gitlab member' do
         create(:user, email: 'dmitriy.zaporozhets@gmail.com')
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        expect { push_changes(changes) }.not_to raise_error
       end
     end
 
     describe "file names check" do
       let(:start_sha) { '913c66a37b4a45b9769037c55c2d238bd0942d2e' }
-      let(:end_sha)   { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
+      let(:end_sha) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
+      let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
 
       before do
         allow(project.repository).to receive(:new_commits)
@@ -140,19 +143,20 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
       it 'returns false when filename is prohibited' do
         project.create_push_rule(file_name_regex: "jpg$")
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
       end
 
       it 'returns true if file name is allowed' do
         project.create_push_rule(file_name_regex: "exe$")
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        expect { push_changes(changes) }.not_to raise_error
       end
     end
 
     describe "max file size check" do
       let(:start_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
       let(:end_sha)   { 'c84ff944ff4529a70788a5e9003c2b7feae29047' }
+      let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
 
       before do
         project.add_developer(user)
@@ -161,20 +165,20 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
       it "returns false when size is too large" do
         project.create_push_rule(max_file_size: 1)
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.to raise_error(described_class::UnauthorizedError)
+        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 { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        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)
 
-        expect { access.send(:check_push_access!, "#{start_sha} #{end_sha} refs/heads/master") }.not_to raise_error
+        expect { push_changes(changes) }.not_to raise_error
       end
     end
   end
@@ -197,9 +201,19 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
 
       it 'rejects the push' do
         expect do
-          access.send(:check_push_access!, "#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
+          push_changes("#{start_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")
+          end.not_to raise_error
+        end
+      end
     end
 
     context 'when repository size is below the limit' do
@@ -212,7 +226,7 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
       context 'when new change exceeds the limit' do
         it 'rejects the push' do
           expect do
-            access.send(:check_push_access!, "#{start_sha} #{sha_with_2_mb_file} refs/heads/master")
+            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
@@ -220,16 +234,15 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
       context 'when new change does not exceeds the limit' do
         it 'accepts the push' do
           expect do
-            access.send(:check_push_access!, "#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
+            push_changes("#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
           end.not_to raise_error
         end
       end
 
       context 'when a file is modified' do
-        # file created
-        let(:old) { 'd2d430676773caa88cdaf7c55944073b2fd5561a' }
-        # file modified
-        let(:new) { '5f923865dde3436854e9ceb9cdb7815618d4e849' }
+        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
@@ -237,43 +250,35 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
         end
 
         it 'just add the difference between the two versions to the total size' do
-          expect do
-            access.send(:check_push_access!, "#{old} #{new} refs/heads/master")
-          end.not_to raise_error
+          expect { push_changes(changes) }.not_to raise_error
         end
       end
 
       context 'when a file is renamed' do
-        # file deleted
-        let(:old) { '281d3a76f31c812dbf48abce82ccf6860adedd81' }
-        # file added with different name
-        let(:new) { 'c347ca2e140aa667b968e51ed0ffe055501fe4f4' }
+        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
 
         it 'does not modify the total size given the content is the same' do
-          expect do
-            access.send(:check_push_access!, "#{old} #{new} refs/heads/master")
-          end.not_to raise_error
+          expect { push_changes(changes) }.not_to raise_error
         end
       end
 
       context 'when a file is deleted' do
-        # file deleted
-        let(:old) { 'c1acaa58bbcbc3eafe538cb8274ba387047b69f8' }
-        # New changes introduced
-        let(:new) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' }
+        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
 
         it 'subtracts the size of the deleted file before calculate the new total' do
-          expect do
-            access.send(:check_push_access!, "#{old} #{new} refs/heads/master")
-          end.not_to raise_error
+          expect { push_changes(changes) }.not_to raise_error
         end
       end
     end
@@ -281,6 +286,10 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
 
   private
 
+  def push_changes(changes = '_any')
+    access.check('git-receive-pack', changes)
+  end
+
   def raise_unauthorized(message)
     raise_error(Gitlab::GitAccess::UnauthorizedError, message)
   end

lib/gitlab/git_access.rb

@@ -5,6 +5,7 @@ module Gitlab
     prepend ::EE::Gitlab::GitAccess
     include ActionView::Helpers::SanitizeHelper
     include PathLocksHelper
+    include Gitlab::Utils::StrongMemoize
 
     UnauthorizedError = Class.new(StandardError)
     NotFoundError = Class.new(StandardError)
@@ -30,7 +31,7 @@ module Gitlab
     PUSH_COMMANDS = %w{ git-receive-pack }.freeze
     ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
 
-    attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path, :auth_result_type
+    attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path, :auth_result_type, :changes
 
     def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, project_path: nil, redirected_path: nil, auth_result_type: nil)
       @actor    = actor
@@ -44,6 +45,8 @@ module Gitlab
     end
 
     def check(cmd, changes)
+      @changes = changes
+
       check_protocol!
       check_valid_actor!
       check_active_user!
@@ -62,7 +65,7 @@ module Gitlab
       when *DOWNLOAD_COMMANDS
         check_download_access!
       when *PUSH_COMMANDS
-        check_push_access!(changes)
+        check_push_access!
       end
 
       true
@@ -223,7 +226,7 @@ module Gitlab
     end
 
     # TODO: please clean this up
-    def check_push_access!(changes)
+    def check_push_access!
       if project.repository_read_only?
         raise UnauthorizedError, ERROR_MESSAGES[:read_only]
       end
@@ -240,7 +243,7 @@ module Gitlab
 
       return if changes.blank? # Allow access this is needed for EE.
 
-      if project.above_size_limit?
+      if check_size_limit? && project.above_size_limit?
         raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).push_error
       end
 
@@ -249,17 +252,15 @@ module Gitlab
         raise UnauthorizedError, strip_tags(message)
       end
 
-      check_change_access!(changes)
+      check_change_access!
     end
 
-    def check_change_access!(changes)
+    def check_change_access!
       # If there are worktrees with a HEAD pointing to a non-existent object,
       # calls to `git rev-list --all` will fail in git 2.15+. This should also
       # clear stale lock files.
       project.repository.clean_stale_repository_files
 
-      changes_list = Gitlab::ChangesList.new(changes)
-
       push_size_in_bytes = 0
 
       # Iterate over all changes to find if user allowed all of them to be applied
@@ -275,7 +276,7 @@ module Gitlab
         end
       end
 
-      if project.changes_will_exceed_size_limit?(push_size_in_bytes)
+      if check_size_limit? && project.changes_will_exceed_size_limit?(push_size_in_bytes)
         raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).new_changes_error
       end
     end
@@ -368,6 +369,18 @@ module Gitlab
 
     protected
 
+    def check_size_limit?
+      strong_memoize(:check_size_limit) do
+        changes_list.any? do |change|
+          change[:newrev] && change[:newrev] != ::Gitlab::Git::BLANK_SHA
+        end
+      end
+    end
+
+    def changes_list
+      @changes_list ||= Gitlab::ChangesList.new(changes)
+    end
+
     def user
       return @user if defined?(@user)
 

spec/lib/gitlab/git_access_spec.rb

@@ -13,14 +13,6 @@ describe Gitlab::GitAccess do
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
   let(:auth_result_type) { nil }
-
-  let(:access) do
-    described_class.new(actor, project,
-      protocol, authentication_abilities: authentication_abilities,
-                namespace_path: namespace_path, project_path: project_path,
-                redirected_path: redirected_path, auth_result_type: auth_result_type)
-  end
-
   let(:changes) { '_any' }
   let(:push_access_check) { access.check('git-receive-pack', changes) }
   let(:pull_access_check) { access.check('git-upload-pack', changes) }
@@ -786,7 +778,7 @@ describe Gitlab::GitAccess do
 
           aggregate_failures do
             matrix.each do |action, allowed|
-              check = -> { access.send(:check_push_access!, changes[action]) }
+              check = -> { push_changes(changes[action]) }
 
               if allowed
                 expect(&check).not_to raise_error,
@@ -811,7 +803,7 @@ describe Gitlab::GitAccess do
 
           aggregate_failures do
             matrix.each do |action, allowed|
-              check = -> { access.send(:check_push_access!, changes[action]) }
+              check = -> { push_changes(changes[action]) }
 
               if allowed
                 expect(&check).not_to raise_error,
@@ -1280,6 +1272,17 @@ describe Gitlab::GitAccess do
 
   private
 
+  def access
+    described_class.new(actor, project, protocol,
+                        authentication_abilities: authentication_abilities,
+                        namespace_path: namespace_path, project_path: project_path,
+                        redirected_path: redirected_path, auth_result_type: auth_result_type)
+  end
+
+  def push_changes(changes)
+    access.check('git-receive-pack', changes)
+  end
+
   def raise_unauthorized(message)
     raise_error(Gitlab::GitAccess::UnauthorizedError, message)
   end

spec/requests/git_http_spec.rb

@@ -792,23 +792,6 @@ describe 'Git HTTP requests' do
           end
         end
 
-        context "when repository is above size limit" do
-          let(:env) { { user: user.username, password: user.password } }
-
-          before do
-            project.add_master(user)
-          end
-
-          it 'responds with status 403 Forbidden' do
-            allow_any_instance_of(EE::Project)
-              .to receive(:above_size_limit?).and_return(true)
-
-            upload(path, env) do |response|
-              expect(response).to have_gitlab_http_status(:forbidden)
-            end
-          end
-        end
-
         context 'when license is not provided' do
           let(:env) { { user: user.username, password: user.password } }