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

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

See merge request gitlab-org/gitlab-ce!23990

14 files changed, 101 insertions, 79 deletions

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index c0aa39d87c6..30e436365de 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -80,9 +80,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
 
diff --git a/app/models/project.rb b/app/models/project.rb
index cd558752080..a1a8ae7fc54 100644
--- a/app/models/project.rb
+++ b/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
diff --git a/lib/gitlab/checks/base_checker.rb b/lib/gitlab/checks/base_checker.rb
index 7fbcf6a4ff4..09b17b5b76b 100644
--- a/lib/gitlab/checks/base_checker.rb
+++ b/lib/gitlab/checks/base_checker.rb
@@ -18,12 +18,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?
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 7778d3068cc..8a57a3a6d9a 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -10,7 +10,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)
@@ -18,7 +18,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
 
@@ -27,8 +26,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
diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb
index 63da9a3d6b5..ea0d8c85a66 100644
--- a/lib/gitlab/checks/diff_check.rb
+++ b/lib/gitlab/checks/diff_check.rb
@@ -11,7 +11,7 @@ module Gitlab
       }.freeze
 
       def validate!
-        return if deletion? || newrev.nil?
+        return if deletion?
         return unless should_run_diff_validations?
         return if commits.empty?
 
diff --git a/lib/gitlab/checks/push_check.rb b/lib/gitlab/checks/push_check.rb
index f3a52f09868..91f8d0bdbc8 100644
--- a/lib/gitlab/checks/push_check.rb
+++ b/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
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 802fa65dd63..010bd0e520c 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -12,6 +12,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.',
@@ -24,7 +28,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
@@ -199,7 +204,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)
 
@@ -256,24 +261,34 @@ module Gitlab
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 
-      return if changes.blank? # Allow access this is needed for EE.
-
       check_change_access!
     end
 
     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
-
-      # 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)
+      # Deploy keys with write access can push anything
+      return if deploy_key?
+
+      if changes == ANY
+        can_push = user_access.can_do_action?(:push_code) ||
+          project.any_branch_allows_collaboration?(user_access.user)
+
+        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
     end
 
@@ -282,7 +297,6 @@ module Gitlab
         change,
         user_access: user_access,
         project: project,
-        skip_authorization: deploy_key?,
         skip_lfs_integrity_check: skip_lfs_integrity_check,
         protocol: protocol,
         logger: logger
@@ -348,7 +362,7 @@ module Gitlab
     protected
 
     def changes_list
-      @changes_list ||= Gitlab::ChangesList.new(changes)
+      @changes_list ||= Gitlab::ChangesList.new(changes == ANY ? [] : changes)
     end
 
     def user
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 3f24001e4ee..0af91957fa8 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -15,7 +15,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
diff --git a/spec/lib/gitlab/checks/push_check_spec.rb b/spec/lib/gitlab/checks/push_check_spec.rb
index 25f0d428cb9..e1bd52d6c0b 100644
--- a/spec/lib/gitlab/checks/push_check_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 8d8eb50ad76..3e34dd592f2 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/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} "\
@@ -798,6 +799,7 @@ describe Gitlab::GitAccess do
 
     permissions_matrix = {
       admin: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: true,
@@ -809,6 +811,7 @@ describe Gitlab::GitAccess do
       },
 
       maintainer: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: true,
@@ -820,6 +823,7 @@ describe Gitlab::GitAccess do
       },
 
       developer: {
+        any: true,
         push_new_branch: true,
         push_master: true,
         push_protected_branch: false,
@@ -831,6 +835,7 @@ describe Gitlab::GitAccess do
       },
 
       reporter: {
+        any: false,
         push_new_branch: false,
         push_master: false,
         push_protected_branch: false,
@@ -842,6 +847,7 @@ describe Gitlab::GitAccess do
       },
 
       guest: {
+        any: false,
         push_new_branch: false,
         push_master: false,
         push_protected_branch: false,
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 9c6c9fe13bf..6ba65b56618 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/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)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4b6592020c1..fa0f3620806 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3831,6 +3831,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,
@@ -3875,14 +3885,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
 
@@ -3913,13 +3932,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') }
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index cf57776346a..79aa32b29bb 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/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(34)
+          expect(recorded.count).to be_within(2).of(38)
           expect(recorded.cached_count).to eq(0)
         end
       end
diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb
index 55445e71539..75042b29bea 100644
--- a/spec/services/ci/retry_pipeline_service_spec.rb
+++ b/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)