Skip to content

Commit

Permalink
FIX: limit number of users addable to group at once
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lis2 committed Aug 24, 2020
1 parent 62f4fc7 commit 6bd92a5
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 0 deletions.
6 changes: 6 additions & 0 deletions app/controllers/groups_controller.rb
Expand Up @@ -36,6 +36,7 @@ class GroupsController < ApplicationController
groups.where(automatic: true)
}
}
ADD_MEMBERS_LIMIT = 1000

def index
unless SiteSetting.enable_group_directory? || current_user&.staff?
Expand Down Expand Up @@ -329,6 +330,11 @@ def add_members
'usernames or emails must be present'
)
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)
if usernames_already_in_group.present? && usernames_already_in_group.length == users.length
render_json_error(I18n.t(
Expand Down
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Expand Up @@ -411,6 +411,7 @@ en:
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."
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:
everyone: "everyone"
admins: "admins"
Expand Down
22 changes: 22 additions & 0 deletions spec/requests/groups_controller_spec.rb
Expand Up @@ -1245,6 +1245,28 @@ def expect_type_to_return_right_groups(type, expected_group_ids)
count: 3
))
end

it 'display error when try to add to many users at once' do
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
))
end
end

it "returns 422 if member already exists" do
Expand Down

0 comments on commit 6bd92a5

Please sign in to comment.