From 52fd043c337d8a83cf55cd0fa2417e8189c85105 Mon Sep 17 00:00:00 2001 From: Vaidehi Joshi Date: Tue, 6 Oct 2020 18:02:05 -0700 Subject: [PATCH] [deploy] Limit articles created by newer users (#10686) * Limit articles created by newer users * Fix typo in test --- app/controllers/admin/configs_controller.rb | 1 + app/helpers/rate_limit_checker_helper.rb | 7 ++++++- app/models/site_config.rb | 1 + app/services/articles/creator.rb | 8 +++++++- app/services/rate_limit_checker.rb | 8 ++++++++ spec/requests/admin/configs_spec.rb | 7 +++++++ spec/requests/articles/articles_create_spec.rb | 18 +++++++++++++++++- spec/services/rate_limit_checker_spec.rb | 16 ++++++++++++++-- 8 files changed, 61 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/configs_controller.rb b/app/controllers/admin/configs_controller.rb index 6c71b4bba..a8fe62e6e 100644 --- a/app/controllers/admin/configs_controller.rb +++ b/app/controllers/admin/configs_controller.rb @@ -165,6 +165,7 @@ module Admin rate_limit_follow_count_daily rate_limit_image_upload rate_limit_published_article_creation + rate_limit_published_article_antispam_creation rate_limit_organization_creation rate_limit_user_subscription_creation rate_limit_article_update diff --git a/app/helpers/rate_limit_checker_helper.rb b/app/helpers/rate_limit_checker_helper.rb index 176c67b9f..ef85b950d 100644 --- a/app/helpers/rate_limit_checker_helper.rb +++ b/app/helpers/rate_limit_checker_helper.rb @@ -33,7 +33,12 @@ module RateLimitCheckerHelper rate_limit_published_article_creation: { min: 0, placeholder: 9, - description: "The number of articles a user can create within 30 seconds" + description: "The number of articles that a user can create within 30 seconds" + }, + rate_limit_published_article_antispam_creation: { + min: 0, + placeholder: 1, + description: "The number of articles that a 3-day-old user can create within 5 minutes" }, rate_limit_image_upload: { min: 0, diff --git a/app/models/site_config.rb b/app/models/site_config.rb index da083ccfc..9bce84c9f 100644 --- a/app/models/site_config.rb +++ b/app/models/site_config.rb @@ -121,6 +121,7 @@ class SiteConfig < RailsSettings::Base field :rate_limit_comment_creation, type: :integer, default: 9 field :rate_limit_listing_creation, type: :integer, default: 1 field :rate_limit_published_article_creation, type: :integer, default: 9 + field :rate_limit_published_article_antispam_creation, type: :integer, default: 1 field :rate_limit_organization_creation, type: :integer, default: 1 field :rate_limit_reaction_creation, type: :integer, default: 10 field :rate_limit_image_upload, type: :integer, default: 9 diff --git a/app/services/articles/creator.rb b/app/services/articles/creator.rb index 64efadcde..7d5583a5f 100644 --- a/app/services/articles/creator.rb +++ b/app/services/articles/creator.rb @@ -31,7 +31,13 @@ module Articles attr_reader :user, :article_params, :event_dispatcher def rate_limit! - user.rate_limiter.check_limit!(:published_article_creation) + rate_limit_to_use = if user.created_at > 3.days.ago.beginning_of_day + :published_article_antispam_creation + else + :published_article_creation + end + + user.rate_limiter.check_limit!(rate_limit_to_use) end def dispatch_event(article) diff --git a/app/services/rate_limit_checker.rb b/app/services/rate_limit_checker.rb index a1f687bd4..b7fa8f5ef 100644 --- a/app/services/rate_limit_checker.rb +++ b/app/services/rate_limit_checker.rb @@ -9,6 +9,7 @@ class RateLimitChecker listing_creation: { retry_after: 60 }, organization_creation: { retry_after: 300 }, published_article_creation: { retry_after: 30 }, + published_article_antispam_creation: { retry_after: 300 }, reaction_creation: { retry_after: 30 }, send_email_confirmation: { retry_after: 120 }, user_subscription_creation: { retry_after: 30 }, @@ -85,10 +86,17 @@ class RateLimitChecker end def check_published_article_creation_limit + # TODO: Vaidehi Joshi - We should make this time frame configurable. user.articles.published.where("created_at > ?", 30.seconds.ago).size > SiteConfig.rate_limit_published_article_creation end + def check_published_article_antispam_creation_limit + # TODO: Vaidehi Joshi - We should make this time frame configurable. + user.articles.published.where("created_at > ?", 5.minutes.ago).size > + SiteConfig.rate_limit_published_article_antispam_creation + end + def check_follow_account_limit user_today_follow_count > SiteConfig.rate_limit_follow_count_daily end diff --git a/spec/requests/admin/configs_spec.rb b/spec/requests/admin/configs_spec.rb index 7d2f6d0c5..f9237f0f5 100644 --- a/spec/requests/admin/configs_spec.rb +++ b/spec/requests/admin/configs_spec.rb @@ -468,6 +468,13 @@ RSpec.describe "/admin/config", type: :request do end.to change(SiteConfig, :rate_limit_published_article_creation).from(9).to(3) end + it "updates rate_limit_published_article_antispam_creation" do + expect do + post "/admin/config", params: { site_config: { rate_limit_published_article_antispam_creation: 3 }, + confirmation: confirmation_message } + end.to change(SiteConfig, :rate_limit_published_article_antispam_creation).from(1).to(3) + end + it "updates rate_limit_organization_creation" do expect do post "/admin/config", params: { site_config: { rate_limit_organization_creation: 3 }, diff --git a/spec/requests/articles/articles_create_spec.rb b/spec/requests/articles/articles_create_spec.rb index a30323222..bdf381d5e 100644 --- a/spec/requests/articles/articles_create_spec.rb +++ b/spec/requests/articles/articles_create_spec.rb @@ -104,13 +104,29 @@ RSpec.describe "ArticlesCreate", type: :request do end context "when creation limit is reached" do - it "returns a too_many_requests response if rate limit is reached" do + it "returns a too_many_requests response if antispam rate limit is reached" do rate_limit_checker = RateLimitChecker.new(user) allow(RateLimitChecker).to receive(:new).and_return(rate_limit_checker) allow(rate_limit_checker).to receive(:limit_by_action).and_return(true) post articles_path, params: { article: { body_markdown: "123" } } + expect(response).to have_http_status(:too_many_requests) + expected_retry_after = RateLimitChecker::ACTION_LIMITERS.dig(:published_article_antispam_creation, :retry_after) + expect(response.headers["Retry-After"]).to eq(expected_retry_after) + end + + it "returns a too_many_requests response if rate limit is reached" do + # Explicitly create this user more than 3.days.ago, since we + # check for this in Articles::Creator#rate_limit! + user.update!(created_at: 4.days.ago) + + rate_limit_checker = RateLimitChecker.new(user) + allow(RateLimitChecker).to receive(:new).and_return(rate_limit_checker) + allow(rate_limit_checker).to receive(:limit_by_action).and_return(true) + + post articles_path, params: { article: { body_markdown: "123 i love to spam" } } + expect(response).to have_http_status(:too_many_requests) expected_retry_after = RateLimitChecker::ACTION_LIMITERS.dig(:published_article_creation, :retry_after) expect(response.headers["Retry-After"]).to eq(expected_retry_after) diff --git a/spec/services/rate_limit_checker_spec.rb b/spec/services/rate_limit_checker_spec.rb index d1db70854..faa73f3f5 100644 --- a/spec/services/rate_limit_checker_spec.rb +++ b/spec/services/rate_limit_checker_spec.rb @@ -26,8 +26,10 @@ RSpec.describe RateLimitChecker, type: :service do expect { limiter.limit_by_action(action) }.to raise_error("Invalid Cache Key: no unique component present") end - # published_article_creation limit we check against the database rather than our cache - described_class::ACTION_LIMITERS.except(:published_article_creation).each do |action, _options| + # We check published_article_creation + :published_article_antispam_creation + # limit against database, rather than our cache. + described_class::ACTION_LIMITERS.except(:published_article_creation, + :published_article_antispam_creation).each do |action, _options| it "returns true if #{action} limit has been reached" do allow(Rails.cache).to receive(:read).with( cache_key(action), raw: true @@ -60,6 +62,12 @@ RSpec.describe RateLimitChecker, type: :service do end end + it "returns true if too many published articles at once and potentially spammy" do + allow(SiteConfig).to receive(:rate_limit_published_article_antispam_creation).and_return(1) + create_list(:article, 2, user_id: user.id, published: true) + expect(rate_limit_checker.limit_by_action("published_article_antispam_creation")).to be(true) + end + it "returns true if too many published articles at once" do allow(SiteConfig).to receive(:rate_limit_published_article_creation).and_return(1) create_list(:article, 2, user_id: user.id, published: true) @@ -90,6 +98,10 @@ RSpec.describe RateLimitChecker, type: :service do expect(rate_limit_checker.limit_by_action("follow_account")).to be(false) end + it "returns false if published articles antispam limit has not been reached" do + expect(described_class.new(user).limit_by_action("published_article_antispam_creation")).to be(false) + end + it "returns false if published articles limit has not been reached" do expect(described_class.new(user).limit_by_action("published_article_creation")).to be(false) end