Merge branch 'dm-git-access-any' into 'master'

Don't run checks for changed refs when specific changes are unknown

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

app/controllers/projects/git_http_controller.rb

@@ -81,9 +81,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def access_check
-    # Use the magic string '_any' to indicate we do not know what the
-    # changes are. This is also what gitlab-shell does.
-    access.check(git_command, '_any')
+    access.check(git_command, Gitlab::GitAccess::ANY)
     @project ||= access.project
   end
 

app/models/project.rb

@@ -1928,23 +1928,15 @@ class Project < ActiveRecord::Base
                                 .where('project_authorizations.project_id = merge_requests.target_project_id')
                                 .limit(1)
                                 .select(1)
-    source_of_merge_requests.opened
-      .where(allow_collaboration: true)
-      .where('EXISTS (?)', developer_access_exists)
+    merge_requests_allowing_collaboration.where('EXISTS (?)', developer_access_exists)
   end
 
-  def branch_allows_collaboration?(user, branch_name)
-    return false unless user
-
-    cache_key = "user:#{user.id}:#{branch_name}:branch_allows_push"
-
-    memoized_results = strong_memoize(:branch_allows_collaboration) do
-      Hash.new do |result, cache_key|
-        result[cache_key] = fetch_branch_allows_collaboration?(user, branch_name)
-      end
-    end
+  def any_branch_allows_collaboration?(user)
+    fetch_branch_allows_collaboration(user)
+  end
 
-    memoized_results[cache_key]
+  def branch_allows_collaboration?(user, branch_name)
+    fetch_branch_allows_collaboration(user, branch_name)
   end
 
   def licensed_features
@@ -2018,6 +2010,12 @@ class Project < ActiveRecord::Base
 
   private
 
+  def merge_requests_allowing_collaboration(source_branch = nil)
+    relation = source_of_merge_requests.opened.where(allow_collaboration: true)
+    relation = relation.where(source_branch: source_branch) if source_branch
+    relation
+  end
+
   def create_new_pool_repository
     pool = begin
              create_pool_repository!(shard: Shard.by_name(repository_storage), source_project: self)
@@ -2142,26 +2140,19 @@ class Project < ActiveRecord::Base
     raise ex
   end
 
-  def fetch_branch_allows_collaboration?(user, branch_name)
-    check_access = -> do
-      next false if empty_repo?
+  def fetch_branch_allows_collaboration(user, branch_name = nil)
+    return false unless user
 
-      merge_requests = source_of_merge_requests.opened
-                         .where(allow_collaboration: true)
+    Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
+      next false if empty_repo?
 
       # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/49322
       Gitlab::GitalyClient.allow_n_plus_1_calls do
-        if branch_name
-          merge_requests.find_by(source_branch: branch_name)&.can_be_merged_by?(user)
-        else
-          merge_requests.any? { |merge_request| merge_request.can_be_merged_by?(user) }
+        merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
+          merge_request.can_be_merged_by?(user)
         end
       end
     end
-
-    Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
-      check_access.call
-    end
   end
 
   def services_templates

ee/lib/ee/gitlab/checks/push_rules/branch_check.rb

@@ -9,7 +9,7 @@ module EE
           LOG_MESSAGE = "Checking if branch follows the naming patterns defined by the project...".freeze
 
           def validate!
-            return unless newrev && oldrev && push_rule
+            return unless push_rule
 
             logger.log_timed(LOG_MESSAGE) do
               unless branch_name_allowed_by_push_rule?

ee/lib/ee/gitlab/checks/push_rules/commit_check.rb

@@ -13,7 +13,7 @@ module EE
           LOG_MESSAGE = "Checking if commits follow defined push rules...".freeze
 
           def validate!
-            return unless newrev && oldrev && push_rule
+            return unless push_rule
 
             commit_validation = push_rule.commit_validation?
             # if newrev is blank, the branch was deleted

ee/lib/ee/gitlab/checks/push_rules/tag_check.rb

@@ -6,7 +6,7 @@ module EE
       module PushRules
         class TagCheck < ::Gitlab::Checks::BaseChecker
           def validate!
-            return unless newrev && oldrev && push_rule
+            return unless push_rule
 
             logger.log_timed("Checking if you are allowed to delete a tag...") do
               if tag_deletion_denied_by_push_rule?

ee/lib/ee/gitlab/git_access.rb

@@ -38,6 +38,17 @@ module EE
         super
       end
 
+      override :check_change_access!
+      def check_change_access!
+        check_size_before_push!
+
+        check_if_license_blocks_changes!
+
+        super
+
+        check_push_size!
+      end
+
       override :check_active_user!
       def check_active_user!
         return if geo?
@@ -54,6 +65,45 @@ module EE
       def geo?
         actor == :geo
       end
