Disallow MR approvals from the MR author

CHANGELOG-EE

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.9.0 (unreleased)
   - Fix nil user handling in UpdateMirrorService
   - Allow LDAP to mark users as external based on their group membership. !432
+  - Forbid MR authors from approving their own MRs
   - Instrument instance methods of Gitlab::InsecureKeyFingerprint class
   - Add API endpoint for Merge Request Approvals !449
 

app/models/merge_request.rb

@@ -475,8 +475,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def approvers_left
-    user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id)
-    User.where id: user_ids
+    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
   end
 
   def approvals_required
@@ -508,8 +507,10 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_approve?(user)
+    return false if user == self.author
+
     approvers_left.include?(user) ||
-    (any_approver_allowed? && !approved_by?(user))
+      (any_approver_allowed? && !approved_by?(user))
   end
 
   def any_approver_allowed?

doc/workflow/merge_request_approvals.md

@@ -33,7 +33,7 @@ independent of changes to the merge request.
 ### Approvers
 
 At approvers you can define the default set of users that need to approve a
-merge request.
+merge request. The author of a merge request cannot approve that merge request.
 
 If there are more approvers than required approvals, any subset of these users
 can approve the merge request.
@@ -63,4 +63,4 @@ the process is still enforced.
 
 To approve a merge request, simply press the button.
 
-![Merge request approval](merge_request_approvals/2_approvals.png)
\ No newline at end of file
+![Merge request approval](merge_request_approvals/2_approvals.png)

features/project/merge_requests.feature

@@ -327,13 +327,6 @@ Feature: Project Merge Requests
     And I click link "Close"
     Then I should see closed merge request "Bug NS-04"
 
-  Scenario: I approve merge request
-    Given merge request 'Bug NS-04' must be approved
-    And I click link "Bug NS-04"
-    And I should not see merge button
-    When I click link "Approve"
-    Then I should see approved merge request "Bug NS-04"
-
   Scenario: Reporter can approve merge request
     Given I am a "Shop" reporter
     And I visit project "Shop" merge requests page
@@ -343,13 +336,12 @@ Feature: Project Merge Requests
     When I click link "Approve"
     Then I should see message that merge request can be merged
 
-  Scenario: I approve merge request if I am an approver
-    Given merge request 'Bug NS-04' must be approved by current user
+  Scenario: I can not approve Merge request if I am the author
+    Given merge request 'Bug NS-04' must be approved
     And I click link "Bug NS-04"
     And I should not see merge button
-    And I should see message that MR require an approval from me
-    When I click link "Approve"
-    Then I should see approved merge request "Bug NS-04"
+    When I should not see Approve button
+    And I should see message that MR require an approval
 
   Scenario: I can not approve merge request if I am not an approver
     Given merge request 'Bug NS-04' must be approved by some user

lib/gitlab/authority_analyzer.rb

@@ -21,7 +21,9 @@ module Gitlab
 
       list_of_involved_files.each do |path|
         @repo.commits(@merge_request.target_branch, path: path, limit: COMMITS_TO_CONSIDER).each do |commit|
-          @users[commit.author] += 1 if commit.author
+          if commit.author && commit.author != @merge_request.author
+            @users[commit.author] += 1
+          end
         end
       end
     end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -268,6 +268,49 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  describe 'POST #approve' do
+    def approve(user)
+      post :approve, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid, user: user
+    end
+
+    context 'when the user is the author of the MR' do
+      before { merge_request.approvers.create(user: merge_request.author) }
+
+      it "returns a 404" do
+        approve(merge_request.author)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'when the user is not allowed to approve the MR' do
+      it "returns a 404" do
+        approve(user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'when the user is allowed to approve the MR' do
+      before { merge_request.approvers.create(user: user) }
+
+      it 'creates an approval' do
+        service = double(:approval_service)
+
+        expect(MergeRequests::ApprovalService).to receive(:new).with(project, anything).and_return(service)
+        expect(service).to receive(:execute).with(merge_request)
+
+        approve(user)
+      end
+
+      it 'redirects to the MR' do
+        approve(user)
+
+        expect(response).to redirect_to(namespace_project_merge_request_path)
+      end
+    end
+  end
+
   describe "DELETE #destroy" do
     it "denies access to users unless they're admin or project owner" do
       delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid

spec/lib/gitlab/authority_analyzer_spec.rb

@@ -19,25 +19,41 @@ describe Gitlab::AuthorityAnalyzer, lib: true do
       ]
     end
 
