Prevent merge requests from being approved by authors of it commits

This is accomplished by taking the merge request's commits that aren't
merge commits, and removing their authors from the list of eligable
approvers.

app/models/commit_collection.rb

@@ -19,6 +19,12 @@ class CommitCollection
     commits.each(&block)
   end
 
+  def committers
+    emails = commits.reject(&:merge_commit?).map(&:committer_email).uniq
+
+    User.by_any_email(emails)
+  end
+
   # Sets the pipeline status for every commit.
   #
   # Setting this status ahead of time removes the need for running a query for

app/models/merge_request.rb

@@ -286,6 +286,14 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def committers
+    @committers ||= commits.committers
+  end
+
+  def authors
+    User.from_union([committers, User.where(id: self.author_id)])
+  end
+
   # Verifies if title has changed not taking into account WIP prefix
   # for merge requests.
   def wipless_title_changed(old_title)
@@ -329,13 +337,15 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def commits
-    if persisted?
-      merge_request_diff.commits
-    elsif compare_commits
-      compare_commits.reverse
-    else
-      []
-    end
+    return merge_request_diff.commits if persisted?
+
+    commits_arr = if compare_commits
+                    compare_commits.reverse
+                  else
+                    []
+                  end
+
+    CommitCollection.new(source_project, commits_arr, source_branch)
   end
 
   def commits_count

doc/user/project/merge_requests/merge_request_approvals.md

