Skip to content

G
gitlab-ce
Commit dbfad621 authored by Tan Le's avatar Tan Le
Browse files
Options

Use instance MR approval settings on projects 

When instance-level MR approval settings are scoped to a compliance
framework, all compliance projects will use these instance-level
settings as SSOT. MR approval settings in project settings page will be
locked out for further changes as they are now centrally managed in
Admin area.

If compliance framework is not selected at instance-level or project has
no longer has compliance label applied, the project-level MR approval
settings will refer to project-level ones as SSOT.

This change is behind a feature toggle
`project_compliance_merge_request_approval_settings`
parent 42109dd9
Hide whitespace changes
Inline Side-by-side
Showing with 320 additions and 252 deletions
ee/app/models/ee/project.rb
View file @ dbfad621
......@@ -214,6 +214,13 @@ module EE
end
end
def has_regulated_settings?
return unless compliance_framework_setting
compliance_framework_id = ::ComplianceManagement::ComplianceFramework::FRAMEWORKS[compliance_framework_setting.framework.to_sym]
::Gitlab::CurrentSettings.current_application_settings.compliance_frameworks.include?(compliance_framework_id)
end
def can_store_security_reports?
namespace.store_security_reports_available? || public?
end
......@@ -672,29 +679,36 @@ module EE
def disable_overriding_approvers_per_merge_request
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return (::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super) unless project_compliance_mr_approval_settings?
return super unless has_regulated_settings?
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ||
super
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
end
alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
def merge_requests_author_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return false if !project_compliance_mr_approval_settings? && ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
return super if !project_compliance_mr_approval_settings? && !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
return super unless has_regulated_settings?
return false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
super
!::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
end
alias_method :merge_requests_author_approval?, :merge_requests_author_approval
def merge_requests_disable_committers_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
return (::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super) unless project_compliance_mr_approval_settings?
return super unless has_regulated_settings?
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ||
super
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
end
alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
def project_compliance_mr_approval_settings?
::Feature.enabled?(:project_compliance_merge_request_approval_settings, self)
end
def license_compliance
strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
end
......
ee/app/policies/ee/project_policy.rb
View file @ dbfad621
......@@ -75,6 +75,36 @@ module EE
.prevent_merge_requests_committers_approval
end
with_scope :subject
condition(:regulated_merge_request_approval_settings) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
@subject.has_regulated_settings?
end
condition(:cannot_modify_approvers_rules) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_approvers_rules? && !admin?
end
end
condition(:cannot_modify_merge_request_author_setting) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_author_setting? && !admin?
end
end
condition(:cannot_modify_merge_request_committer_setting) do
if @subject.project_compliance_mr_approval_settings?
regulated_merge_request_approval_settings?
else
owner_cannot_modify_merge_request_committer_setting? && !admin?
end
end
with_scope :global
condition(:cluster_health_available) do
License.feature_available?(:cluster_health)
......@@ -372,24 +402,21 @@ module EE
prevent :read_project
end
rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
rule { cannot_modify_approvers_rules }.policy do
prevent :modify_approvers_rules
prevent :modify_approvers_list
end
rule { owner_cannot_modify_merge_request_author_setting & ~admin }.policy do
rule { cannot_modify_merge_request_author_setting }.policy do
prevent :modify_merge_request_author_setting
end
rule { owner_cannot_modify_merge_request_committer_setting & ~admin }.policy do
rule { cannot_modify_merge_request_committer_setting }.policy do
prevent :modify_merge_request_committer_setting
end
rule { can?(:read_cluster) & cluster_health_available }.enable :read_cluster_health
rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
prevent :modify_approvers_list
end
rule { can?(:read_merge_request) & code_review_analytics_enabled }.enable :read_code_review_analytics
rule { can?(:read_project) & requirements_available }.enable :read_requirement
......
ee/spec/models/approval_state_spec.rb
View file @ dbfad621
......@@ -50,133 +50,44 @@ RSpec.describe ApprovalState do
before do
allow(merge_request).to receive(:committers).and_return(User.where(id: committers))
project.update!(
merge_requests_author_approval: merge_requests_author_approval,
merge_requests_disable_committers_approval: merge_requests_disable_committers_approval
)
allow(project).to receive(:merge_requests_author_approval?).and_return(merge_requests_author_approval)
allow(project).to receive(:merge_requests_disable_committers_approval?).and_return(merge_requests_disable_committers_approval)
create_rule(users: committers)
end
context 'when self approval is disabled on project level' do
context 'when self approval is disabled on project' do
let(:merge_requests_author_approval) { false }
it 'excludes authors' do
expect(results).not_to include(merge_request.author)
end
context 'when self approval is enabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_author_approval: false)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'excludes author' do
expect(results).not_to include(merge_request.author)
end
end
context 'when self approval is disabled on instance level' do
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_application_setting(prevent_merge_requests_author_approval: true)
end
it 'excludes authors' do
expect(results).not_to include(merge_request.author)
end
end
end
context 'when self approval is enabled on project level' do
context 'when self approval is enabled on project' do
let(:merge_requests_author_approval) { true }
it 'includes author' do
expect(results).to include(merge_request.author)
end
context 'when self approval is enabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_author_approval: false)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'includes author' do
expect(results).to include(merge_request.author)
end
end
context 'when self approval is disabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_author_approval: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'excludes authors' do
expect(results).not_to include(merge_request.author)
end
end
end
context 'when committers approval is enabled on project level' do
context 'when committers approval is enabled on project' do
let(:merge_requests_author_approval) { true }
let(:merge_requests_disable_committers_approval) { false }
it 'includes committers' do
expect(results).to include(*committers)
end
context 'when committers approval is enabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_committers_approval: false)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'includes committers' do
expect(results).to include(*committers)
end
end
context 'when committers approval is disabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_committers_approval: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'excludes committers' do
expect(results).not_to include(*committers)
end
end
end
context 'when committers approval is disabled on project level' do
context 'when committers approval is disabled on project' do
let(:merge_requests_author_approval) { true }
let(:merge_requests_disable_committers_approval) { true }
it 'excludes committers' do
expect(results).not_to include(*committers)
end
context 'when committers approval is enabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_committers_approval: false)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'excludes committers' do
expect(results).not_to include(*committers)
end
end
context 'when committers approval is disabled on instance level' do
before do
stub_application_setting(prevent_merge_requests_committers_approval: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
end
it 'excludes committers' do
expect(results).not_to include(*committers)
end
end
end
end
......
ee/spec/models/project_spec.rb
View file @ dbfad621
......@@ -520,31 +520,68 @@ RSpec.describe Project do
context 'merge requests related settings' do
shared_examples 'setting modified by application setting' do
using RSpec::Parameterized::TableSyntax
context 'when compliance merge request approval settings feature flag is enabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | true
false | true | true | true
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | true
false | true | true | false
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
with_them do
let(:project) { create(:project) }
context 'when compliance merge request approval settings feature flag is disabled' do
using RSpec::Parameterized::TableSyntax
before do
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | true
false | true | true | true
true | false | true | true
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
end
......@@ -566,39 +603,106 @@ RSpec.describe Project do
end
describe '#merge_requests_author_approval' do
using RSpec::Parameterized::TableSyntax
let(:project) { create(:project) }
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval }
context 'when compliance merge request approval settings feature flag is enabled' do
using RSpec::Parameterized::TableSyntax
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | true
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
with_them do
let(:project) { create(:project) }
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
with_them do
let(:project) { create(:project) }
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval }
context 'when compliance merge request approval settings feature flag is disabled' do
using RSpec::Parameterized::TableSyntax
before do
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
true | true | true | false
false | true | true | true
true | false | true | false
false | false | true | false
true | true | false | true
false | true | false | true
true | false | false | false
false | false | false | false
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
with_them do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
stub_licensed_features(feature => feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
end
end
describe '#has_regulated_settings?' do
let_it_be(:framework) { ComplianceManagement::ComplianceFramework::FRAMEWORKS.first }
let_it_be(:compliance_framework_setting) { create(:compliance_framework_project_setting, framework: framework.first.to_s) }
let_it_be(:project) { create(:project, compliance_framework_setting: compliance_framework_setting) }
subject { project.has_regulated_settings? }
context 'framework is regulated' do
before do
stub_application_setting(compliance_frameworks: [framework.last])
end
it { is_expected.to be_truthy }
end
context 'framework is not regulated' do
before do
stub_application_setting(compliance_frameworks: [])
end
it { is_expected.to be_falsey }
end
context 'project does not have compliance framework' do
let_it_be(:project) { create(:project) }
it { is_expected.to be_falsey }
end
end
describe '#has_active_hooks?' do
context "with group hooks" do
let(:group) { create(:group) }
......
ee/spec/policies/project_policy_spec.rb
View file @ dbfad621
......@@ -1325,60 +1325,129 @@ RSpec.describe ProjectPolicy do
shared_examples 'merge request rules' do
let(:project) { create(:project, namespace: owner.namespace) }
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
context 'when compliance merge request approval settings feature flag is enabled' do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: true)
end
with_them do
let(:current_user) { public_send(role) }
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | false
end
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
context 'with merge request approvers not available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
end
context 'with merge request approvers not available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
context 'when compliance merge request approval settings feature flag is disabled' do
before do
stub_feature_flags(project_compliance_merge_request_approval_settings: false)
end
with_them do
let(:current_user) { public_send(role) }
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
context 'with merge request approvers not available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
end
end
......@@ -1405,66 +1474,9 @@ RSpec.describe ProjectPolicy do
end
describe ':modify_approvers_list' do
let(:setting_name) { :disable_overriding_approvers_per_merge_request }
let(:policy) { :modify_approvers_list }
let(:project) { create(:project, namespace: owner.namespace) }
using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | false
:owner | false | nil | true
:owner | true | nil | false
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: true)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
context 'with merge request approvers not available in license' do
where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false
:reporter | true | nil | false
:developer | true | nil | false
:maintainer | false | nil | true
:maintainer | true | nil | true
:owner | false | nil | true
:owner | true | nil | true
:admin | false | false | false
:admin | false | true | true
:admin | true | false | false
:admin | true | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: false)
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
it_behaves_like 'merge request rules' do
let(:setting_name) { :disable_overriding_approvers_per_merge_request }
let(:policy) { :modify_approvers_list }
end
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7