From 9b1d0baf45c6b6744a960a68a4b088693fadcf5d Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 10 Jun 2013 13:17:09 -0400 Subject: [PATCH] Send a message to moderators when a newuser_spam_host_threshold is exceeded. Send it no more than once per day per user. --- app/services/group_message.rb | 66 ++++++++++++ app/services/spam_rules_enforcer.rb | 16 +-- config/locales/server.en.yml | 11 +- lib/post_creator.rb | 5 +- spec/components/post_creator_spec.rb | 8 ++ spec/services/group_message_spec.rb | 119 ++++++++++++++++++++++ spec/services/spam_rules_enforcer_spec.rb | 8 +- 7 files changed, 210 insertions(+), 23 deletions(-) create mode 100644 app/services/group_message.rb create mode 100644 spec/services/group_message_spec.rb diff --git a/app/services/group_message.rb b/app/services/group_message.rb new file mode 100644 index 000000000..2095f3d73 --- /dev/null +++ b/app/services/group_message.rb @@ -0,0 +1,66 @@ +# GroupMessage sends a private message to a group. +# It will also avoid sending the same message repeatedly, which can happen with +# notifications to moderators when spam is detected. +# +# Options: +# +# user: (User) If the message is about a user, pass the user object. +# limit_once_per: (seconds) Limit sending the given type of message once every X seconds. +# The default is 24 hours. Set to false to always send the message. + +require_dependency 'post_creator' +require_dependency 'topic_subtype' +require_dependency 'discourse' + +class GroupMessage + + include Rails.application.routes.url_helpers + + def self.create(group_name, message_type, opts={}) + GroupMessage.new(group_name, message_type, opts).create + end + + def initialize(group_name, message_type, opts={}) + @group_name = group_name + @message_type = message_type + @opts = opts + end + + def create + unless sent_recently? + post = PostCreator.create( Discourse.system_user, + target_group_names: [@group_name], + archetype: Archetype.private_message, + subtype: TopicSubtype.system_message, + title: I18n.t("system_messages.#{@message_type}.subject_template", message_params), + raw: I18n.t("system_messages.#{@message_type}.text_body_template", message_params) ) + remember_message_sent + post + else + false + end + end + + def message_params + @message_params ||= { + username: @opts[:user].username, + user_url: admin_user_path(@opts[:user].username) + } + end + + def sent_recently? + return false if @opts[:limit_once_per] == false + $redis.get(sent_recently_key).present? + end + + # default is to send no more than once every 24 hours (24 * 60 * 60 = 86,400 seconds) + def remember_message_sent + $redis.setex(sent_recently_key, @opts[:limit_once_per].try(:to_i) || 86_400, 1) unless @opts[:limit_once_per] == false + end + + private + + def sent_recently_key + "grpmsg:#{@group_name}:#{@message_type}:#{@opts[:user] ? @opts[:user].username : ''}" + end +end diff --git a/app/services/spam_rules_enforcer.rb b/app/services/spam_rules_enforcer.rb index df6621312..d98c0ae38 100644 --- a/app/services/spam_rules_enforcer.rb +++ b/app/services/spam_rules_enforcer.rb @@ -53,7 +53,7 @@ class SpamRulesEnforcer def punish_user Post.update_all(["hidden = true, hidden_reason_id = COALESCE(hidden_reason_id, ?)", Post.hidden_reasons[:new_user_spam_threshold_reached]], user_id: @user.id) SystemMessage.create(@user, :too_many_spam_flags) - notify_moderators + GroupMessage.create(Group[:moderators].name, :user_automatically_blocked, {user: @user, limit_once_per: false}) @user.blocked = true @user.save end @@ -64,18 +64,4 @@ class SpamRulesEnforcer @user.save end - - private - - def notify_moderators - title = I18n.t("system_messages.user_automatically_blocked.subject_template", {username: @user.username}) - raw_body = I18n.t("system_messages.user_automatically_blocked.text_body_template", {username: @user.username, blocked_user_url: admin_user_path(@user.username)}) - PostCreator.create( Discourse.system_user, - target_group_names: [Group[:moderators].name], - archetype: Archetype.private_message, - subtype: TopicSubtype.system_message, - title: title, - raw: raw_body ) - end - end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0d913ac14..c634369b2 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -858,9 +858,16 @@ en: user_automatically_blocked: subject_template: "User %{username} was automatically blocked" text_body_template: | - This is an automated message to inform you that [%{username}](%{blocked_user_url}) has been automatically blocked because too many people submitted flags against %{username}'s post(s). + This is an automated message to inform you that [%{username}](%{user_url}) has been automatically blocked because too many people submitted flags against %{username}'s post(s). - Please [review the flags](/admin/flags). If %{username} should not be blocked from posting, click the unblock button on [the admin user page](%{blocked_user_url}). + Please [review the flags](/admin/flags). If %{username} should not be blocked from posting, click the unblock button on [the admin user page](%{user_url}). + + spam_post_blocked: + subject_template: "Spam was detected in a post by %{username}" + text_body_template: | + This is an automated message to inform you that [%{username}](%{user_url}) tried to make a post with links, but it was stopped as spam based on the newuser_spam_host_threshold site setting. + + Please [review the user](%{user_url}). unblocked: subject_template: "Account unblocked" diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 529e35ece..f8a45276a 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -69,6 +69,10 @@ class PostCreator @post.save_reply_relationships end + if @spam + GroupMessage.create( Group[:moderators].name, :spam_post_blocked, {user: @user, limit_once_per: 24.hours} ) + end + enqueue_jobs @post end @@ -261,7 +265,6 @@ class PostCreator def enqueue_jobs if @post && !@post.errors.present? - # We need to enqueue jobs after the transaction. Otherwise they might begin before the data has # been comitted. topic_id = @opts[:topic_id] || @topic.try(:id) diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index ebcf723f9..a3b550f60 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -216,11 +216,19 @@ describe PostCreator do end it "does not create the post" do + GroupMessage.stubs(:create) creator.create creator.errors.should be_present creator.spam?.should be_true end + it "sends a message to moderators" do + GroupMessage.expects(:create).with do |group_name, msg_type, params| + group_name == Group[:moderators].name and msg_type == :spam_post_blocked and params[:user].id == user.id + end + creator.create + end + end # more integration testing ... maximise our testing diff --git a/spec/services/group_message_spec.rb b/spec/services/group_message_spec.rb new file mode 100644 index 000000000..1908140db --- /dev/null +++ b/spec/services/group_message_spec.rb @@ -0,0 +1,119 @@ +require 'spec_helper' + +describe GroupMessage do + + let(:moderators_group) { Group[:moderators].name } + + let!(:admin) { Fabricate.build(:admin, id: 999) } + let!(:user) { Fabricate.build(:user, id: 111) } + + before do + Discourse.stubs(:system_user).returns(admin) + end + + subject(:send_group_message) { GroupMessage.create(moderators_group, :user_automatically_blocked, {user: user}) } + + describe 'not sent recently' do + before { GroupMessage.any_instance.stubs(:sent_recently?).returns(false) } + + it 'should send a private message to the given group' do + PostCreator.expects(:create).with do |from_user, opts| + from_user.id == admin.id and + opts[:target_group_names] and opts[:target_group_names].include?(Group[:moderators].name) and + opts[:archetype] == Archetype.private_message and + opts[:title].present? and + opts[:raw].present? + end.returns(stub_everything) + send_group_message + end + + it 'returns whatever PostCreator returns' do + the_output = stub_everything + PostCreator.stubs(:create).returns(the_output) + expect(send_group_message).to eq(the_output) + end + + it "remembers that it was sent so it doesn't spam the group with the same message" do + PostCreator.stubs(:create).returns(stub_everything) + GroupMessage.any_instance.expects(:remember_message_sent) + send_group_message + end + end + + describe 'sent recently' do + before { GroupMessage.any_instance.stubs(:sent_recently?).returns(true) } + subject { GroupMessage.create(moderators_group, :user_automatically_blocked, {user: user}) } + + it { should eq(false) } + + it 'should not send the same notification again' do + PostCreator.expects(:create).never + subject + end + end + + describe 'message_params' do + let(:user) { Fabricate.build(:user, id: 123123) } + shared_examples 'common message params for group messages' do + it 'returns the correct params' do + expect(subject[:username]).to eq(user.username) + expect(subject[:user_url]).to be_present + end + end + + context 'user_automatically_blocked' do + subject { GroupMessage.new(moderators_group, :user_automatically_blocked, {user: user}).message_params } + include_examples 'common message params for group messages' + end + + context 'spam_post_blocked' do + subject { GroupMessage.new(moderators_group, :spam_post_blocked, {user: user}).message_params } + include_examples 'common message params for group messages' + end + end + + describe 'methods that use redis' do + let(:user) { Fabricate.build(:user, id: 123123) } + subject(:group_message) { GroupMessage.new(moderators_group, :user_automatically_blocked, {user: user}) } + before do + PostCreator.stubs(:create).returns(stub_everything) + group_message.stubs(:sent_recently_key).returns('the_key') + end + + describe 'sent_recently?' do + it 'returns true if redis says so' do + $redis.stubs(:get).with(group_message.sent_recently_key).returns('1') + expect(group_message.sent_recently?).to be_true + end + + it 'returns false if redis returns nil' do + $redis.stubs(:get).with(group_message.sent_recently_key).returns(nil) + expect(group_message.sent_recently?).to be_false + end + + it 'always returns false if limit_once_per is false' do + gm = GroupMessage.new(moderators_group, :user_automatically_blocked, {user: user, limit_once_per: false}) + gm.stubs(:sent_recently_key).returns('the_key') + $redis.stubs(:get).with(gm.sent_recently_key).returns('1') + expect(gm.sent_recently?).to be_false + end + end + + describe 'remember_message_sent' do + it 'stores a key in redis that expires after 24 hours' do + $redis.expects(:setex).with(group_message.sent_recently_key, 24 * 60 * 60, anything).returns('OK') + group_message.remember_message_sent + end + + it 'can use a given expiry time' do + $redis.expects(:setex).with(anything, 30 * 60, anything).returns('OK') + GroupMessage.new(moderators_group, :user_automatically_blocked, {user: user, limit_once_per: 30.minutes}).remember_message_sent + end + + it 'can be disabled' do + $redis.expects(:setex).never + GroupMessage.new(moderators_group, :user_automatically_blocked, {user: user, limit_once_per: false}).remember_message_sent + end + end + end +end diff --git a/spec/services/spam_rules_enforcer_spec.rb b/spec/services/spam_rules_enforcer_spec.rb index b62f64887..34abfebfe 100644 --- a/spec/services/spam_rules_enforcer_spec.rb +++ b/spec/services/spam_rules_enforcer_spec.rb @@ -120,11 +120,9 @@ describe SpamRulesEnforcer do it 'sends private messages to the user and to moderators' do SystemMessage.expects(:create).with(user, anything, anything) moderator = Fabricate(:moderator) - PostCreator.expects(:create).with do |from_user, opts| - from_user.id == admin.id && - opts[:target_group_names] && opts[:target_group_names].include?(Group[:moderators].name) && - opts[:archetype] == Archetype.private_message - end.returns(stub_everything) + GroupMessage.expects(:create).with do |group, msg_type, params| + group == Group[:moderators].name and msg_type == :user_automatically_blocked and params[:user].id == user.id + end subject.punish_user end