summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-09-07 22:11:20 +0000
committerDouwe Maan <douwe@gitlab.com>2017-09-07 22:11:20 +0000
commit3955dcb4cc4cf305282412988751155771bbc036 (patch)
treea58615fe3f10662977b2eff3b741841bd5518b4c
parentaad764871d8f9433b9ec1d3d4eb096aba870bfac (diff)
parent62bb6235c229a869052180f9709c4801116f02cc (diff)
downloadgitlab-ce-3955dcb4cc4cf305282412988751155771bbc036.tar.gz
Merge branch '30473-allow-creation-of-subgroups-with-gitlab_default_can_create_group-set-to-false' into 'master'
Make Members with Owner and Master roles always able to create subgroups Closes #30473 See merge request !14046
-rw-r--r--app/controllers/groups_controller.rb22
-rw-r--r--app/policies/group_policy.rb2
-rw-r--r--app/services/groups/create_service.rb38
-rw-r--r--lib/api/groups.rb7
-rw-r--r--lib/api/helpers.rb2
-rw-r--r--spec/controllers/groups_controller_spec.rb124
-rw-r--r--spec/policies/group_policy_spec.rb4
-rw-r--r--spec/requests/api/groups_spec.rb24
-rw-r--r--spec/services/groups/create_service_spec.rb43
9 files changed, 235 insertions, 31 deletions
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 994e736d66e..3769a2cde33 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController
# Authorize
before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
- before_action :authorize_create_group!, only: [:new, :create]
+ before_action :authorize_create_group!, only: [:new]
before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests]
before_action :group_merge_requests, only: [:merge_requests]
@@ -25,14 +25,7 @@ class GroupsController < Groups::ApplicationController
end
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
+ @group = Group.new(params.permit(:parent_id))
end
def create
@@ -128,9 +121,14 @@ class GroupsController < Groups::ApplicationController
end
def authorize_create_group!
- unless can?(current_user, :create_group)
- return render_404
- end
+ allowed = if params[:parent_id].present?
+ parent = Group.find_by(id: params[:parent_id])
+ can?(current_user, :create_subgroup, parent)
+ else
+ can?(current_user, :create_group)
+ end
+
+ render_404 unless allowed
end
def determine_layout
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index d9fd6501419..420991ff6d6 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -49,7 +49,7 @@ class GroupPolicy < BasePolicy
enable :change_visibility_level
end
- rule { owner & can_create_group & nested_groups_supported }.enable :create_subgroup
+ rule { owner & 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 c7c27621085..70e50aa0f12 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -8,15 +8,7 @@ module Groups
def execute
@group = Group.new(params)
- unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
- deny_visibility_level(@group)
- return @group
- end
-
- if @group.parent && !can?(current_user, :create_subgroup, @group.parent)
- @group.parent = nil
- @group.errors.add(:parent_id, 'You don’t have permission to create a subgroup in this group.')
-
+ unless can_use_visibility_level? && can_create_group?
return @group
end
@@ -39,5 +31,33 @@ module Groups
def create_chat_team?
Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil?
end
+
+ def can_create_group?
+ if @group.subgroup?
+ unless can?(current_user, :create_subgroup, @group.parent)
+ @group.parent = nil
+ @group.errors.add(:parent_id, 'You don’t have permission to create a subgroup in this group.')
+
+ return false
+ end
+ else
+ unless can?(current_user, :create_group)
+ @group.errors.add(:base, 'You don’t have permission to create groups.')
+
+ return false
+ end
+ end
+
+ true
+ end
+
+ def can_use_visibility_level?
+ unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+ deny_visibility_level(@group)
+ return false
+ end
+
+ true
+ end
end
end
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 31a918eda60..e817dcbbc4b 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -74,7 +74,12 @@ module API
use :optional_params
end
post do
- authorize! :create_group
+ parent_group = find_group!(params[:parent_id]) if params[:parent_id].present?
+ if parent_group
+ authorize! :create_subgroup, parent_group
+ else
+ authorize! :create_group
+ end
group = ::Groups::CreateService.new(current_user, declared_params(include_missing: false)).execute
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 8b03df65ae4..00dbc2aee7a 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -93,7 +93,7 @@ module API
end
def find_group(id)
- if id =~ /^\d+$/
+ if id.to_s =~ /^\d+$/
Group.find_by(id: id)
else
Group.find_by_full_path(id)
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index c2ada8c8df7..b0564e27a68 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -2,9 +2,133 @@ require 'rails_helper'
describe GroupsController do
let(:user) { create(:user) }
+ let(:admin) { create(:admin) }
let(:group) { create(:group, :public) }
let(:project) { create(:project, namespace: group) }
let!(:group_member) { create(:group_member, group: group, user: user) }
+ let!(:owner) { group.add_owner(create(:user)).user }
+ let!(:master) { group.add_master(create(:user)).user }
+ let!(:developer) { group.add_developer(create(:user)).user }
+ let!(:guest) { group.add_guest(create(:user)).user }
+
+ shared_examples 'member with ability to create subgroups' do
+ it 'renders the new page' do
+ sign_in(member)
+
+ get :new, parent_id: group.id
+
+ expect(response).to render_template(:new)
+ end
+ end
+
+ shared_examples 'member without ability to create subgroups' do
+ it 'renders the 404 page' do
+ sign_in(member)
+
+ get :new, parent_id: group.id
+
+ expect(response).not_to render_template(:new)
+ expect(response.status).to eq(404)
+ end
+ end
+
+ describe 'GET #new' do
+ context 'when creating subgroups', :nested_groups do
+ [true, false].each do |can_create_group_status|
+ context "and can_create_group is #{can_create_group_status}" do
+ before do
+ User.where(id: [admin, owner, master, developer, guest]).update_all(can_create_group: can_create_group_status)
+ end
+
+ [:admin, :owner].each do |member_type|
+ context "and logged in as #{member_type.capitalize}" do
+ it_behaves_like 'member with ability to create subgroups' do
+ let(:member) { send(member_type) }
+ end
+ end
+ end
+
+ [:guest, :developer, :master].each do |member_type|
+ context "and logged in as #{member_type.capitalize}" do
+ it_behaves_like 'member without ability to create subgroups' do
+ let(:member) { send(member_type) }
+ end
+ end
+ end
+ end
+ end
+ end
+ end
+
+ describe 'POST #create' do
+ context 'when creating subgroups', :nested_groups do
+ [true, false].each do |can_create_group_status|
+ context "and can_create_group is #{can_create_group_status}" do
+ context 'and logged in as Owner' do
+ it 'creates the subgroup' do
+ owner.update_attribute(:can_create_group, can_create_group_status)
+ sign_in(owner)
+
+ post :create, group: { parent_id: group.id, path: 'subgroup' }
+
+ expect(response).to be_redirect
+ expect(response.body).to match(%r{http://test.host/#{group.path}/subgroup})
+ end
+ end
+
+ context 'and logged in as Developer' do
+ it 'renders the new template' do
+ developer.update_attribute(:can_create_group, can_create_group_status)
+ sign_in(developer)
+
+ previous_group_count = Group.count
+
+ post :create, group: { parent_id: group.id, path: 'subgroup' }
+
+ expect(response).to render_template(:new)
+ expect(Group.count).to eq(previous_group_count)
+ end
+ end
+ end
+ end
+ end
+
+ context 'when creating a top level group' do
+ before do
+ sign_in(developer)
+ end
+
+ context 'and can_create_group is enabled' do
+ before do
+ developer.update_attribute(:can_create_group, true)
+ end
+
+ it 'creates the Group' do
+ original_group_count = Group.count
+
+ post :create, group: { path: 'subgroup' }
+
+ expect(Group.count).to eq(original_group_count + 1)
+ expect(response).to be_redirect
+ end
+ end
+
+ context 'and can_create_group is disabled' do
+ before do
+ developer.update_attribute(:can_create_group, false)
+ end
+
+ it 'does not create the Group' do
+ original_group_count = Group.count
+
+ post :create, group: { path: 'subgroup' }
+
+ expect(Group.count).to eq(original_group_count)
+ expect(response).to render_template(:new)
+ end
+ end
+ end
+ end
describe 'GET #index' do
context 'as a user' do
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 0c4044dc7ab..b186a78e44a 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -24,8 +24,8 @@ describe GroupPolicy do
:admin_namespace,
:admin_group_member,
:change_visibility_level,
- :create_subgroup
- ]
+ (Gitlab::Database.postgresql? ? :create_subgroup : nil)
+ ].compact
end
before do
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 77c43f92456..42f0079e173 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -431,6 +431,30 @@ describe API::Groups do
expect(response).to have_http_status(403)
end
+
+ context 'as owner', :nested_groups do
+ before do
+ group2.add_owner(user1)
+ end
+
+ it 'can create subgroups' do
+ post api("/groups", user1), parent_id: group2.id, name: 'foo', path: 'foo'
+
+ expect(response).to have_http_status(201)
+ end
+ end
+
+ context 'as master', :nested_groups do
+ before do
+ group2.add_master(user1)
+ end
+
+ it 'cannot create subgroups' do
+ post api("/groups", user1), parent_id: group2.id, name: 'foo', path: 'foo'
+
+ expect(response).to have_http_status(403)
+ end
+ end
end
context "when authenticated as user with group permissions" do
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index 10dda45d2a1..224e933bebc 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -22,6 +22,26 @@ describe Groups::CreateService, '#execute' do
end
end
+ describe 'creating a top level group' do
+ let(:service) { described_class.new(user, group_params) }
+
+ context 'when user can create a group' do
+ before do
+ user.update_attribute(:can_create_group, true)
+ end
+
+ it { is_expected.to be_persisted }
+ end
+
+ context 'when user can not create a group' do
+ before do
+ user.update_attribute(:can_create_group, false)
+ end
+
+ it { is_expected.not_to be_persisted }
+ end
+ end
+
describe 'creating subgroup', :nested_groups do
let!(:group) { create(:group) }
let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) }
@@ -44,13 +64,26 @@ describe Groups::CreateService, '#execute' do
end
end
- context 'as guest' do
- it 'does not save group and returns an error' do
+ context 'when nested groups feature is enabled' do
+ before do
allow(Group).to receive(:supports_nested_groups?).and_return(true)
+ end
+
+ context 'as guest' do
+ it 'does not save group and returns an error' do
+ is_expected.not_to be_persisted
+
+ 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
+
+ context 'as owner' do
+ before do
+ group.add_owner(user)
+ end
- is_expected.not_to be_persisted
- 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
+ it { is_expected.to be_persisted }
end
end
end