FIX: limit number of users addable to group at once (#10510)

When someone wants to add > 1000 users at once they will hit a timeout.
Therefore, we should introduce limit and inform the user when limit is exceeded.
This commit is contained in:
Krzysztof Kotlarek 2020-08-25 08:55:21 +10:00 committed by GitHub
parent ed30f49315
commit 7b6f8517bf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 34 additions and 0 deletions

View File

@ -36,6 +36,7 @@ class GroupsController < ApplicationController
groups.where(automatic: true) groups.where(automatic: true)
} }
} }
ADD_MEMBERS_LIMIT = 1000
def index def index
unless SiteSetting.enable_group_directory? || current_user&.staff? unless SiteSetting.enable_group_directory? || current_user&.staff?
@ -329,6 +330,11 @@ class GroupsController < ApplicationController
'usernames or emails must be present' 'usernames or emails must be present'
) )
end end
if users.length > ADD_MEMBERS_LIMIT
return render_json_error(
I18n.t("groups.errors.adding_too_many_users", limit: ADD_MEMBERS_LIMIT)
)
end
usernames_already_in_group = group.users.where(id: users.map(&:id)).pluck(:username) usernames_already_in_group = group.users.where(id: users.map(&:id)).pluck(:username)
if usernames_already_in_group.present? && usernames_already_in_group.length == users.length if usernames_already_in_group.present? && usernames_already_in_group.length == users.length
render_json_error(I18n.t( render_json_error(I18n.t(

View File

@ -411,6 +411,7 @@ en:
email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'." email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'."
cant_allow_membership_requests: "You cannot allow membership requests for a group without any owners." cant_allow_membership_requests: "You cannot allow membership requests for a group without any owners."
already_requested_membership: "You have already requested membership for this group." already_requested_membership: "You have already requested membership for this group."
adding_too_many_users: "Maximum %{limit} users can be added at once"
default_names: default_names:
everyone: "everyone" everyone: "everyone"
admins: "admins" admins: "admins"

View File

@ -1245,6 +1245,33 @@ describe GroupsController do
count: 3 count: 3
)) ))
end end
it 'display error when try to add to many users at once' do
begin
GroupsController.send(:remove_const, "ADD_MEMBERS_LIMIT")
GroupsController.const_set("ADD_MEMBERS_LIMIT", 4)
user1.update!(username: 'john')
user2.update!(username: 'alice')
user3 = Fabricate(:user, username: 'bob')
user4 = Fabricate(:user, username: 'anna')
user5 = Fabricate(:user, username: 'sarah')
expect do
put "/groups/#{group.id}/members.json",
params: { user_emails: [user1.email, user2.email, user3.email, user4.email, user5.email].join(",") }
end.to change { group.users.count }.by(0)
expect(response.status).to eq(422)
expect(response.parsed_body["errors"]).to include(I18n.t(
"groups.errors.adding_too_many_users",
limit: 4
))
ensure
GroupsController.send(:remove_const, "ADD_MEMBERS_LIMIT")
GroupsController.const_set("ADD_MEMBERS_LIMIT", 1000)
end
end
end end
it "returns 422 if member already exists" do it "returns 422 if member already exists" do