Skip to content

G
gitlab-ce
Commit cba00ad0 authored by Ash McKenzie's avatar Ash McKenzie
Browse files
Options

Merge branch 'tancnle/219359-apply-compliance-mr-approvals-settings' into 'master' 

Use instance MR approval settings on compliance projects

See merge request gitlab-org/gitlab!34605
parents 6957a03d dbfad621
Hide whitespace changes
Inline Side-by-side
Showing with 320 additions and 252 deletions
ee/app/models/ee/project.rb
View file @ cba00ad0
...@@ -214,6 +214,13 @@ module EE ...@@ -214,6 +214,13 @@ module EE
end end
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? def can_store_security_reports?
namespace.store_security_reports_available? || public? namespace.store_security_reports_available? || public?
end end
...@@ -672,29 +679,36 @@ module EE ...@@ -672,29 +679,36 @@ module EE
def disable_overriding_approvers_per_merge_request def disable_overriding_approvers_per_merge_request
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? || ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
super
end end
alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
def merge_requests_author_approval def merge_requests_author_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
super
end end
alias_method :merge_requests_author_approval?, :merge_requests_author_approval alias_method :merge_requests_author_approval?, :merge_requests_author_approval
def merge_requests_disable_committers_approval def merge_requests_disable_committers_approval
return super unless License.feature_available?(:admin_merge_request_approvers_rules) 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? || ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
super
end end
alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval 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 def license_compliance
strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) } strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
end end
......
ee/app/policies/ee/project_policy.rb
View file @ cba00ad0
...@@ -75,6 +75,36 @@ module EE ...@@ -75,6 +75,36 @@ module EE
.prevent_merge_requests_committers_approval .prevent_merge_requests_committers_approval
end 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 with_scope :global
condition(:cluster_health_available) do condition(:cluster_health_available) do
License.feature_available?(:cluster_health) License.feature_available?(:cluster_health)
...@@ -372,24 +402,21 @@ module EE ...@@ -372,24 +402,21 @@ module EE
prevent :read_project prevent :read_project
end end
rule { owner_cannot_modify_approvers_rules & ~admin }.policy do rule { cannot_modify_approvers_rules }.policy do
prevent :modify_approvers_rules prevent :modify_approvers_rules
prevent :modify_approvers_list
end 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 prevent :modify_merge_request_author_setting
end 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 prevent :modify_merge_request_committer_setting
end end
rule { can?(:read_cluster) & cluster_health_available }.enable :read_cluster_health 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_merge_request) & code_review_analytics_enabled }.enable :read_code_review_analytics
rule { can?(:read_project) & requirements_available }.enable :read_requirement rule { can?(:read_project) & requirements_available }.enable :read_requirement
......
ee/spec/models/approval_state_spec.rb
View file @ cba00ad0
...@@ -50,133 +50,44 @@ RSpec.describe ApprovalState do ...@@ -50,133 +50,44 @@ RSpec.describe ApprovalState do
before do before do
allow(merge_request).to receive(:committers).and_return(User.where(id: committers)) allow(merge_request).to receive(:committers).and_return(User.where(id: committers))
project.update!( allow(project).to receive(:merge_requests_author_approval?).and_return(merge_requests_author_approval)
merge_requests_author_approval: merge_requests_author_approval, allow(project).to receive(:merge_requests_disable_committers_approval?).and_return(merge_requests_disable_committers_approval)
merge_requests_disable_committers_approval: merge_requests_disable_committers_approval
)
create_rule(users: committers) create_rule(users: committers)
end 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 } let(:merge_requests_author_approval) { false }
it 'excludes authors' do it 'excludes authors' do
expect(results).not_to include(merge_request.author) expect(results).not_to include(merge_request.author)
end 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 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 } let(:merge_requests_author_approval) { true }
it 'includes author' do it 'includes author' do
expect(results).to include(merge_request.author) expect(results).to include(merge_request.author)
end 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 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_author_approval) { true }
let(:merge_requests_disable_committers_approval) { false } let(:merge_requests_disable_committers_approval) { false }
it 'includes committers' do it 'includes committers' do
expect(results).to include(*committers) expect(results).to include(*committers)
end 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 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_author_approval) { true }
let(:merge_requests_disable_committers_approval) { true } let(:merge_requests_disable_committers_approval) { true }
it 'excludes committers' do it 'excludes committers' do
expect(results).not_to include(*committers) expect(results).not_to include(*committers)
end 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
end end
......
ee/spec/models/project_spec.rb
View file @ cba00ad0
...@@ -520,31 +520,68 @@ RSpec.describe Project do ...@@ -520,31 +520,68 @@ RSpec.describe Project do
context 'merge requests related settings' do context 'merge requests related settings' do
shared_examples 'setting modified by application setting' 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 where(:app_setting, :project_setting, :regulated_settings, :final_setting) do
true | true | true | true true | true | true | true
false | true | true | true false | true | true | false
true | false | true | true true | false | true | true
false | false | true | false false | false | true | false
true | true | false | true true | true | false | true
false | true | false | true false | true | false | true
true | false | false | false true | false | false | false
false | 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 end
with_them do context 'when compliance merge request approval settings feature flag is disabled' do
let(:project) { create(:project) } using RSpec::Parameterized::TableSyntax
before do where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
stub_licensed_features(feature => feature_enabled) true | true | true | true
stub_application_setting(application_setting => app_setting) false | true | true | true
project.update(setting => project_setting) 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 end
it 'shows proper setting' do with_them do
expect(project.send(setting)).to eq(final_setting) let(:project) { create(:project) }
expect(project.send("#{setting}?")).to eq(final_setting)
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
end end
...@@ -566,39 +603,106 @@ RSpec.describe Project do ...@@ -566,39 +603,106 @@ RSpec.describe Project do
end end
describe '#merge_requests_author_approval' do 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 with_them do
true | true | true | false let(:project) { create(:project) }
false | true | true | true
true | false | true | false before do
false | false | true | false stub_feature_flags(project_compliance_merge_request_approval_settings: true)
true | true | false | true stub_licensed_features(admin_merge_request_approvers_rules: true)
false | true | false | true
true | false | false | false allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
false | false | false | false 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
with_them do context 'when compliance merge request approval settings feature flag is disabled' do
let(:project) { create(:project) } using RSpec::Parameterized::TableSyntax
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval }
before do where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
stub_licensed_features(feature => feature_enabled) true | true | true | false
stub_application_setting(application_setting => app_setting) false | true | true | true
project.update(setting => project_setting) 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 end
it 'shows proper setting' do with_them do
expect(project.send(setting)).to eq(final_setting) before do
expect(project.send("#{setting}?")).to eq(final_setting) 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
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 describe '#has_active_hooks?' do
context "with group hooks" do context "with group hooks" do
let(:group) { create(:group) } let(:group) { create(:group) }
......
ee/spec/policies/project_policy_spec.rb
View file @ cba00ad0
...@@ -1325,60 +1325,129 @@ RSpec.describe ProjectPolicy do ...@@ -1325,60 +1325,129 @@ RSpec.describe ProjectPolicy do
shared_examples 'merge request rules' do shared_examples 'merge request rules' do
let(:project) { create(:project, namespace: owner.namespace) } let(:project) { create(:project, namespace: owner.namespace) }
using RSpec::Parameterized::TableSyntax context 'when compliance merge request approval settings feature flag is enabled' do
context 'with merge request approvers rules available in license' do before do
where(:role, :setting, :admin_mode, :allowed) do stub_feature_flags(project_compliance_merge_request_approval_settings: true)
: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 end
with_them do using RSpec::Parameterized::TableSyntax
let(:current_user) { public_send(role) } 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 with_them do
stub_licensed_features(admin_merge_request_approvers_rules: true) let(:current_user) { public_send(role) }
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode 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
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
end end
context 'with merge request approvers not available in license' do context 'when compliance merge request approval settings feature flag is disabled' do
where(:role, :setting, :admin_mode, :allowed) do before do
:guest | true | nil | false stub_feature_flags(project_compliance_merge_request_approval_settings: 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 end
with_them do using RSpec::Parameterized::TableSyntax
let(:current_user) { public_send(role) } 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 with_them do
stub_licensed_features(admin_merge_request_approvers_rules: false) let(:current_user) { public_send(role) }
stub_application_setting(setting_name => setting)
enable_admin_mode!(current_user) if admin_mode 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
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 end
end end
...@@ -1405,66 +1474,9 @@ RSpec.describe ProjectPolicy do ...@@ -1405,66 +1474,9 @@ RSpec.describe ProjectPolicy do
end end
describe ':modify_approvers_list' do describe ':modify_approvers_list' do
let(:setting_name) { :disable_overriding_approvers_per_merge_request } it_behaves_like 'merge request rules' do
let(:policy) { :modify_approvers_list } let(:setting_name) { :disable_overriding_approvers_per_merge_request }
let(:project) { create(:project, namespace: owner.namespace) } let(:policy) { :modify_approvers_list }
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
end end
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