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

10 files changed, 249 insertions, 26 deletions

diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index 509d8944e3a..1123563c1e3 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/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
diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb
index 18a0c2ec731..b67ab955ffc 100644
--- a/spec/factories/groups.rb
+++ b/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
diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb
index bac2972b0fb..a08902c30be 100644
--- a/spec/features/admin/admin_groups_spec.rb
+++ b/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)
 
diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb
index 5cef5f0521f..676769c25fe 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/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb
index 9671a4d8c49..bed998a0859 100644
--- a/spec/features/groups/show_spec.rb
+++ b/spec/features/groups/show_spec.rb
@@ -56,32 +56,103 @@ describe 'Group show page' do
   end
 
   context 'subgroup support' do
-    let(:user) { create(:user) }
+    let(:restricted_group) do
+      create(:group, subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS)
+    end
 
-    before do
-      group.add_owner(user)
-      sign_in(user)
+    let(:relaxed_group) do
+      create(:group, subgroup_creation_level: ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
     end
 
-    context 'when subgroups are supported', :js, :nested_groups do
+    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
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 470ce65707d..c7fb0f51075 100644
--- a/spec/models/group_spec.rb
+++ b/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
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 59f3a961d50..dc3675a7b9e 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) 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
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/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index c5ff6cdbacd..a7c95428485 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/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
 
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[