Merge branch '35845-improve-subgroup-creation-permissions' into 'master'

Improves subgroup creation permissions

Closes #35845

See merge request !13418

app/controllers/groups_controller.rb

@@ -26,6 +26,13 @@ class GroupsController < Groups::ApplicationController
 
   def new
     @group = Group.new
+
+    if params[:parent_id].present?
+      parent = Group.find_by(id: params[:parent_id])
+      if can?(current_user, :create_subgroup, parent)
+        @group.parent = parent
+      end
+    end
   end
 
   def create

app/policies/group_policy.rb

@@ -13,6 +13,8 @@ class GroupPolicy < BasePolicy
   condition(:master) { access_level >= GroupMember::MASTER }
   condition(:reporter) { access_level >= GroupMember::REPORTER }
 
+  condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? }
+
   condition(:has_projects) do
     GroupProjectsFinder.new(group: @subject, current_user: @user).execute.any?
   end
@@ -42,7 +44,7 @@ class GroupPolicy < BasePolicy
     enable :change_visibility_level
   end
 
-  rule { owner & can_create_group }.enable :create_subgroup
+  rule { owner & can_create_group & nested_groups_supported }.enable :create_subgroup
 
   rule { public_group | logged_in_viewable }.enable :view_globally
 

app/services/groups/create_service.rb

@@ -13,9 +13,9 @@ module Groups
         return @group
       end
 
-      if @group.parent && !can?(current_user, :admin_group, @group.parent)
+      if @group.parent && !can?(current_user, :create_subgroup, @group.parent)
         @group.parent = nil
-        @group.errors.add(:parent_id, 'manage access required to create subgroup')
+        @group.errors.add(:parent_id, 'You don’t have permission to create a subgroup in this group.')
 
         return @group
       end

app/views/shared/_group_form.html.haml

 - content_for :page_specific_javascripts do
   = page_specific_javascript_bundle_tag('group')
-- parent = GroupFinder.new(current_user).execute(id: params[:parent_id] || @group.parent_id)
+- parent = @group.parent
 - group_path = root_url
 - group_path << parent.full_path + '/' if parent
 
@@ -13,13 +13,12 @@
         %span>= root_url
         - if parent
           %strong= parent.full_path + '/'
+      = f.hidden_field :parent_id
       = f.text_field :path, placeholder: 'open-source', class: 'form-control',
         autofocus: local_assigns[:autofocus] || false, required: true,
         pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS,
         title: 'Please choose a group path with no special characters.',
         "data-bind-in" => "#{'create_chat_team' if Gitlab.config.mattermost.enabled}"
-      - if parent
-        = f.hidden_field :parent_id, value: parent.id
 
     - if @group.persisted?
       .alert.alert-warning.prepend-top-10

changelogs/unreleased/35845-improve-subgroup-creation-permissions.yml

+---
+title: Improves subgroup creation permissions
+merge_request: 13418
+author:
+type: bugifx

spec/features/groups_spec.rb

@@ -104,18 +104,15 @@ feature 'Group' do
     end
 
     context 'as group owner' do
-      let(:user) { create(:user) }
+      it 'creates a nested group' do
+        user = create(:user)
 
-      before do
         group.add_owner(user)
         sign_out(:user)
         sign_in(user)
 
         visit subgroups_group_path(group)
         click_link 'New Subgroup'
-      end
-
-      it 'creates a nested group' do
         fill_in 'Group path', with: 'bar'
         click_button 'Create group'
 
@@ -123,6 +120,16 @@ feature 'Group' do
         expect(page).to have_content("Group 'bar' was successfully created.")
       end
     end
+
+    context 'when nested group feature is disabled' do
+      it 'renders 404' do
+        allow(Group).to receive(:supports_nested_groups?).and_return(false)
+
+        visit subgroups_group_path(group)
+
+        expect(page.status_code).to eq(404)
+      end
+    end
   end
 
   it 'checks permissions to avoid exposing groups by parent_id' do

spec/policies/group_policy_spec.rb

@@ -123,6 +123,36 @@ describe GroupPolicy do
     end
   end
 
+  describe 'when nested group support feature is disabled' do
+    before do
+      allow(Group).to receive(:supports_nested_groups?).and_return(false)
+    end
+
+    context 'admin' do
+      let(:current_user) { admin }
+
+      it 'allows every owner permission except creating subgroups' do
+        create_subgroup_permission = [:create_subgroup]
+        updated_owner_permissions = owner_permissions - create_subgroup_permission
+
+        expect_disallowed(*create_subgroup_permission)
+        expect_allowed(*updated_owner_permissions)
+      end
+    end
+
+    context 'owner' do
+      let(:current_user) { owner }
+
+      it 'allows every owner permission except creating subgroups' do
+        create_subgroup_permission = [:create_subgroup]
+        updated_owner_permissions = owner_permissions - create_subgroup_permission
+
+        expect_disallowed(*create_subgroup_permission)
+        expect_allowed(*updated_owner_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) }
 

spec/services/groups/create_service_spec.rb

@@ -32,12 +32,24 @@ describe Groups::CreateService, '#execute' do
       end
 
       it { is_expected.to be_persisted }
+
+      context 'when nested groups feature is disabled' do
+        it 'does not save group and returns an error' do
+          allow(Group).to receive(:supports_nested_groups?).and_return(false)
+
+          is_expected.not_to be_persisted
+          expect(subject.errors[:parent_id]).to include('You don’t have permission to create a subgroup in this group.')
+          expect(subject.parent_id).to be_nil
+        end
+      end
     end
 
     context 'as guest' do
       it 'does not save group and returns an error' do
+        allow(Group).to receive(:supports_nested_groups?).and_return(true)
+
         is_expected.not_to be_persisted
-        expect(subject.errors[:parent_id].first).to eq('manage access required to create subgroup')
+        expect(subject.errors[:parent_id].first).to eq('You don’t have permission to create a subgroup in this group.')
         expect(subject.parent_id).to be_nil
       end
     end