Commit fe432157 authored by Max Woolf's avatar Max Woolf

Enforce cascading MR approval settings

Behind a feature flag, adds a check with the compliance
management resolver for instance -> group -> project
MR approval setting enforcement.
parent c214bc54
......@@ -2678,10 +2678,6 @@ class Project < ApplicationRecord
ProjectStatistics.increment_statistic(self, statistic, delta)
end
def merge_requests_author_approval
!!read_attribute(:merge_requests_author_approval)
end
def ci_forward_deployment_enabled?
return false unless ci_cd_settings
......
......@@ -5,7 +5,7 @@ info: "To determine the technical writer assigned to the Stage/Group associated
type: reference, concepts
---
# Merge request approval settings **(FREE)**
# Merge request approval settings **(PREMIUM)**
You can configure the settings for [merge request approvals](index.md) to
ensure the approval rules meet your use case. You can also configure
......@@ -30,7 +30,7 @@ In this section of general settings, you can configure the following settings:
| [Require user password to approve](#require-user-password-to-approve) | Force potential approvers to first authenticate with a password. |
| [Remove all approvals when commits are added to the source branch](#remove-all-approvals-when-commits-are-added-to-the-source-branch) | When enabled, remove all existing approvals on a merge request when more changes are added to it. |
## Prevent approval by author **(PREMIUM)**
## Prevent approval by author
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/3349) in GitLab 11.3.
> - Moved to GitLab Premium in 13.9.
......@@ -52,7 +52,7 @@ this setting, unless you configure one of these options:
at the instance level, you can't edit this setting at the project or individual
merge request levels.
## Prevent approvals by users who add commits **(PREMIUM)**
## Prevent approvals by users who add commits
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/10441) in GitLab 11.10.
> - Moved to GitLab Premium in 13.9.
......@@ -126,13 +126,25 @@ merge request could introduce a vulnerability.
To learn more, see [Security approvals in merge requests](../../../application_security/index.md#security-approvals-in-merge-requests).
## Code coverage check approvals **(PREMIUM)**
## Code coverage check approvals
You can require specific approvals if a merge request would result in a decline in code test
coverage.
To learn more, see [Coverage check approval rule](../../../../ci/pipelines/settings.md#coverage-check-approval-rule).
## Merge request approval settings cascading
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.4. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md), disabled by default.
FLAG:
On self-managed GitLab, by default this feature is not available. To make it available per group, ask an administrator to [enable the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md). On GitLab.com, this feature is not available.
You should not use this feature for production environments
You can now enforce merge request approval settings at an instance level which will apply to all groups on an instance and, by extension, all projects. It is also possible to enforce merge request approval settings on an individual root group which will apply to all subgroups and projects.
If the settings are inherited by a group or project, they cannot be overridden by the group or project that inherited them.
## Related links
- [Instance-level merge request approval settings](../../../admin_area/merge_requests_approvals.md)
......
......@@ -534,7 +534,13 @@ module EE
end
def reset_approvals_on_push
super && feature_available?(:merge_request_approvers)
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self.group&.root_ancestor)
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
.retain_approvals_on_push
.value && feature_available?(:merge_request_approvers)
else
super && feature_available?(:merge_request_approvers)
end
end
alias_method :reset_approvals_on_push?, :reset_approvals_on_push
......@@ -562,7 +568,13 @@ module EE
end
def require_password_to_approve
super && password_authentication_enabled_for_web?
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self&.group&.root_ancestor)
ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
.require_password_to_approve
.value && password_authentication_enabled_for_web?
else
super && password_authentication_enabled_for_web?
end
end
def require_password_to_approve?
......@@ -719,9 +731,17 @@ module EE
def disable_overriding_approvers_per_merge_request
strong_memoize(:disable_overriding_approvers_per_merge_request) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
super unless feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_overrides_to_approver_list_per_merge_request
.value
else
next super unless feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
end
end
end
......@@ -731,10 +751,18 @@ module EE
def merge_requests_author_approval
strong_memoize(:merge_requests_author_approval) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
super unless feature_available?(:admin_merge_request_approvers_rules)
super
ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_author_approval
.value
else
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
!!read_attribute(:merge_requests_author_approval)
end
end
end
......@@ -744,9 +772,17 @@ module EE
def merge_requests_disable_committers_approval
strong_memoize(:merge_requests_disable_committers_approval) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
super unless feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_committer_approval
.value
else
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
end
end
end
......
......@@ -94,7 +94,6 @@ class License < ApplicationRecord
group_forking_protection
group_ip_restriction
group_merge_request_analytics
group_merge_request_approval_settings
group_milestone_project_releases
group_project_templates
group_repository_analytics
......
......@@ -122,7 +122,7 @@ module EE
end
condition(:group_merge_request_approval_settings_enabled) do
@subject.feature_available?(:group_merge_request_approval_settings) && @subject.root?
@subject.feature_available?(:merge_request_approvers) && @subject.root?
end
condition(:over_storage_limit, scope: :subject) { @subject.over_storage_limit? }
......
......@@ -33,7 +33,7 @@ module EE
end
condition(:group_merge_request_approval_settings_enabled) do
@subject.feature_available?(:group_merge_request_approval_settings)
@subject.feature_available?(:merge_request_approvers)
end
with_scope :global
......
......@@ -20,7 +20,7 @@ module ComplianceManagement
def allow_author_approval
instance_value = !instance_settings.prevent_merge_requests_author_approval
group_value = group_settings&.allow_author_approval
project_value = @project&.merge_requests_author_approval
project_value = @project && @project.read_attribute(:merge_requests_author_approval)
setting(instance_value, group_value, project_value)
end
......@@ -28,7 +28,7 @@ module ComplianceManagement
def allow_committer_approval
instance_value = !instance_settings.prevent_merge_requests_committers_approval
group_value = group_settings&.allow_committer_approval
project_value = @project ? !@project.merge_requests_disable_committers_approval : nil
project_value = @project && !@project.read_attribute(:merge_requests_disable_committers_approval)
setting(instance_value, group_value, project_value)
end
......@@ -36,21 +36,21 @@ module ComplianceManagement
def allow_overrides_to_approver_list_per_merge_request
instance_value = !instance_settings.disable_overriding_approvers_per_merge_request
group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request
project_value = @project ? !@project.disable_overriding_approvers_per_merge_request : nil
project_value = @project && !@project.read_attribute(:disable_overriding_approvers_per_merge_request)
setting(instance_value, group_value, project_value)
end
def retain_approvals_on_push
group_value = group_settings&.retain_approvals_on_push
project_value = @project ? !@project.reset_approvals_on_push : nil
project_value = @project && !@project.read_attribute(:reset_approvals_on_push)
setting(nil, group_value, project_value)
end
def require_password_to_approve
group_value = group_settings&.require_password_to_approve
project_value = @project&.require_password_to_approve
project_value = @project && @project.read_attribute(:require_password_to_approve)
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
value: require_password_value(group_value, project_value),
......
......@@ -322,7 +322,7 @@ RSpec.describe 'Edit group settings' do
context 'when feature flag is enabled and group is licensed' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(group_merge_request_approval_settings: true)
stub_licensed_features(merge_request_approvers: true)
end
it 'allows to save settings' do
......@@ -355,7 +355,7 @@ RSpec.describe 'Edit group settings' do
context 'when feature flag is disabled and group is not licensed' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
stub_licensed_features(group_merge_request_approval_settings: false)
stub_licensed_features(merge_request_approvers: false)
end
it 'is not visible' do
......
......@@ -140,7 +140,7 @@ RSpec.describe 'Projects > Audit Events', :js do
describe 'changing merge request approval permission for authors and reviewers' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: feature_enabled)
stub_licensed_features(group_merge_request_approval_settings: feature_enabled)
stub_licensed_features(merge_request_approvers: true)
project.add_developer(pete)
end
......@@ -157,6 +157,9 @@ RSpec.describe 'Projects > Audit Events', :js do
page.within('.sidebar-top-level-items') do
click_link 'Security & Compliance'
wait_for_all_requests
click_link 'Audit Events'
end
......
......@@ -785,23 +785,121 @@ RSpec.describe ApprovalState do
end
describe '#authors_can_approve?' do
context 'when project allows author approval' do
context 'group_merge_request_approval_settings_feature_flag is enabled' do
before do
project.update!(merge_requests_author_approval: true)
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
context 'allow_author_approval is resolved to not be permitted' do
before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
allow(instance).to receive(:allow_author_approval).and_return(
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: false, locked: false, inherited_from: nil)
)
end
end
it 'returns true' do
expect(subject.authors_can_approve?).to eq(true)
it 'returns false' do
expect(subject.authors_can_approve?).to be false
end
end
context 'allow_author_approval is resolved to be permitted' do
before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
allow(instance).to receive(:allow_author_approval).and_return(
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: true, locked: false, inherited_from: nil)
)
end
end
it 'returns true' do
expect(subject.authors_can_approve?).to be true
end
end
end
context 'when project disallows author approval' do
context 'group_merge_request_approval_settings_feature_flag is disabled' do
before do
project.update!(merge_requests_author_approval: false)
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
context 'when project allows author approval' do
before do
project.update!(merge_requests_author_approval: true)
end
it 'returns true' do
expect(subject.authors_can_approve?).to eq(false)
it 'returns true' do
expect(subject.authors_can_approve?).to eq(true)
end
end
context 'when project disallows author approval' do
before do
project.update!(merge_requests_author_approval: false)
end
it 'returns true' do
expect(subject.authors_can_approve?).to eq(false)
end
end
end
end
describe '#committers_can_approve?' do
context 'group_merge_request_approval_settings_feature_flag is enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
context 'allow_committer_approval is resolved to not be permitted' do
before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
allow(instance).to receive(:allow_committer_approval).and_return(
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: false, locked: false, inherited_from: nil)
)
end
end
it 'returns false' do
expect(subject.committers_can_approve?).to be false
end
end
context 'allow_committer_approval is resolved to be permitted' do
before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
allow(instance).to receive(:allow_committer_approval).and_return(
ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: true, locked: false, inherited_from: nil)
)
end
end
it 'returns false' do
expect(subject.committers_can_approve?).to be true
end
end
end
context 'group_merge_request_approval_settings_feature_flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
context 'when project allows committer approval' do
before do
project.update!(merge_requests_disable_committers_approval: false)
end
it 'returns true' do
expect(subject.committers_can_approve?).to eq(true)
end
end
context 'when project disallows committer approval' do
before do
project.update!(merge_requests_disable_committers_approval: true)
end
it 'returns true' do
expect(subject.committers_can_approve?).to eq(false)
end
end
end
end
......
This diff is collapsed.
......@@ -1589,7 +1589,7 @@ RSpec.describe GroupPolicy do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(group_merge_request_approval_settings: licensed)
stub_licensed_features(merge_request_approvers: licensed)
enable_admin_mode!(current_user) if admin_mode
group.parent = build(:group) unless root_group
end
......
......@@ -1425,7 +1425,7 @@ RSpec.describe ProjectPolicy do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(group_merge_request_approval_settings: licensed)
stub_licensed_features(merge_request_approvers: licensed)
enable_admin_mode!(current_user) if role == :admin
end
......
......@@ -232,7 +232,7 @@ RSpec.describe API::MergeRequestApprovalSettings do
group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(group_merge_request_approval_settings: true)
stub_licensed_features(merge_request_approvers: true)
allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project)
.and_return(true)
......@@ -291,7 +291,7 @@ RSpec.describe API::MergeRequestApprovalSettings do
group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(group_merge_request_approval_settings: true)
stub_licensed_features(merge_request_approvers: true)
allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project)
.and_return(true)
......
......@@ -495,23 +495,6 @@ RSpec.describe Project, factory_default: :keep do
end
end
describe '#merge_requests_author_approval' do
where(:attribute_value, :return_value) do
true | true
false | false
nil | false
end
with_them do
let(:project) { create(:project, merge_requests_author_approval: attribute_value) }
it 'returns expected value' do
expect(project.merge_requests_author_approval).to eq(return_value)
expect(project.merge_requests_author_approval?).to eq(return_value)
end
end
end
describe '#all_pipelines' do
let_it_be(:project) { create(:project) }
......
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