From 29f6853ee9b3aabd7edfa8a5a7a3f7247a9f5a28 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Tue, 30 Nov 2021 12:45:12 -0500 Subject: [PATCH] Refactoring Spam Handler (#15412) * Refactoring Spam Handler There's considerable repeated logic between checking spam for an article and spam for a comment and user. This attempts to send things through channels that are similar and close in organization. * Fixing broken spec * Fixing spec around recent user * Update app/models/reaction.rb Co-authored-by: Michael Kohl * Update app/models/reaction.rb Co-authored-by: Michael Kohl * Consolidating new user query logic Prior to this commit there were two separate queries around new user logic. With this commit, we're changing the logic to repurpose a site wide setting. * Generalizing a previously specific message * Fixing method name As part of a recommended refactor, I extracted a method, then renamed it. I failed to account for that renaming. This commit fixes that. Co-authored-by: Michael Kohl --- app/decorators/user_decorator.rb | 5 +- app/models/article.rb | 2 +- app/models/comment.rb | 25 +---- app/models/reaction.rb | 12 +- app/models/settings/rate_limit.rb | 14 +++ app/models/user.rb | 11 +- app/services/spam/handler.rb | 89 +++++++++++++++ spec/decorators/user_decorator_spec.rb | 16 +-- spec/models/article_spec.rb | 6 +- spec/models/comment_spec.rb | 42 +------ spec/models/reaction_spec.rb | 4 +- spec/models/settings/rate_limit_spec.rb | 30 +++++ spec/models/user_spec.rb | 30 +---- spec/services/spam/article_handler_spec.rb | 52 --------- spec/services/spam/handler_spec.rb | 125 +++++++++++++++++++++ 15 files changed, 288 insertions(+), 175 deletions(-) create mode 100644 app/services/spam/handler.rb delete mode 100644 spec/services/spam/article_handler_spec.rb create mode 100644 spec/services/spam/handler_spec.rb diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 0811d5d36..092820521 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -102,10 +102,7 @@ class UserDecorator < ApplicationDecorator end def considered_new? - min_days = Settings::RateLimit.user_considered_new_days - return false unless min_days.positive? - - created_at.after?(min_days.days.ago) + Settings::RateLimit.user_considered_new?(user: self) end # Returns the user's public email if it is set and the display_email_on_profile diff --git a/app/models/article.rb b/app/models/article.rb index d2a523037..586fd03d6 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -812,7 +812,7 @@ class Article < ApplicationRecord end def create_conditional_autovomits - Spam::ArticleHandler.handle!(article: self) + Spam::Handler.handle_article!(article: self) end def async_bust diff --git a/app/models/comment.rb b/app/models/comment.rb index e0f2beb2a..542999443 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -282,34 +282,13 @@ class Comment < ApplicationRecord end def synchronous_spam_score_check - return unless - Settings::RateLimit.spam_trigger_terms.any? { |term| Regexp.new(term.downcase).match?(title.downcase) } + return unless Settings::RateLimit.trigger_spam_for?(text: [title, body_markdown].join("\n")) self.score = -1 # ensure notification is not sent if possibly spammy end def create_conditional_autovomits - return unless - Settings::RateLimit.spam_trigger_terms.any? { |term| Regexp.new(term.downcase).match?(title.downcase) } && - user.registered_at > 5.days.ago - - Reaction.create( - user_id: Settings::General.mascot_user_id, - reactable_id: id, - reactable_type: "Comment", - category: "vomit", - ) - - return unless Reaction.comment_vomits.where(reactable_id: user.comments.pluck(:id)).size > 2 - - user.add_role(:suspended) - Note.create( - author_id: Settings::General.mascot_user_id, - noteable_id: user_id, - noteable_type: "User", - reason: "automatic_suspend", - content: "User suspended for too many spammy articles, triggered by autovomit.", - ) + Spam::Handler.handle_comment!(comment: self) end def should_send_email_notification? diff --git a/app/models/reaction.rb b/app/models/reaction.rb index 83d8744a0..d83de0e94 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -81,8 +81,16 @@ class Reaction < ApplicationRecord # # @return [TrueClass] yup, they're spamming the system. # @return [FalseClass] they're not (yet) spamming the system - def user_has_been_given_too_many_spammy_reactions?(user:) - article_vomits.where(reactable_type: "Article", reactable_id: user.articles.pluck(:id)).size > 2 + def user_has_been_given_too_many_spammy_article_reactions?(user:, threshold: 2) + article_vomits.where(reactable_type: "Article", reactable_id: user.articles.ids).size > threshold + end + + # @param user [User] the user who might be spamming the system + # + # @return [TrueClass] yup, they're spamming the system. + # @return [FalseClass] they're not (yet) spamming the system + def user_has_been_given_too_many_spammy_comment_reactions?(user:, threshold: 2) + article_vomits.where(reactable_type: "Comment", reactable_id: user.comments.ids).size > threshold end end diff --git a/app/models/settings/rate_limit.rb b/app/models/settings/rate_limit.rb index b33e84dda..8cd183379 100644 --- a/app/models/settings/rate_limit.rb +++ b/app/models/settings/rate_limit.rb @@ -22,6 +22,20 @@ module Settings setting :user_subscription_creation, type: :integer, default: 3 setting :user_update, type: :integer, default: 15 + # A helper function to determine if we should consider the user a "new" user. + # + # @note A "new" user is more likely to start spamming than an "old" user. + # + # @param user [User, UserDecorator] + # + # @return [Boolean] + def self.user_considered_new?(user:) + return true unless user + return false unless user_considered_new_days.positive? + + user.created_at.after?(user_considered_new_days.days.ago) + end + # A helper function to determine if text is spammy. # # @param text [String] text to check for "spamminess" diff --git a/app/models/user.rb b/app/models/user.rb index c210eec73..bac0b0296 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -609,16 +609,7 @@ class User < ApplicationRecord end def create_conditional_autovomits - return unless Settings::RateLimit.spam_trigger_terms.any? do |term| - name.match?(/#{term}/i) - end - - Reaction.create!( - user_id: Settings::General.mascot_user_id, - reactable_id: id, - reactable_type: "User", - category: "vomit", - ) + Spam::Handler.handle_user!(user: self) end # TODO: @citizen428 I don't want to completely remove this method yet, as we diff --git a/app/services/spam/handler.rb b/app/services/spam/handler.rb new file mode 100644 index 000000000..a0b794bbc --- /dev/null +++ b/app/services/spam/handler.rb @@ -0,0 +1,89 @@ +module Spam + # This module is responsible for handling spam in our various user input sources. + # + # @note We may not immediately block spam but instead slowly escalate our response. + module Handler + # Test the article for spamminess. If it's not spammy, don't do anything. + # + # If it is spammy, escalate the situation! + # + # @param article [Article] the article to check for spamminess + # @param attributes [Array] test these attributes of the article. + def self.handle_article!(article:, attributes: %i[title body_markdown]) + text = attributes.map { |attr| article.public_send(attr) }.join("\n") + return :not_spam unless Settings::RateLimit.trigger_spam_for?(text: text) + + issue_spam_reaction_for!(reactable: article) + + return unless Reaction.user_has_been_given_too_many_spammy_article_reactions?(user: article.user) + + suspend!(user: article.user) + end + + # Test the comment for spamminess. If it's not spammy, don't do anything. + # + # If it is spammy, escalate the situation! + # + # @param comment [Comment] the comment to check for spamminess + def self.handle_comment!(comment:) + # TODO: Is this correct logic? I was trying to reason through + # the original logic and I think there's something off on that + # original logic. + # + # I believe the intention of the past logic was that we want to + # treat recently registered users with a bit of suspicion. + return :not_spam unless Settings::RateLimit.user_considered_new?(user: comment&.user) + return :not_spam unless Settings::RateLimit.trigger_spam_for?(text: comment.body_markdown) + + issue_spam_reaction_for!(reactable: comment) + + return unless Reaction.user_has_been_given_too_many_spammy_comment_reactions?(user: comment.user) + + suspend!(user: comment.user) + end + + # Test the user for spamminess. If it's not spammy, don't do anything. + # + # If it is spammy, escalate the situation! + # + # @param user [User] the user to check for spamminess + def self.handle_user!(user:) + return :not_spam unless Settings::RateLimit.trigger_spam_for?(text: user.name) + + issue_spam_reaction_for!(reactable: user) + end + + # Suspend the given user because of too many spammy actions. + # + # @param user [User] + # + # @todo What should we do with their active session? I think we + # might want to explore later raising an exception that the + # controller rescues and logs the user out. Because, as + # written they might still be logged in but have limited + # abilities. + def self.suspend!(user:) + user.add_role(:suspended) + + Note.create( + author_id: Settings::General.mascot_user_id, + noteable: user, + reason: "automatic_suspend", + content: "User suspended for too many spammy interactions, triggered by autovomit.", + ) + end + private_class_method :suspend! + + # Have the mascot of this Forem react negatively to this reactable. + # + # @param reactable [ActiveRecord::Base] + def self.issue_spam_reaction_for!(reactable:) + Reaction.create( + user_id: Settings::General.mascot_user_id, + reactable: reactable, + category: "vomit", + ) + end + private_class_method :issue_spam_reaction_for! + end +end diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb index a558f429e..37f16e1fe 100644 --- a/spec/decorators/user_decorator_spec.rb +++ b/spec/decorators/user_decorator_spec.rb @@ -179,18 +179,12 @@ RSpec.describe UserDecorator, type: :decorator do end describe "#considered_new?" do - before do - allow(Settings::RateLimit).to receive(:user_considered_new_days).and_return(3) - end + let(:decorated_user) { user.decorate } - it "returns true for new users" do - user.created_at = 1.day.ago - expect(user.decorate.considered_new?).to be(true) - end - - it "returns false for new users" do - user.created_at = 1.year.ago - expect(user.decorate.considered_new?).to be(false) + it "delegates to Settings::RateLimit.considered_new?" do + allow(Settings::RateLimit).to receive(:user_considered_new?).with(user: decorated_user).and_return(true) + expect(decorated_user.considered_new?).to be(true) + expect(Settings::RateLimit).to have_received(:user_considered_new?).with(user: decorated_user) end end end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index a762bb351..3c838b99a 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -1033,10 +1033,10 @@ RSpec.describe Article, type: :model do end describe "spam" do - it "delegates spam handling to Spam::ArticleHandler" do - allow(Spam::ArticleHandler).to receive(:handle!).with(article: article).and_call_original + it "delegates spam handling to Spam::Handler.handle_article!" do + allow(Spam::Handler).to receive(:handle_article!).with(article: article).and_call_original article.save - expect(Spam::ArticleHandler).to have_received(:handle!).with(article: article) + expect(Spam::Handler).to have_received(:handle_article!).with(article: article) end end diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index ae9a4c21f..f7620eddc 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -438,46 +438,10 @@ RSpec.describe Comment, type: :model do end describe "spam" do - before do - allow(Settings::General).to receive(:mascot_user_id).and_return(user.id) - allow(Settings::RateLimit).to receive(:spam_trigger_terms).and_return(["yahoomagoo gogo", "anothertestterm"]) - end - - it "creates vomit reaction if possible spam" do - comment.body_markdown = "This post is about Yahoomagoo gogo" + it "delegates spam handling to Spam::Handler.handle_comment!" do + allow(Spam::Handler).to receive(:handle_comment!).with(comment: comment).and_call_original comment.save - expect(Reaction.last.category).to eq("vomit") - expect(Reaction.last.user_id).to eq(user.id) - end - - it "does no suspend user if only single vomit" do - comment.body_markdown = "This post is about Yahoomagoo gogo" - comment.save - expect(comment.user.suspended?).to be false - end - - it "suspends user with 3 comment vomits" do - comment.body_markdown = "This post is about Yahoomagoo gogo" - second_comment = create(:comment, user: comment.user, body_markdown: "This post is about Yahoomagoo gogo") - third_comment = create(:comment, user: comment.user, body_markdown: "This post is about Yahoomagoo gogo") - - comment.save - second_comment.save - third_comment.save - expect(comment.user.suspended?).to be true - expect(Note.last.reason).to eq "automatic_suspend" - end - - it "does not create vomit reaction if user is established in this context" do - user.update_column(:registered_at, 10.days.ago) - comment.body_markdown = "This post is about Yahoomagoo gogo" - comment.save - expect(Reaction.last).to be nil - end - - it "does not create vomit reaction if does not have matching title" do - comment.save - expect(Reaction.last).to be nil + expect(Spam::Handler).to have_received(:handle_comment!).with(comment: comment) end end diff --git a/spec/models/reaction_spec.rb b/spec/models/reaction_spec.rb index c72810a23..9d576e943 100644 --- a/spec/models/reaction_spec.rb +++ b/spec/models/reaction_spec.rb @@ -13,9 +13,9 @@ RSpec.describe Reaction, type: :model do it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(%i[reactable_id reactable_type category]) } end - describe ".user_has_been_given_too_many_spammy_reactions?" do + describe ".user_has_been_given_too_many_spammy_article_reactions?" do it "performs a valid query for the user" do - expect { described_class.user_has_been_given_too_many_spammy_reactions?(user: user) }.not_to raise_error + expect { described_class.user_has_been_given_too_many_spammy_article_reactions?(user: user) }.not_to raise_error end end diff --git a/spec/models/settings/rate_limit_spec.rb b/spec/models/settings/rate_limit_spec.rb index a4511d9b3..329b951b0 100644 --- a/spec/models/settings/rate_limit_spec.rb +++ b/spec/models/settings/rate_limit_spec.rb @@ -1,6 +1,36 @@ require "rails_helper" RSpec.describe Settings::RateLimit, type: :model do + describe ".user_considered_new?" do + subject(:function_call) { described_class.user_considered_new?(user: user) } + + before do + allow(described_class).to receive(:user_considered_new_days).and_return(5) + end + + context "when given a nil user" do + let(:user) { nil } + + it { is_expected.to be_truthy } + end + + context "when given a decorated user that was created months ago" do + let(:user) { create(:user).decorate } + + before { allow(user).to receive(:created_at).and_return(30.months.ago) } + + it { is_expected.to be_falsey } + end + + context "when given a decorated user that was created within the user_considered_new_days" do + let(:user) { create(:user).decorate } + + before { allow(user).to receive(:created_at).and_return(3.days.ago) } + + it { is_expected.to be_truthy } + end + end + describe ".trigger_spam_for?" do subject { described_class.trigger_spam_for?(text: text) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 44a67f246..19d9e551d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -538,36 +538,10 @@ RSpec.describe User, type: :model do end describe "spam" do - before do - allow(Settings::General).to receive(:mascot_user_id).and_return(user.id) - allow(Settings::RateLimit).to receive(:spam_trigger_terms).and_return( - ["yahoomagoo gogo", "testtestetest", "magoo.+magee"], - ) - end + it "delegates spam handling to Spam::Handler.handle_user!" do + allow(Spam::Handler).to receive(:handle_user!).with(user: user).and_call_original - it "creates vomit reaction if possible spam" do - user.name = "Hi my name is Yahoomagoo gogo" user.save - expect(Reaction.last.category).to eq("vomit") - expect(Reaction.last.reactable_id).to eq(user.id) - end - - it "creates vomit reaction if possible spam based on pattern" do - user.name = "Hi my name is magoo to the magee" - user.save - expect(Reaction.last.category).to eq("vomit") - expect(Reaction.last.reactable_id).to eq(user.id) - end - - it "does not create vomit reaction if does not have matching title" do - user.save - expect(Reaction.last).to be nil - end - - it "does not create vomit reaction if does not have pattern match" do - user.name = "Hi my name is magoo to" - user.save - expect(Reaction.last).to be nil end end diff --git a/spec/services/spam/article_handler_spec.rb b/spec/services/spam/article_handler_spec.rb deleted file mode 100644 index c11e85462..000000000 --- a/spec/services/spam/article_handler_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require "rails_helper" - -RSpec.describe Spam::ArticleHandler, type: :service do - describe ".handle!" do - subject(:handler) { described_class.handle!(article: article) } - - let!(:article) { create(:article) } - let(:mascot_user) { create(:user) } - - before do - allow(Settings::General).to receive(:mascot_user_id).and_return(mascot_user.id) - end - - context "when non-spammy content" do - before do - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(false) - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false) - end - - it { is_expected.to eq(:not_spam) } - end - - context "when first time spammy content" do - before do - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(true) - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false) - allow(Reaction).to receive(:user_has_been_given_too_many_spammy_reactions?) - .with(user: article.user).and_return(false) - end - - it "creates a reaction but does not suspend the user" do - expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1) - expect(article.user.reload).not_to be_suspended - end - end - - context "when multiple offender of spammy" do - before do - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.title).and_return(true) - allow(Settings::RateLimit).to receive(:trigger_spam_for?).with(text: article.body_markdown).and_return(false) - allow(Reaction).to receive(:user_has_been_given_too_many_spammy_reactions?) - .with(user: article.user).and_return(true) - end - - it "creates a reaction, suspends the user, and creates a note for the user" do - expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1) - expect(article.user.reload).to be_suspended - expect(Note.where(noteable: article.user, reason: "automatic_suspend").count).to eq(1) - end - end - end -end diff --git a/spec/services/spam/handler_spec.rb b/spec/services/spam/handler_spec.rb new file mode 100644 index 000000000..fd85411a2 --- /dev/null +++ b/spec/services/spam/handler_spec.rb @@ -0,0 +1,125 @@ +require "rails_helper" + +RSpec.describe Spam::Handler, type: :service do + describe ".handle_article!" do + subject(:handler) { described_class.handle_article!(article: article) } + + let!(:article) { create(:article) } + let(:mascot_user) { create(:user) } + + before do + allow(Settings::General).to receive(:mascot_user_id).and_return(mascot_user.id) + end + + context "when non-spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(false) + end + + it { is_expected.to eq(:not_spam) } + end + + context "when first time spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(true) + allow(Reaction).to receive(:user_has_been_given_too_many_spammy_article_reactions?) + .with(user: article.user).and_return(false) + end + + it "creates a reaction but does not suspend the user" do + expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1) + expect(article.user.reload).not_to be_suspended + end + end + + context "when multiple offender of spammy" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(true) + allow(Reaction).to receive(:user_has_been_given_too_many_spammy_article_reactions?) + .with(user: article.user).and_return(true) + end + + it "creates a reaction, suspends the user, and creates a note for the user" do + expect { handler }.to change { Reaction.where(reactable: article, category: "vomit").count }.by(1) + expect(article.user.reload).to be_suspended + expect(Note.where(noteable: article.user, reason: "automatic_suspend").count).to eq(1) + end + end + end + + describe ".handle_comment!" do + subject(:handler) { described_class.handle_comment!(comment: comment) } + + let!(:comment) { create(:comment) } + let(:mascot_user) { create(:user) } + + before do + allow(Settings::RateLimit).to receive(:user_considered_new?).with(user: comment.user).and_return(true) + allow(Settings::General).to receive(:mascot_user_id).and_return(mascot_user.id) + end + + context "when non-spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(false) + end + + it { is_expected.to eq(:not_spam) } + end + + context "when first time spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(true) + allow(Reaction).to receive(:user_has_been_given_too_many_spammy_article_reactions?) + .with(user: comment.user).and_return(false) + end + + it "creates a reaction but does not suspend the user" do + expect { handler }.to change { Reaction.where(reactable: comment, category: "vomit").count }.by(1) + expect(comment.user.reload).not_to be_suspended + end + end + + context "when multiple offender of spammy" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(true) + allow(Reaction).to receive(:user_has_been_given_too_many_spammy_comment_reactions?) + .with(user: comment.user).and_return(true) + end + + it "creates a reaction, suspends the user, and creates a note for the user" do + expect { handler }.to change { Reaction.where(reactable: comment, category: "vomit").count }.by(1) + expect(comment.user.reload).to be_suspended + expect(Note.where(noteable: comment.user, reason: "automatic_suspend").count).to eq(1) + end + end + end + + describe ".handle_user!" do + subject(:handler) { described_class.handle_user!(user: user) } + + let!(:user) { create(:user) } + let(:mascot_user) { create(:user) } + + before do + allow(Settings::General).to receive(:mascot_user_id).and_return(mascot_user.id) + end + + context "when non-spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(false) + end + + it { is_expected.to eq(:not_spam) } + end + + context "when first time spammy content" do + before do + allow(Settings::RateLimit).to receive(:trigger_spam_for?).and_return(true) + end + + it "creates a reaction but does not suspend the user" do + expect { handler }.to change { Reaction.where(reactable: user, category: "vomit").count }.by(1) + end + end + end +end