Merge branch 'security-7754' into 'master'

[master] Filter out non-project member approvers

See merge request gitlab/gitlab-ee!774

ee/app/models/concerns/visible_approvable.rb

@@ -31,17 +31,19 @@ module VisibleApprovable
 
     if approvers_overwritten?
       code_owners ||= [] # already persisted into database, no need to recompute
-      approvers_relation = approvers
+      approvers_relation = approver_users
     else
       code_owners ||= self.code_owners.dup
-      approvers_relation = target_project.approvers
+      approvers_relation = target_project.approver_users
     end
 
+    approvers_relation = project.members_among(approvers_relation)
+
     if authors.any? && !authors_can_approve?
-      approvers_relation = approvers_relation.where.not(user_id: authors.select(:id))
+      approvers_relation = approvers_relation.where.not(id: authors.select(:id))
     end
 
-    results = code_owners.concat(approvers_relation.includes(:user).map(&:user))
+    results = code_owners.concat(approvers_relation)
     results.uniq!
     results
   end

ee/app/models/ee/merge_request.rb

@@ -15,6 +15,7 @@ module EE
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
       has_many :draft_notes

ee/app/models/ee/project.rb

@@ -43,6 +43,7 @@ module EE
 
       has_many :reviews, inverse_of: :project
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalProjectRule'
       has_many :audit_events, as: :entity

ee/app/presenters/merge_request_approver_presenter.rb

@@ -40,7 +40,9 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
   private
 
   def users
-    @users ||= users_from_git_log_authors
+    strong_memoize(:users) do
+      merge_request.project.members_among(users_from_git_log_authors)
+    end
   end
 
   def code_owner_enabled?

ee/changelogs/unreleased/security-7754.yml

+---
+title: Filter out non-project member approvers
+merge_request:
+author:
+type: security

ee/spec/factories/approvers.rb

@@ -4,5 +4,14 @@ FactoryBot.define do
   factory :approver do
     target factory: :merge_request
     user
+
+    after(:create) do |approver, evaluator|
+      case approver.target
+      when Project
+        approver.target.add_developer(approver.user)
+      else
+        approver.target.project.add_developer(approver.user)
+      end
+    end
   end
 end

ee/spec/javascripts/fixtures/merge_requests.rb

@@ -9,7 +9,11 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
   let(:namespace) { create(:namespace, name: 'frontend-fixtures' )}
   let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: admin, approvals_before_merge: 2) }
-  let(:suggested_approvers) { create_list(:user, 3) }
+  let(:suggested_approvers) do
+    create_list(:user, 3).tap do |users|
+      users.each { |user| project.add_developer(user) }
+    end
+  end
 
   render_views
 

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -9,6 +9,7 @@ merge_requests:
 - approval_rules
 - approvals
 - approvers
+- approver_users
 - approver_groups
 - approved_by_users
 - draft_notes
@@ -53,6 +54,7 @@ project:
 - index_status
 - approval_rules
 - approvers
+- approver_users
 - pages_domains
 - audit_events
 - path_locks

ee/spec/models/merge_request_spec.rb

@@ -12,6 +12,7 @@ describe MergeRequest do
     it { is_expected.to have_many(:reviews).inverse_of(:merge_request) }
     it { is_expected.to have_many(:approvals).dependent(:delete_all) }
     it { is_expected.to have_many(:approvers).dependent(:delete_all) }
+    it { is_expected.to have_many(:approver_users).through(:approvers) }
     it { is_expected.to have_many(:approver_groups).dependent(:delete_all) }
     it { is_expected.to have_many(:approved_by_users) }
   end

ee/spec/models/project_spec.rb

@@ -24,6 +24,8 @@ describe Project do
     it { is_expected.to have_many(:source_pipelines) }
     it { is_expected.to have_many(:audit_events).dependent(false) }
     it { is_expected.to have_many(:protected_environments) }
+    it { is_expected.to have_many(:approvers).dependent(:destroy) }
+    it { is_expected.to have_many(:approver_users).through(:approvers) }
     it { is_expected.to have_many(:approver_groups).dependent(:destroy) }
     it { is_expected.to have_many(:packages).class_name('Packages::Package') }
     it { is_expected.to have_many(:package_files).class_name('Packages::PackageFile') }