+
+      def check_size_before_push!
+        if check_size_limit? && project.above_size_limit?
+          raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).push_error
+        end
+      end
+
+      def check_if_license_blocks_changes!
+        if ::License.block_changes?
+          message = ::LicenseHelper.license_message(signed_in: true, is_admin: (user && user.admin?))
+          raise ::Gitlab::GitAccess::UnauthorizedError, strip_tags(message)
+        end
+      end
+
+      def check_push_size!
+        return unless check_size_limit?
+
+        # 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
+
+        push_size_in_bytes = 0
+
+        changes_list.each do |change|
+          push_size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size) # rubocop: disable CodeReuse/ActiveRecord
+
+          if project.changes_will_exceed_size_limit?(push_size_in_bytes)
+            raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error
+          end
+        end
+      end
+
+      def check_size_limit?
+        strong_memoize(:check_size_limit) do
+          project.size_limit_enabled? &&
+            changes_list.any? { |change| !::Gitlab::Git.blank_ref?(change[:newrev]) }
+        end
+      end
     end
   end
 end

ee/spec/lib/gitlab/git_access_spec.rb

@@ -253,7 +253,7 @@ describe Gitlab::GitAccess do
     let(:actor) { :geo }
 
     it { expect { pull_changes }.not_to raise_error }
-    it { expect { push_changes }.to raise_unauthorized(Gitlab::GitAccess::ERROR_MESSAGES[:push_code]) }
+    it { expect { push_changes }.to raise_unauthorized(Gitlab::GitAccess::ERROR_MESSAGES[:upload]) }
   end
 
   private

ee/spec/lib/gitlab/git_access_wiki_spec.rb

@@ -37,7 +37,7 @@ describe Gitlab::GitAccessWiki do
 
   private
 
-  def push_changes(changes = '_any')
+  def push_changes(changes = Gitlab::GitAccess::ANY)
     access.check('git-receive-pack', changes)
   end
 

lib/gitlab/checks/base_checker.rb

@@ -19,12 +19,16 @@ module Gitlab
 
       private
 
+      def creation?
+        Gitlab::Git.blank_ref?(oldrev)
+      end
+
       def deletion?
         Gitlab::Git.blank_ref?(newrev)
       end
 
       def update?
-        !Gitlab::Git.blank_ref?(oldrev) && !deletion?
+        !creation? && !deletion?
       end
 
       def updated_from_web?

lib/gitlab/checks/change_access.rb

@@ -12,7 +12,7 @@ module Gitlab
       attr_reader(*ATTRIBUTES)
 
       def initialize(
-        change, user_access:, project:, skip_authorization: false,
+        change, user_access:, project:,
         skip_lfs_integrity_check: false, protocol:, logger:
       )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@@ -20,7 +20,6 @@ module Gitlab
         @tag_name = Gitlab::Git.tag_name(@ref)
         @user_access = user_access
         @project = project
-        @skip_authorization = skip_authorization
         @skip_lfs_integrity_check = skip_lfs_integrity_check
         @protocol = protocol
 
@@ -29,8 +28,6 @@ module Gitlab
       end
 
       def exec
-        return true if skip_authorization
-
         ref_level_checks
         # Check of commits should happen as the last step
         # given they're expensive in terms of performance

lib/gitlab/checks/diff_check.rb

@@ -12,7 +12,7 @@ module Gitlab
       }.freeze
 
       def validate!
-        return if deletion? || newrev.nil?
+        return if deletion?
         return unless should_run_diff_validations?
         return if commits.empty?
 

lib/gitlab/checks/push_check.rb

@@ -6,7 +6,7 @@ module Gitlab
       def validate!
         logger.log_timed("Checking if you are allowed to push...") do
           unless can_push?
-            raise GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.'
+            raise GitAccess::UnauthorizedError, GitAccess::ERROR_MESSAGES[:push_code]
           end
         end
       end
@@ -15,7 +15,7 @@ module Gitlab
 
       def can_push?
         user_access.can_do_action?(:push_code) ||
-          user_access.can_push_to_branch?(branch_name)
+          project.branch_allows_collaboration?(user_access.user, branch_name)
       end
     end
   end

lib/gitlab/git_access.rb

@@ -15,6 +15,10 @@ module Gitlab
     TimeoutError = Class.new(StandardError)
     ProjectMovedError = Class.new(NotFoundError)
 
+    # Use the magic string '_any' to indicate we do not know what the
+    # changes are. This is also what gitlab-shell does.
+    ANY = '_any'
+
     ERROR_MESSAGES = {
       upload: 'You are not allowed to upload code for this project.',
       download: 'You are not allowed to download code from this project.',
@@ -27,7 +31,8 @@ module Gitlab
       upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
       receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
       read_only: 'The repository is temporarily read-only. Please try again later.',
-      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance."
+      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.",
+      push_code: 'You are not allowed to push code to this project.'
     }.freeze
 
     INTERNAL_TIMEOUT = 50.seconds.freeze
