diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 91f8bc33e3ed1c295dac00de5b84afe72faa789f..a6c3c97a8735515a6152620fa8041ee5792ae1e8 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -49,6 +49,10 @@ def can_change_group_visibility_level?(group) can?(current_user, :change_visibility_level, group) end + def can_update_default_branch_protection?(group) + can?(current_user, :update_default_branch_protection, group) + end + def can_change_share_with_group_lock?(group) can?(current_user, :change_share_with_group_lock, group) end diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 9353b361c2a63e7795b6dec8e80b83e659c24c3c..b6cf945bf5a81155dba248ae9358a49aca087fad 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -74,6 +74,10 @@ class GlobalPolicy < BasePolicy enable :create_group end + rule { can?(:create_group) }.policy do + enable :create_group_with_default_branch_protection + end + rule { can_create_fork }.policy do enable :create_fork end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 728c4b764984bcac4005bcf49df5fee86f5070fb..7d503b49c144120a0c339e956e908e975ed493ea 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -123,6 +123,7 @@ class GroupPolicy < BasePolicy enable :set_note_created_at enable :set_emails_disabled + enable :update_default_branch_protection end rule { can?(:read_nested_project_resources) }.policy do diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 8cc312006899ef9dc90f0ede353967a1a63900ba..eb1b8d4fcc06f3c63eb5549f18ffd48bf9c10504 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -38,6 +38,10 @@ def after_build_hook(group, params) # overridden in EE end + def remove_unallowed_params + params.delete(:default_branch_protection) unless can?(current_user, :create_group_with_default_branch_protection) + end + def create_chat_team? Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil? end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 8635b82461b9bc10285da56e008dadfeaeadb632..948540619aeb75151a553250db5b7a6a51df62de 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -66,6 +66,7 @@ def reject_parent_id! # overridden in EE def remove_unallowed_params params.delete(:emails_disabled) unless can?(current_user, :set_emails_disabled, group) + params.delete(:default_branch_protection) unless can?(current_user, :update_default_branch_protection, group) end def valid_share_with_group_lock_change? diff --git a/app/views/groups/settings/_default_branch_protection.html.haml b/app/views/groups/settings/_default_branch_protection.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..e0e901cbc4ac8b063e0b40bb06ddd766c50a6941 --- /dev/null +++ b/app/views/groups/settings/_default_branch_protection.html.haml @@ -0,0 +1,3 @@ +- return unless can_update_default_branch_protection?(group) + += render 'shared/default_branch_protection', f: f, selected_level: group.default_branch_protection diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml index 1ddaa855e626d4d154c4923ca6d128bd777f6458..e886c99a656d3bb22f44114bed96d1a24120b5e0 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -33,7 +33,7 @@ = render_if_exists 'groups/settings/ip_restriction', f: f, group: @group = render_if_exists 'groups/settings/allowed_email_domain', f: f, group: @group = render 'groups/settings/lfs', f: f - = render 'shared/default_branch_protection', f: f, selected_level: @group.default_branch_protection + = render 'groups/settings/default_branch_protection', f: f, group: @group = render 'groups/settings/project_creation_level', f: f, group: @group = render 'groups/settings/subgroup_creation_level', f: f, group: @group = render 'groups/settings/two_factor_auth', f: f diff --git a/changelogs/unreleased/211944-provide-instance-level-setting-to-enable-or-disable-default-branch-add-policies.yml b/changelogs/unreleased/211944-provide-instance-level-setting-to-enable-or-disable-default-branch-add-policies.yml new file mode 100644 index 0000000000000000000000000000000000000000..e8762cf6dcdd3dbfc43b42caec18ceb9747a6d0f --- /dev/null +++ b/changelogs/unreleased/211944-provide-instance-level-setting-to-enable-or-disable-default-branch-add-policies.yml @@ -0,0 +1,5 @@ +--- +title: Add policies for managing 'default_branch_protection' setting in groups +merge_request: 29879 +author: +type: added diff --git a/ee/app/services/ee/groups/create_service.rb b/ee/app/services/ee/groups/create_service.rb index 9194df913209cf05f4642290e80a623544cf49a7..288ba23d0a3215da3e98e2c0d4b0e25adc7bd9fa 100644 --- a/ee/app/services/ee/groups/create_service.rb +++ b/ee/app/services/ee/groups/create_service.rb @@ -25,6 +25,8 @@ def remove_unallowed_params params.delete(:shared_runners_minutes_limit) params.delete(:extra_shared_runners_minutes_limit) end + + super end def log_audit_event diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 93478bbff1d06a8debe8f299373bf2714fb7e3e7..354c9e047c8a85b64a025d61b2fde8093b03e189 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -270,6 +270,37 @@ it { expect(subject).to render_template(:new) } end + + context 'when creating a group with `default_branch_protection` attribute' do + before do + sign_in(user) + end + + subject do + post :create, params: { group: { name: 'new_group', path: 'new_group', default_branch_protection: Gitlab::Access::PROTECTION_NONE } } + end + + context 'for users who have the ability to create a group with `default_branch_protection`' do + it 'creates group with the specified branch protection level' do + subject + + expect(response).to have_gitlab_http_status(:found) + expect(Group.last.default_branch_protection).to eq(Gitlab::Access::PROTECTION_NONE) + end + end + + context 'for users who do not have the ability to create a group with `default_branch_protection`' do + it 'does not create the group with the specified branch protection level' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :create_group_with_default_branch_protection) { false } + + subject + + expect(response).to have_gitlab_http_status(:found) + expect(Group.last.default_branch_protection).not_to eq(Gitlab::Access::PROTECTION_NONE) + end + end + end end describe 'GET #index' do @@ -423,11 +454,31 @@ expect(group.reload.project_creation_level).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) end - it 'updates the default_branch_protection successfully' do - post :update, params: { id: group.to_param, group: { default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE } } + context 'updating default_branch_protection' do + subject do + put :update, params: { id: group.to_param, group: { default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE } } + end + + context 'for users who have the ability to update default_branch_protection' do + it 'updates the attribute' do + subject - expect(response).to have_gitlab_http_status(:found) - expect(group.reload.default_branch_protection).to eq(::Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + expect(response).to have_gitlab_http_status(:found) + expect(group.reload.default_branch_protection).to eq(::Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + end + + context 'for users who do not have the ability to update default_branch_protection' do + it 'does not update the attribute' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :update_default_branch_protection, group) { false } + + subject + + expect(response).to have_gitlab_http_status(:found) + expect(group.reload.default_branch_protection).not_to eq(::Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + end end context 'when a project inside the group has container repositories' do diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb index ac2f028f937a46a6f591755be5a72261815e4fa7..5be247c5b49824bdf369653a3a1de1856cf3179b 100644 --- a/spec/helpers/groups_helper_spec.rb +++ b/spec/helpers/groups_helper_spec.rb @@ -340,4 +340,31 @@ end end end + + describe '#can_update_default_branch_protection?' do + let(:current_user) { create(:user) } + let(:group) { create(:group) } + + subject { helper.can_update_default_branch_protection?(group) } + + before do + allow(helper).to receive(:current_user) { current_user } + end + + context 'for users who can update default branch protection of the group' do + before do + allow(helper).to receive(:can?).with(current_user, :update_default_branch_protection, group) { true } + end + + it { is_expected.to be_truthy } + end + + context 'for users who cannot update default branch protection of the group' do + before do + allow(helper).to receive(:can?).with(current_user, :update_default_branch_protection, group) { false } + end + + it { is_expected.to be_falsey } + end + end end diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index 5e77b64a4088fca7c287559a9b2b39fb3c5d041c..ac7f5db980c9541638da56d6694a94f04b77dcd5 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -80,6 +80,34 @@ end end + describe 'create group' do + context 'when user has the ability to create group' do + let(:current_user) { create(:user, can_create_group: true) } + + it { is_expected.to be_allowed(:create_group) } + end + + context 'when user does not have the ability to create group' do + let(:current_user) { create(:user, can_create_group: false) } + + it { is_expected.not_to be_allowed(:create_group) } + end + end + + describe 'create group with default branch protection' do + context 'when user has the ability to create group' do + let(:current_user) { create(:user, can_create_group: true) } + + it { is_expected.to be_allowed(:create_group_with_default_branch_protection) } + end + + context 'when user does not have the ability to create group' do + let(:current_user) { create(:user, can_create_group: false) } + + it { is_expected.not_to be_allowed(:create_group_with_default_branch_protection) } + end + end + describe 'custom attributes' do context 'regular user' do it { is_expected.not_to be_allowed(:read_custom_attribute) } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 30c1f99569b883f7f158480c3a6005fef5bf5e1e..bca7d54d3a803a284805b4395bbe257a8ba1c009 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -642,6 +642,33 @@ def make_upload_request expect(json_response['default_branch_protection']).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) end + context 'updating the `default_branch_protection` attribute' do + subject do + put api("/groups/#{group1.id}", user1), params: { default_branch_protection: ::Gitlab::Access::PROTECTION_NONE } + end + + context 'for users who have the ability to update default_branch_protection' do + it 'updates the attribute' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_NONE) + end + end + + context 'for users who does not have the ability to update default_branch_protection`' do + it 'does not update the attribute' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user1, :update_default_branch_protection, group1) { false } + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['default_branch_protection']).not_to eq(Gitlab::Access::PROTECTION_NONE) + end + end + end + context 'malicious group name' do subject { put api("/groups/#{group1.id}", user1), params: { name: "" } } @@ -1111,6 +1138,33 @@ def make_upload_request it { expect { subject }.not_to change { Group.count } } end + context 'when creating a group with `default_branch_protection` attribute' do + let(:params) { attributes_for_group_api default_branch_protection: Gitlab::Access::PROTECTION_NONE } + + subject { post api("/groups", user3), params: params } + + context 'for users who have the ability to create a group with `default_branch_protection`' do + it 'creates group with the specified branch protection level' do + subject + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_NONE) + end + end + + context 'for users who do not have the ability to create a group with `default_branch_protection`' do + it 'does not create the group with the specified branch protection level' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user3, :create_group_with_default_branch_protection) { false } + + subject + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['default_branch_protection']).not_to eq(Gitlab::Access::PROTECTION_NONE) + end + end + end + it "does not create group, duplicate" do post api("/groups", user3), params: { name: 'Duplicate Test', path: group2.path } diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 5cde9a3ed456698254c7ecbac5791759cc98121e..c0e876cce3397f64a3b62211907176f7bff293c7 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -24,6 +24,27 @@ end end + context 'creating a group with `default_branch_protection` attribute' do + let(:params) { group_params.merge(default_branch_protection: Gitlab::Access::PROTECTION_NONE) } + let(:service) { described_class.new(user, params) } + let(:created_group) { service.execute } + + context 'for users who have the ability to create a group with `default_branch_protection`' do + it 'creates group with the specified branch protection level' do + expect(created_group.default_branch_protection).to eq(Gitlab::Access::PROTECTION_NONE) + end + end + + context 'for users who do not have the ability to create a group with `default_branch_protection`' do + it 'does not create the group with the specified branch protection level' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :create_group_with_default_branch_protection) { false } + + expect(created_group.default_branch_protection).not_to eq(Gitlab::Access::PROTECTION_NONE) + end + end + end + describe 'creating a top level group' do let(:service) { described_class.new(user, group_params) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 1aa7e06182b430ef33de43e193e93086c222f92c..b17d78505d1ede6616d6030654361dca73021939 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -148,6 +148,26 @@ end end + context 'updating default_branch_protection' do + let(:service) do + described_class.new(internal_group, user, default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end + + context 'for users who have the ability to update default_branch_protection' do + it 'updates the attribute' do + internal_group.add_owner(user) + + expect { service.execute }.to change { internal_group.default_branch_protection }.to(Gitlab::Access::PROTECTION_NONE) + end + end + + context 'for users who do not have the ability to update default_branch_protection' do + it 'does not update the attribute' do + expect { service.execute }.not_to change { internal_group.default_branch_protection } + end + end + end + context 'rename group' do let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index c2797c49c028e146dfde22d3ee6223469dfb666e..de64cea64741fd041aac94b17c710c9b894477a9 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -35,7 +35,8 @@ :change_visibility_level, :set_note_created_at, :create_subgroup, - :read_statistics + :read_statistics, + :update_default_branch_protection ].compact end