ee/spec/models/visible_approvable_spec.rb

@@ -39,6 +39,7 @@ describe VisibleApprovable do
 
     before do
       allow(resource).to receive(:code_owners).and_return([code_owner])
+      project.add_developer(project_approver.user)
     end
 
     subject { resource.overall_approvers }
@@ -91,10 +92,22 @@ describe VisibleApprovable do
     context 'when approvers are overwritten' do
       let!(:approver) { create(:approver, target: resource) }
 
+      before do
+        project.add_developer(approver.user)
+      end
+
       it 'returns the list of all the merge request user approvers' do
         is_expected.to contain_exactly(approver.user)
       end
     end
+
+    context 'when approver is no longer part of project' do
+      it 'excludes non-project members' do
+        project.team.find_member(project_approver.user).destroy!
+
+        is_expected.not_to include(project_approver.user)
+      end
+    end
   end
 
   describe '#overall_approver_groups' do
@@ -124,6 +137,10 @@ describe VisibleApprovable do
     let!(:approver_group) { create(:approver_group, target: resource, group: group) }
     let!(:approver) { create(:approver, target: resource) }
 
+    before do
+      project.add_developer(approver.user)
+    end
+
     subject { resource.all_approvers_including_groups }
 
     it 'only queries once' do
@@ -153,7 +170,8 @@ describe VisibleApprovable do
 
   describe '#reset_approval_cache!' do
     before do
-      create(:approver, target: resource)
+      approver = create(:approver, target: resource)
+      project.add_developer(approver.user)
     end
 
     subject { resource.reset_approval_cache! }

ee/spec/presenters/merge_request_approver_presenter_spec.rb

@@ -20,6 +20,9 @@ describe MergeRequestApproverPresenter do
     allow(merge_request).to receive(:modified_paths).and_return(file_paths)
     allow(merge_request).to receive(:approvals_required).and_return(approvals_required)
 
+    project.add_developer(committer_a)
+    project.add_developer(committer_b)
+
     stub_licensed_features(code_owners: enable_code_owner)
   end
 

spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -17,6 +17,7 @@ describe "User creates a merge request", :js do
 
   before do
     project.add_maintainer(user)
+    project.add_maintainer(user2)
     project.add_maintainer(approver)
     sign_in(user)
 

spec/models/merge_request_spec.rb

@@ -885,8 +885,8 @@ describe MergeRequest do
     it "returns correct value" do
       user = create(:user)
       user1 = create(:user)
-      merge_request.approvers.create(user_id: user.id)
-      merge_request.approvers.create(user_id: user1.id)
+      create(:approver, target: merge_request, user: user)
+      create(:approver, target: merge_request, user: user1)
       merge_request.approvals.create(user_id: user1.id)
 
       expect(merge_request.approvers_left).to eq [user]
@@ -899,9 +899,9 @@ describe MergeRequest do
       group = create(:group)
 
       group.add_developer(user2)
-      merge_request.approver_groups.create(group: group)
-      merge_request.approvers.create(user_id: user.id)
-      merge_request.approvers.create(user_id: user1.id)
+      create(:approver_group, target: merge_request, group: group)
+      create(:approver, target: merge_request, user: user)
+      create(:approver, target: merge_request, user: user1)
       merge_request.approvals.create(user_id: user1.id)
 
       expect(merge_request.approvers_left).to match_array [user, user2]

spec/services/merge_requests/update_service_spec.rb

@@ -489,6 +489,10 @@ describe MergeRequests::UpdateService, :mailer do
       let(:new_approver) { create(:user) }
 
       before do
+        project.add_developer(existing_approver)
+        project.add_developer(removed_approver)
+        project.add_developer(new_approver)
+
         perform_enqueued_jobs do
           update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
         end

spec/services/notification_service_spec.rb

@@ -1310,6 +1310,7 @@ describe NotificationService, :mailer do
         before do
           merge_request.target_project.update(approvals_before_merge: 1)
           project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
+          reset_delivered_emails!
         end
 
         it 'emails the approvers' do
@@ -1330,6 +1331,7 @@ describe NotificationService, :mailer do
 
           before do
             mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
+            reset_delivered_emails!
           end
 
           it 'emails the MR approvers' do