From bc3efab8169276036025bf58792420d065f9f394 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 7 Feb 2019 13:46:05 -0500 Subject: [PATCH] FIX: When disagreeing with a flag that silenced a user, unsilence them Previously it would unhide their post but leave them silenced. This fix also cleans up some of the helper classes to make it easier to pass extra data to the silencing code (for example, a link to the post that caused the user to be silenced.) This patch also refactors the auto_silence specs to avoid using stubs. --- app/controllers/admin/flags_controller.rb | 2 - app/models/post_action.rb | 11 +- app/services/spam_rule/auto_silence.rb | 53 +++--- app/services/spam_rule/flag_sockpuppets.rb | 12 +- app/services/spam_rules_enforcer.rb | 22 --- app/services/user_silencer.rb | 6 + lib/guardian/post_guardian.rb | 3 +- lib/post_creator.rb | 2 +- spec/integration/same_ip_spammers_spec.rb | 2 +- spec/integration/spam_rules_spec.rb | 4 +- spec/requests/admin/flags_controller_spec.rb | 22 +++ spec/services/auto_silence_spec.rb | 185 ++++++++++--------- spec/services/spam_rules_enforcer_spec.rb | 29 --- spec/services/user_silencer_spec.rb | 7 + 14 files changed, 186 insertions(+), 174 deletions(-) delete mode 100644 app/services/spam_rules_enforcer.rb delete mode 100644 spec/services/spam_rules_enforcer_spec.rb diff --git a/app/controllers/admin/flags_controller.rb b/app/controllers/admin/flags_controller.rb index a94eae4352f..5df6bcd0f06 100644 --- a/app/controllers/admin/flags_controller.rb +++ b/app/controllers/admin/flags_controller.rb @@ -114,8 +114,6 @@ class Admin::FlagsController < Admin::AdminController PostAction.clear_flags!(post, current_user) - post.unhide! - render body: nil end diff --git a/app/models/post_action.rb b/app/models/post_action.rb index f0d7f07e66b..c814f55efea 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -231,6 +231,8 @@ class PostAction < ActiveRecord::Base end update_flagged_posts_count + + undo_hide_and_silence(post) end def self.defer_flags!(post, moderator, delete_post = false) @@ -375,6 +377,13 @@ class PostAction < ActiveRecord::Base PostAction.where(where_attrs).first end + def self.undo_hide_and_silence(post) + return unless post.hidden? + + post.unhide! + UserSilencer.unsilence(post.user) if UserSilencer.was_silenced_for?(post) + end + def self.copy(original_post, target_post) cols_to_copy = (column_names - %w{id post_id}).join(', ') @@ -537,7 +546,7 @@ class PostAction < ActiveRecord::Base post = Post.with_deleted.where(id: post_id).first PostAction.auto_close_if_threshold_reached(post.topic) PostAction.auto_hide_if_needed(user, post, post_action_type_key) - SpamRulesEnforcer.enforce!(post.user) + SpamRule::AutoSilence.new(post.user, post).perform end def create_user_action diff --git a/app/services/spam_rule/auto_silence.rb b/app/services/spam_rule/auto_silence.rb index ac5173ef2c0..7eeb9baa9cc 100644 --- a/app/services/spam_rule/auto_silence.rb +++ b/app/services/spam_rule/auto_silence.rb @@ -1,37 +1,38 @@ class SpamRule::AutoSilence - def initialize(user) + attr_reader :group_message + + def initialize(user, post = nil) @user = user - end - - def self.silence?(user) - self.new(user).silence? - end - - def self.punish!(user) - self.new(user).silence_user + @post = post end def perform - silence_user if silence? + I18n.with_locale(SiteSetting.default_locale) do + silence_user if should_autosilence? + end end - def silence? - return true if @user.silenced? + def self.prevent_posting?(user) + user.blank? || user.silenced? || new(user).should_autosilence? + end + + def should_autosilence? + return false if @user.blank? return false if @user.staged? return false if @user.has_trust_level?(TrustLevel[1]) - if SiteSetting.num_spam_flags_to_silence_new_user > (0) && - SiteSetting.num_users_to_silence_new_user > (0) && - num_spam_flags_against_user >= (SiteSetting.num_spam_flags_to_silence_new_user) && - num_users_who_flagged_spam_against_user >= (SiteSetting.num_users_to_silence_new_user) + if SiteSetting.num_spam_flags_to_silence_new_user > 0 && + SiteSetting.num_users_to_silence_new_user > 0 && + num_spam_flags_against_user >= SiteSetting.num_spam_flags_to_silence_new_user && + num_users_who_flagged_spam_against_user >= SiteSetting.num_users_to_silence_new_user return true end - if SiteSetting.num_tl3_flags_to_silence_new_user > (0) && - SiteSetting.num_tl3_users_to_silence_new_user > (0) && - num_tl3_flags_against_user >= (SiteSetting.num_tl3_flags_to_silence_new_user) && - num_tl3_users_who_flagged >= (SiteSetting.num_tl3_users_to_silence_new_user) + if SiteSetting.num_tl3_flags_to_silence_new_user > 0 && + SiteSetting.num_tl3_users_to_silence_new_user > 0 && + num_tl3_flags_against_user >= SiteSetting.num_tl3_flags_to_silence_new_user && + num_tl3_users_who_flagged >= SiteSetting.num_tl3_users_to_silence_new_user return true end @@ -72,8 +73,16 @@ class SpamRule::AutoSilence def silence_user Post.transaction do - if UserSilencer.silence(@user, Discourse.system_user, message: :too_many_spam_flags) && SiteSetting.notify_mods_when_user_silenced - GroupMessage.create(Group[:moderators].name, :user_automatically_silenced, user: @user, limit_once_per: false) + + silencer = UserSilencer.new( + @user, + Discourse.system_user, + message: :too_many_spam_flags, + post_id: @post&.id + ) + + if silencer.silence && SiteSetting.notify_mods_when_user_silenced + @group_message = GroupMessage.create(Group[:moderators].name, :user_automatically_silenced, user: @user, limit_once_per: false) end end end diff --git a/app/services/spam_rule/flag_sockpuppets.rb b/app/services/spam_rule/flag_sockpuppets.rb index e3df8a88f8f..16100a75653 100644 --- a/app/services/spam_rule/flag_sockpuppets.rb +++ b/app/services/spam_rule/flag_sockpuppets.rb @@ -5,11 +5,13 @@ class SpamRule::FlagSockpuppets end def perform - if SiteSetting.flag_sockpuppets && reply_is_from_sockpuppet? - flag_sockpuppet_users - true - else - false + I18n.with_locale(SiteSetting.default_locale) do + if SiteSetting.flag_sockpuppets && reply_is_from_sockpuppet? + flag_sockpuppet_users + true + else + false + end end end diff --git a/app/services/spam_rules_enforcer.rb b/app/services/spam_rules_enforcer.rb deleted file mode 100644 index 7a5133c1a67..00000000000 --- a/app/services/spam_rules_enforcer.rb +++ /dev/null @@ -1,22 +0,0 @@ -# The SpamRulesEnforcer class takes action against users based on flags that their posts -# receive, their trust level, etc. -class SpamRulesEnforcer - - def self.enforce!(arg) - SpamRulesEnforcer.new(arg).enforce! - end - - def initialize(arg) - @user = arg if arg.is_a?(User) - @post = arg if arg.is_a?(Post) - end - - def enforce! - I18n.with_locale(SiteSetting.default_locale) do - SpamRule::AutoSilence.new(@user).perform if @user - SpamRule::FlagSockpuppets.new(@post).perform if @post - end - true - end - -end diff --git a/app/services/user_silencer.rb b/app/services/user_silencer.rb index 3044431725c..be1bbeb938d 100644 --- a/app/services/user_silencer.rb +++ b/app/services/user_silencer.rb @@ -16,6 +16,12 @@ class UserSilencer UserSilencer.new(user, by_user, opts).unsilence end + def self.was_silenced_for?(post) + return false unless post.present? + + UserHistory.where(action: UserHistory.actions[:silence_user], post: post).exists? + end + def silence hide_posts unless @opts[:keep_posts] unless @user.silenced_till.present? diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index fd48124dcb7..c49d3cd22f3 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -108,10 +108,9 @@ module PostGuardian # Creating Method def can_create_post?(parent) - return false if !SiteSetting.enable_system_message_replies? && parent.try(:subtype) == "system_message" - (!SpamRule::AutoSilence.silence?(@user) || (!!parent.try(:private_message?) && parent.allowed_users.include?(@user))) && ( + (!SpamRule::AutoSilence.prevent_posting?(@user) || (!!parent.try(:private_message?) && parent.allowed_users.include?(@user))) && ( !parent || !parent.category || Category.post_create_allowed(self).where(id: parent.category.id).count == 1 diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 4c4c0793256..d8bee4047ca 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -359,7 +359,7 @@ class PostCreator limit_once_per: 24.hours, message_params: { domains: @post.linked_hosts.keys.join(', ') }) elsif @post && errors.blank? && !skip_validations? - SpamRulesEnforcer.enforce!(@post) + SpamRule::FlagSockpuppets.new(@post).perform end end diff --git a/spec/integration/same_ip_spammers_spec.rb b/spec/integration/same_ip_spammers_spec.rb index 63f339fcc49..827dba10d77 100644 --- a/spec/integration/same_ip_spammers_spec.rb +++ b/spec/integration/same_ip_spammers_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -describe SpamRulesEnforcer do +describe "spammers on same IP" do let(:ip_address) { '182.189.119.174' } let!(:spammer1) { Fabricate(:user, ip_address: ip_address) } diff --git a/spec/integration/spam_rules_spec.rb b/spec/integration/spam_rules_spec.rb index 1f8d98f9ea5..6691aa3fc75 100644 --- a/spec/integration/spam_rules_spec.rb +++ b/spec/integration/spam_rules_spec.rb @@ -2,9 +2,9 @@ require 'rails_helper' -describe SpamRulesEnforcer do +describe "spam rules for users" do - describe 'auto-silenceing users based on flagging' do + describe 'auto-silence users based on flagging' do let!(:admin) { Fabricate(:admin) } # needed to send a system message let!(:moderator) { Fabricate(:moderator) } let(:user1) { Fabricate(:user) } diff --git a/spec/requests/admin/flags_controller_spec.rb b/spec/requests/admin/flags_controller_spec.rb index 0e5d91d0341..1c572fe7650 100644 --- a/spec/requests/admin/flags_controller_spec.rb +++ b/spec/requests/admin/flags_controller_spec.rb @@ -102,4 +102,26 @@ RSpec.describe Admin::FlagsController do expect(first_post.deleted_at).to eq(nil) end end + + context '#disagree' do + it "unhides the post and unsilences the user if disagreed" do + SiteSetting.num_spam_flags_to_silence_new_user = 1 + SiteSetting.num_users_to_silence_new_user = 1 + + new_user = Fabricate(:newuser) + new_post = create_post(user: new_user) + + PostAction.act(Fabricate(:leader), new_post, PostActionType.types[:spam]) + + post "/admin/flags/disagree/#{new_post.id}.json" + expect(response.status).to eq(200) + + new_post.reload + new_user.reload + + expect(new_post).to_not be_hidden + expect(new_post.spam_count).to eq(0) + expect(new_user).to_not be_silenced + end + end end diff --git a/spec/services/auto_silence_spec.rb b/spec/services/auto_silence_spec.rb index 4a21a55f135..f86bcab24bd 100644 --- a/spec/services/auto_silence_spec.rb +++ b/spec/services/auto_silence_spec.rb @@ -9,24 +9,25 @@ describe SpamRule::AutoSilence do end describe 'perform' do - let(:post) { Fabricate.build(:post, user: Fabricate.build(:user, trust_level: TrustLevel[0])) } - subject { described_class.new(post.user) } + let(:user) { Fabricate.build(:newuser) } + let(:post) { Fabricate(:post, user: user) } + subject { described_class.new(post.user) } it 'takes no action if user should not be silenced' do - subject.stubs(:silence?).returns(false) - subject.expects(:silence_user).never subject.perform + expect(post.user.reload).not_to be_silenced end it 'delivers punishment when user should be silenced' do - subject.stubs(:silence?).returns(true) - subject.expects(:silence_user) + SiteSetting.num_spam_flags_to_silence_new_user = 1 + SiteSetting.num_users_to_silence_new_user = 1 + PostAction.act(Discourse.system_user, post, PostActionType.types[:spam]) subject.perform + expect(post.user.reload).to be_silenced end end describe 'num_spam_flags_against_user' do - before { described_class.any_instance.stubs(:silence_user) } let(:post) { Fabricate(:post) } let(:enforcer) { described_class.new(post.user) } subject { enforcer.num_spam_flags_against_user } @@ -53,7 +54,6 @@ describe SpamRule::AutoSilence do end describe 'num_users_who_flagged_spam_against_user' do - before { described_class.any_instance.stubs(:silence_user) } let(:post) { Fabricate(:post) } let(:enforcer) { described_class.new(post.user) } subject { enforcer.num_users_who_flagged_spam_against_user } @@ -130,146 +130,157 @@ describe SpamRule::AutoSilence do let!(:post) { Fabricate(:post, user: user) } subject { described_class.new(user) } - before do - described_class.stubs(:silence?).with { |u| u.id != user.id }.returns(false) - described_class.stubs(:silence?).with { |u| u.id == user.id }.returns(true) - subject.stubs(:silence?).returns(true) - end - context 'user is not silenced' do - before do - UserSilencer.expects(:silence).with(user, Discourse.system_user, message: :too_many_spam_flags).returns(true) - end - it 'prevents the user from making new posts' do subject.silence_user + expect(user).to be_silenced expect(Guardian.new(user).can_create_post?(nil)).to be_falsey end - it 'sends private message to moderators' do - SiteSetting.notify_mods_when_user_silenced = true - moderator = Fabricate(:moderator) - GroupMessage.expects(:create).with do |group, msg_type, params| - group == (Group[:moderators].name) && msg_type == (:user_automatically_silenced) && params[:user].id == (user.id) - end - subject.silence_user - end + context "with a moderator" do + let!(:moderator) { Fabricate(:moderator) } - it "doesn't send a pm to moderators if notify_mods_when_user_silenced is false" do - SiteSetting.notify_mods_when_user_silenced = false - GroupMessage.expects(:create).never - subject.silence_user + it 'sends private message to moderators' do + SiteSetting.notify_mods_when_user_silenced = true + subject.silence_user + expect(subject.group_message).to be_present + end + + it "doesn't send a pm to moderators if notify_mods_when_user_silenced is false" do + SiteSetting.notify_mods_when_user_silenced = false + subject.silence_user + expect(subject.group_message).to be_blank + end end end context 'user is already silenced' do before do - UserSilencer.expects(:silence).with(user, Discourse.system_user, message: :too_many_spam_flags).returns(false) + UserSilencer.silence(user) end it "doesn't send a pm to moderators if the user is already silenced" do - GroupMessage.expects(:create).never subject.silence_user + expect(subject.group_message).to be_blank end end end - describe 'silence?' do + describe 'autosilenced?' do + let(:user) { Fabricate(:newuser) } + let(:flagger) { Fabricate(:user) } + let(:flagger2) { Fabricate(:user) } + let(:post) { Fabricate(:post, user: user) } + let(:post2) { Fabricate(:post, user: user) } - context 'never been silenced' do - shared_examples "can't be silenced" do - it "returns false" do - enforcer = described_class.new(user) - enforcer.expects(:num_spam_flags_against_user).never - enforcer.expects(:num_users_who_flagged_spam_against_user).never - enforcer.expects(:num_flags_against_user).never - enforcer.expects(:num_users_who_flagged).never - expect(enforcer.silence?).to eq(false) - end - end + context "higher trust levels or staff" do + it "should not autosilence any of them" do + PostAction.act(flagger, post, PostActionType.types[:spam]) + PostAction.act(flagger2, post, PostActionType.types[:spam]) - (1..4).each do |trust_level| - context "user has trust level #{trust_level}" do - let(:user) { Fabricate(:user, trust_level: trust_level) } - include_examples "can't be silenced" - end - end + enforcer = described_class.new(user) + expect(enforcer.should_autosilence?).to eq(true) - context "user is an admin" do - let(:user) { Fabricate(:admin) } - include_examples "can't be silenced" - end + user.trust_level = 1 + expect(enforcer.should_autosilence?).to eq(false) - context "user is a moderator" do - let(:user) { Fabricate(:moderator) } - include_examples "can't be silenced" + user.trust_level = 2 + expect(enforcer.should_autosilence?).to eq(false) + + user.trust_level = 3 + expect(enforcer.should_autosilence?).to eq(false) + + user.trust_level = 4 + expect(enforcer.should_autosilence?).to eq(false) + + user.trust_level = 0 + user.moderator = true + expect(enforcer.should_autosilence?).to eq(false) + + user.moderator = false + user.admin = true + expect(enforcer.should_autosilence?).to eq(false) end end context 'new user' do - let(:user) { Fabricate(:user, trust_level: TrustLevel[0]) } - subject { described_class.new(user) } + subject { described_class.new(user) } it 'returns false if there are no spam flags' do - subject.stubs(:num_spam_flags_against_user).returns(0) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(0) - expect(subject.silence?).to be_falsey + expect(subject.num_spam_flags_against_user).to eq(0) + expect(subject.num_users_who_flagged_spam_against_user).to eq(0) + expect(subject.should_autosilence?).to eq(false) end it 'returns false if there are not received enough flags' do - subject.stubs(:num_spam_flags_against_user).returns(1) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(2) - expect(subject.silence?).to be_falsey + PostAction.act(flagger, post, PostActionType.types[:spam]) + expect(subject.num_spam_flags_against_user).to eq(1) + expect(subject.num_users_who_flagged_spam_against_user).to eq(1) + expect(subject.should_autosilence?).to eq(false) end it 'returns false if there have not been enough users' do - subject.stubs(:num_spam_flags_against_user).returns(2) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(1) - expect(subject.silence?).to be_falsey + PostAction.act(flagger, post, PostActionType.types[:spam]) + PostAction.act(flagger, post2, PostActionType.types[:spam]) + expect(subject.num_spam_flags_against_user).to eq(2) + expect(subject.num_users_who_flagged_spam_against_user).to eq(1) + expect(subject.should_autosilence?).to eq(false) end it 'returns false if num_spam_flags_to_silence_new_user is 0' do SiteSetting.num_spam_flags_to_silence_new_user = 0 - subject.stubs(:num_spam_flags_against_user).returns(100) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(100) - expect(subject.silence?).to be_falsey + PostAction.act(flagger, post, PostActionType.types[:spam]) + PostAction.act(flagger2, post, PostActionType.types[:spam]) + expect(subject.should_autosilence?).to eq(false) end it 'returns false if num_users_to_silence_new_user is 0' do SiteSetting.num_users_to_silence_new_user = 0 - subject.stubs(:num_spam_flags_against_user).returns(100) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(100) - expect(subject.silence?).to be_falsey + PostAction.act(flagger, post, PostActionType.types[:spam]) + PostAction.act(flagger2, post, PostActionType.types[:spam]) + expect(subject.should_autosilence?).to eq(false) end it 'returns true when there are enough flags from enough users' do - subject.stubs(:num_spam_flags_against_user).returns(2) - subject.stubs(:num_users_who_flagged_spam_against_user).returns(2) - expect(subject.silence?).to be_truthy + PostAction.act(flagger, post, PostActionType.types[:spam]) + PostAction.act(flagger2, post, PostActionType.types[:spam]) + expect(subject.num_spam_flags_against_user).to eq(2) + expect(subject.num_users_who_flagged_spam_against_user).to eq(2) + expect(subject.should_autosilence?).to eq(true) end context "all types of flags" do + let(:leader1) { Fabricate(:leader) } + let(:leader2) { Fabricate(:leader) } + before do SiteSetting.num_tl3_flags_to_silence_new_user = 3 SiteSetting.num_tl3_users_to_silence_new_user = 2 end it 'returns false if there are not enough flags' do - subject.stubs(:num_tl3_flags_against_user).returns(1) - subject.stubs(:num_tl3_users_who_flagged).returns(1) - expect(subject.silence?).to be_falsey + PostAction.act(leader1, post, PostActionType.types[:inappropriate]) + expect(subject.num_tl3_flags_against_user).to eq(1) + expect(subject.num_tl3_users_who_flagged).to eq(1) + expect(subject.should_autosilence?).to be_falsey end it 'returns false if enough flags but not enough users' do - subject.stubs(:num_tl3_flags_against_user).returns(3) - subject.stubs(:num_tl3_users_who_flagged).returns(1) - expect(subject.silence?).to be_falsey + PostAction.act(leader1, post, PostActionType.types[:inappropriate]) + PostAction.act(leader1, post2, PostActionType.types[:inappropriate]) + PostAction.act(leader1, Fabricate(:post, user: user), PostActionType.types[:inappropriate]) + expect(subject.num_tl3_flags_against_user).to eq(3) + expect(subject.num_tl3_users_who_flagged).to eq(1) + expect(subject.should_autosilence?).to eq(false) end it 'returns true if enough flags and users' do - subject.stubs(:num_tl3_flags_against_user).returns(3) - subject.stubs(:num_tl3_users_who_flagged).returns(2) - expect(subject.silence?).to eq(true) + PostAction.act(leader1, post, PostActionType.types[:inappropriate]) + PostAction.act(leader1, post2, PostActionType.types[:inappropriate]) + PostAction.act(leader2, post, PostActionType.types[:inappropriate]) + expect(subject.num_tl3_flags_against_user).to eq(3) + expect(subject.num_tl3_users_who_flagged).to eq(2) + expect(subject.should_autosilence?).to eq(true) end end end @@ -279,7 +290,7 @@ describe SpamRule::AutoSilence do subject { described_class.new(user) } it 'returns false' do - expect(subject.silence?).to be_truthy + expect(subject.should_autosilence?).to eq(false) end end end diff --git a/spec/services/spam_rules_enforcer_spec.rb b/spec/services/spam_rules_enforcer_spec.rb deleted file mode 100644 index 12ee2e2ce22..00000000000 --- a/spec/services/spam_rules_enforcer_spec.rb +++ /dev/null @@ -1,29 +0,0 @@ -require 'rails_helper' - -describe SpamRulesEnforcer do - - before do - SystemMessage.stubs(:create) - end - - describe 'enforce!' do - context 'post argument' do - subject(:enforce) { described_class.enforce!(Fabricate.build(:post)) } - - it 'performs the FlagSockpuppetRule' do - SpamRule::FlagSockpuppets.any_instance.expects(:perform).once - enforce - end - end - - context 'user argument' do - subject(:enforce) { described_class.enforce!(Fabricate.build(:user)) } - - it 'performs the AutoSilence' do - SpamRule::AutoSilence.any_instance.expects(:perform).once - enforce - end - end - end - -end diff --git a/spec/services/user_silencer_spec.rb b/spec/services/user_silencer_spec.rb index 73bab2e19ff..a0c40b34594 100644 --- a/spec/services/user_silencer_spec.rb +++ b/spec/services/user_silencer_spec.rb @@ -112,6 +112,13 @@ describe UserSilencer do expect(post.topic.reload).to_not be_visible end + it "allows us to silence the user for a particular post" do + expect(UserSilencer.was_silenced_for?(post)).to eq(false) + UserSilencer.new(user, Discourse.system_user, post_id: post.id).silence + expect(user).to be_silenced + expect(UserSilencer.was_silenced_for?(post)).to eq(true) + end + it "doesn't hide posts if user is TL1" do user.trust_level = 1 subject.silence