Merge branch...

Merge branch '225485-allow-reporters-to-approve-mrs-if-they-are-explicitly-listed-in-the-approval-rules' into 'master'

Allow reporters to approve MRs

Closes #225485

See merge request gitlab-org/gitlab!40491

app/models/protected_branch.rb

@@ -38,9 +38,9 @@ class ProtectedBranch < ApplicationRecord
     project.protected_branches
   end
 
+  # overridden in EE
   def self.branch_requires_code_owner_approval?(project, branch_name)
-    # NOOP
-    #
+    false
   end
 
   def self.by_name(query)

ee/app/models/concerns/approval_rule_like.rb

@@ -26,6 +26,7 @@ module ApprovalRuleLike
 
     scope :with_users, -> { preload(:users, :group_users) }
     scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) }
+    scope :for_groups, -> (groups) { joins(:groups).where(approval_project_rules_groups: { group_id: groups }) }
   end
 
   def audit_add

ee/app/models/concerns/ee/protected_branch.rb

@@ -5,6 +5,8 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
+      has_and_belongs_to_many :approval_project_rules
+
       protected_ref_access_levels :unprotect
     end
 

ee/app/models/ee/group_member.rb

@@ -21,6 +21,7 @@ module EE
         joins(user: :identities).where(identities: { saml_provider_id: provider })
       end
 
+      scope :reporters, -> { where(access_level: ::Gitlab::Access::REPORTER) }
       scope :non_owners, -> { where("members.access_level < ?", ::Gitlab::Access::OWNER) }
       scope :by_user_id, ->(user_id) { where(user_id: user_id) }
     end

ee/app/policies/ee/merge_request_policy.rb

@@ -12,11 +12,29 @@ module EE
 
       condition(:over_storage_limit, scope: :subject) { @subject.target_project&.namespace&.over_storage_limit? }
 
+      condition(:merge_request_group_approver, score: 140) do
+        project = @subject.target_project
+        protected_branch = project
+          .protected_branches
+          .requiring_code_owner_approval
+          .find { |pb| pb.matches?(@subject.target_branch) }
+
+        protected_branch.present? && group_access?(protected_branch)
+      end
+
+      def group_access?(protected_branch)
+        protected_branch.approval_project_rules.for_groups(@user.group_members.reporters.select(:source_id)).exists?
+      end
+
       rule { ~can_override_approvers }.prevent :update_approvers
       rule { can?(:update_merge_request) }.policy do
         enable :update_approvers
       end
 
+      rule { merge_request_group_approver }.policy do
+        enable :approve_merge_request
+      end
+
       rule { over_storage_limit }.policy do
         prevent :approve_merge_request
         prevent :update_merge_request

ee/changelogs/unreleased/225485-allow-reporters-to-approve-mrs-if-they-are-explicitly-listed-in.yml

+---
+title: Allow reporters to approve MRs
+merge_request: 40491
+author:
+type: added

ee/spec/factories/approver_groups.rb

 # frozen_string_literal: true
 
-# Read about factories at https://github.com/thoughtbot/factory_bot
-
 FactoryBot.define do
   factory :approver_group do
     target factory: :merge_request

ee/spec/policies/merge_request_policy_spec.rb

@@ -135,6 +135,92 @@ RSpec.describe MergeRequestPolicy do
     end
   end
 
+  context 'for a merge request on a protected branch' do
+    let(:branch_name) { 'feature' }
+    let_it_be(:user) { create :user }
+    let(:protected_branch) { create(:protected_branch, project: project, name: branch_name, code_owner_approval_required: true) }
+    let_it_be(:approver_group) { create(:group) }
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project, target_branch: branch_name) }
+
+    before do
+      project.add_reporter(user)
+    end
+
+    subject { described_class.new(user, merge_request) }
+
+    context 'when the reporter nor the group is added' do
+      specify do
+        expect(subject).not_to be_allowed(:approve_merge_request)
+      end
+    end
+
+    context 'when a group-level approval rule exists' do
+      let(:approval_project_rule) { create :approval_project_rule, project: project, approvals_required: 1 }
+
+      context 'when the merge request targets the protected branch' do
+        before do
+          approval_project_rule.protected_branches << protected_branch
+          approval_project_rule.groups << approver_group
+        end
+
+        context 'when the reporter is not a group member' do
+          specify do
+            expect(subject).not_to be_allowed(:approve_merge_request)
+          end
+        end
+
+        context 'when the reporter is a group member' do
+          before do
+            approver_group.add_reporter(user)
+          end
+
+          specify do
+            expect(subject).to be_allowed(:approve_merge_request)
+          end
+        end
+      end
+
+      context 'when the reporter has permission for a different protected branch' do
+        let(:protected_branch2) { create(:protected_branch, project: project, name: branch_name, code_owner_approval_required: true) }
+
+        before do
+          approval_project_rule.protected_branches << protected_branch2
+          approval_project_rule.groups << approver_group
+        end
+
+        it 'does not allow approval of the merge request' do
+          expect(subject).not_to be_allowed(:approve_merge_request)
+        end
+      end
+
+      context 'when the protected branch name is a wildcard' do
+        let(:wildcard_protected_branch) { create(:protected_branch, project: project, name: '*-stable', code_owner_approval_required: true) }
+
+        before do
+          approval_project_rule.protected_branches << wildcard_protected_branch
+          approval_project_rule.groups << approver_group
+          approver_group.add_reporter(user)
+        end
+
+        context 'when the reporter has permission for the wildcarded branch' do
+          let(:branch_name) { '13-4-stable' }
+
+          it 'does allows approval of the merge request' do
+            expect(subject).to be_allowed(:approve_merge_request)
+          end
+        end
+
+        context 'when the reporter does not have permission for the wildcarded branch' do
+          let(:branch_name) { '13-4-pre' }
+
+          it 'does allows approval of the merge request' do
+            expect(subject).not_to be_allowed(:approve_merge_request)
+          end
+        end
+      end
+    end
+  end
+
   context 'when checking for namespace whether exceeding storage limit' do
     context 'when namespace does exceeds storage limit' do
       before do

spec/lib/gitlab/import_export/all_models.yml

@@ -299,6 +299,7 @@ protected_branches:
 - merge_access_levels
 - push_access_levels
 - unprotect_access_levels
+- approval_project_rules
 protected_tags:
 - project
 - create_access_levels