Prevent author from approving their own MR

- Don't include the author when checking approvers remaining.
- Don't allow the author to be added as an approver when editing an MR.
- Don't allow the current user (who will be the author) to be added as
  an approver when creating an MR.

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 8.10.0 (unreleased)
   - Rename Git Hooks to Push Rules
   - Fix EE keys fingerprint add index migration if came from CE
   - Add todos for MR approvers !547
+  - Prevent the author of an MR from being on the approvers list
   - Isolate EE LDAP library code in EE module (Part 1) !511
   - Make Elasticsearch indexer run as an async task
 

app/models/merge_request.rb

@@ -112,7 +112,7 @@ class MergeRequest < ActiveRecord::Base
   scope :references_project, -> { references(:target_project) }
 
   participant :approvers_left
- 
+
   after_save :keep_around_commit
 
   def self.reference_prefix
@@ -565,8 +565,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
@@ -577,12 +576,14 @@ class MergeRequest < ActiveRecord::Base
     approvals_required.nonzero?
   end
 
-  def overall_approvers
-    if approvers.any?
-      approvers
-    else
-      target_project.approvers
-    end
+  # Before a merge request has been created, author will be nil, so pass the current user
+  # on the MR create page.
+  #
+  def overall_approvers(exclude_user: nil)
+    exclude_user ||= author
+    approvers_relation = approvers.any? ? approvers : target_project.approvers
+
+    exclude_user ? approvers_relation.where.not(user_id: exclude_user.id) : approvers_relation
   end
 
   def approved?
@@ -598,8 +599,9 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_approve?(user)
-    approvers_left.include?(user) ||
-    (any_approver_allowed? && !approved_by?(user))
+    return true if approvers_left.include?(user)
+
+    any_approver_allowed? && !approved_by?(user) && user != author && user.can?(:update_merge_request, self)
   end
 
   def any_approver_allowed?
@@ -640,6 +642,8 @@ class MergeRequest < ActiveRecord::Base
 
   def approver_ids=(value)
     value.split(",").map(&:strip).each do |user_id|
+      next if user_id == author.id
+
       approvers.find_or_initialize_by(user_id: user_id, target_id: id)
     end
   end

app/views/shared/issuable/_form.html.haml

@@ -134,7 +134,9 @@
       = f.label :approver_ids, class: 'control-label' do
         Approvers
       .col-sm-10
-        = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
+        - author = @merge_request.author || current_user
+        - skip_users = @merge_request.overall_approvers.map(&:user) + [author]
+        = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true, skip_users: skip_users)
         .help-block
           This merge request must be approved by these users.
           You can override the project settings by setting your own list of approvers.
@@ -143,26 +145,22 @@
           .panel-heading
             Approvers
           %ul.well-list.approver-list
-            - if @merge_request.new_record?
-              - @merge_request.target_project.approvers.each do |approver|
-                %li.project-approvers{id: dom_id(approver.user)}
-                  = link_to approver.user.name, approver.user
-                  .pull-right
+            - using_project_approvers = @merge_request.approvers.empty?
+            - item_class = 'project-approvers' if using_project_approvers
+            - @merge_request.overall_approvers(exclude_user: author).each do |approver|
+              %li{id: dom_id(approver.user), class: item_class}
+                = link_to approver.user.name, approver.user
+                .pull-right
+                  - if using_project_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
-              - if @merge_request.target_project.approvers.empty?
-                %li.no-approvers There are no approvers
-            - else
-              - @merge_request.approvers.each do |approver|
-                %li{id: dom_id(approver.user)}
-                  = link_to approver.user.name, approver.user
-                  .pull-right
+                  - else
                     = link_to namespace_project_merge_request_approver_path(@project.namespace, @project, @merge_request, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
-              - if @merge_request.approvers.empty?
-                %li.no-approvers There are no approvers
+            - if @merge_request.overall_approvers.empty?
+              %li.no-approvers There are no approvers
         .help-block.suggested-approvers
           - if @suggested_approvers.any?
             Suggested approvers:

doc/workflow/merge_request_approvals.md

@@ -33,7 +33,8 @@ 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 be set as an approver for
+that merge request.
 
 If there are more approvers than required approvals, any subset of these users
 can approve the merge request.
@@ -64,4 +65,4 @@ yet 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

@@ -320,15 +320,8 @@ 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
+  Scenario: Developer can approve merge request
+    Given I am a "Shop" developer
     And I visit project "Shop" merge requests page
     And merge request 'Bug NS-04' must be approved
     And I click link "Bug NS-04"
@@ -336,14 +329,6 @@ 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
-    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"
-
   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
     And I click link "Bug NS-04"
@@ -353,15 +338,19 @@ Feature: Project Merge Requests
 
   @javascript
   Scenario: I see suggested approvers on new merge request form
-    Given project settings contain list of approvers
+    Given I am a "Shop" developer
+    And project settings contain list of approvers
+    And I visit project "Shop" merge requests page
     When I click link "New Merge Request"
     And I select "feature_conflict" as source
     Then I see suggested approver
 
   @javascript
   Scenario: I see auto-suggested approvers on new merge request form
-    Given project settings contain list of approvers
+    Given I am a "Shop" developer
+    And project settings contain list of approvers
     And there is one auto-suggested approver
+    And I visit project "Shop" merge requests page
     When I click link "New Merge Request"
     And I select "feature_conflict" as source
     Then I see auto-suggested approver

features/steps/project/merge_requests.rb

@@ -641,10 +641,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).to have_content('diff --git')
   end
 
