Limit queries to a user-branch combination

The query becomes a lot simpler if we can check the branch name as
well instead of having to load all branch names.

8 files changed, 109 insertions, 65 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 0128967e038..a2b2e0554b1 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1800,12 +1800,31 @@ class Project < ActiveRecord::Base
     Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
   end
 
-  def branches_allowing_maintainer_access_to_user(user)
-    @branches_allowing_maintainer_access_to_user ||= Hash.new do |result, user|
-      result[user] = fetch_branches_allowing_maintainer_access_to_user(user)
+  def merge_requests_allowing_push_to_user(user)
+    return MergeRequest.none unless user
+
+    developer_access_exists = user.project_authorizations
+                                .where('access_level >= ? ', Gitlab::Access::DEVELOPER)
+                                .where('project_authorizations.project_id = merge_requests.target_project_id')
+                                .limit(1)
+                                .select(1)
+    source_of_merge_requests.opened
+      .where(allow_maintainer_to_push: true)
+      .where('EXISTS (?)', developer_access_exists)
+  end
+
+  def branch_allows_maintainer_push?(user, branch_name)
+    return false unless user
+
+    cache_key = "user:#{user.id}:#{branch_name}:branch_allows_push"
+
+    memoized_results = strong_memoize(:branch_allows_maintainer_push) do
+      Hash.new do |result, cache_key|
+        result[cache_key] = fetch_branch_allows_maintainer_push?(user, branch_name)
+      end
     end
 
-    @branches_allowing_maintainer_access_to_user[user]
+    memoized_results[cache_key]
   end
 
   private
@@ -1931,23 +1950,13 @@ class Project < ActiveRecord::Base
     raise ex
   end
 
-  def fetch_branches_allowing_maintainer_access_to_user(user)
-    return [] unless user
-
-    projects_with_developer_access = user.project_authorizations
-                                       .where('access_level >= ? ', Gitlab::Access::DEVELOPER)
-                                       .select(:project_id)
-    merge_requests_allowing_push = source_of_merge_requests.opened
-                                     .where(allow_maintainer_to_push: true)
-                                     .where(target_project_id: projects_with_developer_access)
-                                     .select(:source_branch).uniq
-
+  def fetch_branch_allows_maintainer_push?(user, branch_name)
     if RequestStore.active?
-      RequestStore.fetch("project-#{id}:user-#{user.id}:branches_allowing_maintainer_access") do
-        merge_requests_allowing_push.pluck(:source_branch)
+      RequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_maintainer_push") do
+        merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
       end
     else
-      merge_requests_allowing_push.pluck(:source_branch)
+      merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
     end
   end
 end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index e143a32e350..57ab0c23dcd 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -61,9 +61,9 @@ class ProjectPolicy < BasePolicy
   desc "Project has request access enabled"
   condition(:request_access_enabled, scope: :subject) { project.request_access_enabled }
 
