Check authorization in merge services

Move authorization checks to merge services
instead relying solely on external checks.

5 files changed, 74 insertions, 24 deletions

diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb
index 4c5a0d96957..61049f394aa 100644
--- a/app/services/merge_requests/merge_base_service.rb
+++ b/app/services/merge_requests/merge_base_service.rb
@@ -27,6 +27,10 @@ module MergeRequests
 
     private
 
+    def raise_error(message)
+      raise MergeError, message
+    end
+
     def handle_merge_error(*args)
       # No-op
     end
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index b1d01955d85..f5d66589196 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -18,7 +18,7 @@ module MergeRequests
 
       @merge_request = merge_request
 
-      error_check!
+      validate!
 
       merge_request.in_locked_state do
         if commit
@@ -34,6 +34,17 @@ module MergeRequests
 
     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 @@ module MergeRequests
           'No source for merge'
         end
 
-      raise MergeError, error if error
+      raise_error(error) if error
     end
 
     def commit
@@ -54,7 +65,7 @@ module MergeRequests
       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 @@ module MergeRequests
       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
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
index c4a40044143..6b99c4fc483 100644
--- a/app/services/merge_requests/merge_to_ref_service.rb
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -14,11 +14,11 @@ module MergeRequests
     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 @@ module MergeRequests
 
     private
 
+    def validate!
+      authorization_check!
+      error_check!
+    end
+
     def error_check!
       error =
         if !merge_method_supported?
@@ -41,7 +46,13 @@ module MergeRequests
           '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
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 04a62aa454d..ede79b87bcc 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -224,6 +224,18 @@ describe MergeRequests::MergeService do
         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'
 
diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb
index 435a863cbd4..696f1b83157 100644
--- a/spec/services/merge_requests/merge_to_ref_service_spec.rb
+++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -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 @@ describe MergeRequests::MergeToRefService do
         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 @@ describe MergeRequests::MergeToRefService do
 
       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