Merge branch 'vij-mr-policies' into 'master'

Adjust Merge Request policies for read-only state

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106172



Merged-by: Doug Stull <dstull@gitlab.com>
Approved-by: Mohamed Hamda <mhamda@gitlab.com>
Approved-by: Doug Stull <dstull@gitlab.com>
Reviewed-by: Doug Stull <dstull@gitlab.com>
Reviewed-by: Vijay Hawoldar <vhawoldar@gitlab.com>
Reviewed-by: Mohamed Hamda <mhamda@gitlab.com>
Co-authored-by: Vijay Hawoldar <vhawoldar@gitlab.com>

app/policies/merge_request_policy.rb

 # frozen_string_literal: true
 
 class MergeRequestPolicy < IssuablePolicy
+  condition(:can_approve) { can_approve? }
+
   rule { locked }.policy do
     prevent :reopen_merge_request
   end
@@ -14,7 +16,7 @@ class MergeRequestPolicy < IssuablePolicy
     prevent :accept_merge_request
   end
 
-  rule { can?(:update_merge_request) & is_project_member }.policy do
+  rule { can_approve }.policy do
     enable :approve_merge_request
   end
 
@@ -40,6 +42,12 @@ class MergeRequestPolicy < IssuablePolicy
   rule { can?(:admin_merge_request) }.policy do
     enable :set_merge_request_metadata
   end
+
+  private
+
+  def can_approve?
+    can?(:update_merge_request) && is_project_member?
+  end
 end
 
 MergeRequestPolicy.prepend_mod_with('MergeRequestPolicy')

ee/app/policies/ee/merge_request_policy.rb

@@ -3,6 +3,7 @@
 module EE
   module MergeRequestPolicy
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     prepended do
       with_scope :subject
@@ -15,7 +16,7 @@ module MergeRequestPolicy
           can?(:developer_access, @subject.target_project)
       end
 
-      condition(:read_only, scope: :subject) { @subject.target_project&.namespace&.read_only? }
+      condition(:read_only, scope: :subject) { read_only? }
 
       condition(:merge_request_group_approver, score: 140) do
         project = @subject.target_project
@@ -31,6 +32,10 @@ module MergeRequestPolicy
         @subject.target_project.licensed_feature_available?(:report_approver_rules)
       end
 
+      def read_only?
+        @subject.target_project&.namespace&.read_only?
+      end
+
       def group_access?(protected_branch)
         protected_branch.approval_project_rules.for_groups(@user.group_members.reporters.select(:source_id)).exists?
       end
@@ -47,14 +52,19 @@ def group_access?(protected_branch)
       rule { external_status_checks_enabled }.enable :provide_status_check_response
 
       rule { read_only }.policy do
-        prevent :approve_merge_request
         prevent :update_merge_request
-        prevent :reopen_merge_request
-        prevent :create_note
-        prevent :resolve_note
       end
 
       rule { approval_rules_licence_enabled }.enable :create_merge_request_approval_rules
     end
+
+    private
+
+    override :can_approve?
+    def can_approve?
+      return can?(:developer_access) if read_only?
+
+      super
+    end
   end
 end

ee/app/policies/ee/readonly_abilities.rb

@@ -7,7 +7,6 @@ module ReadonlyAbilities
     READONLY_ABILITIES = %i[
       admin_tag
       push_to_delete_protected_branch
-      resolve_note
       create_merge_request_from
       create_merge_request_in
       admin_software_license_policy

ee/spec/policies/merge_request_policy_spec.rb

@@ -233,12 +233,12 @@ def policy_for(user)
         allow(merge_request.target_project.namespace).to receive(:read_only?).and_return(true)
       end
 
-      it 'does not allow few policies for all users including maintainer' do
-        expect(policy_for(maintainer)).to be_disallowed(:approve_merge_request,
-                                                        :update_merge_request,
-                                                        :reopen_merge_request,
-                                                        :create_note,
-                                                        :resolve_note)
+      it 'does not allow update_merge_request for all users including maintainer' do
+        expect(policy_for(maintainer)).to be_disallowed(:update_merge_request)
+      end
+
+      it 'does allow approval of the merge request' do
+        expect(policy_for(developer)).to be_allowed(:approve_merge_request)
       end
     end