-  step 'I am a "Shop" reporter' do
+  step 'I am a "Shop" developer' do
     user = create(:user, name: "Mike")
     project = Project.find_by(name: "Shop")
-    project.team << [user, :reporter]
+    project.team << [user, :developer]
 
     logout
     login_with user

spec/features/merge_requests/approvals_spec.rb

+require 'rails_helper'
+
+feature 'Merge request approvals', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, approvals_before_merge: 1) }
+
+  context 'when editing an MR with a different author' do
+    let(:author) { create(:user) }
+    let(:merge_request) { create(:merge_request, author: author, source_project: project) }
+
+    before do
+      project.team << [user, :developer]
+      project.team << [author, :developer]
+
+      login_as(user)
+      visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+      find('#s2id_merge_request_approver_ids .select2-input').click
+    end
+
+    it 'does not allow setting the author as an approver' do
+      expect(find('.select2-results')).not_to have_content(author.name)
+    end
+
+    it 'allows setting the current user as an approver' do
+      expect(find('.select2-results')).to have_content(user.name)
+    end
+  end
+
+  context 'when creating an MR' do
+    let(:other_user) { create(:user) }
+
+    before do
+      project.team << [user, :developer]
+      project.team << [other_user, :developer]
+
+      login_as(user)
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature' })
+
+      find('#s2id_merge_request_approver_ids .select2-input').click
+    end
+
+    it 'allows setting other users as approvers' do
+      expect(find('.select2-results')).to have_content(other_user.name)
+    end
+
+    it 'does not allow setting the current user as an approver' do
+      expect(find('.select2-results')).not_to have_content(user.name)
+    end
+  end
+end

spec/helpers/merge_request_helper_spec.rb

@@ -56,45 +56,47 @@ describe MergeRequestsHelper do
   end
 
   describe 'render_require_section' do
+    let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
+    let(:approver) { create(:user) }
+
+    before do
+      5.times { project.team << [create(:user), :developer] }
+      project.team << [approver, :developer]
+    end
+
     it "returns correct value in case - one approval" do
       project.update(approvals_before_merge: 1)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
+
       expect(render_require_section(merge_request)).to eq("Requires one more approval")
     end
 
     it "returns correct value in case - two approval" do
       project.update(approvals_before_merge: 2)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
+
       expect(render_require_section(merge_request)).to eq("Requires 2 more approvals")
     end
 
     it "returns correct value in case - one approver" do
       project.update(approvals_before_merge: 1)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      merge_request.approvers.create(user_id: user.id)
+      create(:approver, user: approver, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{user.name})")
+      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{approver.name})")
     end
 
     it "returns correct value in case - one approver and one more" do
       project.update(approvals_before_merge: 2)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      merge_request.approvers.create(user_id: user.id)
+      create(:approver, user: approver, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{user.name} and 1 more)")
+      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{approver.name} and 1 more)")
     end
 
     it "returns correct value in case - two approver and one more" do
       project.update(approvals_before_merge: 3)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      user1 = create :user
