diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index be5f02f41ac..67c844dbe61 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -687,11 +687,12 @@ class TopicsController < ApplicationController def invite_group group = Group.find_by(name: params[:group]) - raise Discourse::NotFound unless group + raise Discourse::NotFound if !group topic = Topic.find_by(id: params[:topic_id]) + raise Discourse::NotFound if !topic - unless pm_has_slots?(topic) + if !pm_has_slots?(topic) return render_json_error( I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) ) @@ -708,29 +709,29 @@ class TopicsController < ApplicationController def invite topic = Topic.find_by(id: params[:topic_id]) - raise Discourse::InvalidParameters.new unless topic + raise Discourse::NotFound if !topic - username_or_email = params[:user] ? fetch_username : fetch_email + if !topic.private_message? + return render_json_error(I18n.t("topic_invite.not_pm")) + end - groups = Group.lookup_groups( - group_ids: params[:group_ids], - group_names: params[:group_names] - ) - - unless pm_has_slots?(topic) + if !pm_has_slots?(topic) return render_json_error( I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) ) end guardian.ensure_can_invite_to!(topic) - group_ids = groups.map(&:id) + + username_or_email = params[:user] ? fetch_username : fetch_email + group_ids = Group.lookup_groups( + group_ids: params[:group_ids], + group_names: params[:group_names] + ).pluck(:id) begin if topic.invite(current_user, username_or_email, group_ids, params[:custom_message]) - user = User.find_by_username_or_email(username_or_email) - - if user + if user = User.find_by_username_or_email(username_or_email) render_json_dump BasicUserSerializer.new(user, scope: guardian, root: 'user') else render json: success_json diff --git a/app/models/topic.rb b/app/models/topic.rb index 924b38f88cf..88578cdce4d 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1029,11 +1029,9 @@ class Topic < ActiveRecord::Base end def invite(invited_by, username_or_email, group_ids = nil, custom_message = nil) - target_user = User.find_by_username_or_email(username_or_email) guardian = Guardian.new(invited_by) - is_email = username_or_email =~ /^.+@.+$/ - if target_user + if target_user = User.find_by_username_or_email(username_or_email) if topic_allowed_users.exists?(user_id: target_user.id) raise UserExists.new(I18n.t("topic_invite.user_exists")) end @@ -1064,7 +1062,7 @@ class Topic < ActiveRecord::Base else !!invite_to_topic(invited_by, target_user, group_ids, guardian) end - elsif is_email && guardian.can_invite_via_email?(self) + elsif username_or_email =~ /^.+@.+$/ && guardian.can_invite_via_email?(self) !!Invite.generate(invited_by, email: username_or_email, topic: self, @@ -1781,6 +1779,13 @@ class Topic < ActiveRecord::Base SiteSetting.max_topic_invitations_per_day, 1.day.to_i ).performed! + + RateLimiter.new( + invited_by, + "topic-invitations-per-minute", + SiteSetting.max_topic_invitations_per_minute, + 1.day.to_i + ).performed! end def cannot_permanently_delete_reason(user) @@ -1853,8 +1858,9 @@ class Topic < ActiveRecord::Base )) end + rate_limit_topic_invitation(invited_by) + Topic.transaction do - rate_limit_topic_invitation(invited_by) topic_allowed_users.create!(user_id: target_user.id) unless topic_allowed_users.exists?(user_id: target_user.id) user_in_allowed_group = (user.group_ids & topic_allowed_groups.map(&:group_id)).present? diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 02a5f4e6935..eab63f07355 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -279,6 +279,7 @@ en: topic_invite: failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." + not_pm: "You can only invite to private messages." user_exists: "Sorry, that user has already been invited. You may only invite a user to a topic once." muted_topic: "Sorry, that user muted this topic." receiver_does_not_allow_pm: "Sorry, that user does not allow you to send them private messages." @@ -1791,6 +1792,7 @@ en: max_personal_messages_per_day: "Maximum number of new personal message topics a user can create per day." max_invites_per_day: "Maximum number of invites a user can send per day." max_topic_invitations_per_day: "Maximum number of topic invitations a user can send per day." + max_topic_invitations_per_minute: "Maximum number of topic invitations a user can send per minute." max_logins_per_ip_per_hour: "Maximum number of logins allowed per IP address per hour" max_logins_per_ip_per_minute: "Maximum number of logins allowed per IP address per minute" diff --git a/config/site_settings.yml b/config/site_settings.yml index 8243bd1a7bd..6ad7ebda537 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1893,6 +1893,7 @@ rate_limits: max_edits_per_day: 30 max_invites_per_day: 10 max_topic_invitations_per_day: 30 + max_topic_invitations_per_minute: 5 max_topics_in_first_day: 3 max_replies_in_first_day: 10 tl2_additional_likes_per_day_multiplier: 1.5 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 56abc419aed..3b569b901e6 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -723,7 +723,6 @@ RSpec.describe Topic do context 'with rate limits' do before do - SiteSetting.max_topic_invitations_per_day = 1 RateLimiter.enable Group.refresh_automatic_groups! end @@ -732,30 +731,68 @@ RSpec.describe Topic do RateLimiter.clear_all! end - it "rate limits topic invitations" do - start = Time.now.tomorrow.beginning_of_day - freeze_time(start) + context 'when per day' do + before do + SiteSetting.max_topic_invitations_per_day = 1 + end - topic = Fabricate(:topic, user: trust_level_2) + it "rate limits topic invitations" do + start = Time.now.tomorrow.beginning_of_day + freeze_time(start) - topic.invite(topic.user, user.username) + topic = Fabricate(:topic, user: trust_level_2) - expect { - topic.invite(topic.user, user1.username) - }.to raise_error(RateLimiter::LimitExceeded) + topic.invite(topic.user, user.username) + + expect { + topic.invite(topic.user, user1.username) + }.to raise_error(RateLimiter::LimitExceeded) + end + + it "rate limits PM invitations" do + start = Time.now.tomorrow.beginning_of_day + freeze_time(start) + + topic = Fabricate(:private_message_topic, user: trust_level_2) + + topic.invite(topic.user, user.username) + + expect { + topic.invite(topic.user, user1.username) + }.to raise_error(RateLimiter::LimitExceeded) + end end - it "rate limits PM invitations" do - start = Time.now.tomorrow.beginning_of_day - freeze_time(start) + context 'when per minute' do + before do + SiteSetting.max_topic_invitations_per_minute = 1 + end - topic = Fabricate(:private_message_topic, user: trust_level_2) + it "rate limits topic invitations" do + start = Time.now.tomorrow.beginning_of_minute + freeze_time(start) - topic.invite(topic.user, user.username) + topic = Fabricate(:topic, user: trust_level_2) - expect { - topic.invite(topic.user, user1.username) - }.to raise_error(RateLimiter::LimitExceeded) + topic.invite(topic.user, user.username) + + expect { + topic.invite(topic.user, user1.username) + }.to raise_error(RateLimiter::LimitExceeded) + end + + it "rate limits PM invitations" do + start = Time.now.tomorrow.beginning_of_minute + freeze_time(start) + + topic = Fabricate(:private_message_topic, user: trust_level_2) + + topic.invite(topic.user, user.username) + + expect { + topic.invite(topic.user, user1.username) + }.to raise_error(RateLimiter::LimitExceeded) + end end end diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index 79dcf58525c..ac690323c3f 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -229,7 +229,7 @@ RSpec.describe 'topics' do let(:username) { Fabricate(:user).username } let(:request_body) { { user: username } } - let(:id) { Fabricate(:topic).id } + let(:id) { Fabricate(:private_message_topic).id } run_test! do |response| data = JSON.parse(response.body) diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 212c55466d2..fe79743a80b 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -4047,77 +4047,17 @@ RSpec.describe TopicsController do sign_in(user) end - context 'as a valid user' do + context 'when topic id is not PM' do fab!(:user_topic) { Fabricate(:topic, user: user) } it 'should return the right response' do user.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) - expect do - post "/t/#{user_topic.id}/invite.json", params: { - email: 'someguy@email.com' - } - end.to change { Invite.where(invited_by_id: user.id).count }.by(1) - - expect(response.status).to eq(200) - expect(Jobs::InviteEmail.jobs.first['args'].first['invite_to_topic']).to be_truthy - end - end - - context 'when user is a group manager' do - fab!(:group) { Fabricate(:group).tap { |g| g.add_owner(user) } } - fab!(:private_category) { Fabricate(:private_category, group: group) } - - fab!(:group_private_topic) do - Fabricate(:topic, category: private_category, user: user) - end - - let!(:recipient) { 'jake@adventuretime.ooo' } - - before_all do - user.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) - end - - it "should attach group to the invite" do - post "/t/#{group_private_topic.id}/invite.json", params: { - user: recipient, - group_ids: "#{group.id},9999999" + post "/t/#{user_topic.id}/invite.json", params: { + email: 'someguy@email.com' } - expect(response.status).to eq(200) - expect(Invite.find_by(email: recipient).groups).to contain_exactly(group) - end - - context 'when group is available to automatic groups only' do - before do - group.update!(automatic: true) - end - - it 'should return the right response' do - post "/t/#{group_private_topic.id}/invite.json", params: { - user: user - } - - expect(response.status).to eq(403) - end - end - - context 'when user is not part of the required group' do - it 'should return the right response' do - post "/t/#{group_private_topic.id}/invite.json", params: { - user: user - } - - expect(response.status).to eq(422) - - response_body = response.parsed_body - - expect(response_body["errors"]).to eq([ - I18n.t("topic_invite.failed_to_invite", - group_names: group.name - ) - ]) - end + expect(response.status).to eq(422) end end @@ -4129,13 +4069,13 @@ RSpec.describe TopicsController do email: user.email } - expect(response.status).to eq(400) + expect(response.status).to eq(404) end end it 'requires an email parameter' do post "/t/#{topic.id}/invite.json" - expect(response.status).to eq(400) + expect(response.status).to eq(422) end context "when PM has reached maximum allowed numbers of recipients" do @@ -4180,28 +4120,6 @@ RSpec.describe TopicsController do end end end - - context "when inviting a group to a topic" do - fab!(:group) { Fabricate(:group) } - - before do - sign_in(admin) - end - - it "should work correctly" do - email = 'hiro@from.heros' - - post "/t/#{topic.id}/invite.json", params: { - email: email, group_ids: group.id - } - - expect(response.status).to eq(200) - - groups = Invite.find_by(email: email).groups - expect(groups.count).to eq(1) - expect(groups.first.id).to eq(group.id) - end - end end describe 'invite_group' do