Merge branch '388-option-to-disallow-author-to-approve-merge-request' into 'master'

Resolve "Option to disallow author to approve merge request"

This prevents an MR from being approved by the author of the MR. The
author won't see the approve MR button, and if they try via the internal
or public API, it will fail.

Closes #388.

See merge request !455

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.8.1
-  - Add documentation for the "Health Check" feature
-  - Allow anonymous users to access a public project's pipelines
 v 8.9.0 (unreleased)
   - Fix Error 500 when using closes_issues API with an external issue tracker
   - Bulk assign/unassign labels to issues.

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
   - Distribute RepositoryUpdateMirror jobs in time and add exclusive lease on them by project_id

app/controllers/projects/merge_requests_controller.rb

@@ -376,8 +376,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def set_suggested_approvers
     if @merge_request.requires_approve?
       @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request,
-        current_user
+        @merge_request
       ).calculate(@merge_request.approvals_required)
     end
   end

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,6 +507,8 @@ 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))
   end

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.

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

@@ -2,9 +2,8 @@ module Gitlab
   class AuthorityAnalyzer
     COMMITS_TO_CONSIDER = 5
 
-    def initialize(merge_request, current_user)
+    def initialize(merge_request)
       @merge_request = merge_request
-      @current_user = current_user
       @users = Hash.new(0)
     end
 
@@ -12,7 +11,7 @@ module Gitlab
       involved_users
 
       # Picks most active users from hash like: {user1: 2, user2: 6}
-      @users.sort_by { |user, count| count }.map(&:first).take(number_of_approvers)
+      @users.sort_by { |user, count| -count }.map(&:first).take(number_of_approvers)
     end
 
     private
@@ -22,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

+require 'spec_helper'
+
+describe Gitlab::AuthorityAnalyzer, lib: true do
+  describe '#calculate' do
+    let(:project) { create(:project) }
+    let(:author) { create(:user) }
+    let(:user_a) { create(:user) }
+    let(:user_b) { create(:user) }
+    let(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: author) }
+    let(:files) { [double(:file, deleted_file: true, old_path: 'foo')] }
+
+    let(:commits) do
+      [
+        double(:commit, author: author),
+        double(:commit, author: user_a),
+        double(:commit, author: user_a),
+        double(:commit, author: user_b),
+        double(:commit, author: author)
+      ]
+    end
+
+    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 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
+      it 'returns only the top n contributors' do
+        approvers = calculate_approvers(1)
+
+        expect(approvers).to contain_exactly(user_a)
+      end
+    end
+  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
+    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(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