Commit 191986b9 authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch '273765-disallow-editing-project-level-mr-settings' into 'master'

Disallow editing of project-level MR approval settings that are enabled at the instance level

See merge request gitlab-org/gitlab!46637
parents 5a68fedd 9ea96825
...@@ -5,50 +5,29 @@ info: To determine the technical writer assigned to the Stage/Group associated w ...@@ -5,50 +5,29 @@ info: To determine the technical writer assigned to the Stage/Group associated w
type: reference, concepts type: reference, concepts
--- ---
# Instance-level merge request approval rules **(PREMIUM ONLY)** # Merge request approval rules **(PREMIUM ONLY)**
> Introduced in [GitLab Premium](https://gitlab.com/gitlab-org/gitlab/-/issues/39060) 12.8. > Introduced in [GitLab Premium](https://gitlab.com/gitlab-org/gitlab/-/issues/39060) 12.8.
Merge request approvals rules prevent users overriding certain settings on a project Merge request approval rules prevent users from overriding certain settings on the project
level. When configured, only administrators can change these settings on a project level level. When enabled at the instance level, these settings are no longer editable on the
if they are enabled at an instance level. project level.
To enable merge request approval rules for an instance: To enable merge request approval rules for an instance:
1. Navigate to **Admin Area >** **{push-rules}** **Push Rules** and expand **Merge 1. Navigate to **Admin Area >** **{push-rules}** **Push Rules** and expand **Merge
requests approvals**. requests approvals**.
1. Set the required rule. 1. Set the required rule.
1. Click **Save changes**. 1. Click **Save changes**.
GitLab administrators can later override these settings in a project’s settings.
## Available rules ## Available rules
Merge request approval rules that can be set at an instance level are: Merge request approval rules that can be set at an instance level are:
- **Prevent approval of merge requests by merge request author**. Prevents project - **Prevent approval of merge requests by merge request author**. Prevents project
maintainers from allowing request authors to merge their own merge requests. maintainers from allowing request authors to merge their own merge requests.
- **Prevent approval of merge requests by merge request committers**. Prevents project - **Prevent approval of merge requests by merge request committers**. Prevents project
maintainers from allowing users to approve merge requests if they have submitted maintainers from allowing users to approve merge requests if they have submitted
any commits to the source branch. any commits to the source branch.
- **Can override approvers and approvals required per merge request**. Allows project - **Prevent users from modifying merge request approvers list**. Prevents users from
maintainers to modify the approvers list in individual merge requests. modifying the approvers list in project settings or in individual merge requests.
## Scope rules to compliance-labeled projects
> Introduced in [GitLab Premium](https://gitlab.com/groups/gitlab-org/-/epics/3432) 13.2.
Merge request approval rules can be further scoped to specific compliance frameworks.
When the compliance framework label is selected and the project is assigned the compliance
label, the instance-level MR approval settings will take effect and the
[project-level settings](../project/merge_requests/merge_request_approvals.md#adding--editing-a-default-approval-rule)
is locked for modification.
When the compliance framework label is not selected or the project is not assigned the
compliance label, the project-level MR approval settings will take effect and the users with
Maintainer role and above can modify these.
| Instance-level | Project-level |
| -------------- | ------------- |
| ![Scope MR approval settings to compliance frameworks](img/scope_mr_approval_settings_v13_5.png) | ![MR approval settings on compliance projects](img/mr_approval_settings_compliance_project_v13_5.png) |
...@@ -6,8 +6,6 @@ module EE ...@@ -6,8 +6,6 @@ module EE
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
extend ActiveSupport::Concern extend ActiveSupport::Concern
include ::Admin::MergeRequestApprovalSettingsHelper
prepended do prepended do
before_action :elasticsearch_reindexing_task, only: [:general] before_action :elasticsearch_reindexing_task, only: [:general]
...@@ -54,10 +52,6 @@ module EE ...@@ -54,10 +52,6 @@ module EE
attrs += EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes attrs += EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes
end end
if show_compliance_merge_request_approval_settings?
attrs << { compliance_frameworks: [] }
end
if ::Gitlab::Geo.license_allows? && ::Feature.enabled?(:maintenance_mode) if ::Gitlab::Geo.license_allows? && ::Feature.enabled?(:maintenance_mode)
attrs << :maintenance_mode attrs << :maintenance_mode
attrs << :maintenance_mode_message attrs << :maintenance_mode_message
......
...@@ -129,7 +129,7 @@ module EE ...@@ -129,7 +129,7 @@ module EE
attrs << :merge_requests_disable_committers_approval attrs << :merge_requests_disable_committers_approval
end end
if can?(current_user, :modify_overriding_approvers_per_merge_request_setting, project) if can?(current_user, :modify_approvers_rules, project)
attrs << :disable_overriding_approvers_per_merge_request attrs << :disable_overriding_approvers_per_merge_request
end end
......
# frozen_string_literal: true
module Admin
module MergeRequestApprovalSettingsHelper
def show_compliance_merge_request_approval_settings?
License.feature_available?(:admin_merge_request_approvers_rules)
end
end
end
...@@ -66,12 +66,6 @@ module ComplianceManagement ...@@ -66,12 +66,6 @@ module ComplianceManagement
validates :color, color: true, allow_blank: false, length: { maximum: 10 } validates :color, color: true, allow_blank: false, length: { maximum: 10 }
validates :namespace_id, uniqueness: { scope: :name } validates :namespace_id, uniqueness: { scope: :name }
def merge_request_approval_rules_enforced?
return false unless default_framework_definition
::Gitlab::CurrentSettings.current_application_settings.compliance_frameworks.include?(default_framework_definition.id)
end
def default_framework_definition def default_framework_definition
strong_memoize(:default_framework_definition) do strong_memoize(:default_framework_definition) do
DEFAULT_FRAMEWORKS.find { |framework| framework.name.eql?(name) } DEFAULT_FRAMEWORKS.find { |framework| framework.name.eql?(name) }
......
...@@ -256,12 +256,6 @@ module EE ...@@ -256,12 +256,6 @@ module EE
end end
end end
def has_regulated_settings?
strong_memoize(:has_regulated_settings) do
compliance_framework_setting&.compliance_management_framework&.merge_request_approval_rules_enforced?
end
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
...@@ -685,9 +679,8 @@ module EE ...@@ -685,9 +679,8 @@ module EE
def disable_overriding_approvers_per_merge_request def disable_overriding_approvers_per_merge_request
strong_memoize(:disable_overriding_approvers_per_merge_request) do strong_memoize(:disable_overriding_approvers_per_merge_request) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules) next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next super unless has_regulated_settings?
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
end end
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
...@@ -695,9 +688,9 @@ module EE ...@@ -695,9 +688,9 @@ module EE
def merge_requests_author_approval def merge_requests_author_approval
strong_memoize(:merge_requests_author_approval) do strong_memoize(:merge_requests_author_approval) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules) next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next super unless has_regulated_settings? next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
!::Gitlab::CurrentSettings.prevent_merge_requests_author_approval? super
end end
end end
alias_method :merge_requests_author_approval?, :merge_requests_author_approval alias_method :merge_requests_author_approval?, :merge_requests_author_approval
...@@ -705,9 +698,8 @@ module EE ...@@ -705,9 +698,8 @@ module EE
def merge_requests_disable_committers_approval def merge_requests_disable_committers_approval
strong_memoize(:merge_requests_disable_committers_approval) do strong_memoize(:merge_requests_disable_committers_approval) do
next super unless License.feature_available?(:admin_merge_request_approvers_rules) next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next super unless has_regulated_settings?
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
end end
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
......
...@@ -33,10 +33,22 @@ module EE ...@@ -33,10 +33,22 @@ module EE
!PushRule.global&.commit_committer_check !PushRule.global&.commit_committer_check
end end
with_scope :subject with_scope :global
condition(:regulated_merge_request_approval_settings) do condition(:locked_approvers_rules) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request
end
with_scope :global
condition(:locked_merge_request_author_setting) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
::Gitlab::CurrentSettings.prevent_merge_requests_author_approval
end
with_scope :global
condition(:locked_merge_request_committer_setting) do
License.feature_available?(:admin_merge_request_approvers_rules) && License.feature_available?(:admin_merge_request_approvers_rules) &&
@subject.has_regulated_settings? ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval
end end
condition(:project_merge_request_analytics_available) do condition(:project_merge_request_analytics_available) do
...@@ -226,7 +238,6 @@ module EE ...@@ -226,7 +238,6 @@ module EE
enable :admin_path_locks enable :admin_path_locks
enable :update_approvers enable :update_approvers
enable :modify_approvers_rules enable :modify_approvers_rules
enable :modify_overriding_approvers_per_merge_request_setting
enable :modify_auto_fix_setting enable :modify_auto_fix_setting
enable :modify_merge_request_author_setting enable :modify_merge_request_author_setting
enable :modify_merge_request_committer_setting enable :modify_merge_request_committer_setting
...@@ -305,9 +316,15 @@ module EE ...@@ -305,9 +316,15 @@ module EE
prevent :read_project prevent :read_project
end end
rule { regulated_merge_request_approval_settings }.policy do rule { locked_approvers_rules }.policy do
prevent :modify_overriding_approvers_per_merge_request_setting prevent :modify_approvers_rules
end
rule { locked_merge_request_author_setting }.policy do
prevent :modify_merge_request_author_setting prevent :modify_merge_request_author_setting
end
rule { locked_merge_request_committer_setting }.policy do
prevent :modify_merge_request_committer_setting prevent :modify_merge_request_committer_setting
end end
......
...@@ -6,10 +6,7 @@ ...@@ -6,10 +6,7 @@
%button.btn.js-settings-toggle{ type: 'button' } %button.btn.js-settings-toggle{ type: 'button' }
= expanded_by_default? ? _('Collapse') : _('Expand') = expanded_by_default? ? _('Collapse') : _('Expand')
%p %p
- if show_compliance_merge_request_approval_settings? = _('Settings to prevent self-approval across all projects in the instance. Only an administrator can modify these settings.')
= _('Regulate approvals by authors/committers, based on compliance frameworks. Can be changed only at the instance level.')
- else
= _('Settings to prevent self-approval across all projects in the instance. Only an administrator can modify these settings.')
.settings-content .settings-content
%hr.clearfix.mt-0 %hr.clearfix.mt-0
...@@ -19,17 +16,4 @@ ...@@ -19,17 +16,4 @@
= render 'merge_request_approvals_fields', f: f = render 'merge_request_approvals_fields', f: f
- if show_compliance_merge_request_approval_settings?
.sub-section
%h5
= _('Compliance frameworks')
.form-group
.form-text.text-muted.mb-2
= _('The above settings apply to all projects with the selected compliance framework(s).')
= f.collection_check_boxes(:compliance_frameworks, compliance_framework_checkboxes, :first, :last) do |b|
.form-check
= b.check_box(class: "form-check-input")
= b.label(class: "form-check-label")
= f.submit _('Save changes'), class: "gl-button btn btn-success" = f.submit _('Save changes'), class: "gl-button btn btn-success"
...@@ -9,6 +9,6 @@ ...@@ -9,6 +9,6 @@
= f.label :prevent_merge_requests_committers_approval, class: 'form-check-label' do = f.label :prevent_merge_requests_committers_approval, class: 'form-check-label' do
= _('Prevent approval of merge requests by merge request committers') = _('Prevent approval of merge requests by merge request committers')
.form-check .form-check
= f.check_box :disable_overriding_approvers_per_merge_request , { class: 'form-check-input' }, false, true = f.check_box :disable_overriding_approvers_per_merge_request , class: 'form-check-input'
= f.label :disable_overriding_approvers_per_merge_request , class: 'form-check-label' do = f.label :disable_overriding_approvers_per_merge_request , class: 'form-check-label' do
= _('Can override approvers and approvals required per merge request') = _('Prevent users from modifying merge request approvers list')
- can_modify_overriding_approvers_per_merge_request_settings = can?(current_user, :modify_overriding_approvers_per_merge_request_setting, @project)
- can_modify_merge_request_author_settings = can?(current_user, :modify_merge_request_author_setting, @project) - can_modify_merge_request_author_settings = can?(current_user, :modify_merge_request_author_setting, @project)
- can_modify_merge_request_committer_settings = can?(current_user, :modify_merge_request_committer_setting, @project) - can_modify_merge_request_committer_settings = can?(current_user, :modify_merge_request_committer_setting, @project)
...@@ -22,7 +21,7 @@ ...@@ -22,7 +21,7 @@
.form-group .form-group
.form-check .form-check
= form.check_box :disable_overriding_approvers_per_merge_request, { class: 'form-check-input', disabled: !can_modify_overriding_approvers_per_merge_request_settings }, false, true = form.check_box(:disable_overriding_approvers_per_merge_request, { class: 'form-check-input', disabled: !can_modify_approvers }, false, true)
= form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do = form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
%span= _('Can override approvers and approvals required per merge request') %span= _('Can override approvers and approvals required per merge request')
= link_to sprite_icon('question-o'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'prevent-overriding-default-approvals'), target: '_blank' = link_to sprite_icon('question-o'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'prevent-overriding-default-approvals'), target: '_blank'
......
---
title: Disallow editing of project-level MR approval settings when enabled at the instance level
merge_request: 46637
author:
type: changed
...@@ -15,7 +15,7 @@ module API ...@@ -15,7 +15,7 @@ module API
def filter_params(params) def filter_params(params)
params params
.then { |params| filter_forbidden_param(params, :modify_merge_request_committer_setting, :merge_requests_disable_committers_approval) } .then { |params| filter_forbidden_param(params, :modify_merge_request_committer_setting, :merge_requests_disable_committers_approval) }
.then { |params| filter_forbidden_param(params, :modify_overriding_approvers_per_merge_request_setting, :disable_overriding_approvers_per_merge_request) } .then { |params| filter_forbidden_param(params, :modify_approvers_rules, :disable_overriding_approvers_per_merge_request) }
.then { |params| filter_forbidden_param(params, :modify_merge_request_author_setting, :merge_requests_author_approval) } .then { |params| filter_forbidden_param(params, :modify_merge_request_author_setting, :merge_requests_author_approval) }
end end
end end
......
...@@ -199,13 +199,6 @@ RSpec.describe Admin::ApplicationSettingsController do ...@@ -199,13 +199,6 @@ RSpec.describe Admin::ApplicationSettingsController do
it_behaves_like 'settings for licensed features' it_behaves_like 'settings for licensed features'
end end
context 'compliance frameworks' do
let(:settings) { { compliance_frameworks: [1, 2, 3, 4, 5] } }
let(:feature) { :admin_merge_request_approvers_rules }
it_behaves_like 'settings for licensed features'
end
context 'required instance ci template' do context 'required instance ci template' do
let(:settings) { { required_instance_ci_template: 'Auto-DevOps' } } let(:settings) { { required_instance_ci_template: 'Auto-DevOps' } }
let(:feature) { :required_ci_templates } let(:feature) { :required_ci_templates }
......
...@@ -424,21 +424,17 @@ RSpec.describe ProjectsController do ...@@ -424,21 +424,17 @@ RSpec.describe ProjectsController do
shared_examples 'merge request approvers rules' do shared_examples 'merge request approvers rules' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
where(:license_value, :setting_value, :param_value, :final_value) do where(:can_modify, :param_value, :final_value) do
false | false | false | false true | true | true
false | true | false | false true | false | false
false | false | true | true false | true | nil
false | true | true | true false | false | nil
true | false | false | false
true | true | false | false
true | false | true | true
true | true | true | true
end end
with_them do with_them do
before do before do
stub_licensed_features(admin_merge_request_approvers_rules: license_value) allow(controller).to receive(:can?).and_call_original
stub_application_setting(app_setting => setting_value) allow(controller).to receive(:can?).with(user, rule_name, project).and_return(can_modify)
end end
it 'updates project if needed' do it 'updates project if needed' do
...@@ -458,21 +454,21 @@ RSpec.describe ProjectsController do ...@@ -458,21 +454,21 @@ RSpec.describe ProjectsController do
describe ':disable_overriding_approvers_per_merge_request' do describe ':disable_overriding_approvers_per_merge_request' do
it_behaves_like 'merge request approvers rules' do it_behaves_like 'merge request approvers rules' do
let(:app_setting) { :disable_overriding_approvers_per_merge_request } let(:rule_name) { :modify_approvers_rules }
let(:setting) { :disable_overriding_approvers_per_merge_request } let(:setting) { :disable_overriding_approvers_per_merge_request }
end end
end end
describe ':merge_requests_author_approval' do describe ':merge_requests_author_approval' do
it_behaves_like 'merge request approvers rules' do it_behaves_like 'merge request approvers rules' do
let(:app_setting) { :prevent_merge_requests_author_approval } let(:rule_name) { :modify_merge_request_author_setting }
let(:setting) { :merge_requests_author_approval } let(:setting) { :merge_requests_author_approval }
end end
end end
describe ':merge_requests_disable_committers_approval' do describe ':merge_requests_disable_committers_approval' do
it_behaves_like 'merge request approvers rules' do it_behaves_like 'merge request approvers rules' do
let(:app_setting) { :prevent_merge_requests_committers_approval } let(:rule_name) { :modify_merge_request_committer_setting }
let(:setting) { :merge_requests_disable_committers_approval } let(:setting) { :merge_requests_disable_committers_approval }
end end
end end
......
...@@ -5,9 +5,9 @@ require 'spec_helper' ...@@ -5,9 +5,9 @@ require 'spec_helper'
RSpec.describe 'Admin interacts with merge requests approvals settings' do RSpec.describe 'Admin interacts with merge requests approvals settings' do
include StubENV include StubENV
let_it_be(:hippa) { ComplianceManagement::Framework::DEFAULT_FRAMEWORKS_BY_IDENTIFIER[:hipaa] } let_it_be(:application_settings) { create(:application_setting) }
let_it_be(:application_settings) { create(:application_setting, compliance_frameworks: [hippa.id]) }
let_it_be(:user) { create(:admin) } let_it_be(:user) { create(:admin) }
let_it_be(:project) { create(:project, creator: user) }
before do before do
stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
...@@ -17,28 +17,27 @@ RSpec.describe 'Admin interacts with merge requests approvals settings' do ...@@ -17,28 +17,27 @@ RSpec.describe 'Admin interacts with merge requests approvals settings' do
visit(admin_push_rule_path) visit(admin_push_rule_path)
end end
it 'updates compliance frameworks' do it 'updates instance-level merge request approval settings and enforces project-level ones' do
page.within('.merge-request-approval-settings') do page.within('.merge-request-approval-settings') do
check 'SOC 2' check 'Prevent approval of merge requests by merge request author'
check 'Prevent approval of merge requests by merge request committers'
check 'Prevent users from modifying merge request approvers list'
click_button('Save changes') click_button('Save changes')
end end
visit(admin_push_rule_path) visit(admin_push_rule_path)
expect(page.find_field('SOC 2')).to be_checked expect(find_field('Prevent approval of merge requests by merge request author')).to be_checked
end expect(find_field('Prevent approval of merge requests by merge request committers')).to be_checked
expect(find_field('Prevent users from modifying merge request approvers list')).to be_checked
it 'unsets all compliance frameworks' do
checkbox_selector = 'input[name="application_setting[compliance_frameworks][]"]'
page.within('.merge-request-approval-settings') do visit edit_project_path(project)
page.all(checkbox_selector).each { |checkbox| checkbox.set(false) }
click_button('Save changes') page.within('#js-merge-request-approval-settings') do
expect(find('#project_merge_requests_author_approval')).to be_disabled.and be_checked
expect(find('#project_merge_requests_disable_committers_approval')).to be_disabled.and be_checked
expect(find('#project_disable_overriding_approvers_per_merge_request')).to be_disabled
expect(find('#project_disable_overriding_approvers_per_merge_request')).not_to be_checked
end end
visit(admin_push_rule_path)
expect(page.all(checkbox_selector).map(&:checked?)).to all(be_falsey)
end end
end end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Admin::MergeRequestApprovalSettingsHelper do
describe '#show_compliance_merge_request_approval_settings?' do
using RSpec::Parameterized::TableSyntax
subject { helper.show_compliance_merge_request_approval_settings? }
where(:licensed, :result) do
true | true
false | false
end
with_them do
before do
stub_licensed_features(admin_merge_request_approvers_rules: licensed)
end
it { is_expected.to eq(result) }
end
end
end
...@@ -564,14 +564,14 @@ RSpec.describe Project do ...@@ -564,14 +564,14 @@ 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
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do where(:feature_enabled, :app_setting, :project_setting, :final_setting) do
true | true | true | true true | true | true | true
false | true | true | false
true | false | true | true true | false | true | true
false | false | true | false
true | true | false | true true | true | false | true
false | true | false | true
true | false | false | false true | false | false | false
false | true | true | true
false | false | true | true
false | true | false | false
false | false | false | false false | false | false | false
end end
...@@ -579,9 +579,8 @@ RSpec.describe Project do ...@@ -579,9 +579,8 @@ RSpec.describe Project do
let(:project) { create(:project) } let(:project) { create(:project) }
before do before do
stub_licensed_features(admin_merge_request_approvers_rules: true) stub_licensed_features(admin_merge_request_approvers_rules: feature_enabled)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting) stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting) project.update(setting => project_setting)
end end
...@@ -595,7 +594,6 @@ RSpec.describe Project do ...@@ -595,7 +594,6 @@ RSpec.describe Project do
describe '#disable_overriding_approvers_per_merge_request' do describe '#disable_overriding_approvers_per_merge_request' do
it_behaves_like 'setting modified by application setting' do it_behaves_like 'setting modified by application setting' do
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :disable_overriding_approvers_per_merge_request } let(:setting) { :disable_overriding_approvers_per_merge_request }
let(:application_setting) { :disable_overriding_approvers_per_merge_request } let(:application_setting) { :disable_overriding_approvers_per_merge_request }
end end
...@@ -603,26 +601,23 @@ RSpec.describe Project do ...@@ -603,26 +601,23 @@ RSpec.describe Project do
describe '#merge_requests_disable_committers_approval' do describe '#merge_requests_disable_committers_approval' do
it_behaves_like 'setting modified by application setting' do it_behaves_like 'setting modified by application setting' do
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :merge_requests_disable_committers_approval } let(:setting) { :merge_requests_disable_committers_approval }
let(:application_setting) { :prevent_merge_requests_committers_approval } let(:application_setting) { :prevent_merge_requests_committers_approval }
end end
end end
describe '#merge_requests_author_approval' do describe '#merge_requests_author_approval' do
let(:project) { create(:project) }
let(:feature) { :admin_merge_request_approvers_rules }
let(:setting) { :merge_requests_author_approval } let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval } let(:application_setting) { :prevent_merge_requests_author_approval }
where(:app_setting, :project_setting, :regulated_settings, :final_setting) do where(:feature_enabled, :app_setting, :project_setting, :final_setting) do
true | true | true | false true | true | true | false
true | false | true | true
true | true | false | false
true | false | false | false
false | true | true | true false | true | true | true
true | false | true | false
false | false | true | true false | false | true | true
true | true | false | true false | true | false | false
false | true | false | true
true | false | false | false
false | false | false | false false | false | false | false
end end
...@@ -630,9 +625,8 @@ RSpec.describe Project do ...@@ -630,9 +625,8 @@ RSpec.describe Project do
let(:project) { create(:project) } let(:project) { create(:project) }
before do before do
stub_licensed_features(admin_merge_request_approvers_rules: true) stub_licensed_features(admin_merge_request_approvers_rules: feature_enabled)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_settings)
stub_application_setting(application_setting => app_setting) stub_application_setting(application_setting => app_setting)
project.update(setting => project_setting) project.update(setting => project_setting)
end end
...@@ -645,36 +639,6 @@ RSpec.describe Project do ...@@ -645,36 +639,6 @@ RSpec.describe Project do
end end
end end
describe '#has_regulated_settings?' do
let(:gdpr_framework_definition) { ComplianceManagement::Framework::DEFAULT_FRAMEWORKS_BY_IDENTIFIER[:gdpr] }
let(:compliance_framework_setting) { build(:compliance_framework_project_setting, :gdpr) }
let(:project) { build(:project, compliance_framework_setting: compliance_framework_setting) }
subject { project.has_regulated_settings? }
context 'framework is regulated' do
before do
stub_application_setting(compliance_frameworks: [gdpr_framework_definition.id])
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(:project) { build(: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) }
......
...@@ -1195,13 +1195,13 @@ RSpec.describe ProjectPolicy do ...@@ -1195,13 +1195,13 @@ RSpec.describe ProjectPolicy do
end end
end end
shared_examples 'regulated merge request approval settings' do shared_examples 'merge request approval settings' do
let(:project) { private_project } let(:project) { private_project }
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
context 'with merge request approvers rules available in license' do context 'with merge request approvers rules available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false :guest | true | nil | false
:reporter | true | nil | false :reporter | true | nil | false
:developer | true | nil | false :developer | true | nil | false
...@@ -1220,7 +1220,7 @@ RSpec.describe ProjectPolicy do ...@@ -1220,7 +1220,7 @@ RSpec.describe ProjectPolicy do
before do before do
stub_licensed_features(admin_merge_request_approvers_rules: true) stub_licensed_features(admin_merge_request_approvers_rules: true)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting) stub_application_setting(app_setting => setting)
enable_admin_mode!(current_user) if admin_mode enable_admin_mode!(current_user) if admin_mode
end end
...@@ -1229,7 +1229,7 @@ RSpec.describe ProjectPolicy do ...@@ -1229,7 +1229,7 @@ RSpec.describe ProjectPolicy do
end end
context 'with merge request approvers rules not available in license' do context 'with merge request approvers rules not available in license' do
where(:role, :regulated_setting, :admin_mode, :allowed) do where(:role, :setting, :admin_mode, :allowed) do
:guest | true | nil | false :guest | true | nil | false
:reporter | true | nil | false :reporter | true | nil | false
:developer | true | nil | false :developer | true | nil | false
...@@ -1248,7 +1248,7 @@ RSpec.describe ProjectPolicy do ...@@ -1248,7 +1248,7 @@ RSpec.describe ProjectPolicy do
before do before do
stub_licensed_features(admin_merge_request_approvers_rules: false) stub_licensed_features(admin_merge_request_approvers_rules: false)
allow(project).to receive(:has_regulated_settings?).and_return(regulated_setting) stub_application_setting(app_setting => setting)
enable_admin_mode!(current_user) if admin_mode enable_admin_mode!(current_user) if admin_mode
end end
...@@ -1258,45 +1258,22 @@ RSpec.describe ProjectPolicy do ...@@ -1258,45 +1258,22 @@ RSpec.describe ProjectPolicy do
end end
describe ':modify_approvers_rules' do describe ':modify_approvers_rules' do
let(:policy) { :modify_approvers_rules } it_behaves_like 'merge request approval settings' do
let(:app_setting) { :disable_overriding_approvers_per_merge_request }
using RSpec::Parameterized::TableSyntax let(:policy) { :modify_approvers_rules }
where(:role, :admin_mode, :allowed) do
:guest | nil | false
:reporter | nil | false
:developer | nil | false
:maintainer | nil | true
:owner | nil | true
:admin | false | false
:admin | true | true
end
with_them do
let(:current_user) { public_send(role) }
before do
enable_admin_mode!(current_user) if admin_mode
end
it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
end
end
describe ':modify_overriding_approvers_per_merge_request_setting' do
it_behaves_like 'regulated merge request approval settings' do
let(:policy) { :modify_overriding_approvers_per_merge_request_setting }
end end
end end
describe ':modify_merge_request_author_setting' do describe ':modify_merge_request_author_setting' do
it_behaves_like 'regulated merge request approval settings' do it_behaves_like 'merge request approval settings' do
let(:app_setting) { :prevent_merge_requests_author_approval }
let(:policy) { :modify_merge_request_author_setting } let(:policy) { :modify_merge_request_author_setting }
end end
end end
describe ':modify_merge_request_committer_setting' do describe ':modify_merge_request_committer_setting' do
it_behaves_like 'regulated merge request approval settings' do it_behaves_like 'merge request approval settings' do
let(:app_setting) { :prevent_merge_requests_committers_approval }
let(:policy) { :modify_merge_request_committer_setting } let(:policy) { :modify_merge_request_committer_setting }
end end
end end
......
...@@ -144,7 +144,7 @@ RSpec.describe API::ProjectApprovals do ...@@ -144,7 +144,7 @@ RSpec.describe API::ProjectApprovals do
context 'updates merge requests settings' do context 'updates merge requests settings' do
it_behaves_like 'updates merge requests settings when possible' do it_behaves_like 'updates merge requests settings when possible' do
let(:current_user) { admin } let(:current_user) { admin }
let(:permission) { :modify_overriding_approvers_per_merge_request_setting } let(:permission) { :modify_approvers_rules }
let(:setting) { :disable_overriding_approvers_per_merge_request } let(:setting) { :disable_overriding_approvers_per_merge_request }
end end
......
...@@ -11,35 +11,10 @@ RSpec.describe 'admin/push_rules/_merge_request_approvals' do ...@@ -11,35 +11,10 @@ RSpec.describe 'admin/push_rules/_merge_request_approvals' do
stub_licensed_features(admin_merge_request_approvers_rules: true) stub_licensed_features(admin_merge_request_approvers_rules: true)
end end
it 'shows settings form' do it 'shows settings form', :aggregate_failures do
render render
expect(rendered).to have_content('Merge requests approvals') expect(rendered).to have_content('Merge requests approvals')
end expect(rendered).to have_content('Settings to prevent self-approval across all projects')
context 'when show compliance merge request approval settings' do
before do
allow(view).to receive(:show_compliance_merge_request_approval_settings?).and_return(true)
end
it 'shows compliance framework content', :aggregate_failures do
render
expect(rendered).to have_content('Regulate approvals by authors/committers')
expect(rendered).to have_content('Compliance frameworks')
end
end
context 'when not show compliance merge request approval settings' do
before do
allow(view).to receive(:show_compliance_merge_request_approval_settings?).and_return(false)
end
it 'shows non-compliance framework content', :aggregate_failures do
render
expect(rendered).to have_content('Settings to prevent self-approval across all projects')
expect(rendered).not_to have_content('Compliance frameworks')
end
end end
end end
...@@ -6958,9 +6958,6 @@ msgstr "" ...@@ -6958,9 +6958,6 @@ msgstr ""
msgid "Compliance framework (optional)" msgid "Compliance framework (optional)"
msgstr "" msgstr ""
msgid "Compliance frameworks"
msgstr ""
msgid "ComplianceDashboard|created by:" msgid "ComplianceDashboard|created by:"
msgstr "" msgstr ""
...@@ -20342,6 +20339,9 @@ msgstr "" ...@@ -20342,6 +20339,9 @@ msgstr ""
msgid "Prevent users from changing their profile name" msgid "Prevent users from changing their profile name"
msgstr "" msgstr ""
msgid "Prevent users from modifying merge request approvers list"
msgstr ""
msgid "Prevent users from performing write operations on GitLab while performing maintenance." msgid "Prevent users from performing write operations on GitLab while performing maintenance."
msgstr "" msgstr ""
...@@ -22255,9 +22255,6 @@ msgstr "" ...@@ -22255,9 +22255,6 @@ msgstr ""
msgid "Registry setup" msgid "Registry setup"
msgstr "" msgstr ""
msgid "Regulate approvals by authors/committers, based on compliance frameworks. Can be changed only at the instance level."
msgstr ""
msgid "Reindexing status" msgid "Reindexing status"
msgstr "" msgstr ""
...@@ -26662,9 +26659,6 @@ msgstr "" ...@@ -26662,9 +26659,6 @@ msgstr ""
msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS." msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS."
msgstr "" msgstr ""
msgid "The above settings apply to all projects with the selected compliance framework(s)."
msgstr ""
msgid "The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential." msgid "The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential."
msgstr "" msgstr ""
......
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