Skip to content

G
gitlab-ce
Commit ff0a219a authored by Imre Farkas's avatar Imre Farkas
Browse files
Options

Merge branch... 

Merge branch '207250-allow-admins-to-modify-mr-approval-settings-at-the-project-level-after-enabling-them-in' into 'master'

Allow only admins to modify MR approval settings at the project-level

See merge request gitlab-org/gitlab!30358
parents 2e0b9a1d 34a55a04
Hide whitespace changes
Inline Side-by-side
Showing with 155 additions and 329 deletions
doc/user/admin_area/merge_requests_approvals.md
View file @ ff0a219a
......@@ -19,6 +19,22 @@ To enable merge request approval rules for an instance:
GitLab administrators can later override these settings in a project’s settings.
## Merge request controls **(PREMIUM)**
> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/207250) in GitLab 13.0.
Merge request approval settings, by default, are inherited by all projects in an instance.
However, organizations with regulated projects may also have unregulated projects
that should not inherit these same controls.
Project-level merge request approval rules can now be edited by administrators.
Project owners and maintainers can still view project-level merge request approval rules.
In upcoming releases, we plan to provide a more holistic experience to scope instance-level merge request settings.
For more information, review our plans to provide custom [approval settings for compliance-
labeled projects](https://gitlab.com/gitlab-org/gitlab/-/issues/213601).
## Available rules
Merge request approval rules that can be set at an instance level are:
......
ee/app/controllers/ee/admin/application_settings_controller.rb
View file @ ff0a219a
......@@ -5,6 +5,8 @@ module EE
module ApplicationSettingsController
extend ::Gitlab::Utils::Override
include ::Admin::PushRulesHelper
EE_VALID_SETTING_PANELS = %w(templates).freeze
EE_VALID_SETTING_PANELS.each do |action|
......@@ -50,7 +52,7 @@ module EE
attrs << :updating_name_disabled_for_users
end
if License.feature_available?(:admin_merge_request_approvers_rules)
if show_merge_request_approvals_settings?
attrs += EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes
end
......
ee/app/helpers/admin/push_rules_helper.rb 0 → 100644
View file @ ff0a219a
# frozen_string_literal: true
module Admin
module PushRulesHelper
def show_merge_request_approvals_settings?
Feature.enabled?(:admin_merge_request_approvals_settings) &&
License.feature_available?(:admin_merge_request_approvers_rules)
end
end
end
ee/app/models/ee/project.rb
View file @ ff0a219a
......@@ -687,31 +687,6 @@ module EE
packages.where(package_type: package_type).exists?
end
def disable_overriding_approvers_per_merge_request
return super unless License.feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ||
super
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 ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
super
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)
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ||
super
end
alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
def license_compliance
strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
end
......
ee/app/policies/ee/project_policy.rb
View file @ ff0a219a
......@@ -55,24 +55,8 @@ module EE
end
with_scope :global
condition(:owner_cannot_modify_approvers_rules) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
::Gitlab::CurrentSettings.current_application_settings
.disable_overriding_approvers_per_merge_request
end
with_scope :global
condition(:owner_cannot_modify_merge_request_author_setting) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
::Gitlab::CurrentSettings.current_application_settings
.prevent_merge_requests_author_approval
end
with_scope :global
condition(:owner_cannot_modify_merge_request_committer_setting) do
License.feature_available?(:admin_merge_request_approvers_rules) &&
::Gitlab::CurrentSettings.current_application_settings
.prevent_merge_requests_committers_approval
condition(:admin_merge_request_approvers_rules_available) do
License.feature_available?(:admin_merge_request_approvers_rules)
end
with_scope :global
......@@ -362,24 +346,15 @@ module EE
prevent :read_project
end
rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
rule { admin_merge_request_approvers_rules_available & ~admin }.policy do
prevent :modify_approvers_rules
end
rule { owner_cannot_modify_merge_request_author_setting & ~admin }.policy do
prevent :modify_approvers_list
prevent :modify_merge_request_author_setting
end
rule { owner_cannot_modify_merge_request_committer_setting & ~admin }.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 { web_ide_terminal_available & can?(:create_pipeline) & can?(:maintainer_access) }.enable :create_web_ide_terminal
rule { build_service_proxy_enabled }.enable :build_service_proxy_enabled
......
ee/app/views/admin/push_rules/_merge_request_approvals.html.haml
View file @ ff0a219a
- return unless License.feature_available?(:admin_merge_request_approvers_rules)
- return unless show_merge_request_approvals_settings?
%section.settings.merge-request-approval-settings.no-animate{ class: ('expanded' if expanded_by_default?) }
.settings-header
%h4
......
ee/changelogs/unreleased/207250-only-admins-modify-project-mr-approvals.yml 0 → 100644
View file @ ff0a219a
---
title: Allow only admins to modify MR approval settings at the project-level
merge_request: 30358
author:
type: changed
ee/lib/ee/api/entities/application_setting.rb
View file @ ff0a219a
......@@ -6,12 +6,14 @@ module EE
module ApplicationSetting
extend ActiveSupport::Concern
include ::Admin::PushRulesHelper
prepended do
expose(*EE::ApplicationSettingsHelper.repository_mirror_attributes, if: ->(_instance, _options) do
::License.feature_available?(:repository_mirrors)
end)
expose(*EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes, if: ->(_instance, _options) do
::License.feature_available?(:admin_merge_request_approvers_rules)
show_merge_request_approvals_settings?
end)
expose :email_additional_text, if: ->(_instance, _opts) { ::License.feature_available?(:email_additional_text) }
expose :file_template_project_id, if: ->(_instance, _opts) { ::License.feature_available?(:custom_file_templates) }
......
ee/lib/ee/api/settings.rb
View file @ ff0a219a
......@@ -9,6 +9,8 @@ module EE
helpers do
extend ::Gitlab::Utils::Override
include ::Admin::PushRulesHelper
override :filter_attributes_using_license
def filter_attributes_using_license(attrs)
unless ::License.feature_available?(:repository_mirrors)
......@@ -35,7 +37,7 @@ module EE
attrs = attrs.except(:updating_name_disabled_for_users)
end
unless License.feature_available?(:admin_merge_request_approvers_rules)
unless show_merge_request_approvals_settings?
attrs = attrs.except(*EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes)
end
......
ee/spec/controllers/admin/application_settings_controller_spec.rb
View file @ ff0a219a
......@@ -157,7 +157,27 @@ describe Admin::ApplicationSettingsController do
end
let(:feature) { :admin_merge_request_approvers_rules }
it_behaves_like 'settings for licensed features'
context 'when feature flag is enabled' do
before do
stub_feature_flags(admin_merge_request_approvals_settings: true)
end
it_behaves_like 'settings for licensed features'
end
context 'when feature flag is disabled' do
before do
stub_licensed_features(feature => true)
stub_feature_flags(admin_merge_request_approvals_settings: false)
end
it 'does not update settings' do
attribute_names = settings.keys.map(&:to_s)
expect { put :update, params: { application_setting: settings } }
.not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
end
end
end
context 'required instance ci template' do
......
ee/spec/controllers/projects_controller_spec.rb
View file @ ff0a219a
......@@ -324,21 +324,17 @@ describe ProjectsController do
shared_examples 'merge request approvers rules' do
using RSpec::Parameterized::TableSyntax
where(:license_value, :setting_value, :param_value, :final_value) do
false | false | false | false
false | true | false | false
false | false | true | true
false | true | true | true
true | false | false | false
true | true | false | nil
true | false | true | true
true | true | true | nil
where(:can_modify, :param_value, :final_value) do
true | true | true
true | false | false
false | true | nil
false | false | nil
end
with_them do
before do
stub_licensed_features(admin_merge_request_approvers_rules: license_value)
stub_application_setting(app_setting => setting_value)
allow(controller).to receive(:can?).and_call_original
allow(controller).to receive(:can?).with(user, rule_name, project).and_return(can_modify)
end
it 'updates project if needed' do
......@@ -357,21 +353,21 @@ describe ProjectsController do
describe ':disable_overriding_approvers_per_merge_request' 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 }
end
end
describe ':merge_requests_author_approval' 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 }
end
end
describe ':merge_requests_disable_committers_approval' 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 }
end
end
......
ee/spec/helpers/admin/push_rules_helper_spec.rb 0 → 100644
View file @ ff0a219a
# frozen_string_literal: true
require 'spec_helper'
describe Admin::PushRulesHelper do
describe '#show_merge_request_approvals_settings?' do
using RSpec::Parameterized::TableSyntax
subject { helper.show_merge_request_approvals_settings? }
where(:feature_flag, :licensed, :result) do
true | true | true
true | false | false
false | true | false
false | false | false
end
with_them do
before do
stub_feature_flags(admin_merge_request_approvals_settings: feature_flag)
stub_licensed_features(admin_merge_request_approvers_rules: licensed)
end
it { is_expected.to eq(result) }
end
end
end
ee/spec/models/approval_state_spec.rb
View file @ ff0a219a
......@@ -63,28 +63,6 @@ describe ApprovalState do
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
......@@ -93,28 +71,6 @@ describe ApprovalState do
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
......@@ -124,28 +80,6 @@ describe ApprovalState do
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
......@@ -155,28 +89,6 @@ describe ApprovalState do
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 @ ff0a219a
......@@ -514,87 +514,6 @@ describe Project do
end
end
context 'merge requests related settings' do
shared_examples 'setting modified by application setting' 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
end
with_them do
let(:project) { create(:project) }
before do
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
describe '#disable_overriding_approvers_per_merge_request' 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(:application_setting) { :disable_overriding_approvers_per_merge_request }
end
end
describe '#merge_requests_disable_committers_approval' 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(:application_setting) { :prevent_merge_requests_committers_approval }
end
end
describe '#merge_requests_author_approval' do
using RSpec::Parameterized::TableSyntax
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
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 }
before do
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
describe '#has_active_hooks?' do
context "with group hooks" do
let(:group) { create(:group) }
......
ee/spec/policies/project_policy_spec.rb
View file @ ff0a219a
......@@ -1387,18 +1387,14 @@ describe ProjectPolicy do
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
where(:role, :admin_mode, :allowed) do
:guest | nil | false
:reporter | nil | false
:developer | nil | false
:maintainer | nil | false
:owner | nil | false
:admin | false | false
:admin | true | true
end
with_them do
......@@ -1406,7 +1402,6 @@ describe ProjectPolicy do
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
......@@ -1415,18 +1410,14 @@ describe ProjectPolicy do
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
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
......@@ -1434,7 +1425,6 @@ describe ProjectPolicy do
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
......@@ -1465,66 +1455,9 @@ 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
......
ee/spec/requests/api/settings_spec.rb
View file @ ff0a219a
......@@ -181,7 +181,39 @@ describe API::Settings, 'EE Settings' do
end
let(:feature) { :admin_merge_request_approvers_rules }
it_behaves_like 'settings for licensed features'
context 'when feature flag is enabled' do
before do
stub_feature_flags(admin_merge_request_approvals_settings: true)
end
it_behaves_like 'settings for licensed features'
end
context 'when feature flag is disabled' do
let(:attribute_names) { settings.keys.map(&:to_s) }
before do
stub_licensed_features(feature => true)
stub_feature_flags(admin_merge_request_approvals_settings: false)
end
it 'hides the attributes in the API' do
get api("/application/settings", admin)
expect(response).to have_gitlab_http_status(:ok)
attribute_names.each do |attribute|
expect(json_response.keys).not_to include(attribute)
end
end
it 'does not update application settings' do
# Make sure the settings exist before the specs
get api("/application/settings", admin)
expect { put api("/application/settings", admin), params: settings }
.not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
end
end
end
context 'updating npm packages request forwarding' do
......
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