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 <citizen428@forem.com>

* Update app/models/reaction.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* 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 <citizen428@forem.com>

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

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

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?

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
 

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"

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

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<Symbol>] 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

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

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
 

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
 

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
 

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) }
 

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
 

spec/services/spam/article_handler_spec.rb

@@ -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

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