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 From f72757f808fc818da6faa5ae38e2cbab56558e2d 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 c32b477ed876950593c82f06a6ed9d8cea69c170 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 97404196e0d89a2a72d96c127c0d6b9e8e450822 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 eb27c2b164418da3ed75052657b364b740505b51 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 e5f7ef0e77c5ab11f753347711088c8117b5b5dd 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 0239fb86ae4ca77ce93555ca60a09ba6abd61b7d 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 470ce65707d..fd40061dd3a 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,4 +994,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 58c627c2dbba1b2e1bd80d688a77872dc341caeb 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 e55c85af81e9002ac48e7f7ea8235edc559b09ed 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 1dbcc55b10ebba2a71afa0071128139b2118e576 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 3991a992f49ddd6abd1f0ca317bfd21b190b9422 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 797833e3f91..aff418faae5 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -192,7 +192,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 26c6e75d9ef48217eef4ce9ce3b09642e5a73a28 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 0da1f1ba7f5..d3375e00bad 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -20,6 +20,7 @@ = render_if_exists 'groups/settings/ip_restriction', f: f, group: @group = 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 d4b2ff2e2295d8a40b019c4c3bc1e9669c89ed82 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 8e89c7ecfb1..44bc6c8288a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -414,6 +414,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 6d6897219af7bb3bea66c012c33e2588248cecb8 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 9a6cf8338ab8a98d49cc1c0612b04bb5f746a146 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 6dd6f98143f29695e7e4c767149f3a99beac7573 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 582495df08c4e8ba5efb0b63a96741da9a0b1869 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 | 1 + 1 file changed, 1 insertion(+) diff --git a/db/schema.rb b/db/schema.rb index 8876be1cb34..4b2bb95d413 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,6 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false + t.integer "subgroup_creation_level", default: 0, null: false t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree -- cgit v1.2.1 From 9470dc25d8639499dafbbd366eda00b359cbd417 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 fd40061dd3a..6627177ad61 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -999,7 +999,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 23c549d4a38efc15a54ff7cc57e38a9aefb632f8 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 6627177ad61..470ce65707d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,13 +994,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 a781822664a4f9c8edffaa1d75fec856b13bbb1a 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 44bc6c8288a..47e7bf34ec8 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 @@ -414,10 +416,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 @@ -447,4 +445,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 470ce65707d..9ae18d7bab7 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,4 +994,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 e93df68a442486dc08887773c4e96b055f013c57 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 4b2bb95d413..e777cc575c0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,7 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false - 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 ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree 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 9ae18d7bab7..c7fb0f51075 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -997,9 +997,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 832a598cf05b70cdff2eb081cd02e410849c39e2 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 47e7bf34ec8..8e89c7ecfb1 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 @@ -445,8 +443,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 128b5e33dbd4729f25bd3146c967861e814be9e6 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 849e87e8d2c955d91f55c38911b20574c839035a 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 71822fc5de60f073e6f43507b0419c6d52540c8d 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 From b0196e855a6c82ffa96b384e1063df77f2b42249 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 13:41:32 -0700 Subject: Apply changes recomended by merge request coach --- app/models/group.rb | 4 ++++ app/views/groups/_group_admin_settings.html.haml | 6 ++++++ ...190626175626_add_group_creation_level_to_namespaces.rb | 15 +++++---------- db/schema.rb | 2 +- spec/controllers/admin/groups_controller_spec.rb | 6 ++---- spec/features/admin/admin_groups_spec.rb | 9 +++++++++ spec/features/groups/show_spec.rb | 4 ++-- spec/services/groups/create_service_spec.rb | 9 --------- 8 files changed, 29 insertions(+), 26 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 8e89c7ecfb1..44bc6c8288a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -414,6 +414,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 diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml index b8f632d11d3..733cb36cc3d 100644 --- a/app/views/groups/_group_admin_settings.html.haml +++ b/app/views/groups/_group_admin_settings.html.haml @@ -16,6 +16,12 @@ .col-sm-10 = f.select :project_creation_level, options_for_select(::Gitlab::Access.project_creation_options, @group.project_creation_level), {}, class: 'form-control' +.form-group.row + .col-sm-2.col-form-label + = f.label s_('SubgroupCreationlevel|Allowed to create subgroups') + .col-sm-10 + = f.select :subgroup_creation_level, options_for_select(::Gitlab::Access.subgroup_creation_options, @group.subgroup_creation_level), {}, class: 'form-control' + .form-group.row .col-sm-2.col-form-label.pt-0 = f.label :require_two_factor_authentication, 'Two-factor authentication' 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 85ac89af46e..867ec3b7c91 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -7,18 +7,13 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] disable_ddl_transaction! def up - unless column_exists?(:namespaces, :subgroup_creation_level) - add_column_with_default(:namespaces, - :subgroup_creation_level, - :integer, - default: 0) - change_column_default(:namespaces, :subgroup_creation_level, 1) - end + add_column(:namespaces, :subgroup_creation_level, :integer) + change_column_default(:namespaces, + :subgroup_creation_level, + ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end def down - if column_exists?(:namespaces, :subgroup_creation_level) - remove_column(:namespaces, :subgroup_creation_level) - end + remove_column(:namespaces, :subgroup_creation_level) end end diff --git a/db/schema.rb b/db/schema.rb index e777cc575c0..56f66b45595 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,7 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false - t.integer "subgroup_creation_level", default: 1, null: false + t.integer "subgroup_creation_level", default: 1 t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 398f587bafe..1123563c1e3 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -70,13 +70,11 @@ describe Admin::GroupsController do end it 'updates the subgroup_creation_level successfully' do - OWNER = ::Gitlab::Access::OWNER_SUBGROUP_ACCESS - expect do post :update, params: { id: group.to_param, - group: { subgroup_creation_level: OWNER } } - end.to change { group.reload.subgroup_creation_level }.to(OWNER) + group: { subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS } } + end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::OWNER_SUBGROUP_ACCESS) end end end diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb index 735ca60f7da..35c384dd458 100644 --- a/spec/features/admin/admin_groups_spec.rb +++ b/spec/features/admin/admin_groups_spec.rb @@ -102,6 +102,15 @@ describe 'Admin Groups' do expect_selected_visibility(group.visibility_level) end + it 'shows the subgroup creation level dropdown populated with the group subgroup_creation_level value' do + group = create(:group, :private, :owner_subgroup_creation_only) + + visit admin_group_edit_path(group) + + expect(page).to have_select("group_subgroup_creation_level", + selected: ::Gitlab::Access.subgroup_creation_options.keys[group.subgroup_creation_level]) + end + it 'edit group path does not change group name', :js do group = create(:group, :private) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index ef0e885ee5f..5096abadb79 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -71,7 +71,7 @@ describe 'Group show page' do sign_in(owner) end - context 'when subgroups are supported', :js, :nested_groups do + context 'when subgroups are supported', :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } visit path @@ -101,7 +101,7 @@ describe 'Group show page' do sign_in(maintainer) end - context 'when subgroups are supported', :js, :nested_groups do + context 'when subgroups are supported', :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } end diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index b4e6ddddfac..c5ff6cdbacd 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -1,4 +1,3 @@ -# coding: utf-8 # frozen_string_literal: true require 'spec_helper' @@ -88,14 +87,6 @@ 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 -- cgit v1.2.1 From 142a39437eb76c957a984204bc79b0bf5db5be35 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 15:29:54 -0700 Subject: Adjust the documentation on subgroups --- doc/user/group/subgroups/index.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index 1c6cca049c5..e3f7539a9b6 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -75,13 +75,16 @@ structure. ## Creating a subgroup NOTE: **Note:** -You must be an Owner of a group to create a subgroup. For -more information check the [permissions table](../../permissions.md#group-members-permissions). +In order to create a subgroup you must either be an Owner or a Maintainer of the +group, depending on the group's setting. By default groups allow both Owners and +Maintainers to create subgroups, but this can be changed by an Owner or +Administrator to only allow Owners to create subgroups. For more information +check the [permissions table](../../permissions.md#group-members-permissions). For a list of words that are not allowed to be used as group names see the -[reserved names](../../reserved_names.md). -Users can always create subgroups if they are explicitly added as an Owner to -a parent group, even if group creation is disabled by an administrator in their -settings. +[reserved names](../../reserved_names.md). Users can always create subgroups if +they are explicitly added as an Owner (or Maintainer, if that setting is +enabled) to a parent group, even if group creation is disabled by an +administrator in their settings. To create a subgroup: -- cgit v1.2.1 From 650b88fc41d51f3d313e74bb94cbd211ca54003b Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 15:39:43 -0700 Subject: Adjust the documentation on subgroup permissions --- doc/user/permissions.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 03abef9fc62..20b23b75cb9 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -209,13 +209,15 @@ group. | Create project in group | | | ✓ | ✓ | ✓ | | Create/edit/delete group milestones | | | ✓ | ✓ | ✓ | | Enable/disable a dependency proxy **[PREMIUM]** | | | ✓ | ✓ | ✓ | +| Create subgroup | | | | ✓* | ✓ | | Edit group | | | | | ✓ | -| Create subgroup | | | | | ✓ | | Manage group members | | | | | ✓ | | Remove group | | | | | ✓ | | Delete group epic **[ULTIMATE]** | | | | | ✓ | | View group Audit Events | | | | | ✓ | +*Groups can be set to allow either Owners or Owners and maintainers to create subgroups + ### Subgroup permissions When you add a member to a subgroup, they inherit the membership and -- cgit v1.2.1 From 0f659a0da634bf61378766d6ec247cc49595becf Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 16:27:49 -0700 Subject: Fix some code style issues --- spec/features/groups/show_spec.rb | 12 ++++++++---- spec/policies/group_policy_spec.rb | 13 ++++++------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 5096abadb79..f1501181432 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -56,10 +56,14 @@ 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(:restricted_group) do + create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + end + + let(:relaxed_group) do + create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end + let(:owner) { create(:user) } let(:maintainer) { create(:user) } diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 020b51f776e..dc3675a7b9e 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -99,9 +99,9 @@ describe GroupPolicy do let(:current_user) { maintainer } context 'with subgroup_creation level set to maintainer' do - let(:group) { create(:group, - :private, - subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + let(:group) do + create(:group, :private, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end it 'allows every maintainer permission plus creating subgroups' do allow(Group).to receive(:supports_nested_objects?).and_return(true) @@ -207,10 +207,9 @@ 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, - :owner_subgroup_creation_only, - parent: group) } + let(:nested_group) do + create(:group, :private, :owner_subgroup_creation_only, parent: group) + end before do nested_group.add_guest(guest) -- cgit v1.2.1 From a0485e75005e7930acba832089d830744d6cbb14 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Tue, 2 Jul 2019 08:38:38 -0700 Subject: Apply recomended changes from merge coach --- ...190626175626_add_group_creation_level_to_namespaces.rb | 1 - spec/features/admin/admin_groups_spec.rb | 3 +-- spec/features/groups/show_spec.rb | 15 +++++++++------ 3 files changed, 10 insertions(+), 9 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 867ec3b7c91..3b75c92e518 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -4,7 +4,6 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] include Gitlab::Database::MigrationHelpers DOWNTIME = false - disable_ddl_transaction! def up add_column(:namespaces, :subgroup_creation_level, :integer) diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb index 35c384dd458..c1ad73779c9 100644 --- a/spec/features/admin/admin_groups_spec.rb +++ b/spec/features/admin/admin_groups_spec.rb @@ -107,8 +107,7 @@ describe 'Admin Groups' do visit admin_group_edit_path(group) - expect(page).to have_select("group_subgroup_creation_level", - selected: ::Gitlab::Access.subgroup_creation_options.keys[group.subgroup_creation_level]) + expect(page).to have_content('Allowed to create subgroups') end it 'edit group path does not change group name', :js do diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index f1501181432..bed998a0859 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -57,11 +57,11 @@ describe 'Group show page' do context 'subgroup support' do let(:restricted_group) do - create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) end let(:relaxed_group) do - create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end let(:owner) { create(:user) } @@ -78,10 +78,11 @@ describe 'Group show page' do context 'when subgroups are supported', :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 @@ -90,10 +91,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 @@ -113,11 +115,12 @@ describe 'Group show page' do context 'when subgroup_creation_level is set to maintainers' do before do relaxed_group.add_maintainer(maintainer) - path = group_path(relaxed_group) - visit path end it 'allows creating subgroups' do + path = group_path(relaxed_group) + visit path + expect(page) .to have_css("li[data-text='New subgroup']", visible: false) end -- cgit v1.2.1 From bab77650eae67d40ec4070597f83bcc1d406697a 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 467ed9d4e79b27557f6407ed4fd567473b83ab76 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 6d50040c9469d2b1b6c137897b5d7403f3e3b7d5 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 e520893b3450d6b9da4fbe74da943fbfcc24fedd 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 90abe7189177e9d6ecc41c3a3d77a3181904ae8e 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 ae703dbc4a8cc58ee44a4f99e789636c867c7267 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 470ce65707d..fd40061dd3a 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,4 +994,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 b6ce065f4b0476933845ba265e0fe92fb24d1e6e 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 1f4d2ab096284187d92d00f40ebea4df961b2c8e 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 83a7545554fc3ccd44cd6c6d620b860d1d4aeb08 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 65e9fd31d5ca0e0e6eb2b44599f18c333a366a47 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 797833e3f91..aff418faae5 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -192,7 +192,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 84c1455c5b26017ad28ad0f48433d8f41d8b95d3 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 0da1f1ba7f5..d3375e00bad 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -20,6 +20,7 @@ = render_if_exists 'groups/settings/ip_restriction', f: f, group: @group = 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 6175ff7a9b6bc97929a0b81bfe70ff39eaffb58c 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 8e89c7ecfb1..44bc6c8288a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -414,6 +414,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 806088d72e9ec7b1d1ef97db43d76b248dd19186 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 7c7a94f2088ee2687bc21434371c6b27ae56bd1b 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 770e3de7d613d2c87c33bbc993ca5cec1eda4635 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 8547d6d2b4840bca951d708b3f949f665b2760e5 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 | 1 + 1 file changed, 1 insertion(+) diff --git a/db/schema.rb b/db/schema.rb index 7948f919c57..2c6f577c784 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,6 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false + t.integer "subgroup_creation_level", default: 0, null: false t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree -- cgit v1.2.1 From ba836a046df0f805686f95cc73598db9211612e2 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 fd40061dd3a..6627177ad61 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -999,7 +999,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 13d3123425d2575773550015240509e297c1177a 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 6627177ad61..470ce65707d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,13 +994,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 4601b2d17927ca80f2e93a0211bca809dd264b98 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 44bc6c8288a..47e7bf34ec8 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 @@ -414,10 +416,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 @@ -447,4 +445,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 470ce65707d..9ae18d7bab7 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -994,4 +994,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 2497a1e1cf0f52347684552184a24bd246f7bcc8 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 2c6f577c784..272f0fe747b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,7 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false - 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 ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree 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 9ae18d7bab7..c7fb0f51075 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -997,9 +997,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 c440275520cbe3863c7b70b3893d2013f04f3f70 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 47e7bf34ec8..8e89c7ecfb1 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 @@ -445,8 +443,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 6913cfa9aa9c9c4bda85c2c3a5c2e4c0bcdc1e48 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 6e56a915304603a3cc7716823c2d98f108707611 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 3bb1539f24ed642971b16bd9dbb75d7cbb71b08a 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 From 0cacb35552e6c30f9cb03f17d1953063fc551aa5 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 13:41:32 -0700 Subject: Apply changes recomended by merge request coach --- app/models/group.rb | 4 ++++ app/views/groups/_group_admin_settings.html.haml | 6 ++++++ ...190626175626_add_group_creation_level_to_namespaces.rb | 15 +++++---------- db/schema.rb | 2 +- spec/controllers/admin/groups_controller_spec.rb | 6 ++---- spec/features/admin/admin_groups_spec.rb | 9 +++++++++ spec/features/groups/show_spec.rb | 4 ++-- spec/services/groups/create_service_spec.rb | 9 --------- 8 files changed, 29 insertions(+), 26 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 8e89c7ecfb1..44bc6c8288a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -414,6 +414,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 diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml index b8f632d11d3..733cb36cc3d 100644 --- a/app/views/groups/_group_admin_settings.html.haml +++ b/app/views/groups/_group_admin_settings.html.haml @@ -16,6 +16,12 @@ .col-sm-10 = f.select :project_creation_level, options_for_select(::Gitlab::Access.project_creation_options, @group.project_creation_level), {}, class: 'form-control' +.form-group.row + .col-sm-2.col-form-label + = f.label s_('SubgroupCreationlevel|Allowed to create subgroups') + .col-sm-10 + = f.select :subgroup_creation_level, options_for_select(::Gitlab::Access.subgroup_creation_options, @group.subgroup_creation_level), {}, class: 'form-control' + .form-group.row .col-sm-2.col-form-label.pt-0 = f.label :require_two_factor_authentication, 'Two-factor authentication' 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 85ac89af46e..867ec3b7c91 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -7,18 +7,13 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] disable_ddl_transaction! def up - unless column_exists?(:namespaces, :subgroup_creation_level) - add_column_with_default(:namespaces, - :subgroup_creation_level, - :integer, - default: 0) - change_column_default(:namespaces, :subgroup_creation_level, 1) - end + add_column(:namespaces, :subgroup_creation_level, :integer) + change_column_default(:namespaces, + :subgroup_creation_level, + ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end def down - if column_exists?(:namespaces, :subgroup_creation_level) - remove_column(:namespaces, :subgroup_creation_level) - end + remove_column(:namespaces, :subgroup_creation_level) end end diff --git a/db/schema.rb b/db/schema.rb index 272f0fe747b..d121ad8b372 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2112,7 +2112,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.integer "extra_shared_runners_minutes_limit" t.string "ldap_sync_status", default: "ready", null: false t.boolean "membership_lock", default: false - t.integer "subgroup_creation_level", default: 1, null: false + t.integer "subgroup_creation_level", default: 1 t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 398f587bafe..1123563c1e3 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -70,13 +70,11 @@ describe Admin::GroupsController do end it 'updates the subgroup_creation_level successfully' do - OWNER = ::Gitlab::Access::OWNER_SUBGROUP_ACCESS - expect do post :update, params: { id: group.to_param, - group: { subgroup_creation_level: OWNER } } - end.to change { group.reload.subgroup_creation_level }.to(OWNER) + group: { subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS } } + end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::OWNER_SUBGROUP_ACCESS) end end end diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb index 735ca60f7da..35c384dd458 100644 --- a/spec/features/admin/admin_groups_spec.rb +++ b/spec/features/admin/admin_groups_spec.rb @@ -102,6 +102,15 @@ describe 'Admin Groups' do expect_selected_visibility(group.visibility_level) end + it 'shows the subgroup creation level dropdown populated with the group subgroup_creation_level value' do + group = create(:group, :private, :owner_subgroup_creation_only) + + visit admin_group_edit_path(group) + + expect(page).to have_select("group_subgroup_creation_level", + selected: ::Gitlab::Access.subgroup_creation_options.keys[group.subgroup_creation_level]) + end + it 'edit group path does not change group name', :js do group = create(:group, :private) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index ef0e885ee5f..5096abadb79 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -71,7 +71,7 @@ describe 'Group show page' do sign_in(owner) end - context 'when subgroups are supported', :js, :nested_groups do + context 'when subgroups are supported', :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } visit path @@ -101,7 +101,7 @@ describe 'Group show page' do sign_in(maintainer) end - context 'when subgroups are supported', :js, :nested_groups do + context 'when subgroups are supported', :nested_groups do before do allow(Group).to receive(:supports_nested_objects?) { true } end diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index b4e6ddddfac..c5ff6cdbacd 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -1,4 +1,3 @@ -# coding: utf-8 # frozen_string_literal: true require 'spec_helper' @@ -88,14 +87,6 @@ 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 -- cgit v1.2.1 From 4834d9359334edd40bf44cbe6f9fc9d10ce7265e 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 74389c4d82b..d596317462a 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 4ab48e3374590e5300e86565bc47691c30316125 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/support/shared_contexts/policies/group_policy_shared_context.rb | 4 ---- 1 file changed, 4 deletions(-) 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 d596317462a..5a55bbac788 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 1c68fd4ad4dc588440cf58e21be5c2c844e02648 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 15:29:54 -0700 Subject: Adjust the documentation on subgroups --- doc/user/group/subgroups/index.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index 1c6cca049c5..e3f7539a9b6 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -75,13 +75,16 @@ structure. ## Creating a subgroup NOTE: **Note:** -You must be an Owner of a group to create a subgroup. For -more information check the [permissions table](../../permissions.md#group-members-permissions). +In order to create a subgroup you must either be an Owner or a Maintainer of the +group, depending on the group's setting. By default groups allow both Owners and +Maintainers to create subgroups, but this can be changed by an Owner or +Administrator to only allow Owners to create subgroups. For more information +check the [permissions table](../../permissions.md#group-members-permissions). For a list of words that are not allowed to be used as group names see the -[reserved names](../../reserved_names.md). -Users can always create subgroups if they are explicitly added as an Owner to -a parent group, even if group creation is disabled by an administrator in their -settings. +[reserved names](../../reserved_names.md). Users can always create subgroups if +they are explicitly added as an Owner (or Maintainer, if that setting is +enabled) to a parent group, even if group creation is disabled by an +administrator in their settings. To create a subgroup: -- cgit v1.2.1 From df5f65af715f310fc90e5d0e9311be29ca3f959f Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 15:39:43 -0700 Subject: Adjust the documentation on subgroup permissions --- doc/user/permissions.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 03abef9fc62..20b23b75cb9 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -209,13 +209,15 @@ group. | Create project in group | | | ✓ | ✓ | ✓ | | Create/edit/delete group milestones | | | ✓ | ✓ | ✓ | | Enable/disable a dependency proxy **[PREMIUM]** | | | ✓ | ✓ | ✓ | +| Create subgroup | | | | ✓* | ✓ | | Edit group | | | | | ✓ | -| Create subgroup | | | | | ✓ | | Manage group members | | | | | ✓ | | Remove group | | | | | ✓ | | Delete group epic **[ULTIMATE]** | | | | | ✓ | | View group Audit Events | | | | | ✓ | +*Groups can be set to allow either Owners or Owners and maintainers to create subgroups + ### Subgroup permissions When you add a member to a subgroup, they inherit the membership and -- cgit v1.2.1 From df96ef8b9754998756afc06a507d708813e1d84b Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Mon, 1 Jul 2019 16:27:49 -0700 Subject: Fix some code style issues --- spec/features/groups/show_spec.rb | 12 ++++++++---- spec/policies/group_policy_spec.rb | 13 ++++++------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index 5096abadb79..f1501181432 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -56,10 +56,14 @@ 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(:restricted_group) do + create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + end + + let(:relaxed_group) do + create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end + let(:owner) { create(:user) } let(:maintainer) { create(:user) } diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 020b51f776e..dc3675a7b9e 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -99,9 +99,9 @@ describe GroupPolicy do let(:current_user) { maintainer } context 'with subgroup_creation level set to maintainer' do - let(:group) { create(:group, - :private, - subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) } + let(:group) do + create(:group, :private, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + end it 'allows every maintainer permission plus creating subgroups' do allow(Group).to receive(:supports_nested_objects?).and_return(true) @@ -207,10 +207,9 @@ 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, - :owner_subgroup_creation_only, - parent: group) } + let(:nested_group) do + create(:group, :private, :owner_subgroup_creation_only, parent: group) + end before do nested_group.add_guest(guest) -- cgit v1.2.1 From 44fabdaade148caed637d0ea387830100eb15fbc Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Tue, 2 Jul 2019 08:38:38 -0700 Subject: Apply recomended changes from merge coach --- ...190626175626_add_group_creation_level_to_namespaces.rb | 1 - spec/features/admin/admin_groups_spec.rb | 3 +-- spec/features/groups/show_spec.rb | 15 +++++++++------ 3 files changed, 10 insertions(+), 9 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 867ec3b7c91..3b75c92e518 100644 --- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb +++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb @@ -4,7 +4,6 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1] include Gitlab::Database::MigrationHelpers DOWNTIME = false - disable_ddl_transaction! def up add_column(:namespaces, :subgroup_creation_level, :integer) diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb index 35c384dd458..c1ad73779c9 100644 --- a/spec/features/admin/admin_groups_spec.rb +++ b/spec/features/admin/admin_groups_spec.rb @@ -107,8 +107,7 @@ describe 'Admin Groups' do visit admin_group_edit_path(group) - expect(page).to have_select("group_subgroup_creation_level", - selected: ::Gitlab::Access.subgroup_creation_options.keys[group.subgroup_creation_level]) + expect(page).to have_content('Allowed to create subgroups') end it 'edit group path does not change group name', :js do diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb index f1501181432..bed998a0859 100644 --- a/spec/features/groups/show_spec.rb +++ b/spec/features/groups/show_spec.rb @@ -57,11 +57,11 @@ describe 'Group show page' do context 'subgroup support' do let(:restricted_group) do - create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS) end let(:relaxed_group) do - create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) + create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS) end let(:owner) { create(:user) } @@ -78,10 +78,11 @@ describe 'Group show page' do context 'when subgroups are supported', :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 @@ -90,10 +91,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 @@ -113,11 +115,12 @@ describe 'Group show page' do context 'when subgroup_creation_level is set to maintainers' do before do relaxed_group.add_maintainer(maintainer) - path = group_path(relaxed_group) - visit path end it 'allows creating subgroups' do + path = group_path(relaxed_group) + visit path + expect(page) .to have_css("li[data-text='New subgroup']", visible: false) end -- cgit v1.2.1 From 9c8f5b7192986918322d23efc2acec7b189fd69b Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Wed, 3 Jul 2019 05:55:20 +0000 Subject: Apply suggestion to doc/user/group/subgroups/index.md --- doc/user/group/subgroups/index.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index e3f7539a9b6..fad7541bdc4 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -81,7 +81,9 @@ Maintainers to create subgroups, but this can be changed by an Owner or Administrator to only allow Owners to create subgroups. For more information check the [permissions table](../../permissions.md#group-members-permissions). For a list of words that are not allowed to be used as group names see the -[reserved names](../../reserved_names.md). Users can always create subgroups if +[reserved names](../../reserved_names.md). + +Users can always create subgroups if they are explicitly added as an Owner (or Maintainer, if that setting is enabled) to a parent group, even if group creation is disabled by an administrator in their settings. -- cgit v1.2.1 From 78e3a413b8d3779bfd462b4864ab3d9813344366 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Wed, 3 Jul 2019 05:55:49 +0000 Subject: Apply suggestion to doc/user/permissions.md --- doc/user/permissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 20b23b75cb9..216e88d0113 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -209,7 +209,7 @@ group. | Create project in group | | | ✓ | ✓ | ✓ | | Create/edit/delete group milestones | | | ✓ | ✓ | ✓ | | Enable/disable a dependency proxy **[PREMIUM]** | | | ✓ | ✓ | ✓ | -| Create subgroup | | | | ✓* | ✓ | +| Create subgroup | | | | ✓ (1) | ✓ | | Edit group | | | | | ✓ | | Manage group members | | | | | ✓ | | Remove group | | | | | ✓ | -- cgit v1.2.1 From 43858b4badc786fc8254937e0cefad3798481d9f Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Wed, 3 Jul 2019 05:56:03 +0000 Subject: Apply suggestion to doc/user/permissions.md --- doc/user/permissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 216e88d0113..190f8bc8cbc 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -216,7 +216,7 @@ group. | Delete group epic **[ULTIMATE]** | | | | | ✓ | | View group Audit Events | | | | | ✓ | -*Groups can be set to allow either Owners or Owners and maintainers to create subgroups +- (1): Groups can be set to allow either Owners or Owners and maintainers to create subgroups ### Subgroup permissions -- cgit v1.2.1 From cea16e03d639ea8ba32cba32c07a47d05220a363 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Wed, 3 Jul 2019 05:56:53 +0000 Subject: Apply suggestion to doc/user/group/subgroups/index.md --- doc/user/group/subgroups/index.md | 1 - 1 file changed, 1 deletion(-) diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index fad7541bdc4..4e88ec5ee76 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -74,7 +74,6 @@ structure. ## Creating a subgroup -NOTE: **Note:** In order to create a subgroup you must either be an Owner or a Maintainer of the group, depending on the group's setting. By default groups allow both Owners and Maintainers to create subgroups, but this can be changed by an Owner or -- cgit v1.2.1 From e8e3fdf7c7f6d0696538ff5819c03d0ea5e9217b Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Wed, 3 Jul 2019 16:43:56 +0000 Subject: Update index.md --- doc/user/group/subgroups/index.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index 4e88ec5ee76..b9313ccdb1e 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -74,18 +74,19 @@ structure. ## Creating a subgroup -In order to create a subgroup you must either be an Owner or a Maintainer of the -group, depending on the group's setting. By default groups allow both Owners and +To create a subgroup you must either be an Owner or a Maintainer of the group, +depending on the group's setting. By default groups allow both Owners and Maintainers to create subgroups, but this can be changed by an Owner or -Administrator to only allow Owners to create subgroups. For more information -check the [permissions table](../../permissions.md#group-members-permissions). -For a list of words that are not allowed to be used as group names see the +Administrator to only allow Owners to create subgroups. + +For more information check the +[permissions table](../../permissions.md#group-members-permissions). For a list +of words that are not allowed to be used as group names see the [reserved names](../../reserved_names.md). -Users can always create subgroups if -they are explicitly added as an Owner (or Maintainer, if that setting is -enabled) to a parent group, even if group creation is disabled by an -administrator in their settings. +Users can always create subgroups if they are explicitly added as an Owner (or +Maintainer, if that setting is enabled) to a parent group, even if group +creation is disabled by an administrator in their settings. To create a subgroup: -- cgit v1.2.1 -- cgit v1.2.1 From 51fa8a5e778ff6909fd7f70fc8062bb33940a822 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 5 Jul 2019 14:52:06 +0000 Subject: Apply suggestion to doc/user/permissions.md --- doc/user/permissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 190f8bc8cbc..10184de8784 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -216,7 +216,7 @@ group. | Delete group epic **[ULTIMATE]** | | | | | ✓ | | View group Audit Events | | | | | ✓ | -- (1): Groups can be set to allow either Owners or Owners and maintainers to create subgroups +- (1): Groups can be set to allow either Owners or Owners and Maintainers to create subgroups ### Subgroup permissions -- cgit v1.2.1 From 11e2eb79f332f4fd5533b8e0b16ce5c7600b4833 Mon Sep 17 00:00:00 2001 From: Fabio Papa Date: Fri, 5 Jul 2019 08:27:46 -0700 Subject: Fixed a failing test --- .../policies/group_policy_shared_context.rb | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) 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 5a55bbac788..599c912ce00 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