From 56861a19a3466342ed8a489b417c3630c9d61fed Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 14 Jun 2019 15:13:24 -0700 Subject: Add changelog --- changelogs/unreleased/maintainers-can-create-subgroup.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/maintainers-can-create-subgroup.yml diff --git a/changelogs/unreleased/maintainers-can-create-subgroup.yml b/changelogs/unreleased/maintainers-can-create-subgroup.yml new file mode 100644 index 00000000000..b537862c8af --- /dev/null +++ b/changelogs/unreleased/maintainers-can-create-subgroup.yml @@ -0,0 +1,5 @@ +--- +title: Maintainers can crete subgroups +merge_request: 29718 +author: Fabio Papa +type: changed -- cgit v1.2.1 From 1f07faa95a60983e4623845f451e89a5b2c92bbe Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 14 Jun 2019 15:16:55 -0700 Subject: Add failing feature spec detailing a maintainer creating a subgroup - Change the two existing feature examples that create a subgroup to elucidate that the owner is creating the subgroup - Nest two more specs inside the 'subgroup support' context detailing what happens when a maintainer attempts to add a subgroup (one with subgroup support, and one without) --- spec/features/groups/show_spec.rb | 61 ++++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 9671a4d8c49..2654d06cd8c 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -56,32 +56,67 @@ describe 'Group show page' do end context 'subgroup support' do - let(:user) { create(:user) } + let(:owner) { create(:user) } + let(:maintainer) { create(:user) } before do - group.add_owner(user) - sign_in(user) + group.add_owner(owner) + group.add_maintainer(maintainer) end - context 'when subgroups are supported', :js, :nested_groups do + context 'for owners' do before do - allow(Group).to receive(:supports_nested_objects?) { true } - visit path + sign_in(owner) end - it 'allows creating subgroups' do - expect(page).to have_css("li[data-text='New subgroup']", visible: false) + context 'when subgroups are supported', :js, :nested_groups do + before do + allow(Group).to receive(:supports_nested_objects?) { true } + visit path + end + + it 'allows creating subgroups' do + expect(page).to have_css("li[data-text='New subgroup']", visible: false) + end + end + + context 'when subgroups are not supported' do + before do + allow(Group).to receive(:supports_nested_objects?) { false } + visit path + end + + it 'allows creating subgroups' do + expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) + end end end - context 'when subgroups are not supported' do + context 'for maintainers' do before do - allow(Group).to receive(:supports_nested_objects?) { false } - visit path + sign_in(maintainer) + end + + context 'when subgroups are supported', :js, :nested_groups do + before do + allow(Group).to receive(:supports_nested_objects?) { true } + visit path + end + + it 'allows creating subgroups' do + expect(page).to have_css("li[data-text='New subgroup']", visible: false) + end end - it 'allows creating subgroups' do - expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) + context 'when subgroups are not supported' do + before do + allow(Group).to receive(:supports_nested_objects?) { false } + visit path + end + + it 'allows creating subgroups' do + expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) + end end end end -- cgit v1.2.1 From 3cc3cf978f60b1a0f2c627345deef6f5e82254a0 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Sun, 16 Jun 2019 11:07:17 -0700 Subject: Add failing unit test specifying a maintainer creating a subgroup --- spec/services/groups/create_service_spec.rb | 9 +++++++++ .../shared_contexts/policies/group_policy_shared_context.rb | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index c5ff6cdbacd..b4e6ddddfac 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -1,3 +1,4 @@ +# coding: utf-8 # frozen_string_literal: true require 'spec_helper' @@ -87,6 +88,14 @@ describe Groups::CreateService, '#execute' do it { is_expected.to be_persisted } end + + context 'as maintainer' do + before do + group.add_maintainer(user) + end + + it { is_expected.to be_persisted } + end end end 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 b4808ac0068..b4b09d3295f 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -23,6 +23,15 @@ RSpec.shared_context 'GroupPolicy context' do create_projects read_cluster create_cluster update_cluster admin_cluster add_cluster ] + [ + :create_projects, + :read_cluster, + :create_cluster, + :update_cluster, + :admin_cluster, + :add_cluster, + (Gitlab::Database.postgresql? ? :create_subgroup : nil) + ].compact end let(:owner_permissions) do [ @@ -30,8 +39,7 @@ RSpec.shared_context 'GroupPolicy context' do :admin_namespace, :admin_group_member, :change_visibility_level, - :set_note_created_at, - (Gitlab::Database.postgresql? ? :create_subgroup : nil) + :set_note_created_at ].compact end -- cgit v1.2.1 From 66b18427755fcc771267a9e3ca87c6d58db4496d Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Sun, 16 Jun 2019 12:23:56 -0700 Subject: Update the group policy to allow >= maintainer to create subgroups All specs passing --- app/policies/group_policy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index ea86858181d..d92bcded19d 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -109,7 +109,7 @@ class GroupPolicy < BasePolicy enable :read_nested_project_resources end - rule { owner & nested_groups_supported }.enable :create_subgroup + rule { maintainer & nested_groups_supported }.enable :create_subgroup rule { public_group | logged_in_viewable }.enable :view_globally -- cgit v1.2.1 From 60a4cdc2e2cdb0a2de061a36ac83ff53e703cb16 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Sun, 16 Jun 2019 13:22:12 -0700 Subject: Modify API spec to expect a maintainer to be able to create subgroup --- spec/requests/api/groups_spec.rb | 4 ++-- spec/support/shared_contexts/policies/group_policy_shared_context.rb | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index c41408fba65..52d926d5484 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -803,10 +803,10 @@ describe API::Groups do group2.add_maintainer(user1) end - it 'cannot create subgroups' do + it 'can create subgroups' do post api("/groups", user1), params: { parent_id: group2.id, name: 'foo', path: 'foo' } - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(201) end end end 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 b4b09d3295f..a40d3087f6e 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -19,10 +19,6 @@ RSpec.shared_context 'GroupPolicy context' do let(:reporter_permissions) { [:admin_label] } let(:developer_permissions) { [:admin_milestone] } let(:maintainer_permissions) do - %i[ - create_projects - read_cluster create_cluster update_cluster admin_cluster add_cluster - ] [ :create_projects, :read_cluster, -- cgit v1.2.1 From a113c5caba4007ff132f6cba01658b9e4cfc64b6 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 18:15:56 +0000 Subject: Apply suggestion to changelogs/unreleased/maintainers-can-create-subgroup.yml --- changelogs/unreleased/maintainers-can-create-subgroup.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/maintainers-can-create-subgroup.yml b/changelogs/unreleased/maintainers-can-create-subgroup.yml index b537862c8af..180f0f7247f 100644 --- a/changelogs/unreleased/maintainers-can-create-subgroup.yml +++ b/changelogs/unreleased/maintainers-can-create-subgroup.yml @@ -1,5 +1,5 @@ --- -title: Maintainers can crete subgroups +title: Maintainers can create subgroups merge_request: 29718 author: Fabio Papa type: changed -- cgit v1.2.1 From a3064967327f7bf3069f3903bd20120d2d3a9ed9 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Tue, 25 Jun 2019 21:59:10 -0700 Subject: Add examples specing the setting to choose who can create subgroups This setting is at the group level only. The default is specified to be maintainers and owners. **Specs only**, all failing. --- spec/controllers/admin/groups_controller_spec.rb | 6 ++ spec/factories/groups.rb | 1 + spec/features/groups/group_settings_spec.rb | 8 +++ spec/models/group_spec.rb | 8 +++ spec/policies/group_policy_spec.rb | 70 ++++++++++++++++++++++ .../policies/group_policy_shared_context.rb | 16 ++--- 6 files changed, 99 insertions(+), 10 deletions(-) diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 509d8944e3a..df11321537f 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -68,5 +68,11 @@ describe Admin::GroupsController do post :update, params: { id: group.to_param, group: { project_creation_level: ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } } end.to change { group.reload.project_creation_level }.to(::Gitlab::Access::NO_ONE_PROJECT_ACCESS) end + + it 'updates the subgroup_creation_level successfully' do + expect do + post :update, params: { id: group.to_param, group: { subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS } } + end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end end end diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index 18a0c2ec731..2f50fbfe2fa 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -5,6 +5,7 @@ FactoryBot.define do type 'Group' owner nil project_creation_level ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS + subgroup_creation_level ::Gitlab::Access::OWNER_SUBGROUP_ACCESS after(:create) do |group| if group.owner diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb index 5cef5f0521f..95534d5a2ba 100644 --- a/spec/features/groups/group_settings_spec.rb +++ b/spec/features/groups/group_settings_spec.rb @@ -85,6 +85,14 @@ describe 'Edit group settings' do end end + describe 'subgroup creation level menu' do + it 'shows the selection menu' do + visit edit_group_path(group) + + expect(page).to have_content('Allowed to create subgroups') + end + end + describe 'edit group avatar' do before do visit edit_group_path(group) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index d7accbef6bd..426e2526a01 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1023,4 +1023,12 @@ describe Group do expect(group.project_creation_level).to eq(Gitlab::CurrentSettings.default_project_creation) end end + + describe 'subgroup_creation_level' do + it 'outputs the default one if it is nil' do + group = create(:group, subgroup_creation_level: nil) + + expect(group.subgroup_creation_level).to eq(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end + end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 59f3a961d50..aed9a8e34ff 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -163,6 +163,18 @@ describe GroupPolicy do expect_allowed(*updated_owner_permissions) end end + + context 'maintainer' do + let(:current_user) { maintainer } + + it 'allows every maintainer permission except creating subgroups' do + create_subgroup_permission = [:create_subgroup] + updated_maintainer_permissions = maintainer_permissions - create_subgroup_permission + + expect_disallowed(*create_subgroup_permission) + expect_allowed(*updated_maintainer_permissions) + end + end end describe 'private nested group use the highest access level from the group and inherited permissions', :nested_groups do @@ -461,6 +473,64 @@ describe GroupPolicy do end end + context "create_subgroup" do + context 'when group has subgroup creation level set to owner' do + let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) } + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:create_subgroup) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_disallowed(:create_subgroup) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_disallowed(:create_subgroup) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_allowed(:create_subgroup) } + end + end + + context 'when group has subgroup creation level set to maintainer' do + let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:create_subgroup) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_disallowed(:create_subgroup) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_allowed(:create_subgroup) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_allowed(:create_subgroup) } + end + end + end + it_behaves_like 'clusterable policies' do let(:clusterable) { create(:group) } let(:cluster) do 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 a40d3087f6e..b4808ac0068 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -19,15 +19,10 @@ RSpec.shared_context 'GroupPolicy context' do let(:reporter_permissions) { [:admin_label] } let(:developer_permissions) { [:admin_milestone] } let(:maintainer_permissions) do - [ - :create_projects, - :read_cluster, - :create_cluster, - :update_cluster, - :admin_cluster, - :add_cluster, - (Gitlab::Database.postgresql? ? :create_subgroup : nil) - ].compact + %i[ + create_projects + read_cluster create_cluster update_cluster admin_cluster add_cluster + ] end let(:owner_permissions) do [ @@ -35,7 +30,8 @@ RSpec.shared_context 'GroupPolicy context' do :admin_namespace, :admin_group_member, :change_visibility_level, - :set_note_created_at + :set_note_created_at, + (Gitlab::Database.postgresql? ? :create_subgroup : nil) ].compact end -- cgit v1.2.1 From a1b9ace31ee0d9c602cb266e9bf94f2e37b14ce7 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 14:46:48 -0700 Subject: Reset group policy to only allow >= owners to create subgroups --- app/policies/group_policy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index d92bcded19d..ea86858181d 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -109,7 +109,7 @@ class GroupPolicy < BasePolicy enable :read_nested_project_resources end - rule { maintainer & nested_groups_supported }.enable :create_subgroup + rule { owner & nested_groups_supported }.enable :create_subgroup rule { public_group | logged_in_viewable }.enable :view_globally -- cgit v1.2.1 From 521d4c76db6d18468bdee6d28d5dc408c7c7a09f Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 13:49:27 -0700 Subject: Add a subgroup_creation_level column to the namespaces table --- ...6175626_add_group_creation_level_to_namespaces.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb diff --git a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb new file mode 100644 index 00000000000..b0ea74d4765 --- /dev/null +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + unless column_exists?(:namespaces, :subgroup_creation_level) + add_column_with_default(:namespaces, :subgroup_creation_level, :integer, default: 0) + end + end + + def down + if column_exists?(:namespaces, :subgroup_creation_level) + remove_column(:namespaces, :subgroup_creation_level) + end + end +end -- cgit v1.2.1 From 58e4c7fea3a4ffcccac77290bd54f1b59475af0e Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 13:39:04 -0700 Subject: Add constants representing Owner and Maintainer group access levels --- lib/gitlab/access.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 6eb08f674c2..77076ead47a 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -29,6 +29,10 @@ module Gitlab MAINTAINER_PROJECT_ACCESS = 1 DEVELOPER_MAINTAINER_PROJECT_ACCESS = 2 + # Default subgroup creation level + OWNER_SUBGROUP_ACCESS = 0 + MAINTAINER_SUBGROUP_ACCESS = 1 + class << self delegate :values, to: :options -- cgit v1.2.1 From 5f9bc7992d042eebd425ae3e0a6a3f35b73a7804 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 13:45:58 -0700 Subject: Add subgroup_creation_level to the list of allowed group params For both groups_controller and admin/groups_controller --- app/controllers/admin/groups_controller.rb | 3 ++- app/controllers/groups_controller.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 15f7ef881c8..6317fa7c8d1 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -90,7 +90,8 @@ class Admin::GroupsController < Admin::ApplicationController :visibility_level, :require_two_factor_authentication, :two_factor_grace_period, - :project_creation_level + :project_creation_level, + :subgroup_creation_level ] end end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index e936d771502..6bb72476959 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -188,7 +188,8 @@ class GroupsController < Groups::ApplicationController :chat_team_name, :require_two_factor_authentication, :two_factor_grace_period, - :project_creation_level + :project_creation_level, + :subgroup_creation_level ] end -- cgit v1.2.1 From 50bd9ddf2ec67594eb8c28f2c8fb5b262be2c515 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 14:11:09 -0700 Subject: Add "allowed to create subgroups" dropdown to group settings form --- app/views/groups/settings/_permissions.html.haml | 1 + app/views/groups/settings/_subgroup_creation_level.html.haml | 3 +++ lib/gitlab/access.rb | 7 +++++++ 3 files changed, 11 insertions(+) create mode 100644 app/views/groups/settings/_subgroup_creation_level.html.haml diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml index 0a14830c666..b5562198984 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -19,6 +19,7 @@ = render 'groups/settings/lfs', f: f = 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 = render_if_exists 'groups/member_lock_setting', f: f, group: @group diff --git a/app/views/groups/settings/_subgroup_creation_level.html.haml b/app/views/groups/settings/_subgroup_creation_level.html.haml new file mode 100644 index 00000000000..f36ad192bad --- /dev/null +++ b/app/views/groups/settings/_subgroup_creation_level.html.haml @@ -0,0 +1,3 @@ +.form-group + = f.label s_('SubgroupCreationLevel|Allowed to create subgroups'), class: 'label-bold' + = f.select :subgroup_creation_level, options_for_select(::Gitlab::Access.subgroup_creation_options, group.subgroup_creation_level), {}, class: 'form-control' diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 77076ead47a..7ef9f7ef630 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -110,6 +110,13 @@ module Gitlab def project_creation_level_name(name) project_creation_options.key(name) end + + def subgroup_creation_options + { + s_('SubgroupCreationlevel|Owners') => OWNER_SUBGROUP_ACCESS, + s_('SubgroupCreationlevel|Maintainers') => MAINTAINER_SUBGROUP_ACCESS + } + end end def human_access -- cgit v1.2.1 From 7bf0ce2283334752bd88a8eb63aa16e5b4b33864 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 14:17:24 -0700 Subject: Make the group model return maintainer level when it is not set --- app/models/group.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/group.rb b/app/models/group.rb index dbec211935d..f32312c6f43 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -418,6 +418,10 @@ class Group < Namespace super || ::Gitlab::CurrentSettings.default_project_creation end + def subgroup_creation_level + super || ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS + end + private def update_two_factor_requirement -- cgit v1.2.1 From 075a6328813ee3733e23254d8e7540b59ec789c0 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Thu, 27 Jun 2019 15:53:46 -0700 Subject: Add policy to allow maintainers to create subgroups when enabled --- app/policies/group_policy.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index ea86858181d..bd1eb02ca1f 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -43,6 +43,10 @@ class GroupPolicy < BasePolicy @subject.project_creation_level == ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS end + condition(:maintainer_can_create_group) do + @subject.subgroup_creation_level == ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS + end + rule { public_group }.policy do enable :read_group enable :read_list @@ -110,6 +114,7 @@ class GroupPolicy < BasePolicy end rule { owner & nested_groups_supported }.enable :create_subgroup + rule { maintainer & maintainer_can_create_group & nested_groups_supported }.enable :create_subgroup rule { public_group | logged_in_viewable }.enable :view_globally -- cgit v1.2.1 From 9caf32698110f9c3b7a861db86b994ddab8308a7 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 28 Jun 2019 08:35:09 -0700 Subject: Add feature examples specing maintainers creating subgroups Both with subgroup_creation_level set to owners and to maintainers. Also fixed the naming of some other examples in the same spec as they were contradicting what they were actually performing in the test. These examples were probably copy/pasted, and not renamed. --- spec/features/groups/show_spec.rb | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 2654d06cd8c..68fa3f4e817 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -86,7 +86,7 @@ describe 'Group show page' do visit path end - it 'allows creating subgroups' do + it 'does not allow creating subgroups' do expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) end end @@ -103,8 +103,22 @@ describe 'Group show page' do visit path end - it 'allows creating subgroups' do - expect(page).to have_css("li[data-text='New subgroup']", visible: false) + context 'when subgroup_creation_level is set to maintainer' do + let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + + it 'allows creating subgroups' do + visit path + expect(page).to have_css("li[data-text='New subgroup']", visible: false) + end + end + + context 'when subgroup_creation_level is set to owners' do + let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) } + + it 'does not allow creating subgroups' do + visit path + expect(page).not_to have_css("li[data-text='New subgroup']", visible: false) + end end end @@ -114,7 +128,7 @@ describe 'Group show page' do visit path end - it 'allows creating subgroups' do + it 'does not allow creating subgroups' do expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) end end -- cgit v1.2.1 From e1d0a30b57ea8c225e719670a6a89f48e413ce9d Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 28 Jun 2019 09:22:10 -0700 Subject: Update schema --- db/schema.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 4ed7c0cb248..29367551efb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190611161641) do +ActiveRecord::Schema.define(version: 20190626175626) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1442,6 +1442,7 @@ ActiveRecord::Schema.define(version: 20190611161641) do t.integer "project_creation_level" t.boolean "auto_devops_enabled" t.datetime_with_timezone "last_ci_minutes_notification_at" + t.integer "subgroup_creation_level", default: 0, null: false t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["name", "parent_id"], name: "index_namespaces_on_name_and_parent_id", unique: true, using: :btree t.index ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} -- cgit v1.2.1 From 65d4843421c7bdc1b571f2b72f8db1a64cf0834f Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 28 Jun 2019 13:31:47 -0700 Subject: Style rules; Revert some examples --- ...75626_add_group_creation_level_to_namespaces.rb | 7 +++++-- spec/controllers/admin/groups_controller_spec.rb | 9 ++++++-- spec/features/groups/group_settings_spec.rb | 2 +- spec/features/groups/show_spec.rb | 24 +++++++++++++++------- spec/models/group_spec.rb | 3 ++- spec/policies/group_policy_spec.rb | 21 ++++++++++++++----- spec/requests/api/groups_spec.rb | 4 ++-- spec/services/groups/create_service_spec.rb | 4 ++-- 8 files changed, 52 insertions(+), 22 deletions(-) diff --git a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb index b0ea74d4765..eed0ba25f27 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -5,10 +5,13 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] DOWNTIME = false disable_ddl_transaction! - + def up unless column_exists?(:namespaces, :subgroup_creation_level) - add_column_with_default(:namespaces, :subgroup_creation_level, :integer, default: 0) + add_column_with_default(:namespaces, + :subgroup_creation_level, + :integer, + default: 0) end end diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index df11321537f..72f389513f8 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -70,9 +70,14 @@ describe Admin::GroupsController do end it 'updates the subgroup_creation_level successfully' do + MAINTAINER = ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS + expect do - post :update, params: { id: group.to_param, group: { subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS } } - end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + post :update, + params: { id: group.to_param, + group: { subgroup_creation_level: MAINTAINER } } + end.to change { group.reload.subgroup_creation_level } + .to(MAINTAINER) end end end diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb index 95534d5a2ba..676769c25fe 100644 --- a/spec/features/groups/group_settings_spec.rb +++ b/spec/features/groups/group_settings_spec.rb @@ -92,7 +92,7 @@ describe 'Edit group settings' do expect(page).to have_content('Allowed to create subgroups') end end - + describe 'edit group avatar' do before do visit edit_group_path(group) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 68fa3f4e817..163906010fa 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -76,7 +76,8 @@ describe 'Group show page' do end it 'allows creating subgroups' do - expect(page).to have_css("li[data-text='New subgroup']", visible: false) + expect(page) + .to have_css("li[data-text='New subgroup']", visible: false) end end @@ -87,7 +88,8 @@ describe 'Group show page' do end it 'does not allow creating subgroups' do - expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) + expect(page) + .not_to have_selector("li[data-text='New subgroup']", visible: false) end end end @@ -104,8 +106,11 @@ describe 'Group show page' do end context 'when subgroup_creation_level is set to maintainer' do - let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } - + let(:group) do + create(:group, + subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end + it 'allows creating subgroups' do visit path expect(page).to have_css("li[data-text='New subgroup']", visible: false) @@ -113,11 +118,15 @@ describe 'Group show page' do end context 'when subgroup_creation_level is set to owners' do - let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) } + let(:group) do + create(:group, + subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + end it 'does not allow creating subgroups' do visit path - expect(page).not_to have_css("li[data-text='New subgroup']", visible: false) + expect(page) + .not_to have_css("li[data-text='New subgroup']", visible: false) end end end @@ -129,7 +138,8 @@ describe 'Group show page' do end it 'does not allow creating subgroups' do - expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false) + expect(page) + .not_to have_selector("li[data-text='New subgroup']", visible: false) end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 426e2526a01..2b85b281d33 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1028,7 +1028,8 @@ describe Group do it 'outputs the default one if it is nil' do group = create(:group, subgroup_creation_level: nil) - expect(group.subgroup_creation_level).to eq(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + expect(group.subgroup_creation_level) + .to eq(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index aed9a8e34ff..da186f63eca 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -145,7 +145,8 @@ describe GroupPolicy do it 'allows every owner permission except creating subgroups' do create_subgroup_permission = [:create_subgroup] - updated_owner_permissions = owner_permissions - create_subgroup_permission + updated_owner_permissions = + owner_permissions - create_subgroup_permission expect_disallowed(*create_subgroup_permission) expect_allowed(*updated_owner_permissions) @@ -157,7 +158,8 @@ describe GroupPolicy do it 'allows every owner permission except creating subgroups' do create_subgroup_permission = [:create_subgroup] - updated_owner_permissions = owner_permissions - create_subgroup_permission + updated_owner_permissions = + owner_permissions - create_subgroup_permission expect_disallowed(*create_subgroup_permission) expect_allowed(*updated_owner_permissions) @@ -169,7 +171,8 @@ describe GroupPolicy do it 'allows every maintainer permission except creating subgroups' do create_subgroup_permission = [:create_subgroup] - updated_maintainer_permissions = maintainer_permissions - create_subgroup_permission + updated_maintainer_permissions = + maintainer_permissions - create_subgroup_permission expect_disallowed(*create_subgroup_permission) expect_allowed(*updated_maintainer_permissions) @@ -475,7 +478,11 @@ describe GroupPolicy do context "create_subgroup" do context 'when group has subgroup creation level set to owner' do - let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) } + let(:group) do + create( + :group, + subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + end context 'reporter' do let(:current_user) { reporter } @@ -503,7 +510,11 @@ describe GroupPolicy do end context 'when group has subgroup creation level set to maintainer' do - let(:group) { create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + let(:group) do + create( + :group, + subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end context 'reporter' do let(:current_user) { reporter } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 52d926d5484..c41408fba65 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -803,10 +803,10 @@ describe API::Groups do group2.add_maintainer(user1) end - it 'can create subgroups' do + it 'cannot create subgroups' do post api("/groups", user1), params: { parent_id: group2.id, name: 'foo', path: 'foo' } - expect(response).to have_gitlab_http_status(201) + expect(response).to have_gitlab_http_status(403) end end end diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index b4e6ddddfac..267ad529d3b 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -89,9 +89,9 @@ describe Groups::CreateService, '#execute' do it { is_expected.to be_persisted } end - context 'as maintainer' do + context 'as Owner' do before do - group.add_maintainer(user) + group.add_owner(user) end it { is_expected.to be_persisted } -- cgit v1.2.1 From 3a1d9ce3a1ed86eb169c48896d290b9bf9a91af9 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 28 Jun 2019 14:06:50 -0700 Subject: Remove an example that is no longer necessary --- spec/models/group_spec.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 2b85b281d33..d7accbef6bd 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1023,13 +1023,4 @@ describe Group do expect(group.project_creation_level).to eq(Gitlab::CurrentSettings.default_project_creation) end end - - describe 'subgroup_creation_level' do - it 'outputs the default one if it is nil' do - group = create(:group, subgroup_creation_level: nil) - - expect(group.subgroup_creation_level) - .to eq(::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) - end - end end -- cgit v1.2.1 From d9c6efceda827de6fa0b23bd9b4940b7914d646b Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 28 Jun 2019 16:12:54 -0700 Subject: Make maintainers the default setting for creating subgroups --- app/models/group.rb | 10 ++++++---- spec/models/group_spec.rb | 8 ++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index f32312c6f43..f8be5f33be8 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -58,6 +58,8 @@ class Group < Namespace add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required } + before_create :default_subgroup_creation_level_to_maintainers + after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement @@ -418,10 +420,6 @@ class Group < Namespace super || ::Gitlab::CurrentSettings.default_project_creation end - def subgroup_creation_level - super || ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS - end - private def update_two_factor_requirement @@ -451,4 +449,8 @@ class Group < Namespace errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.") end + + def default_subgroup_creation_level_to_maintainers + self.subgroup_creation_level = ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index d7accbef6bd..3a5ae14ab46 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1023,4 +1023,12 @@ describe Group do expect(group.project_creation_level).to eq(Gitlab::CurrentSettings.default_project_creation) end end + + describe 'subgroup_creation_level' do + it 'defaults to maintainers' do + group = create (:group) + + expect(group.subgroup_creation_level).to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end + end end -- cgit v1.2.1 From f21c03c8e27932d957086a1ec2465d468fb387da Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Sun, 30 Jun 2019 14:40:23 -0700 Subject: Make subgroup_creation_level default to maintainer at SQL level - Migration updates existing groups to "owner", then sets default to "maintainer" so that new groups will default to that - Update spec examples --- ...75626_add_group_creation_level_to_namespaces.rb | 1 + db/schema.rb | 2 +- spec/controllers/admin/groups_controller_spec.rb | 7 ++-- spec/factories/groups.rb | 5 ++- spec/features/groups/show_spec.rb | 17 ++++++--- spec/models/group_spec.rb | 5 +-- spec/policies/group_policy_spec.rb | 43 ++++++++++++++++++---- spec/requests/api/groups_spec.rb | 4 +- .../policies/group_policy_shared_context.rb | 2 +- 9 files changed, 61 insertions(+), 25 deletions(-) diff --git a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb index eed0ba25f27..85ac89af46e 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -12,6 +12,7 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] :subgroup_creation_level, :integer, default: 0) + change_column_default(:namespaces, :subgroup_creation_level, 1) end end diff --git a/db/schema.rb b/db/schema.rb index 29367551efb..683ee685372 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1442,7 +1442,7 @@ ActiveRecord::Schema.define(version: 20190626175626) do t.integer "project_creation_level" t.boolean "auto_devops_enabled" t.datetime_with_timezone "last_ci_minutes_notification_at" - t.integer "subgroup_creation_level", default: 0, null: false + t.integer "subgroup_creation_level", default: 1, null: false t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["name", "parent_id"], name: "index_namespaces_on_name_and_parent_id", unique: true, using: :btree t.index ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 72f389513f8..398f587bafe 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -70,14 +70,13 @@ describe Admin::GroupsController do end it 'updates the subgroup_creation_level successfully' do - MAINTAINER = ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS + OWNER = ::Gitlab::Access::OWNER_SUBGROUP_ACCESS expect do post :update, params: { id: group.to_param, - group: { subgroup_creation_level: MAINTAINER } } - end.to change { group.reload.subgroup_creation_level } - .to(MAINTAINER) + group: { subgroup_creation_level: OWNER } } + end.to change { group.reload.subgroup_creation_level }.to(OWNER) end end end diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index 2f50fbfe2fa..b67ab955ffc 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -5,7 +5,6 @@ FactoryBot.define do type 'Group' owner nil project_creation_level ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS - subgroup_creation_level ::Gitlab::Access::OWNER_SUBGROUP_ACCESS after(:create) do |group| if group.owner @@ -46,5 +45,9 @@ FactoryBot.define do trait :auto_devops_disabled do auto_devops_enabled false end + + trait :owner_subgroup_creation_only do + subgroup_creation_level ::Gitlab::Access::OWNER_SUBGROUP_ACCESS + end end end diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 163906010fa..48ba9064327 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -72,10 +72,11 @@ describe 'Group show page' do context 'when subgroups are supported', :js, :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } - visit path end it 'allows creating subgroups' do + visit path + expect(page) .to have_css("li[data-text='New subgroup']", visible: false) end @@ -84,10 +85,11 @@ describe 'Group show page' do context 'when subgroups are not supported' do before do allow(Group).to receive(:supports_nested_objects?) { false } - visit path end it 'does not allow creating subgroups' do + visit path + expect(page) .not_to have_selector("li[data-text='New subgroup']", visible: false) end @@ -102,10 +104,9 @@ describe 'Group show page' do context 'when subgroups are supported', :js, :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } - visit path end - context 'when subgroup_creation_level is set to maintainer' do + context 'when subgroup_creation_level is set to maintainers' do let(:group) do create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) @@ -113,7 +114,9 @@ describe 'Group show page' do it 'allows creating subgroups' do visit path - expect(page).to have_css("li[data-text='New subgroup']", visible: false) + + expect(page) + .to have_css("li[data-text='New subgroup']", visible: false) end end @@ -125,6 +128,7 @@ describe 'Group show page' do it 'does not allow creating subgroups' do visit path + expect(page) .not_to have_css("li[data-text='New subgroup']", visible: false) end @@ -134,10 +138,11 @@ describe 'Group show page' do context 'when subgroups are not supported' do before do allow(Group).to receive(:supports_nested_objects?) { false } - visit path end it 'does not allow creating subgroups' do + visit path + expect(page) .not_to have_selector("li[data-text='New subgroup']", visible: false) end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 3a5ae14ab46..12b3bfdf015 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1026,9 +1026,8 @@ describe Group do describe 'subgroup_creation_level' do it 'defaults to maintainers' do - group = create (:group) - - expect(group.subgroup_creation_level).to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + expect(group.subgroup_creation_level) + .to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index da186f63eca..7fba62d2aa8 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -98,12 +98,38 @@ describe GroupPolicy do context 'maintainer' do let(:current_user) { maintainer } - it do - expect_allowed(*guest_permissions) - expect_allowed(*reporter_permissions) - expect_allowed(*developer_permissions) - expect_allowed(*maintainer_permissions) - expect_disallowed(*owner_permissions) + context 'with subgroup_creation level set to maintainer' do + let(:group) { create(:group, + :private, + subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + + it do + allow(Group).to receive(:supports_nested_objects?).and_return(true) + + create_subgroup_permission = [:create_subgroup] + updated_maintainer_permissions = + maintainer_permissions + create_subgroup_permission + updated_owner_permissions = + owner_permissions - create_subgroup_permission + + expect_allowed(*guest_permissions) + expect_allowed(*reporter_permissions) + expect_allowed(*developer_permissions) + expect_allowed(*updated_maintainer_permissions) + expect_disallowed(*updated_owner_permissions) + end + end + + context 'with subgroup_creation_level set to owner' do + it do + allow(Group).to receive(:supports_nested_objects?).and_return(true) + + expect_allowed(*guest_permissions) + expect_allowed(*reporter_permissions) + expect_allowed(*developer_permissions) + expect_allowed(*maintainer_permissions) + expect_disallowed(*owner_permissions) + end end end @@ -181,7 +207,10 @@ describe GroupPolicy do end describe 'private nested group use the highest access level from the group and inherited permissions', :nested_groups do - let(:nested_group) { create(:group, :private, parent: group) } + let(:nested_group) { create(:group, + :private, + :owner_subgroup_creation_only, + parent: group) } before do nested_group.add_guest(guest) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index c41408fba65..52d926d5484 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -803,10 +803,10 @@ describe API::Groups do group2.add_maintainer(user1) end - it 'cannot create subgroups' do + it 'can create subgroups' do post api("/groups", user1), params: { parent_id: group2.id, name: 'foo', path: 'foo' } - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(201) end end end 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 b4808ac0068..74389c4d82b 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -7,7 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do let(:maintainer) { create(:user) } let(:owner) { create(:user) } let(:admin) { create(:admin) } - let(:group) { create(:group, :private) } + let(:group) { create(:group, :private, :owner_subgroup_creation_only) } let(:guest_permissions) do %i[ -- cgit v1.2.1 From 8309221555347d537cef74aad83ede4afc855074 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 09:00:34 -0700 Subject: Remove AR hook to set the default subgroup_creation_level --- app/models/group.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index f8be5f33be8..dbec211935d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -58,8 +58,6 @@ class Group < Namespace add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required } - before_create :default_subgroup_creation_level_to_maintainers - after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement @@ -449,8 +447,4 @@ class Group < Namespace errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.") end - - def default_subgroup_creation_level_to_maintainers - self.subgroup_creation_level = ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS - end end -- cgit v1.2.1 From fe438d0287ead7f07d9281261b5b079fd11147be Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 09:17:26 -0700 Subject: Clean up the show_spec examples previously added --- spec/features/groups/show_spec.rb | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 48ba9064327..ef0e885ee5f 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -56,27 +56,28 @@ describe 'Group show page' do end context 'subgroup support' do + let(:restricted_group) { create(:group, + subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) } + let(:relaxed_group) { create(:group, + subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } let(:owner) { create(:user) } let(:maintainer) { create(:user) } - before do - group.add_owner(owner) - group.add_maintainer(maintainer) - end - context 'for owners' do + let(:path) { group_path(restricted_group) } + before do + restricted_group.add_owner(owner) sign_in(owner) end context 'when subgroups are supported', :js, :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } + visit path end it 'allows creating subgroups' do - visit path - expect(page) .to have_css("li[data-text='New subgroup']", visible: false) end @@ -85,11 +86,10 @@ describe 'Group show page' do context 'when subgroups are not supported' do before do allow(Group).to receive(:supports_nested_objects?) { false } + visit path end it 'does not allow creating subgroups' do - visit path - expect(page) .not_to have_selector("li[data-text='New subgroup']", visible: false) end @@ -107,30 +107,30 @@ describe 'Group show page' do end context 'when subgroup_creation_level is set to maintainers' do - let(:group) do - create(:group, - subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + before do + relaxed_group.add_maintainer(maintainer) + path = group_path(relaxed_group) + visit path end it 'allows creating subgroups' do - visit path - expect(page) .to have_css("li[data-text='New subgroup']", visible: false) end end context 'when subgroup_creation_level is set to owners' do - let(:group) do - create(:group, - subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + before do + restricted_group.add_maintainer(maintainer) end it 'does not allow creating subgroups' do + path = group_path(restricted_group) visit path expect(page) - .not_to have_css("li[data-text='New subgroup']", visible: false) + .not_to have_selector("li[data-text='New subgroup']", + visible: false) end end end -- cgit v1.2.1 From a5175d618c11a62c97f16e484fd26cb5bae8153d Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 09:55:00 -0700 Subject: Fix group creat_service_spec to contain maintainer context --- spec/services/groups/create_service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 267ad529d3b..b4e6ddddfac 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -89,9 +89,9 @@ describe Groups::CreateService, '#execute' do it { is_expected.to be_persisted } end - context 'as Owner' do + context 'as maintainer' do before do - group.add_owner(user) + group.add_maintainer(user) end it { is_expected.to be_persisted } -- cgit v1.2.1 From ca6640d2b28e1ec1049576fe2ae4a954cc598988 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 09:56:02 -0700 Subject: Add descriptions to examples --- spec/policies/group_policy_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 7fba62d2aa8..020b51f776e 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -103,7 +103,7 @@ describe GroupPolicy do :private, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } - it do + it 'allows every maintainer permission plus creating subgroups' do allow(Group).to receive(:supports_nested_objects?).and_return(true) create_subgroup_permission = [:create_subgroup] @@ -121,7 +121,7 @@ describe GroupPolicy do end context 'with subgroup_creation_level set to owner' do - it do + it 'allows every maintainer permission' do allow(Group).to receive(:supports_nested_objects?).and_return(true) expect_allowed(*guest_permissions) -- cgit v1.2.1