Commit 62bb6235 authored by Ruben Davila's avatar Ruben Davila

Make Members with Owner and Master roles always able to create subgroups

parent bc955cfc
...@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController ...@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController
# Authorize # Authorize
before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects] 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_projects, only: [:projects, :activity, :issues, :merge_requests]
before_action :group_merge_requests, only: [:merge_requests] before_action :group_merge_requests, only: [:merge_requests]
...@@ -25,14 +25,7 @@ class GroupsController < Groups::ApplicationController ...@@ -25,14 +25,7 @@ class GroupsController < Groups::ApplicationController
end end
def new def new
@group = Group.new @group = Group.new(params.permit(:parent_id))
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 end
def create def create
...@@ -128,9 +121,14 @@ class GroupsController < Groups::ApplicationController ...@@ -128,9 +121,14 @@ class GroupsController < Groups::ApplicationController
end end
def authorize_create_group! def authorize_create_group!
unless can?(current_user, :create_group) allowed = if params[:parent_id].present?
return render_404 parent = Group.find_by(id: params[:parent_id])
end can?(current_user, :create_subgroup, parent)
else
can?(current_user, :create_group)
end
render_404 unless allowed
end end
def determine_layout def determine_layout
......
...@@ -49,7 +49,7 @@ class GroupPolicy < BasePolicy ...@@ -49,7 +49,7 @@ class GroupPolicy < BasePolicy
enable :change_visibility_level enable :change_visibility_level
end 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 rule { public_group | logged_in_viewable }.enable :view_globally
......
...@@ -8,15 +8,7 @@ module Groups ...@@ -8,15 +8,7 @@ module Groups
def execute def execute
@group = Group.new(params) @group = Group.new(params)
unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) unless can_use_visibility_level? && can_create_group?
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.')
return @group return @group
end end
...@@ -39,5 +31,33 @@ module Groups ...@@ -39,5 +31,33 @@ module Groups
def create_chat_team? def create_chat_team?
Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil? Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil?
end 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
end end
...@@ -74,7 +74,12 @@ module API ...@@ -74,7 +74,12 @@ module API
use :optional_params use :optional_params
end end
post do 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 group = ::Groups::CreateService.new(current_user, declared_params(include_missing: false)).execute
......
...@@ -93,7 +93,7 @@ module API ...@@ -93,7 +93,7 @@ module API
end end
def find_group(id) def find_group(id)
if id =~ /^\d+$/ if id.to_s =~ /^\d+$/
Group.find_by(id: id) Group.find_by(id: id)
else else
Group.find_by_full_path(id) Group.find_by_full_path(id)
......
...@@ -2,9 +2,133 @@ require 'rails_helper' ...@@ -2,9 +2,133 @@ require 'rails_helper'
describe GroupsController do describe GroupsController do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:admin) { create(:admin) }
let(:group) { create(:group, :public) } let(:group) { create(:group, :public) }
let(:project) { create(:project, namespace: group) } let(:project) { create(:project, namespace: group) }
let!(:group_member) { create(:group_member, group: group, user: user) } 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 describe 'GET #index' do
context 'as a user' do context 'as a user' do
......
...@@ -24,8 +24,8 @@ describe GroupPolicy do ...@@ -24,8 +24,8 @@ describe GroupPolicy do
:admin_namespace, :admin_namespace,
:admin_group_member, :admin_group_member,
:change_visibility_level, :change_visibility_level,
:create_subgroup (Gitlab::Database.postgresql? ? :create_subgroup : nil)
] ].compact
end end
before do before do
......
...@@ -431,6 +431,30 @@ describe API::Groups do ...@@ -431,6 +431,30 @@ describe API::Groups do
expect(response).to have_http_status(403) expect(response).to have_http_status(403)
end 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 end
context "when authenticated as user with group permissions" do context "when authenticated as user with group permissions" do
......
...@@ -22,6 +22,26 @@ describe Groups::CreateService, '#execute' do ...@@ -22,6 +22,26 @@ describe Groups::CreateService, '#execute' do
end end
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 describe 'creating subgroup', :nested_groups do
let!(:group) { create(:group) } let!(:group) { create(:group) }
let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) }
...@@ -44,13 +64,26 @@ describe Groups::CreateService, '#execute' do ...@@ -44,13 +64,26 @@ describe Groups::CreateService, '#execute' do
end end
end end
context 'as guest' do context 'when nested groups feature is enabled' do
it 'does not save group and returns an error' do before do
allow(Group).to receive(:supports_nested_groups?).and_return(true) 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 it { is_expected.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
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment