diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index 5068d92c3ff49f728baed0b2331fc6c23aaf4d2e..da3eab08371f4451757111705590b44a86c4ac21 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 0000000000000000000000000000000000000000..75a0a5b01948b2d0120d909958b43aba7461b992 --- /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, 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, 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) + @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/models/merge_requests/approval_rule_spec.rb b/ee/spec/models/merge_requests/approval_rule_spec.rb index b38a3f5e7866333fe04162efb606764740e9a942..e08726c691b210dd56af57e86cf0ce02cd3aa429 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 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 4104d01a5da30681c5e3a375e2fe90af3e428966..3cc41271d78496c09cb30842aa7ba970c0e52baf 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 0000000000000000000000000000000000000000..f26affb8e6b1e0e52e63b31794db20b90916611c --- /dev/null +++ b/ee/spec/policies/merge_requests/approval_rule_policy_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'spec_helper' + +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) } + + 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, + rule_type: rule_type + ) + 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 + + 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 + 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 + + 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 } + + 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) + 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 + 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 ensure_consistent_editing_rule flag is not enabled' do + before do + stub_feature_flags(ensure_consistent_editing_rule: false) + 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 + + 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 + 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