-  desc "The project has merge requests open that allow external users to push"
-  condition(:merge_request_allows_push, scope: :subject) do
-    project.branches_allowing_maintainer_access_to_user(@user).any?
+  desc "Has merge requests allowing pushes to user"
+  condition(:has_merge_requests_allowing_pushes, scope: :subject) do
+    project.merge_requests_allowing_push_to_user(user).any?
   end
 
   features = %w[
@@ -245,7 +245,6 @@ class ProjectPolicy < BasePolicy
 
   rule { repository_disabled }.policy do
     prevent :push_code
-    prevent :push_single_branch
     prevent :download_code
     prevent :fork_project
     prevent :read_commit_status
@@ -298,21 +297,14 @@ class ProjectPolicy < BasePolicy
   end
 
   # These rules are included to allow maintainers of projects to push to certain
-  # branches of forks.
-  rule { can?(:public_access) & merge_request_allows_push }.policy do
-    enable :push_single_branch
+  # to run pipelines for the branches they have access to.
+  rule { can?(:public_access) & has_merge_requests_allowing_pushes }.policy do
     enable :create_build
     enable :update_build
     enable :create_pipeline
     enable :update_pipeline
   end
 
-  # A wrapper around `push_code` and `push_single_branch` to avoid several
-  # `push_code`: User can push everything to the repo
-  # `push_single_brach`: User can push to a single branch in the repo
-  # `push_to_repo`: User can push something to this repo.
-  rule { can?(:push_code) | can?(:push_single_branch) }.enable :push_to_repo
-
   private
 
   def team_member?
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 87e9e47b21a..51ba09aa129 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -47,7 +47,7 @@ module Gitlab
       protected
 
       def push_checks
-        if user_access.cannot_do_action?(:push_to_repo)
+        unless can_push?
           raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
         end
       end
@@ -183,6 +183,11 @@ module Gitlab
       def commits
         @commits ||= project.repository.new_commits(newrev)
       end
+
+      def can_push?
+        user_access.can_do_action?(:push_code) ||
+          user_access.can_push_to_branch?(branch_name)
+      end
     end
   end
 end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index fa32776d9f8..fd68b9f2b48 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -70,10 +70,8 @@ module Gitlab
         protected_branch_accessible_to?(ref, action: :push)
       elsif user.can?(:push_code, project)
         true
-      elsif user.can?(:push_single_branch, project)
-        project.branches_allowing_maintainer_access_to_user(user).include?(ref)
       else
-        false
+        project.branch_allows_maintainer_push?(user, ref)
       end
     end
 
diff --git a/spec/features/merge_request/maintainer_edits_fork_spec.rb b/spec/features/merge_request/maintainer_edits_fork_spec.rb
index d11ccb46a26..c1f76202e60 100644
--- a/spec/features/merge_request/maintainer_edits_fork_spec.rb
+++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe 'a maintainer edits files on a source-branch of an MR from a fork', :js do
   include ProjectForksHelper
-  let(:user) { create(:user) }
+  let(:user) { create(:user, username: 'the-maintainer') }
   let(:target_project) { create(:project, :public, :repository) }
   let(:author) { create(:user, username: 'mr-authoring-machine') }
   let(:source_project) { fork_project(target_project, author, repository: true) }
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index 2f173d2e75c..48e9902027c 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -32,7 +32,8 @@ describe Gitlab::Checks::ChangeAccess 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_to_repo).and_return(false)
+        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 { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
       end
@@ -42,7 +43,7 @@ describe Gitlab::Checks::ChangeAccess do
       let(:ref) { 'refs/tags/v1.0.0' }
 
       it 'raises an error if the user is not allowed to update tags' do
-        allow(user_access).to receive(:can_do_action?).with(:push_to_repo).and_return(true)
+        allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
         expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
 
         expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a95a4637234..3463cf2eeca 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3381,8 +3381,8 @@ describe Project do
     end
   end
 
-  describe '#branches_allowing_maintainer_access_to_user' do
-    let(:maintainer) { create(:user) }
+  context 'with cross project merge requests' do
+    let(:user) { create(:user) }
     let(:target_project) { create(:project) }
     let(:project) { fork_project(target_project) }
     let!(:merge_request) do
@@ -3396,37 +3396,76 @@ describe Project do
     end
 
     before do
-      target_project.add_developer(maintainer)
+      target_project.add_developer(user)
     end
 
-    it 'includes branch names for merge requests allowing maintainer access to a user' do
-      expect(project.branches_allowing_maintainer_access_to_user(maintainer))
-        .to include('awesome-feature-1')
-    end
+    describe '#merge_requests_allowing_push_to_user' do
+      it 'returns open merge requests for which the user has developer access to the target project' do
+        expect(project.merge_requests_allowing_push_to_user(user)).to include(merge_request)
+      end
 
-    it 'does not include branches for closed MRs' do
-      create(:merge_request, :closed,
-             target_project: target_project,
-             source_project: project,
-             source_branch: 'rejected-feature-1',
-             allow_maintainer_to_push: true)
+      it 'does not include closed merge requests' do
+        merge_request.close
 
-      expect(project.branches_allowing_maintainer_access_to_user(maintainer))
-        .not_to include('rejected-feature-1')
-    end
+        expect(project.merge_requests_allowing_push_to_user(user)).to be_empty
+      end
+
+      it 'does not include merge requests for guest users' do
+        guest = create(:user)
+        target_project.add_guest(guest)
 
-    it 'only queries once per user' do
-      expect { 3.times { project.branches_allowing_maintainer_access_to_user(maintainer) } }
-        .not_to exceed_query_limit(1)
+        expect(project.merge_requests_allowing_push_to_user(guest)).to be_empty
+      end
+
+      it 'does not include the merge request for other users' do
+        other_user = create(:user)
+
+        expect(project.merge_requests_allowing_push_to_user(other_user)).to be_empty
+      end
+
+      it 'is empty when no user is passed' do
+        expect(project.merge_requests_allowing_push_to_user(nil)).to be_empty
+      end
     end
 
-    context 'when the requeststore is active', :request_store do
-      it 'only queries once per user accross project instances' do
-        # limiting to 3 queries:
-        # 2 times loading the project
-        # once loading the accessible branches
-        expect { 2.times { described_class.find(project.id).branches_allowing_maintainer_access_to_user(maintainer) } }
-          .not_to exceed_query_limit(3)
+    describe '#branch_allows_maintainer_push?' do
+      it 'includes branch names for merge requests allowing maintainer access to a user' do
+        expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
+          .to be_truthy
+      end
+
+      it 'does not allow guest users access' do
+        guest = create(:user)
+        target_project.add_guest(guest)
+
+        expect(project.branch_allows_maintainer_push?(guest, 'awesome-feature-1'))
+          .to be_falsy
+      end
+
+      it 'does not include branches for closed MRs' do
+        create(:merge_request, :closed,
+               target_project: target_project,
+               source_project: project,
+               source_branch: 'rejected-feature-1',
+               allow_maintainer_to_push: true)
+
+        expect(project.branch_allows_maintainer_push?(user, 'rejected-feature-1'))
+          .to be_falsy
+      end
+
+      it 'only queries once per user' do
+        expect { 3.times { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
+          .not_to exceed_query_limit(1)
+      end
+
+      context 'when the requeststore is active', :request_store do
+        it 'only queries once per user accross project instances' do
+          # limiting to 3 queries:
+          # 2 times loading the project
+          # once loading the accessible branches
+          expect { 2.times { described_class.find(project.id).branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
+            .not_to exceed_query_limit(3)
+        end
       end
     end
   end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index f449fbd6b52..ea76e604153 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -323,7 +323,7 @@ describe ProjectPolicy do
       )
     end
     let(:maintainer_abilities) do
-      %w(push_single_branch create_build update_build create_pipeline update_pipeline)
+      %w(create_build update_build create_pipeline update_pipeline)
     end
 
     subject { described_class.new(user, project) }