summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-06-06 20:45:34 +0000
committerDouwe Maan <douwe@gitlab.com>2017-06-06 20:45:34 +0000
commit71f9c43c833531fb6e2231225b74a42dea4ae00c (patch)
treee1e4dac0d9f100c4cf10d265a926065e1d17a07b
parentd1c2d36f0325c79aed9b5ae2e6396591c6ced3be (diff)
parent5c602e306cdf979a70aaa81cd473f491f2eee45a (diff)
downloadgitlab-ce-71f9c43c833531fb6e2231225b74a42dea4ae00c.tar.gz
Merge branch '27148-limit-bulk-create-memberships' into 'master'
Limit non-administrators to adding 100 members at a time to groups and projects Closes #27148 See merge request !11940
-rw-r--r--app/controllers/admin/groups_controller.rb7
-rw-r--r--app/controllers/concerns/membership_actions.rb7
-rw-r--r--app/services/members/create_service.rb22
-rw-r--r--changelogs/unreleased/27148-limit-bulk-create-memberships.yml4
-rw-r--r--spec/controllers/admin/groups_controller_spec.rb9
-rw-r--r--spec/controllers/projects/project_members_controller_spec.rb6
-rw-r--r--spec/services/members/create_service_spec.rb16
7 files changed, 57 insertions, 14 deletions
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 5885b3543bb..5a2a7c7f27b 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -43,12 +43,13 @@ class Admin::GroupsController < Admin::ApplicationController
end
def members_update
- status = Members::CreateService.new(@group, current_user, params).execute
+ member_params = params.permit(:user_ids, :access_level, :expires_at)
+ result = Members::CreateService.new(@group, current_user, member_params.merge(limit: -1)).execute
- if status
+ if result[:status] == :success
redirect_to [:admin, @group], notice: 'Users were successfully added.'
else
- redirect_to [:admin, @group], alert: 'No users specified.'
+ redirect_to [:admin, @group], alert: result[:message]
end
end
diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index b1bacc8ffe5..cefb9b4e766 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -2,14 +2,15 @@ module MembershipActions
extend ActiveSupport::Concern
def create
- status = Members::CreateService.new(membershipable, current_user, params).execute
+ create_params = params.permit(:user_ids, :access_level, :expires_at)
+ result = Members::CreateService.new(membershipable, current_user, create_params).execute
redirect_url = members_page_url
- if status
+ if result[:status] == :success
redirect_to redirect_url, notice: 'Users were successfully added.'
else
- redirect_to redirect_url, alert: 'No users specified.'
+ redirect_to redirect_url, alert: result[:message]
end
end
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 3a58f6c065d..26906ae7167 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -1,22 +1,38 @@
module Members
class CreateService < BaseService
+ DEFAULT_LIMIT = 100
+
def initialize(source, current_user, params = {})
@source = source
@current_user = current_user
@params = params
+ @error = nil
end
def execute
- return false if params[:user_ids].blank?
+ return error('No users specified.') if params[:user_ids].blank?
+
+ user_ids = params[:user_ids].split(',').uniq
+
+ return error("Too many users specified (limit is #{user_limit})") if
+ user_limit && user_ids.size > user_limit
@source.add_users(
- params[:user_ids].split(','),
+ user_ids,
params[:access_level],
expires_at: params[:expires_at],
current_user: current_user
)
- true
+ success
+ end
+
+ private
+
+ def user_limit
+ limit = params.fetch(:limit, DEFAULT_LIMIT)
+
+ limit && limit < 0 ? nil : limit
end
end
end
diff --git a/changelogs/unreleased/27148-limit-bulk-create-memberships.yml b/changelogs/unreleased/27148-limit-bulk-create-memberships.yml
new file mode 100644
index 00000000000..ac4aba2f4e0
--- /dev/null
+++ b/changelogs/unreleased/27148-limit-bulk-create-memberships.yml
@@ -0,0 +1,4 @@
+---
+title: Limit non-administrators to adding 100 members at a time to groups and projects
+merge_request: 11940
+author:
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index c29b2fe8946..ddf38967dd7 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -36,6 +36,15 @@ describe Admin::GroupsController do
expect(group.users).to include group_user
end
+ it 'can add unlimited members' do
+ put :members_update, id: group,
+ user_ids: 1.upto(1000).to_a.join(','),
+ access_level: Gitlab::Access::GUEST
+
+ expect(response).to set_flash.to 'Users were successfully added.'
+ expect(response).to redirect_to(admin_group_path(group))
+ end
+
it 'adds no user to members' do
put :members_update, id: group,
user_ids: '',
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index a4b4392d7cc..2294d5df581 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -36,7 +36,7 @@ describe Projects::ProjectMembersController do
before { project.team << [user, :master] }
it 'adds user to members' do
- expect_any_instance_of(Members::CreateService).to receive(:execute).and_return(true)
+ expect_any_instance_of(Members::CreateService).to receive(:execute).and_return(status: :success)
post :create, namespace_id: project.namespace,
project_id: project,
@@ -48,14 +48,14 @@ describe Projects::ProjectMembersController do
end
it 'adds no user to members' do
- expect_any_instance_of(Members::CreateService).to receive(:execute).and_return(false)
+ expect_any_instance_of(Members::CreateService).to receive(:execute).and_return(status: :failure, message: 'Message')
post :create, namespace_id: project.namespace,
project_id: project,
user_ids: '',
access_level: Gitlab::Access::GUEST
- expect(response).to set_flash.to 'No users specified.'
+ expect(response).to set_flash.to 'Message'
expect(response).to redirect_to(namespace_project_settings_members_path(project.namespace, project))
end
end
diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb
index 0670ac2faa2..5ce8e17976b 100644
--- a/spec/services/members/create_service_spec.rb
+++ b/spec/services/members/create_service_spec.rb
@@ -11,7 +11,7 @@ describe Members::CreateService, services: true do
params = { user_ids: project_user.id.to_s, access_level: Gitlab::Access::GUEST }
result = described_class.new(project, user, params).execute
- expect(result).to be_truthy
+ expect(result[:status]).to eq(:success)
expect(project.users).to include project_user
end
@@ -19,7 +19,19 @@ describe Members::CreateService, services: true do
params = { user_ids: '', access_level: Gitlab::Access::GUEST }
result = described_class.new(project, user, params).execute
- expect(result).to be_falsey
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to be_present
+ expect(project.users).not_to include project_user
+ end
+
+ it 'limits the number of users to 100' do
+ user_ids = 1.upto(101).to_a.join(',')
+ params = { user_ids: user_ids, access_level: Gitlab::Access::GUEST }
+
+ result = described_class.new(project, user, params).execute
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to be_present
expect(project.users).not_to include project_user
end
end