Merge branch 'fix-rebase-permissions-check' into 'master'

Fix access checks for rebasing an MR

Closes #2891

See merge request !2414

app/controllers/ee/projects/merge_requests_controller.rb

@@ -5,11 +5,10 @@ module EE
 
       prepended do
         before_action :check_merge_request_rebase_available!, only: [:rebase]
+        before_action :check_user_can_push_to_source_branch!, only: [:rebase]
       end
 
       def rebase
-        return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-
         RebaseWorker.perform_async(@merge_request.id, current_user.id)
 
         render nothing: true, status: 200
@@ -64,6 +63,16 @@ module EE
 
         attrs
       end
+
+      def check_user_can_push_to_source_branch!
+        return access_denied! unless @merge_request.source_branch_exists?
+
+        access_check = ::Gitlab::UserAccess
+          .new(current_user, project: @merge_request.source_project)
+          .can_push_to_branch?(@merge_request.source_branch)
+
+        access_denied! unless access_check
+      end
     end
   end
 end

changelogs/unreleased-ee/fix-rebase-permissions-check.yml

+---
+title: Fix rebase button when merge request is created from a fork
+merge_request:
+author:

spec/controllers/projects/merge_requests_controller_ee_spec.rb

@@ -311,13 +311,13 @@ describe Projects::MergeRequestsController do
       post :rebase, namespace_id: project.namespace, project_id: project, id: merge_request
     end
 
-    def expect_rebase_worker
-      expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, viewer.id)
+    def expect_rebase_worker_for(user)
+      expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, user.id)
     end
 
     context 'successfully' do
       it 'enqeues a RebaseWorker' do
-        expect_rebase_worker
+        expect_rebase_worker_for(viewer)
 
         post_rebase
 
@@ -329,7 +329,7 @@ describe Projects::MergeRequestsController do
       let(:project) { create(:project, approvals_before_merge: 1) }
 
       it 'returns 200' do
-        expect_rebase_worker
+        expect_rebase_worker_for(viewer)
 
         post_rebase
 
@@ -337,15 +337,18 @@ describe Projects::MergeRequestsController do
       end
     end
 
-    context 'user cannot merge' do
-      let(:viewer) { create(:user) }
+    context 'with a forked project' do
+      let(:fork_project) { create(:project, forked_from_project: project) }
+      let(:fork_owner) { fork_project.owner }
 
       before do
-        project.add_reporter(viewer)
+        merge_request.update!(source_project: fork_project)
+        fork_project.add_reporter(user)
       end
 
+      context 'user cannot push to source branch' do
         it 'returns 404' do
-        expect_rebase_worker.never
+          expect_rebase_worker_for(viewer).never
 
           post_rebase
 
@@ -353,10 +356,27 @@ describe Projects::MergeRequestsController do
         end
       end
 
+      context 'user can push to source branch' do
+        before do
+          project.add_reporter(fork_owner)
+
+          sign_in(fork_owner)
+        end
+
+        it 'returns 200' do
+          expect_rebase_worker_for(fork_owner)
+
+          post_rebase
+
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+
     context 'rebase unavailable in license' do
       it 'returns 404' do
         stub_licensed_features(merge_request_rebase: false)
-        expect_rebase_worker.never
+        expect_rebase_worker_for(viewer).never
 
         post_rebase