Refactor RateLimitChecker (#5521)

app/labor/rate_limit_checker.rb

@@ -1,10 +1,6 @@
 class RateLimitChecker
   attr_reader :user, :action
 
-  def self.daily_account_follow_limit
-    SiteConfig.rate_limit_follow_count_daily
-  end
-
   def initialize(user = nil)
     @user = user
   end
@@ -13,18 +9,9 @@ class RateLimitChecker
   class DailyFollowAccountLimitReached < StandardError; end
 
   def limit_by_action(action)
-    result = case action
-             when "comment_creation"
-               user.comments.where("created_at > ?", 30.seconds.ago).size > 9
-             when "published_article_creation"
-               user.articles.published.where("created_at > ?", 30.seconds.ago).size > 9
-             when "image_upload"
-               Rails.cache.read("#{user.id}_image_upload").to_i > 9
-             when "follow_account"
-               user_today_follow_count > self.class.daily_account_follow_limit
-             else
-               false
-             end
+    check_method = "check_#{action}_limit"
+    result = respond_to?(check_method, true) ? send(check_method) : false
+
     if result
       @action = action
       ping_admins
@@ -40,19 +27,38 @@ class RateLimitChecker
 
   def limit_by_email_recipient_address(address)
     # This is related to the recipient, not the "user" initiator, like in action.
-    EmailMessage.where(to: address).
-      where("sent_at > ?", 2.minutes.ago).size > 5
+    EmailMessage.where(to: address).where("sent_at > ?", 2.minutes.ago).size >
+      SiteConfig.rate_limit_email_recipient
+  end
+
+  private
+
+  def check_comment_creation_limit
+    user.comments.where("created_at > ?", 30.seconds.ago).size >
+      SiteConfig.rate_limit_comment_creation
+  end
+
+  def check_published_article_creation_limit
+    user.articles.published.where("created_at > ?", 30.seconds.ago).size >
+      SiteConfig.rate_limit_published_article_creation
+  end
+
+  def check_image_upload_limit
+    Rails.cache.read("#{user.id}_image_upload").to_i >
+      SiteConfig.rate_limit_image_upload
+  end
+
+  def check_follow_account_limit
+    user_today_follow_count > SiteConfig.rate_limit_follow_count_daily
   end
 
   def ping_admins
     RateLimitCheckerWorker.perform_async(user.id, action)
   end
 
-  private
-
   def user_today_follow_count
     following_users_count = user.following_users_count
-    return following_users_count if following_users_count < self.class.daily_account_follow_limit
+    return following_users_count if following_users_count < SiteConfig.rate_limit_follow_count_daily
 
     now = Time.zone.now
     user.follows.where(created_at: (now.beginning_of_day..now)).size

app/models/site_config.rb

@@ -20,6 +20,10 @@ class SiteConfig < RailsSettings::Base
 
   # rate limits
   field :rate_limit_follow_count_daily, type: :integer, default: 500
+  field :rate_limit_comment_creation, type: :integer, default: 9
+  field :rate_limit_published_article_creation, type: :integer, default: 9
+  field :rate_limit_image_upload, type: :integer, default: 9
+  field :rate_limit_email_recipient, type: :integer, default: 5
 
   # Google Analytics Reporting API v4
   # <https://developers.google.com/analytics/devguides/reporting/core/v4>

spec/labor/rate_limit_checker_spec.rb

@@ -4,83 +4,89 @@ RSpec.describe RateLimitChecker, type: :labor do
   let(:user) { create(:user) }
   let(:article) { create(:article, user_id: user.id) }
 
-  describe "self.daily_account_follow_limit " do
-    it "returns the value set in SiteConfig.rate_limit_follow_count_daily" do
-      expect(described_class.daily_account_follow_limit).to eq(SiteConfig.rate_limit_follow_count_daily)
-    end
-  end
-
   describe "#limit_by_action" do
+    let(:rate_limit_checker) { described_class.new(user) }
+
     it "returns false for invalid action" do
-      expect(described_class.new(user).limit_by_action("random-nothing")).to eq(false)
+      expect(rate_limit_checker.limit_by_action("random-nothing")).to eq(false)
     end
 
-    it "returns true if too many comments at once" do
-      create_list(:comment, 10, user_id: user.id, commentable_id: article.id)
-      expect(described_class.new(user).limit_by_action("comment_creation")).to eq(true)
-    end
+    context "when creating comments" do
+      before do
+        allow(SiteConfig).to receive(:rate_limit_comment_creation).and_return(1)
+      end
 
-    it "triggers ping admin when too many comments" do
-      allow(RateLimitCheckerWorker).to receive(:perform_async)
-      create_list(:comment, 10, user_id: user.id, commentable_id: article.id)
-      described_class.new(user).limit_by_action("comment_creation")
-      expect(RateLimitCheckerWorker).to have_received(:perform_async).with(user.id, "comment_creation")
-    end
+      it "returns true if too many comments at once" do
+        create_list(:comment, 2, user_id: user.id, commentable_id: article.id)
+        expect(rate_limit_checker.limit_by_action("comment_creation")).to eq(true)
+      end
 
-    it "returns false if allowed comment" do
-      create_list(:comment, 2, user_id: user.id, commentable_id: article.id)
-      expect(described_class.new(user).limit_by_action("comment_creation")).to eq(false)
+      it "triggers ping admin when too many comments" do
+        allow(RateLimitCheckerWorker).to receive(:perform_async)
+        create_list(:comment, 2, user_id: user.id, commentable_id: article.id)
+        rate_limit_checker.limit_by_action("comment_creation")
+        expect(RateLimitCheckerWorker).to have_received(:perform_async).with(user.id, "comment_creation")
+      end
+
+      it "returns false if allowed comment" do
+        expect(rate_limit_checker.limit_by_action("comment_creation")).to eq(false)
+      end
     end
 
     it "returns true if too many published articles at once" do
-      create_list(:article, 10, user_id: user.id, published: true)
-      expect(described_class.new(user).limit_by_action("published_article_creation")).to eq(true)
+      allow(SiteConfig).to receive(:rate_limit_published_article_creation).and_return(1)
+      create_list(:article, 2, user_id: user.id, published: true)
+      expect(rate_limit_checker.limit_by_action("published_article_creation")).to eq(true)
     end
 
     it "returns true if a user has followed more than <daily_limit> accounts today" do
-      rate_limit_checker = described_class.new(user)
-
       allow(rate_limit_checker).
         to receive(:user_today_follow_count).
-        and_return(described_class.daily_account_follow_limit + 1)
+        and_return(SiteConfig.rate_limit_follow_count_daily + 1)
 
       expect(rate_limit_checker.limit_by_action("follow_account")).to eq(true)
     end
 
     it "returns false if a user's following_users_count is less than <daily_limit>" do
-      rate_limit_checker = described_class.new(user)
-
       allow(user).
         to receive(:following_users_count).
-        and_return(described_class.daily_account_follow_limit - 1)
+        and_return(SiteConfig.rate_limit_follow_count_daily - 1)
 
       expect(rate_limit_checker.limit_by_action("follow_account")).to eq(false)
     end
 
     it "returns false if a user has followed less than <daily_limit> accounts today" do
-      rate_limit_checker = described_class.new(user)
-
       allow(rate_limit_checker).
         to receive(:user_today_follow_count).
-        and_return(described_class.daily_account_follow_limit - 1)
+        and_return(SiteConfig.rate_limit_image_upload + 1)
 
       expect(rate_limit_checker.limit_by_action("follow_account")).to eq(false)
     end
 
-    it "returns false if published articles comment" do
-      create_list(:article, 2, user_id: user.id, published: true)
+    it "returns false if published articles limit has not been reached" do
       expect(described_class.new(user).limit_by_action("published_article_creation")).to eq(false)
     end
+
+    it "returns false if a user uploads too many images" do
+      allow(rate_limit_checker).
+        to receive(:track_image_uploads).
+        and_return(SiteConfig.rate_limit_follow_count_daily - 1)
+
+      expect(rate_limit_checker.limit_by_action("image_upload")).to eq(false)
+    end
   end
 
   describe "#limit_by_email_recipient_address" do
-    it "returns true if too many published articles at once" do
-      10.times { EmailMessage.create(to: user.email, sent_at: Time.current) }
+    before do
+      allow(SiteConfig).to receive(:rate_limit_email_recipient).and_return(1)
+    end
+
+    it "returns true if too many emails are sent to the same recipient" do
+      2.times { EmailMessage.create(to: user.email, sent_at: Time.current) }
       expect(described_class.new.limit_by_email_recipient_address(user.email)).to eq(true)
     end
 
-    it "returns false if published articles comment" do
-      2.times { EmailMessage.create(to: user.email, sent_at: Time.current) }
+    it "returns false if we are below the message limit for this recipient" do
       expect(described_class.new.limit_by_email_recipient_address(user.email)).to eq(false)
     end
   end

spec/requests/follows_create_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe "Follows #create", type: :request do
 
       allow(rate_limit_checker).
         to receive(:user_today_follow_count).
-        and_return(RateLimitChecker.daily_account_follow_limit + 1)
+        and_return(SiteConfig.rate_limit_follow_count_daily + 1)
 
       allow(RateLimitChecker).
         to receive(:new).