From 6aa779912fa0db7491a364529e5d2c77d59ccd1c Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 20 Apr 2020 11:01:07 +0530 Subject: [PATCH 1/3] =?UTF-8?q?Introduce=20policies=20around=20setting=20?= =?UTF-8?q?=E2=80=9Cdefault=20branch=20protection=E2=80=9D=20in=20groups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change introuduces new policies around setting “default branch protection” in groups. Namely: `create_group_with_default_branch_protection `: this permission determines whether a user can specify the value of `default_branch_protection` when creating a new group. `update_default_branch_protection `: this permission determines whether a user can update the value of `default_branch_protection` of a group. --- app/helpers/groups_helper.rb | 4 ++ app/policies/global_policy.rb | 4 ++ app/policies/group_policy.rb | 1 + app/services/groups/create_service.rb | 4 ++ app/services/groups/update_service.rb | 1 + .../_default_branch_protection.html.haml | 3 + .../groups/settings/_permissions.html.haml | 2 +- ee/app/services/ee/groups/create_service.rb | 2 + spec/controllers/groups_controller_spec.rb | 63 +++++++++++++++++-- spec/helpers/groups_helper_spec.rb | 27 ++++++++ spec/policies/global_policy_spec.rb | 28 +++++++++ spec/requests/api/groups_spec.rb | 58 +++++++++++++++++ spec/services/groups/create_service_spec.rb | 26 ++++++++ spec/services/groups/update_service_spec.rb | 22 +++++++ .../policies/group_policy_shared_context.rb | 3 +- 15 files changed, 242 insertions(+), 6 deletions(-) create mode 100644 app/views/groups/settings/_default_branch_protection.html.haml diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 91f8bc33e3ed1c..a6c3c97a873551 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 9353b361c2a63e..b6cf945bf5a811 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 728c4b764984bc..7d503b49c14412 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 8cc312006899ef..eb1b8d4fcc06f3 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 8635b82461b9bc..948540619aeb75 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 00000000000000..e0e901cbc4ac8b --- /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 1ddaa855e626d4..e886c99a656d3b 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/ee/app/services/ee/groups/create_service.rb b/ee/app/services/ee/groups/create_service.rb index 9194df913209cf..288ba23d0a3215 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 93478bbff1d06a..cb45d3755847c1 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -270,6 +270,39 @@ 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 + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :create_group_with_default_branch_protection) { false } + end + + it 'does not create the group with the specified branch protection level' do + 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 +456,33 @@ 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 - expect(response).to have_gitlab_http_status(:found) - expect(group.reload.default_branch_protection).to eq(::Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + 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) + end + end + + context 'for users who do not have the ability to update default_branch_protection' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :update_default_branch_protection, group) { false } + end + + it 'does not update the attribute' do + 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 ac2f028f937a46..5be247c5b49824 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 5e77b64a4088fc..ac7f5db980c954 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 30c1f99569b883..5cc144d6cea5e0 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -642,6 +642,35 @@ 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 + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user1, :update_default_branch_protection, group1) { false } + end + + it 'does not update the attribute' do + 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 +1140,35 @@ 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 + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user3, :create_group_with_default_branch_protection) { false } + end + + it 'does not create the group with the specified branch protection level' do + 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 5cde9a3ed45669..6d72e86170d95d 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -24,6 +24,32 @@ 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 } + + before do + 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 + 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 + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :create_group_with_default_branch_protection) { false } + end + + it 'does not create the group with the specified branch protection level' do + 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 1aa7e06182b430..bc64a9435bb167 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -148,6 +148,28 @@ 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 + before do + internal_group.add_owner(user) + end + + it 'updates the attribute' do + 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 c2797c49c028e1..de64cea64741fd 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 -- GitLab From 2b3c511cabc1af619a092836c17b61011880e88f Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 20 Apr 2020 17:36:47 +0530 Subject: [PATCH 2/3] Add new changelog --- ...ting-to-enable-or-disable-default-branch-add-policies.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/211944-provide-instance-level-setting-to-enable-or-disable-default-branch-add-policies.yml 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 00000000000000..e8762cf6dcdd3d --- /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 -- GitLab From 2f7ae04679347fe64aec0c817f2d9c276a1006c2 Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 23 Apr 2020 11:03:33 +0530 Subject: [PATCH 3/3] Address code review comments This change addresses code review comments --- spec/controllers/groups_controller_spec.rb | 8 ++------ spec/requests/api/groups_spec.rb | 8 ++------ spec/services/groups/create_service_spec.rb | 7 +------ spec/services/groups/update_service_spec.rb | 4 +--- 4 files changed, 6 insertions(+), 21 deletions(-) diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index cb45d3755847c1..354c9e047c8a85 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -290,12 +290,10 @@ end context 'for users who do not have the ability to create a group with `default_branch_protection`' do - before 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 } - end - it 'does not create the group with the specified branch protection level' do subject expect(response).to have_gitlab_http_status(:found) @@ -471,12 +469,10 @@ end context 'for users who do not have the ability to update default_branch_protection' do - before 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 } - end - it 'does not update the attribute' do subject expect(response).to have_gitlab_http_status(:found) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 5cc144d6cea5e0..bca7d54d3a803a 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -657,12 +657,10 @@ def make_upload_request end context 'for users who does not have the ability to update default_branch_protection`' do - before 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 } - end - it 'does not update the attribute' do subject expect(response).to have_gitlab_http_status(:ok) @@ -1155,12 +1153,10 @@ def make_upload_request end context 'for users who do not have the ability to create a group with `default_branch_protection`' do - before 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 } - end - it 'does not create the group with the specified branch protection level' do subject expect(response).to have_gitlab_http_status(:created) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 6d72e86170d95d..c0e876cce3397f 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -29,9 +29,6 @@ let(:service) { described_class.new(user, params) } let(:created_group) { service.execute } - before do - 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 expect(created_group.default_branch_protection).to eq(Gitlab::Access::PROTECTION_NONE) @@ -39,12 +36,10 @@ end context 'for users who do not have the ability to create a group with `default_branch_protection`' do - before 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 } - end - it 'does not create the group with the specified branch protection level' do expect(created_group.default_branch_protection).not_to eq(Gitlab::Access::PROTECTION_NONE) end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index bc64a9435bb167..b17d78505d1ede 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -154,11 +154,9 @@ end context 'for users who have the ability to update default_branch_protection' do - before do + it 'updates the attribute' do internal_group.add_owner(user) - end - it 'updates the attribute' do expect { service.execute }.to change { internal_group.default_branch_protection }.to(Gitlab::Access::PROTECTION_NONE) end end -- GitLab