@@ -52,8 +52,9 @@ A group can also be added as an approver. [In the future](https://gitlab.com/git
 group approvers will be restricted.
 
 If a user is added as an individual approver and is also part of a group approver,
-then that user is just counted once. The merge request author does not count as
-an eligible approver, unless [self-approval] is explicitly enabled on the project settings.
+then that user is just counted once. The merge request author and users that have committed
+to the merge request do not count as eligible approvers,
+unless [self-approval] is explicitly enabled on the project settings.
 
 ### Implicit approvers
 

ee/app/models/concerns/approvable.rb

@@ -58,7 +58,7 @@ module Approvable
     # they're included/excluded from that list accordingly.
     return true if approvers_left.include?(user)
     # We can safely unauthorize authors if it reaches this guard clause.
-    return false if user == author
+    return false if authors.include?(user)
     return false unless user.can?(:update_merge_request, self)
 
     any_approver_allowed? && approvals.where(user: user).empty?

ee/app/models/concerns/visible_approvable.rb

@@ -37,8 +37,8 @@ module VisibleApprovable
       approvers_relation = target_project.approvers
     end
 
-    if author && !authors_can_approve?
-      approvers_relation = approvers_relation.where.not(user_id: author.id)
+    if authors.any? && !authors_can_approve?
+      approvers_relation = approvers_relation.where.not(user_id: authors.select(:id))
     end
 
     results = code_owners.concat(approvers_relation.includes(:user).map(&:user))
@@ -65,7 +65,12 @@ module VisibleApprovable
 
   def approvers_from_groups
     group_approvers = overall_approver_groups.flat_map(&:users)
-    group_approvers.delete(author) unless authors_can_approve?
+
+    unless authors_can_approve?
+      author_approver_ids = authors.where(id: group_approvers.map(&:id)).pluck(:id)
+      group_approvers.reject! { |user| author_approver_ids.include?(user.id) }
+    end
+
     group_approvers
   end
 

ee/app/views/shared/issuable/_approvals.html.haml

@@ -5,7 +5,6 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless presenter.requires_approve?
 
-- author = issuable.author || current_user
 - can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 .form-group.row
@@ -13,7 +12,7 @@
     Approvers
   .col-sm-10
     - if can_update_approvers
-      - skip_users = [*presenter.all_approvers_including_groups, (author unless presenter.authors_can_approve?)].compact
+      - skip_users = [*presenter.all_approvers_including_groups, *(issuable.authors unless presenter.authors_can_approve?)].compact
 
       = users_select_tag("merge_request[approver_ids]",
         multiple: true,

ee/changelogs/unreleased/3845-committers-cannot-approve-mr.yml

+---
+title: Prevent commit authors from self approvaling merge requests
+merge_request: 9007
+author:
+type: changed

ee/spec/models/approvable_spec.rb

@@ -2,6 +2,8 @@ require 'spec_helper'
 
 describe Approvable do
   let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.project }
+  let(:author) { merge_request.author }
 
   describe '#approvers_overwritten?' do
     subject { merge_request.approvers_overwritten? }
@@ -23,4 +25,102 @@ describe Approvable do
       is_expected.to be true
     end
   end
+
+  describe '#can_approve?' do
+    subject { merge_request.can_approve?(user) }
+
+    it 'returns false if user is nil' do
+      expect(merge_request.can_approve?(nil)).to be false
+    end
+
+    it 'returns true when user is included in the approvers list' do
+      user = create(:approver, target: merge_request).user
+
+      expect(merge_request.can_approve?(user)).to be true
+    end
+
+    context 'when authors can approve' do
+      before do
+        project.update(merge_requests_author_approval: true)
+      end
+
+      context 'when the user is the author' do
+        it 'returns true when user is approver' do
+          create(:approver, target: merge_request, user: author)
+
+          expect(merge_request.can_approve?(author)).to be true
+        end
+
+        it 'returns false when user is not approver' do
+          expect(merge_request.can_approve?(author)).to be false
+        end
+      end
+
+      context 'when user is committer' do
+        let(:user) { create(:user, email: merge_request.commits.first.committer_email) }
+
+        before do
+          project.add_developer(user)
+        end
+
+        it 'returns true when user is approver' do
+          create(:approver, target: merge_request, user: user)
+
+          expect(merge_request.can_approve?(user)).to be true
+        end
+
+        it 'returns false when user is not approver' do
+          expect(merge_request.can_approve?(user)).to be false
+        end
+      end
+    end
+
+    context 'when authors cannot approve' do
+      before do
+        project.update(merge_requests_author_approval: false)
+      end
+
+      it 'returns false when user is the author' do
+        create(:approver, target: merge_request, user: author)
+
+        expect(merge_request.can_approve?(author)).to be false
+      end
+
+      it 'returns false when user is a committer' do
+        user = create(:user, email: merge_request.commits.first.committer_email)
+        project.add_developer(user)
+        create(:approver, target: merge_request, user: user)
+
+        expect(merge_request.can_approve?(user)).to be false
+      end
+    end
+
+    it 'returns false when user is unable to update the merge request' do
+      user = create(:user)
+      project.add_guest(user)
+
+      expect(merge_request.can_approve?(user)).to be false
+    end
+
+    context 'when approvals are required' do
+      before do
+        project.update(approvals_before_merge: 1)
+      end
+
+      it 'returns true when approvals are still accepted and user still has not approved' do
+        user = create(:user)
+        project.add_developer(user)
+
+        expect(merge_request.can_approve?(user)).to be true
+      end
+
+      it 'returns false when there is still one approver missing' do
+        user = create(:user)
+        project.add_developer(user)
+        create(:approver, target: merge_request)
+
+        expect(merge_request.can_approve?(user)).to be false
+      end
+    end
+  end
 end

ee/spec/models/visible_approvable_spec.rb

@@ -20,10 +20,10 @@ describe VisibleApprovable do
 
     subject { resource.approvers_left }
 
-    it 'only queries once' do
-      expect(User).to receive(:where).and_call_original.once
+    it 'avoids N+1 queries' do
+      control = ActiveRecord::QueryRecorder.new { subject }
 
-      3.times { subject }
+      expect { subject }.not_to exceed_query_limit(control)
     end
 
     it 'returns all approvers left' do
@@ -69,6 +69,25 @@ describe VisibleApprovable do
       end
     end
 
+    context 'when committer is approver' do
+      let(:user) { create(:user, email: resource.commits.first.committer_email) }
+      let!(:committer_approver) { create(:approver, target: project, user: user) }
+
+      before do
+        project.add_developer(user)
+      end
+
+      it 'excludes committer if committers cannot approve' do
+        is_expected.not_to include(committer_approver.user)
+      end
+
+      it 'includes committer if committers are able to approve' do
+        project.update(merge_requests_author_approval: true)
+
+        is_expected.to include(committer_approver.user)
+      end
+    end
+
     context 'when approvers are overwritten' do
       let!(:approver) { create(:approver, target: resource) }
 

spec/controllers/projects/merge_requests/creations_controller_spec.rb

@@ -71,7 +71,7 @@ describe Projects::MergeRequests::CreationsController do
         expect(response).to be_success
 
         total = assigns(:total_commit_count)
-        expect(assigns(:commits)).to be_an Array
+        expect(assigns(:commits)).to be_an CommitCollection
         expect(total).to be > 0
         expect(assigns(:hidden_commit_count)).to eq(0)
         expect(response).to have_gitlab_http_status(200)

spec/models/commit_collection_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe CommitCollection do
   let(:project) { create(:project, :repository) }
-  let(:commit) { project.commit }
+  let(:commit) { project.commit("c1c67abbaf91f624347bb3ae96eabe3a1b742478") }
 
   describe '#each' do
     it 'yields every commit' do
@@ -12,6 +12,29 @@ describe CommitCollection do
     end
   end
 
+  describe '.committers' do
+    it 'returns a relation of users when users are found' do
+      user = create(:user, email: commit.committer_email.upcase)
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to contain_exactly(user)
+    end
+
+    it 'returns empty array when committers cannot be found' do
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to be_empty
+    end
+
+    it 'excludes authors of merge commits' do
+      commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98")
+      create(:user, email: commit.committer_email.upcase)
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to be_empty
+    end
+  end
+
   describe '#with_pipeline_status' do
     it 'sets the pipeline status for every commit so no additional queries are necessary' do
       create(

spec/models/merge_request_spec.rb

@@ -1122,6 +1122,34 @@ describe MergeRequest do
     end
   end
 
+  describe '#committers' do
+    it 'returns all the committers of every commit in the merge request' do
+      users = subject.commits.map(&:committer_email).uniq.map do |email|
+        create(:user, email: email)
+      end
+
+      expect(subject.committers).to match_array(users)
+    end
+
+    it 'returns an empty array if no committer is associated with a user' do
+      expect(subject.committers).to be_empty
+    end
+  end
+
+  describe '#authors' do
+    it 'returns a list with all the committers in the merge request and author' do
+      users = subject.commits.map(&:committer_email).uniq.map do |email|
+        create(:user, email: email)
+      end
+
+      expect(subject.authors).to match_array([subject.author, *users])
+    end
+
+    it 'returns only the author if no committer is associated with a user' do
+      expect(subject.authors).to contain_exactly(subject.author)
+    end
+  end
+
   describe '#hook_attrs' do
     it 'delegates to Gitlab::HookData::MergeRequestBuilder#build' do
       builder = double

spec/services/merge_requests/refresh_service_spec.rb

@@ -603,7 +603,7 @@ describe MergeRequests::RefreshService do
           committed_date: Time.now
         )
 
-        allow_any_instance_of(MergeRequest).to receive(:commits).and_return([commit])
+        allow_any_instance_of(MergeRequest).to receive(:commits).and_return(CommitCollection.new(@project, [commit], 'feature'))
       end
 
       context 'when the merge request is sourced from the same project' do