-      merge_request.approvers.create(user_id: user.id)
-      merge_request.approvers.create(user_id: user1.id)
+      approver2 = create(:user)
+      create(:approver, user: approver, target: merge_request)
+      create(:approver, user: approver2, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{user1.name}, #{user.name} and 1 more)")
+      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{approver2.name}, #{approver.name} and 1 more)")
     end
   end
 end

spec/models/merge_request_spec.rb

@@ -707,4 +707,160 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
   end
+
+  describe 'approvals' do
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
+    let(:author) { create(:user) }
+    let(:approver) { create(:user) }
+
+    context 'on a project with only one member' do
+      context 'when there is one approver' do
+        before { project.update_attributes(approvals_before_merge: 1) }
+
+        context 'when that approver is not the MR author' do
+          before do
+            project.team << [approver, :developer]
+            create(:approver, user: approver, target: merge_request)
+          end
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'allows the approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+          end
+        end
+      end
+    end
+
+    context 'on a project with several members' do
+      let(:approver_2) { create(:user) }
+      let(:developer) { create(:user) }
+      let(:reporter) { create(:user) }
+      let(:stranger) { create(:user) }
+
+      before do
+        project.team << [author, :developer]
+        project.team << [approver, :developer]
+        project.team << [approver_2, :developer]
+        project.team << [developer, :developer]
+        project.team << [reporter, :reporter]
+      end
+
+      context 'when there is one approver required' do
+        before { project.update_attributes(approvals_before_merge: 1) }
+
+        context 'when that approver is the MR author' do
+          before { create(:approver, user: author, target: merge_request) }
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'does not allow the author to approve the MR' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+          end
+
+          it 'allows any other project member with write access to approve the MR' do
+            expect(merge_request.can_approve?(developer)).to be_truthy
+
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+
+        context 'when that approver is not the MR author' do
+          before { create(:approver, user: approver, target: merge_request) }
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'only allows the approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+
+            expect(merge_request.can_approve?(author)).to be_falsey
+            expect(merge_request.can_approve?(developer)).to be_falsey
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+      end
+
+      context 'when there are multiple approvers required' do
+        before { project.update_attributes(approvals_before_merge: 3) }
+
+        context 'when one of those approvers is the MR author' do
+          before do
+            create(:approver, user: author, target: merge_request)
+            create(:approver, user: approver, target: merge_request)
+            create(:approver, user: approver_2, target: merge_request)
+          end
+
+          it 'requires the original number of approvals' do
+            expect(merge_request.approvals_left).to eq(3)
+          end
+
+          it 'does not allow the author to approve the MR' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+          end
+
+          it 'allows any other other approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+          end
+
+          context 'when all of the valid approvers have approved the MR' do
+            before do
+              create(:approval, user: approver, merge_request: merge_request)
+              create(:approval, user: approver_2, merge_request: merge_request)
+            end
+
+            it 'requires the original number of approvals' do
+              expect(merge_request.approvals_left).to eq(1)
+            end
+
+            it 'does not allow the author to approve the MR' do
+              expect(merge_request.can_approve?(author)).to be_falsey
+            end
+
+            it 'does not allow the approvers to approve the MR again' do
+              expect(merge_request.can_approve?(approver)).to be_falsey
+              expect(merge_request.can_approve?(approver_2)).to be_falsey
+            end
+
+            it 'allows any other project member with write access to approve the MR' do
+              expect(merge_request.can_approve?(developer)).to be_truthy
+
+              expect(merge_request.can_approve?(reporter)).to be_falsey
+              expect(merge_request.can_approve?(stranger)).to be_falsey
+            end
+          end
+        end
+
+        context 'when the approvers do not contain the MR author' do
+          before do
+            create(:approver, user: developer, target: merge_request)
+            create(:approver, user: approver, target: merge_request)
+            create(:approver, user: approver_2, target: merge_request)
+          end
+
+          it 'requires the original number of approvals' do
+            expect(merge_request.approvals_left).to eq(3)
+          end
+
+          it 'only allows the approvers to approve the MR' do
+            expect(merge_request.can_approve?(developer)).to be_truthy
+            expect(merge_request.can_approve?(approver)).to be_truthy
+            expect(merge_request.can_approve?(approver_2)).to be_truthy
+
+            expect(merge_request.can_approve?(author)).to be_falsey
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+      end
+    end
+  end
 end

spec/requests/api/merge_requests_spec.rb

@@ -704,14 +704,31 @@ 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)
+    before { project.update_attribute(:approvals_before_merge, 2) }
+
+    context 'as the author of the merge request' do
+      before { post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user) }
+
+      it 'returns a 401' do
+        expect(response).to have_http_status(401)
+      end
+    end
 
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+    context 'as a valid approver' do
+      let(:approver) { create(:user) }
 
-      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)
+      before do
+        project.team << [approver, :developer]
+        project.team << [create(:user), :developer]
+
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", approver)
+      end
+
+      it 'approves the merge request' do
+        expect(response.status).to eq(201)
+        expect(json_response['approvals_left']).to eq(1)
+        expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
+      end
     end
   end