-    let(:approvers) { Gitlab::AuthorityAnalyzer.new(merge_request).calculate(number_of_approvers) }
+    def calculate_approvers(count)
+      described_class.new(merge_request).calculate(count)
+    end
 
     before do
       merge_request.compare = double(:compare, diffs: files)
       allow(merge_request.target_project.repository).to receive(:commits).and_return(commits)
     end
 
-    context 'when there are fewer contributors than requested' do
-      let(:number_of_approvers) { 5 }
+    context 'when the MR author is in the top contributors' do
+      it 'does not include the MR author' do
+        approvers = calculate_approvers(2)
+
+        expect(approvers).not_to include(author)
+      end
 
+      it 'returns the correct number of contributors' do
+        approvers = calculate_approvers(2)
+
+        expect(approvers.length).to eq(2)
+      end
+    end
+
+    context 'when there are fewer contributors than requested' do
       it 'returns the full number of users' do
+        approvers = calculate_approvers(5)
+
         expect(approvers.length).to eq(2)
       end
     end
 
     context 'when there are more contributors than requested' do
-      let(:number_of_approvers) { 1 }
-
       it 'returns only the top n contributors' do
+        approvers = calculate_approvers(1)
+
         expect(approvers).to contain_exactly(user_a)
       end
     end

spec/models/merge_request_spec.rb

@@ -223,7 +223,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "approvers_left" do
+  describe "#approvers_left" do
     let(:merge_request) {create :merge_request}
 
     it "returns correct value" do
@@ -237,7 +237,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "approvals_required" do
+  describe "#approvals_required" do
     let(:merge_request) {create :merge_request}
 
     it "takes approvals_before_merge" do
@@ -247,6 +247,58 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe "#can_approve?" do
+    let(:author) { create(:user) }
+    let(:user) { create(:user) }
+    let(:merge_request) { create(:merge_request, author: author) }
+
+    context "when the user is the MR author" do
+      it "returns false" do
+        expect(merge_request.can_approve?(author)).to eq(false)
+      end
+    end
+
+    context "when the user is not the MR author" do
+      context "when the user is in the approvers list" do
+        before { merge_request.approvers.create(user: user) }
+
+        context "when the user has not already approved the MR" do
+          it "returns true" do
+            expect(merge_request.can_approve?(user)).to eq(true)
+          end
+        end
+
+        context "when the user has already approved the MR" do
+          before { merge_request.approvals.create(user: user) }
+
+          it "returns false" do
+            expect(merge_request.can_approve?(user)).to eq(false)
+          end
+        end
+      end
+
+      context "when the user is not in the approvers list" do
+        context "when anyone is allowed to approve the MR" do
+          before { merge_request.target_project.update_attributes(approvals_before_merge: 1) }
+
+          context "when the user has not already approved the MR" do
+            it "returns true" do
+              expect(merge_request.can_approve?(user)).to eq(true)
+            end
+          end
+
+          context "when the user has already approved the MR" do
+            before { merge_request.approvals.create(user: user) }
+
+            it "returns false" do
+              expect(merge_request.can_approve?(user)).to eq(false)
+            end
+          end
+        end
+      end
+    end
+  end
+
   describe '#can_remove_source_branch?' do
     let(:user) { create(:user) }
     let(:user2) { create(:user) }

spec/requests/api/merge_requests_spec.rb

@@ -634,14 +634,24 @@ describe API::API, api: true  do
   end
 
   describe 'POST :id/merge_requests/:merge_request_id/approve' do
-    it 'approves the merge request' do
-      project.update_attribute(:approvals_before_merge, 2)
+    context 'when the user is an allowed approver' do
+      it 'approves the merge request' do
+        project.update_attribute(:approvals_before_merge, 2)
 
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", admin)
 
-      expect(response.status).to eq(201)
-      expect(json_response['approvals_left']).to eq(1)
-      expect(json_response['approved_by'][0]['user']['username']).to eq(user.username)
+        expect(response.status).to eq(201)
+        expect(json_response['approvals_left']).to eq(1)
+        expect(json_response['approved_by'][0]['user']['username']).to eq(admin.username)
+      end
+    end
+
+    context 'when the user is the MR author' do
+      it 'returns a not authorised response' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+
+        expect(response.status).to eq(401)
+      end
     end
   end