Only allow users that can merge to push to sourcebvl-allow-maintainer-to-push

We only allow users that can merge the merge request to push to the
fork.

4 files changed, 33 insertions, 15 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 59fd2d4e4a0..c2bae379a94 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -865,7 +865,7 @@ class MergeRequest < ActiveRecord::Base
 
   def can_be_merged_by?(user)
     access = ::Gitlab::UserAccess.new(user, project: project)
-    access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
+    access.can_update_branch?(target_branch)
   end
 
   def can_be_merged_via_command_line_by?(user)
diff --git a/app/models/project.rb b/app/models/project.rb
index a2b2e0554b1..5f9d9785d64 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1951,12 +1951,20 @@ class Project < ActiveRecord::Base
   end
 
   def fetch_branch_allows_maintainer_push?(user, branch_name)
+    check_access = -> do
+      merge_request = source_of_merge_requests.opened
+                        .where(allow_maintainer_to_push: true)
+                        .find_by(source_branch: branch_name)
+
+      merge_request&.can_be_merged_by?(user)
+    end
+
     if RequestStore.active?
       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?
+        check_access.call
       end
     else
-      merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
+      check_access.call
     end
   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 c1f76202e60..a3323da1b1f 100644
--- a/spec/features/merge_request/maintainer_edits_fork_spec.rb
+++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb
@@ -18,7 +18,7 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
   end
 
   before do
-    target_project.add_developer(user)
+    target_project.add_master(user)
     sign_in(user)
 
     visit project_merge_request_path(target_project, merge_request)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3463cf2eeca..e970cd7dfdb 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3383,12 +3383,13 @@ describe Project do
 
   context 'with cross project merge requests' do
     let(:user) { create(:user) }
-    let(:target_project) { create(:project) }
-    let(:project) { fork_project(target_project) }
+    let(:target_project) { create(:project, :repository) }
+    let(:project) { fork_project(target_project, nil, repository: true) }
     let!(:merge_request) do
       create(
         :merge_request,
         target_project: target_project,
+        target_branch: 'target-branch',
         source_project: project,
         source_branch: 'awesome-feature-1',
         allow_maintainer_to_push: true
@@ -3429,7 +3430,7 @@ describe Project do
     end
 
     describe '#branch_allows_maintainer_push?' do
-      it 'includes branch names for merge requests allowing maintainer access to a user' do
+      it 'allows access if the user can merge the merge request' do
         expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
           .to be_truthy
       end
@@ -3442,9 +3443,10 @@ describe Project do
           .to be_falsy
       end
 
-      it 'does not include branches for closed MRs' do
+      it 'does not allow access to branches for which the merge request was closed' do
         create(:merge_request, :closed,
                target_project: target_project,
+               target_branch: 'target-branch',
                source_project: project,
                source_branch: 'rejected-feature-1',
                allow_maintainer_to_push: true)
@@ -3453,18 +3455,26 @@ describe Project do
           .to be_falsy
       end
 
-      it 'only queries once per user' do
+      it 'does not allow access if the user cannot merge the merge request' do
+        create(:protected_branch, :masters_can_push, project: target_project, name: 'target-branch')
+
+        expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
+          .to be_falsy
+      end
+
+      it 'caches the result' do
+        control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') }
+
         expect { 3.times { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
-          .not_to exceed_query_limit(1)
+          .not_to exceed_query_limit(control)
       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
+        it 'only queries per project across instances' do
+          control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') }
+
           expect { 2.times { described_class.find(project.id).branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
-            .not_to exceed_query_limit(3)
+            .not_to exceed_query_limit(control).with_threshold(2)
         end
       end
     end