Merge branch '3845-committers-cannot-approve-mr' into 'master'

Resolve "Prevent commit authors from self approvaling merge requests"

Closes #3845

See merge request gitlab-org/gitlab-ee!9007

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
+    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