@@ -202,7 +207,7 @@ module Gitlab
 
     def ensure_project_on_push!(cmd, changes)
       return if project || deploy_key?
-      return unless receive_pack?(cmd) && changes == '_any' && authentication_abilities.include?(:push_code)
+      return unless receive_pack?(cmd) && changes == ANY && authentication_abilities.include?(:push_code)
 
       namespace = Namespace.find_by_full_path(namespace_path)
 
@@ -263,54 +268,42 @@ module Gitlab
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 
-      return if changes.blank? # Allow access this is needed for EE.
-
-      if check_size_limit? && project.above_size_limit?
-        raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).push_error
-      end
-
-      if ::License.block_changes?
-        message = ::LicenseHelper.license_message(signed_in: true, is_admin: (user && user.admin?))
-        raise UnauthorizedError, strip_tags(message)
-      end
-
       check_change_access!
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     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
-
-      push_size_in_bytes = 0
-
-      # Iterate over all changes to find if user allowed all of them to be applied
-      changes_list.each.with_index do |change, index|
-        first_change = index == 0
+      # Deploy keys with write access can push anything
+      return if deploy_key?
 
-        # If user does not have access to make at least one change, cancel all
-        # push by allowing the exception to bubble up
-        check_single_change_access(change, skip_lfs_integrity_check: !first_change)
+      if changes == ANY
+        can_push = user_access.can_do_action?(:push_code) ||
+          project.any_branch_allows_collaboration?(user_access.user)
 
-        if project.size_limit_enabled?
-          push_size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size)
+        unless can_push
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
+        end
+      else
+        # 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
+
+        # Iterate over all changes to find if user allowed all of them to be applied
+        changes_list.each.with_index do |change, index|
+          first_change = index == 0
+
+          # If user does not have access to make at least one change, cancel all
+          # push by allowing the exception to bubble up
+          check_single_change_access(change, skip_lfs_integrity_check: !first_change)
         end
-      end
-
-      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
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def check_single_change_access(change, skip_lfs_integrity_check: false)
       change_access = Checks::ChangeAccess.new(
         change,
         user_access: user_access,
         project: project,
-        skip_authorization: deploy_key?,
         skip_lfs_integrity_check: skip_lfs_integrity_check,
         protocol: protocol,
         logger: logger
@@ -375,16 +368,8 @@ 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)
+      @changes_list ||= Gitlab::ChangesList.new(changes == ANY ? [] : changes)
     end
 
     def user

lib/gitlab/git_access_wiki.rb

@@ -17,7 +17,7 @@ module Gitlab
       authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code)
     end
 
-    def check_single_change_access(change, _options = {})
+    def check_change_access!
       unless user_access.can_do_action?(:create_wiki)
         raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
       end

spec/lib/gitlab/checks/push_check_spec.rb

@@ -13,7 +13,7 @@ describe Gitlab::Checks::PushCheck do
     context 'when the user is not allowed to push to the repo' do
       it 'raises an error' do
         expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
-        expect(user_access).to receive(:can_push_to_branch?).with('master').and_return(false)
+        expect(project).to receive(:branch_allows_collaboration?).with(user_access.user, 'master').and_return(false)
 
         expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
       end

spec/lib/gitlab/git_access_spec.rb

@@ -14,7 +14,7 @@ describe Gitlab::GitAccess do
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
   let(:auth_result_type) { nil }
-  let(:changes) { '_any' }
+  let(:changes) { Gitlab::GitAccess::ANY }
   let(:push_access_check) { access.check('git-receive-pack', changes) }
   let(:pull_access_check) { access.check('git-upload-pack', changes) }
 
@@ -437,7 +437,7 @@ describe Gitlab::GitAccess do
         let(:project) { nil }
 
         context 'when changes is _any' do
-          let(:changes) { '_any' }
+          let(:changes) { Gitlab::GitAccess::ANY }
 
           context 'when authentication abilities include push code' do
             let(:authentication_abilities) { [:push_code] }
@@ -483,7 +483,7 @@ describe Gitlab::GitAccess do
       end
 
       context 'when project exists' do
-        let(:changes) { '_any' }
+        let(:changes) { Gitlab::GitAccess::ANY }
         let!(:project) { create(:project) }
 
         it 'does not create a new project' do
@@ -497,7 +497,7 @@ describe Gitlab::GitAccess do
         let(:project_path) { "nonexistent" }
         let(:project) { nil }
         let(:namespace_path) { user.namespace.path }
-        let(:changes) { '_any' }
+        let(:changes) { Gitlab::GitAccess::ANY }
 
         it 'does not create a new project' do
           expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
