Commit 105212ce authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Check authorization in merge services

Move authorization checks to merge services
instead relying solely on external checks.
parent 1ad69967
......@@ -27,6 +27,10 @@ def source
private
def raise_error(message)
raise MergeError, message
end
def handle_merge_error(*args)
# No-op
end
......
......@@ -18,7 +18,7 @@ def execute(merge_request)
@merge_request = merge_request
error_check!
validate!
merge_request.in_locked_state do
if commit
......@@ -34,6 +34,17 @@ def execute(merge_request)
private
def validate!
authorization_check!
error_check!
end
def authorization_check!
unless @merge_request.can_be_merged_by?(current_user)
raise_error('You are not allowed to merge this merge request')
end
end
def error_check!
error =
if @merge_request.should_be_rebased?
......@@ -44,7 +55,7 @@ def error_check!
'No source for merge'
end
raise MergeError, error if error
raise_error(error) if error
end
def commit
......@@ -54,7 +65,7 @@ def commit
if commit_id
log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}")
else
raise MergeError, 'Conflicts detected during merge'
raise_error('Conflicts detected during merge')
end
merge_request.update!(merge_commit_sha: commit_id)
......@@ -64,10 +75,10 @@ def try_merge
repository.merge(current_user, source, merge_request, commit_message)
rescue Gitlab::Git::PreReceiveError => e
handle_merge_error(log_message: e.message)
raise MergeError, 'Something went wrong during merge pre-receive hook'
raise_error('Something went wrong during merge pre-receive hook')
rescue => e
handle_merge_error(log_message: e.message)
raise MergeError, 'Something went wrong during merge'
raise_error('Something went wrong during merge')
ensure
merge_request.update!(in_progress_merge_commit_sha: nil)
end
......
......@@ -14,11 +14,11 @@ class MergeToRefService < MergeRequests::MergeBaseService
def execute(merge_request)
@merge_request = merge_request
error_check!
validate!
commit_id = commit
raise MergeError, 'Conflicts detected during merge' unless commit_id
raise_error('Conflicts detected during merge') unless commit_id
success(commit_id: commit_id)
rescue MergeError => error
......@@ -27,6 +27,11 @@ def execute(merge_request)
private
def validate!
authorization_check!
error_check!
end
def error_check!
error =
if !merge_method_supported?
......@@ -41,7 +46,13 @@ def error_check!
'No source for merge'
end
raise MergeError, error if error
raise_error(error) if error
end
def authorization_check!
unless Ability.allowed?(current_user, :admin_merge_request, project)
raise_error("You are not allowed to merge to this ref")
end
end
def target_ref
......
......@@ -224,6 +224,18 @@
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
end
it 'logs and saves error if user is not authorized' do
unauthorized_user = create(:user)
project.add_reporter(unauthorized_user)
service = described_class.new(project, unauthorized_user)
service.execute(merge_request)
expect(merge_request.merge_error)
.to eq('You are not allowed to merge this merge request')
end
it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message'
......
......@@ -3,6 +3,22 @@
require 'spec_helper'
describe MergeRequests::MergeToRefService do
shared_examples_for 'MergeService for target ref' do
it 'target_ref has the same state of target branch' do
repo = merge_request.target_project.repository
process_merge_to_ref
merge_service.execute(merge_request)
ref_commits = repo.commits(merge_request.merge_ref_path, limit: 3)
target_branch_commits = repo.commits(merge_request.target_branch, limit: 3)
ref_commits.zip(target_branch_commits).each do |ref_commit, target_branch_commit|
expect(ref_commit.parents).to eq(target_branch_commit.parents)
end
end
end
set(:user) { create(:user) }
let(:merge_request) { create(:merge_request, :simple) }
let(:project) { merge_request.project }
......@@ -76,22 +92,6 @@ def process_merge_to_ref
MergeRequests::MergeService.new(project, user, {})
end
shared_examples_for 'MergeService for target ref' do
it 'target_ref has the same state of target branch' do
repo = merge_request.target_project.repository
process_merge_to_ref
merge_service.execute(merge_request)
ref_commits = repo.commits(merge_request.merge_ref_path, limit: 3)
target_branch_commits = repo.commits(merge_request.target_branch, limit: 3)
ref_commits.zip(target_branch_commits).each do |ref_commit, target_branch_commit|
expect(ref_commit.parents).to eq(target_branch_commit.parents)
end
end
end
context 'when merge commit' do
it_behaves_like 'MergeService for target ref'
end
......@@ -176,5 +176,17 @@ def process_merge_to_ref
it { expect(todo).not_to be_done }
end
it 'returns error when user has no authorization to admin the merge request' do
unauthorized_user = create(:user)
project.add_reporter(unauthorized_user)
service = described_class.new(project, unauthorized_user)
result = service.execute(merge_request)
expect(result[:status]).to eq(:error)
expect(result[:message]).to eq('You are not allowed to merge to this ref')
end
end
end
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment