From 43be0e3abecb89e392624b2a061f2fa762bc183e Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Thu, 12 Jun 2025 15:36:34 -0400 Subject: [PATCH 1/8] Use a more specific policy for new approval rule model Add a new policy to add the new approval rule model to be used for project level approval rules --- ee/app/models/merge_requests/approval_rule.rb | 4 - .../merge_requests/approval_rule_policy.rb | 25 +++++ ...approval_merge_request_rule_policy_spec.rb | 84 +++------------ .../approval_rule_policy_spec.rb | 100 ++++++++++++++++++ 4 files changed, 141 insertions(+), 72 deletions(-) create mode 100644 ee/app/policies/merge_requests/approval_rule_policy.rb create mode 100644 ee/spec/policies/merge_requests/approval_rule_policy_spec.rb diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index 5068d92c3ff49f..da3eab08371f44 100644 --- a/ee/app/models/merge_requests/approval_rule.rb +++ b/ee/app/models/merge_requests/approval_rule.rb @@ -51,10 +51,6 @@ class ApprovalRule < ApplicationRecord enum :origin, { group: 0, project: 1, merge_request: 2 }, prefix: :originates_from end - def self.declarative_policy_class - 'ApprovalMergeRequestRulePolicy' - end - def scan_result_policy_read; end def section; end diff --git a/ee/app/policies/merge_requests/approval_rule_policy.rb b/ee/app/policies/merge_requests/approval_rule_policy.rb new file mode 100644 index 00000000000000..7d3ec0f48b4f37 --- /dev/null +++ b/ee/app/policies/merge_requests/approval_rule_policy.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovalRulePolicy < BasePolicy + condition(:project_origin) { @subject.originates_from_project? } + condition(:project_readable) { can?(:read_project, @subject.project) } + condition(:project_editable) { can?(:admin_project, @subject.project) } + + condition(:merge_request_origin) { @subject.originates_from_merge_request? } + condition(:merge_request_readable) { can?(:read_merge_request, @subject.merge_request) } + condition(:merge_request_editable) do + if Feature.enabled?(:ensure_consistent_editing_rule, @subject.merge_request.project) + @subject.editable_by_user?(@user) && + can?(:update_merge_request, @subject.merge_request) + else + can?(:update_merge_request, @subject.merge_request) && @subject.user_defined? + end + end + + rule { project_origin & project_readable }.enable :read_approval_rule + rule { project_origin & project_editable }.enable :edit_approval_rule + rule { merge_request_origin & merge_request_readable }.enable :read_approval_rule + rule { merge_request_origin & merge_request_editable }.enable :edit_approval_rule + end +end diff --git a/ee/spec/policies/approval_merge_request_rule_policy_spec.rb b/ee/spec/policies/approval_merge_request_rule_policy_spec.rb index 4104d01a5da306..3cc41271d78496 100644 --- a/ee/spec/policies/approval_merge_request_rule_policy_spec.rb +++ b/ee/spec/policies/approval_merge_request_rule_policy_spec.rb @@ -51,96 +51,44 @@ def permissions(user, approval_rule) let(:project) { create(:project, :public, disable_overriding_approvers_per_merge_request: false) } let(:user) { merge_request.author } - context 'when v2 approval rule' do - let(:approval_rule) do - create(:merge_requests_approval_rule, merge_request: merge_request, project_id: project.id) - end - - it_behaves_like 'editing a merge request approval policy' - end - - context 'when v1 approval rule' do - let(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } + let(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } - it_behaves_like 'editing a merge request approval policy' - end + it_behaves_like 'editing a merge request approval policy' end context 'when ensure_consistent_editing_rule is off' do let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } before do stub_feature_flags(ensure_consistent_editing_rule: false) end - context 'when v2 approval rule' do - let_it_be(:project) { create(:project) } - let_it_be(:approval_rule) do - create(:merge_requests_approval_rule, merge_request: merge_request, project_id: project.id) + context 'when user can update merge request' do + it 'allows updating approval rule' do + expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule) end - context 'when user can update merge request' do + context 'when rule is any-approval' do + let(:approval_rule) { build(:any_approver_rule, merge_request: merge_request) } + it 'allows updating approval rule' do expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule) end - - context 'when rule is any-approval' do - let(:approval_rule) do - build(:merge_requests_approval_rule, rule_type: :any_approver, merge_request: merge_request) - end - - it 'allows updating approval rule' do - expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule) - end - end - - context 'when rule is not user editable' do - let(:approval_rule) do - build(:merge_requests_approval_rule, rule_type: :code_owner, merge_request: merge_request) - end - - it 'disallows updating approval rule' do - expect(permissions(merge_request.author, approval_rule)).to be_disallowed(:edit_approval_rule) - end - end end - context 'when user cannot update merge request' do + context 'when rule is not user editable' do + let(:approval_rule) { create(:code_owner_rule, merge_request: merge_request) } + it 'disallows updating approval rule' do - expect(permissions(create(:user), approval_rule)).to be_disallowed(:edit_approval_rule) + expect(permissions(merge_request.author, approval_rule)).to be_disallowed(:edit_approval_rule) end end end - context 'when v1 approval rule' do - let_it_be(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } - - context 'when user can update merge request' do - it 'allows updating approval rule' do - expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule) - end - - context 'when rule is any-approval' do - let(:approval_rule) { build(:any_approver_rule, merge_request: merge_request) } - - it 'allows updating approval rule' do - expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule) - end - end - - context 'when rule is not user editable' do - let(:approval_rule) { create(:code_owner_rule, merge_request: merge_request) } - - it 'disallows updating approval rule' do - expect(permissions(merge_request.author, approval_rule)).to be_disallowed(:edit_approval_rule) - end - end - end - - context 'when user cannot update merge request' do - it 'disallows updating approval rule' do - expect(permissions(create(:user), approval_rule)).to be_disallowed(:edit_approval_rule) - end + context 'when user cannot update merge request' do + it 'disallows updating approval rule' do + expect(permissions(create(:user), approval_rule)).to be_disallowed(:edit_approval_rule) end end end diff --git a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb new file mode 100644 index 00000000000000..1080f7b998e538 --- /dev/null +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ApprovalRulePolicy, feature_category: :source_code_management do + let_it_be(:project) { create(:project, :private) } + let(:guest) { create(:user) } + let(:maintainer) { create(:user, maintainer_of: project) } + + let(:user) { guest } + + subject(:permissions) { described_class.new(user, approval_rule) } + + context 'when the rule originates from project' do + let(:approval_rule) do + build(:merge_requests_approval_rule, :from_project, project: project, project_id: project.id) + end + + context 'and the user has permission to read the project' do + let(:user) { maintainer } + + it { is_expected.to be_allowed(:read_approval_rule) } + end + + context 'and the user has permission to change project settings' do + let(:user) { maintainer } + + it { is_expected.to be_allowed(:edit_approval_rule) } + end + + it { is_expected.not_to be_allowed(:edit_approval_rule) } + it { is_expected.not_to be_allowed(:read_approval_rule) } + end + + context 'when the rule originates from a merge request' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:approval_rule) do + build(:merge_requests_approval_rule, :from_merge_request, merge_request: merge_request, project_id: project.id, + rule_type: rule_type) + end + + let(:rule_type) { :regular } + + context 'and the ensure_consistent_editing_rule is enabled' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:project) { create(:project, :public, disable_overriding_approvers_per_merge_request: false) } + let(:user) { merge_request.author } + + context 'when the rule is editable' do + before do + allow(approval_rule).to receive(:editable_by_user?).and_return(true) + end + + context 'when the merge request can be updated' do + it { is_expected.to be_allowed(:edit_approval_rule) } + end + + context 'when the merge request can not be updated' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + before do + project.project_feature.merge_requests_access_level = Featurable::DISABLED + project.save! + end + + it { is_expected.not_to be_allowed(:edit_approval_rule) } + end + end + + context 'when the rule is not editable' do + it 'disallows updating approval rule' do + expect(approval_rule).to receive(:editable_by_user?).and_return(false) + + expect(permissions).not_to be_allowed(:edit_approval_rule) + end + end + end + + context 'and the user has permission to read the merge request' do + let(:user) { maintainer } + + it { is_expected.to be_allowed(:read_approval_rule) } + end + + context 'and the user has permission to change merge request settings' do + let(:user) { maintainer } + + it { is_expected.to be_allowed(:edit_approval_rule) } + + context 'and the approval rule is not user defined' do + let(:rule_type) { :code_owner } + + it { is_expected.not_to be_allowed(:edit_approval_rule) } + end + end + + it { is_expected.not_to be_allowed(:edit_approval_rule) } + it { is_expected.not_to be_allowed(:read_approval_rule) } + end +end -- GitLab From b7fa19a36250481f566f8936570e706fdf346c39 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Mon, 16 Jun 2025 09:46:56 -0400 Subject: [PATCH 2/8] Remove a test --- ee/spec/models/merge_requests/approval_rule_spec.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ee/spec/models/merge_requests/approval_rule_spec.rb b/ee/spec/models/merge_requests/approval_rule_spec.rb index b38a3f5e786633..e08726c691b210 100644 --- a/ee/spec/models/merge_requests/approval_rule_spec.rb +++ b/ee/spec/models/merge_requests/approval_rule_spec.rb @@ -9,13 +9,6 @@ subject(:rule) { build(:merge_requests_approval_rule, attributes) } - describe 'policy' do - it 'uses the v1 policy class' do - expect(described_class.declarative_policy_class) - .to eq('ApprovalMergeRequestRulePolicy') - end - end - describe 'user_defined?' do using RSpec::Parameterized::TableSyntax -- GitLab From da2a217abeab91e9a9ddb7b936263f54d2a51ae8 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Mon, 16 Jun 2025 10:42:15 -0400 Subject: [PATCH 3/8] Refactor to re-use created project in tests --- .../merge_requests/approval_rule_policy_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb index 1080f7b998e538..78ecdb3c1a777d 100644 --- a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe MergeRequests::ApprovalRulePolicy, feature_category: :source_code_management do - let_it_be(:project) { create(:project, :private) } + let_it_be_with_refind(:project) { create(:project, :private) } let(:guest) { create(:user) } let(:maintainer) { create(:user, maintainer_of: project) } @@ -43,9 +43,15 @@ context 'and the ensure_consistent_editing_rule is enabled' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:project) { create(:project, :public, disable_overriding_approvers_per_merge_request: false) } let(:user) { merge_request.author } + before do + project.update!( + disable_overriding_approvers_per_merge_request: false, + visibility_level: Gitlab::VisibilityLevel::PUBLIC + ) + end + context 'when the rule is editable' do before do allow(approval_rule).to receive(:editable_by_user?).and_return(true) -- GitLab From b7ad6eb2e3859848036de2063f7f74ea779c3fee Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Mon, 16 Jun 2025 11:43:13 -0400 Subject: [PATCH 4/8] Handle nil merge request --- ee/app/policies/merge_requests/approval_rule_policy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/policies/merge_requests/approval_rule_policy.rb b/ee/app/policies/merge_requests/approval_rule_policy.rb index 7d3ec0f48b4f37..7686c24a317a6d 100644 --- a/ee/app/policies/merge_requests/approval_rule_policy.rb +++ b/ee/app/policies/merge_requests/approval_rule_policy.rb @@ -9,7 +9,7 @@ class ApprovalRulePolicy < BasePolicy condition(:merge_request_origin) { @subject.originates_from_merge_request? } condition(:merge_request_readable) { can?(:read_merge_request, @subject.merge_request) } condition(:merge_request_editable) do - if Feature.enabled?(:ensure_consistent_editing_rule, @subject.merge_request.project) + if Feature.enabled?(:ensure_consistent_editing_rule, @subject.merge_request&.project) @subject.editable_by_user?(@user) && can?(:update_merge_request, @subject.merge_request) else -- GitLab From f6c6aa643ca8b9d3850afe85e3957f902bd5499e Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Mon, 16 Jun 2025 11:48:27 -0400 Subject: [PATCH 5/8] Refactor to simplify tests --- ee/spec/policies/merge_requests/approval_rule_policy_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb index 78ecdb3c1a777d..49dec1009d185d 100644 --- a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -62,8 +62,6 @@ end context 'when the merge request can not be updated' do - let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - before do project.project_feature.merge_requests_access_level = Featurable::DISABLED project.save! -- GitLab From 6eb56019bbc33ffcc850083791aece53bfa6ad99 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Tue, 17 Jun 2025 10:14:57 -0400 Subject: [PATCH 6/8] Refactor and add tests --- .../approval_rule_policy_spec.rb | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb index 49dec1009d185d..ba6f000760452d 100644 --- a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -13,9 +13,15 @@ context 'when the rule originates from project' do let(:approval_rule) do - build(:merge_requests_approval_rule, :from_project, project: project, project_id: project.id) + build(:merge_requests_approval_rule, :from_project, + project: project, + project_id: project.id, + rule_type: rule_type + ) end + let(:rule_type) { :regular } + context 'and the user has permission to read the project' do let(:user) { maintainer } @@ -28,8 +34,10 @@ it { is_expected.to be_allowed(:edit_approval_rule) } end - it { is_expected.not_to be_allowed(:edit_approval_rule) } - it { is_expected.not_to be_allowed(:read_approval_rule) } + context 'and the user lacks the required access level' do + it { is_expected.not_to be_allowed(:edit_approval_rule) } + it { is_expected.not_to be_allowed(:read_approval_rule) } + end end context 'when the rule originates from a merge request' do @@ -41,7 +49,7 @@ let(:rule_type) { :regular } - context 'and the ensure_consistent_editing_rule is enabled' do + context 'and the ensure_consistent_editing_rule flag is enabled' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:user) { merge_request.author } @@ -80,25 +88,33 @@ end end - context 'and the user has permission to read the merge request' do - let(:user) { maintainer } + context 'and the ensure_consistent_editing_rule flag is not enabled' do + before do + stub_feature_flags(ensure_consistent_editing_rule: false) + end - it { is_expected.to be_allowed(:read_approval_rule) } - end + context 'and the user has permission to read the merge request' do + let(:user) { maintainer } - context 'and the user has permission to change merge request settings' do - let(:user) { maintainer } + it { is_expected.to be_allowed(:read_approval_rule) } + end - it { is_expected.to be_allowed(:edit_approval_rule) } + context 'and the user has permission to change merge request settings' do + let(:user) { maintainer } + + it { is_expected.to be_allowed(:edit_approval_rule) } + + context 'and the approval rule is not user defined' do + let(:rule_type) { :code_owner } - context 'and the approval rule is not user defined' do - let(:rule_type) { :code_owner } + it { is_expected.not_to be_allowed(:edit_approval_rule) } + end + end + context 'and the user lacks the required access level' do it { is_expected.not_to be_allowed(:edit_approval_rule) } + it { is_expected.not_to be_allowed(:read_approval_rule) } end end - - it { is_expected.not_to be_allowed(:edit_approval_rule) } - it { is_expected.not_to be_allowed(:read_approval_rule) } end end -- GitLab From 39260a51450acba4de6283e8622f806e2399ebc8 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Thu, 19 Jun 2025 13:37:30 -0400 Subject: [PATCH 7/8] Add scope and score to conditions --- ee/app/policies/merge_requests/approval_rule_policy.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/policies/merge_requests/approval_rule_policy.rb b/ee/app/policies/merge_requests/approval_rule_policy.rb index 7686c24a317a6d..75a0a5b01948b2 100644 --- a/ee/app/policies/merge_requests/approval_rule_policy.rb +++ b/ee/app/policies/merge_requests/approval_rule_policy.rb @@ -2,11 +2,11 @@ module MergeRequests class ApprovalRulePolicy < BasePolicy - condition(:project_origin) { @subject.originates_from_project? } + condition(:project_origin, scope: :subject, score: 0) { @subject.originates_from_project? } condition(:project_readable) { can?(:read_project, @subject.project) } condition(:project_editable) { can?(:admin_project, @subject.project) } - condition(:merge_request_origin) { @subject.originates_from_merge_request? } + condition(:merge_request_origin, scope: :subject, score: 0) { @subject.originates_from_merge_request? } condition(:merge_request_readable) { can?(:read_merge_request, @subject.merge_request) } condition(:merge_request_editable) do if Feature.enabled?(:ensure_consistent_editing_rule, @subject.merge_request&.project) -- GitLab From d63b1396077f0562470b7cc929ce26deddbbb666 Mon Sep 17 00:00:00 2001 From: "j.seto" Date: Thu, 19 Jun 2025 13:58:00 -0400 Subject: [PATCH 8/8] Refactor and add tests --- .../approval_rule_policy_spec.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb index ba6f000760452d..f26affb8e6b1e0 100644 --- a/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequests::ApprovalRulePolicy, feature_category: :source_code_management do let_it_be_with_refind(:project) { create(:project, :private) } + let(:rule_type) { :regular } let(:guest) { create(:user) } let(:maintainer) { create(:user, maintainer_of: project) } @@ -20,8 +21,6 @@ ) end - let(:rule_type) { :regular } - context 'and the user has permission to read the project' do let(:user) { maintainer } @@ -47,8 +46,6 @@ rule_type: rule_type) end - let(:rule_type) { :regular } - context 'and the ensure_consistent_editing_rule flag is enabled' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:user) { merge_request.author } @@ -117,4 +114,18 @@ end end end + + context 'when the rule originates from a group' do + let(:approval_rule) do + build(:merge_requests_approval_rule, :from_group, + project: project, + project_id: project.id, + rule_type: rule_type + ) + end + + # TODO: Update this once group approval rules have been implemented + it { is_expected.not_to be_allowed(:edit_approval_rule) } + it { is_expected.not_to be_allowed(:read_approval_rule) } + end end -- GitLab