@@ -507,7 +507,7 @@ describe Gitlab::GitAccess do
 
     context 'when pull' do
       let(:cmd) { 'git-upload-pack' }
-      let(:changes) { '_any' }
+      let(:changes) { Gitlab::GitAccess::ANY }
 
       context 'when project does not exist' do
         let(:project_path) { "new-project" }
@@ -736,7 +736,8 @@ describe Gitlab::GitAccess do
     end
 
     let(:changes) do
-      { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
+      { any: Gitlab::GitAccess::ANY,
+        push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
         push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
         push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
         push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
@@ -823,6 +824,7 @@ describe Gitlab::GitAccess do
 
     permissions_matrix = {
       admin: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: true,
@@ -834,6 +836,7 @@ describe Gitlab::GitAccess do
       },
 
       maintainer: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: true,
@@ -845,6 +848,7 @@ describe Gitlab::GitAccess do
       },
 
       developer: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: false,
@@ -856,6 +860,7 @@ describe Gitlab::GitAccess do
       },
 
       reporter: {
+        any: false,
         push_new_branch: false,
         push_master: false,
         push_protected_branch: false,
@@ -867,6 +872,7 @@ describe Gitlab::GitAccess do
       },
 
       guest: {
+        any: false,
         push_new_branch: false,
         push_master: false,
         push_protected_branch: false,

spec/lib/gitlab/git_access_wiki_spec.rb

@@ -38,7 +38,7 @@ describe Gitlab::GitAccessWiki do
   end
 
   describe '#access_check_download!' do
-    subject { access.check('git-upload-pack', '_any') }
+    subject { access.check('git-upload-pack', Gitlab::GitAccess::ANY) }
 
     before do
       project.add_developer(user)

spec/models/project_spec.rb

@@ -4112,6 +4112,16 @@ describe Project do
     let(:user) { create(:user) }
     let(:target_project) { create(:project, :repository) }
     let(:project) { fork_project(target_project, nil, repository: true) }
+    let!(:local_merge_request) do
+      create(
+        :merge_request,
+        target_project: project,
+        target_branch: 'target-branch',
+        source_project: project,
+        source_branch: 'awesome-feature-1',
+        allow_collaboration: true
+      )
+    end
     let!(:merge_request) do
       create(
         :merge_request,
@@ -4156,14 +4166,23 @@ describe Project do
       end
     end
 
-    describe '#branch_allows_collaboration_push?' do
-      it 'allows access if the user can merge the merge request' do
-        expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
+    describe '#any_branch_allows_collaboration?' do
+      it 'allows access when there are merge requests open allowing collaboration' do
+        expect(project.any_branch_allows_collaboration?(user))
           .to be_truthy
       end
 
-      it 'allows access when there are merge requests open but no branch name is given' do
-        expect(project.branch_allows_collaboration?(user, nil))
+      it 'does not allow access when there are no merge requests open allowing collaboration' do
+        merge_request.close!
+
+        expect(project.any_branch_allows_collaboration?(user))
+          .to be_falsey
+      end
+    end
+
+    describe '#branch_allows_collaboration?' do
+      it 'allows access if the user can merge the merge request' do
+        expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
           .to be_truthy
       end
 
@@ -4194,13 +4213,6 @@ describe Project do
           .to be_falsy
       end
 
-      it 'caches the result' do
-        control = ActiveRecord::QueryRecorder.new { project.branch_allows_collaboration?(user, 'awesome-feature-1') }
-
-        expect { 3.times { project.branch_allows_collaboration?(user, 'awesome-feature-1') } }
-          .not_to exceed_query_limit(control)
-      end
-
       context 'when the requeststore is active', :request_store do
         it 'only queries per project across instances' do
           control = ActiveRecord::QueryRecorder.new { project.branch_allows_collaboration?(user, 'awesome-feature-1') }

spec/serializers/pipeline_serializer_spec.rb

@@ -144,7 +144,7 @@ describe PipelineSerializer do
           # pipeline. With the same ref this check is cached but if refs are
           # different then there is an extra query per ref
           # https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
-          expect(recorded.count).to be_within(2).of(40)
+          expect(recorded.count).to be_within(2).of(44)
           expect(recorded.cached_count).to eq(0)
         end
       end

spec/services/ci/retry_pipeline_service_spec.rb

@@ -285,7 +285,7 @@ describe Ci::RetryPipelineService, '#execute' do
     end
 
     it 'allows to retry failed pipeline' do
-      allow_any_instance_of(Project).to receive(:fetch_branch_allows_collaboration?).and_return(true)
+      allow_any_instance_of(Project).to receive(:branch_allows_collaboration?).and_return(true)
       allow_any_instance_of(Project).to receive(:empty_repo?).and_return(false)
 
       service.execute(pipeline)