summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/groups_controller.rb7
-rw-r--r--app/policies/group_policy.rb4
-rw-r--r--app/services/groups/create_service.rb4
-rw-r--r--app/views/shared/_group_form.html.haml5
-rw-r--r--changelogs/unreleased/35845-improve-subgroup-creation-permissions.yml5
-rw-r--r--spec/features/groups_spec.rb17
-rw-r--r--spec/policies/group_policy_spec.rb30
-rw-r--r--spec/services/groups/create_service_spec.rb14
8 files changed, 74 insertions, 12 deletions
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index f76b3f69e9e..994e736d66e 100644
--- a/app/controllers/groups_controller.rb
+++ b/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
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 6defab75fce..8ada661e571 100644
--- a/app/policies/group_policy.rb
+++ b/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
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index c4e9b8fd8e0..c7c27621085 100644
--- a/app/services/groups/create_service.rb
+++ b/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
diff --git a/app/views/shared/_group_form.html.haml b/app/views/shared/_group_form.html.haml
index 8d5b5129454..2e1bd5a088c 100644
--- a/app/views/shared/_group_form.html.haml
+++ b/app/views/shared/_group_form.html.haml
@@ -1,6 +1,6 @@
- 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
diff --git a/changelogs/unreleased/35845-improve-subgroup-creation-permissions.yml b/changelogs/unreleased/35845-improve-subgroup-creation-permissions.yml
new file mode 100644
index 00000000000..eac8dbe23c2
--- /dev/null
+++ b/changelogs/unreleased/35845-improve-subgroup-creation-permissions.yml
@@ -0,0 +1,5 @@
+---
+title: Improves subgroup creation permissions
+merge_request: 13418
+author:
+type: bugifx
diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb
index e59a484d992..20f9818b08b 100644
--- a/spec/features/groups_spec.rb
+++ b/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
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index b17a93e3fbe..cf420ae3ea6 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/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) }
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index b2175717a70..6973e7ff990 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/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
View Lorry Controller Status