Merge branch 'maintainers-can-create-subgroup' into 'master'

Add a group setting to allow Maintainers to create sub-groups

See merge request gitlab-org/gitlab-ce!29718

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

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
 

app/models/group.rb

@@ -416,6 +416,10 @@ class Group < Namespace
     super || ::Gitlab::CurrentSettings.default_project_creation
   end
 
+  def subgroup_creation_level
+    super || ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
+  end
+
   private
 
   def update_two_factor_requirement

app/policies/group_policy.rb

@@ -38,6 +38,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
@@ -105,6 +109,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
 

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'

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
 

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'

changelogs/unreleased/maintainers-can-create-subgroup.yml

@@ -0,0 +1,5 @@
+---
+title: Maintainers can create subgroups
+merge_request: 29718
+author: Fabio Papa
+type: changed

db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    add_column(:namespaces, :subgroup_creation_level, :integer)
+    change_column_default(:namespaces,
+                          :subgroup_creation_level,
+                          ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
+  end
+
+  def down
+    remove_column(:namespaces, :subgroup_creation_level)
+  end
+end

db/schema.rb

@@ -2119,6 +2119,7 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do
     t.string "ldap_sync_status", default: "ready", null: false
     t.boolean "membership_lock", default: false
     t.integer "last_ci_minutes_usage_notification_level"
+    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

doc/user/group/subgroups/index.md

@@ -74,14 +74,24 @@ 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).
-For a list of words that are not allowed to be used as group names see the
+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 created in:
+
+- GitLab 12.2 or later allow both Owners and Maintainers to create subgroups.
+- GitLab 12.1 or earlier only allow Owners to create subgroups.
+
+This setting can be for any group by an Owner or Administrator.
+
+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.
+
+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:
 

doc/user/permissions.md

@@ -209,13 +209,16 @@ group.
 | Create project in group                         |       |          | ✓         | ✓          | ✓     |
 | Create/edit/delete group milestones             |       |          | ✓         | ✓          | ✓     |
 | Enable/disable a dependency proxy **(PREMIUM)** |       |          | ✓         | ✓          | ✓     |
+| Create subgroup                                 |       |          |           | ✓ (1)        | ✓     |
 | Edit group                                      |       |          |           |            | ✓     |
-| Create subgroup                                 |       |          |           |            | ✓     |
 | Manage group members                            |       |          |           |            | ✓     |
 | Remove 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](group/subgroups/index.md#creating-a-subgroup)
+
 ### Subgroup permissions
 
 When you add a member to a subgroup, they inherit the membership and

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
 
@@ -106,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

locale/gitlab.pot

@@ -10138,6 +10138,18 @@ msgstr ""
 msgid "StorageSize|Unknown"
 msgstr ""
 
+msgid "SubgroupCreationLevel|Allowed to create subgroups"
+msgstr ""
+
+msgid "SubgroupCreationlevel|Allowed to create subgroups"
+msgstr ""
+
+msgid "SubgroupCreationlevel|Maintainers"
+msgstr ""
+
+msgid "SubgroupCreationlevel|Owners"
+msgstr ""
+
 msgid "Subgroups"
 msgstr ""
 

spec/controllers/admin/groups_controller_spec.rb

@@ -68,5 +68,13 @@ 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::OWNER_SUBGROUP_ACCESS } }
+      end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::OWNER_SUBGROUP_ACCESS)
+    end
   end
 end

spec/factories/groups.rb

@@ -45,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

spec/features/admin/admin_groups_spec.rb

@@ -103,6 +103,14 @@ 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_content('Allowed to create subgroups')
+    end
+
     it 'edit group path does not change group name', :js do
       group = create(:group, :private)
 

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)

spec/features/groups/show_spec.rb

@@ -56,32 +56,103 @@ describe 'Group show page' do
   end
 
   context 'subgroup support' do
-    let(:user) { create(:user) }
-
-    before do
-      group.add_owner(user)
-      sign_in(user)
+    let(:restricted_group) do
+      create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS)
     end
 
-    context 'when subgroups are supported', :js, :nested_groups do
+    let(:relaxed_group) do
+      create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
+    end
+
+    let(:owner) { create(:user) }
+    let(:maintainer) { create(:user) }
+
+    context 'for owners' do
+      let(:path) { group_path(restricted_group) }
+
       before do
-        allow(Group).to receive(:supports_nested_objects?) { true }
-        visit path
+        restricted_group.add_owner(owner)
+        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', :nested_groups do
+        before do
+          allow(Group).to receive(:supports_nested_objects?) { true }
+        end
+
+        it 'allows creating subgroups' do
+          visit path
+
+          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 }
+        end
+
+        it 'does not allow creating subgroups' do
+          visit path
+
+          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
 
-      it 'allows creating subgroups' do
-        expect(page).not_to have_selector("li[data-text='New subgroup']", visible: false)
+      context 'when subgroups are supported', :nested_groups do
+        before do
+          allow(Group).to receive(:supports_nested_objects?) { true }
+        end
+
+        context 'when subgroup_creation_level is set to maintainers' do
+          before do
+            relaxed_group.add_maintainer(maintainer)
+          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
+        end
+
+        context 'when subgroup_creation_level is set to owners' do
+          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_selector("li[data-text='New subgroup']",
+                                    visible: false)
+          end
+        end
+      end
+
+      context 'when subgroups are not supported' do
+        before do
+          allow(Group).to receive(:supports_nested_objects?) { false }
+        end
+
+        it 'does not allow creating subgroups' do
+          visit path
+
+          expect(page)
+            .not_to have_selector("li[data-text='New subgroup']", visible: false)
+        end
       end
     end
   end

spec/models/group_spec.rb

@@ -994,4 +994,11 @@ 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
+      expect(group.subgroup_creation_level)
+        .to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
+    end
+  end
 end

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) 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)
+
+        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 'allows every maintainer permission' 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
 
@@ -145,7 +171,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,16 +184,32 @@ 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)
       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
-    let(:nested_group) { create(:group, :private, parent: group) }
+    let(:nested_group) do
+      create(:group, :private, :owner_subgroup_creation_only, parent: group)
+    end
 
     before do
       nested_group.add_guest(guest)
@@ -461,6 +504,72 @@ describe GroupPolicy do
     end
   end
 
+  context "create_subgroup" do
+    context 'when group has subgroup creation level set to owner' do
+      let(:group) do
+        create(
+          :group,
+          subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS)
+      end
+
+      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) do
+        create(
+          :group,
+          subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
+      end
+
+      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

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

spec/services/groups/create_service_spec.rb

@@ -87,6 +87,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
 

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[