diff options
| -rw-r--r-- | app/controllers/groups_controller.rb | 22 | ||||
| -rw-r--r-- | app/policies/group_policy.rb | 2 | ||||
| -rw-r--r-- | app/services/groups/create_service.rb | 38 | ||||
| -rw-r--r-- | lib/api/groups.rb | 7 | ||||
| -rw-r--r-- | lib/api/helpers.rb | 2 | ||||
| -rw-r--r-- | spec/controllers/groups_controller_spec.rb | 124 | ||||
| -rw-r--r-- | spec/policies/group_policy_spec.rb | 4 | ||||
| -rw-r--r-- | spec/requests/api/groups_spec.rb | 24 | ||||
| -rw-r--r-- | spec/services/groups/create_service_spec.rb | 43 |
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 |