From 334d4b1c01d16c01c2a8e616a4c4af0d73c5bc3e Mon Sep 17 00:00:00 2001 From: Sean McGivern <sean@gitlab.com> Date: Wed, 12 Jul 2017 12:21:44 +0100 Subject: [PATCH] Fix access checks for rebasing an MR This has been broken since 34457b30, when the feature was implemented: a user who can merge the MR may not have access to push to the source branch, and vice versa. This makes the backend check the correct thing (which the UI already did): whether the user can push to the source branch. --- .../ee/projects/merge_requests_controller.rb | 13 +++++- .../fix-rebase-permissions-check.yml | 4 ++ .../merge_requests_controller_ee_spec.rb | 44 ++++++++++++++----- 3 files changed, 47 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased-ee/fix-rebase-permissions-check.yml diff --git a/app/controllers/ee/projects/merge_requests_controller.rb b/app/controllers/ee/projects/merge_requests_controller.rb index 631ead88eba..4cafe04f4a9 100644 --- a/app/controllers/ee/projects/merge_requests_controller.rb +++ b/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 diff --git a/changelogs/unreleased-ee/fix-rebase-permissions-check.yml b/changelogs/unreleased-ee/fix-rebase-permissions-check.yml new file mode 100644 index 00000000000..7de1082172e --- /dev/null +++ b/changelogs/unreleased-ee/fix-rebase-permissions-check.yml @@ -0,0 +1,4 @@ +--- +title: Fix rebase button when merge request is created from a fork +merge_request: +author: diff --git a/spec/controllers/projects/merge_requests_controller_ee_spec.rb b/spec/controllers/projects/merge_requests_controller_ee_spec.rb index 7000ddd081a..1cf88c72b1f 100644 --- a/spec/controllers/projects/merge_requests_controller_ee_spec.rb +++ b/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,26 +337,46 @@ 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 - it 'returns 404' do - expect_rebase_worker.never + context 'user cannot push to source branch' do + it 'returns 404' do + expect_rebase_worker_for(viewer).never - post_rebase + post_rebase - expect(response.status).to eq(404) + expect(response.status).to eq(404) + 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 -- 2.30.9