diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index bd36a91f8..4982a53c9 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -51,7 +51,7 @@ class CommentsController < ApplicationController # POST /comments.json def create authorize Comment - raise if RateLimitChecker.new(current_user).limit_by_situation("comment_creation") + raise if RateLimitChecker.new(current_user).limit_by_action("comment_creation") @comment = Comment.new(permitted_attributes(Comment)) @comment.user_id = current_user.id diff --git a/app/controllers/image_uploads_controller.rb b/app/controllers/image_uploads_controller.rb index 6b03c52df..3f5e500f2 100644 --- a/app/controllers/image_uploads_controller.rb +++ b/app/controllers/image_uploads_controller.rb @@ -5,7 +5,7 @@ class ImageUploadsController < ApplicationController def create authorize :image_upload begin - raise RateLimitChecker::UploadRateLimitReached if RateLimitChecker.new(current_user).limit_by_situation("image_upload") + raise RateLimitChecker::UploadRateLimitReached if RateLimitChecker.new(current_user).limit_by_action("image_upload") raise CarrierWave::IntegrityError if params[:image].blank? uploaders = image_upload(params[:image]) diff --git a/app/jobs/rate_limit_checker_job.rb b/app/jobs/rate_limit_checker_job.rb index 769ccf381..e2020ed24 100644 --- a/app/jobs/rate_limit_checker_job.rb +++ b/app/jobs/rate_limit_checker_job.rb @@ -1,8 +1,8 @@ class RateLimitCheckerJob < ApplicationJob queue_as :rate_limit_checker - def perform(user_id) + def perform(user_id, action) user = User.find_by(id: user_id) - PingAdmins.call(user) if user + PingAdmins.call(user, action) if user end end diff --git a/app/labor/rate_limit_checker.rb b/app/labor/rate_limit_checker.rb index 0862821ce..3307cd0b2 100644 --- a/app/labor/rate_limit_checker.rb +++ b/app/labor/rate_limit_checker.rb @@ -1,13 +1,14 @@ class RateLimitChecker - attr_accessor :user + attr_reader :user, :action + def initialize(user = nil) @user = user end class UploadRateLimitReached < StandardError; end - def limit_by_situation(situation) - result = case situation + 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" @@ -17,7 +18,10 @@ class RateLimitChecker else false end - ping_admins if result == true + if result + @action = action + ping_admins + end result end @@ -28,16 +32,16 @@ class RateLimitChecker end def limit_by_email_recipient_address(address) - # This is related to the recipient, not the "user" initiator, like in situation. + # 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 end def ping_admins - RateLimitCheckerJob.perform_later(user.id) + RateLimitCheckerJob.perform_later(user.id, action) end def ping_admins_without_delay - RateLimitCheckerJob.perform_now(user.id) + RateLimitCheckerJob.perform_now(user.id, action) end end diff --git a/app/services/articles/creator.rb b/app/services/articles/creator.rb index 1d519e7af..ef6389b0b 100644 --- a/app/services/articles/creator.rb +++ b/app/services/articles/creator.rb @@ -10,7 +10,7 @@ module Articles end def call - raise if RateLimitChecker.new(user).limit_by_situation("published_article_creation") + raise if RateLimitChecker.new(user).limit_by_action("published_article_creation") tags = article_params[:tags] series = article_params[:series] diff --git a/app/services/ping_admins.rb b/app/services/ping_admins.rb index 46cec2bf2..3e621e99e 100644 --- a/app/services/ping_admins.rb +++ b/app/services/ping_admins.rb @@ -1,6 +1,7 @@ class PingAdmins - def initialize(user) + def initialize(user, action = "unknown") @user = user + @action = action end def self.call(*args) @@ -11,7 +12,7 @@ class PingAdmins return unless user && Rails.env.production? SlackBot.ping( - "Rate limit exceeded. https://dev.to#{user.path}", + "Rate limit exceeded (#{action}). https://dev.to#{user.path}", channel: "abuse-reports", username: "rate_limit", icon_emoji: ":hand:", @@ -20,5 +21,5 @@ class PingAdmins private - attr_reader :user + attr_reader :user, :action end diff --git a/spec/jobs/rate_limit_checker_job_spec.rb b/spec/jobs/rate_limit_checker_job_spec.rb index 8271ccd10..0835ced66 100644 --- a/spec/jobs/rate_limit_checker_job_spec.rb +++ b/spec/jobs/rate_limit_checker_job_spec.rb @@ -17,15 +17,15 @@ RSpec.describe RateLimitCheckerJob, type: :job do it "calls a service" do perform_enqueued_jobs do - described_class.perform_now(user.id) + described_class.perform_now(user.id, "test") - expect(service).to have_received(:call).with(user).once + expect(service).to have_received(:call).with(user, "test").once end end it "does nothing for non-existent user" do perform_enqueued_jobs do - described_class.perform_now(nil) + described_class.perform_now(nil, "test") expect(service).not_to have_received(:call) end diff --git a/spec/labor/rate_limit_checker_spec.rb b/spec/labor/rate_limit_checker_spec.rb index 0f4d43b00..fc957434c 100644 --- a/spec/labor/rate_limit_checker_spec.rb +++ b/spec/labor/rate_limit_checker_spec.rb @@ -4,41 +4,48 @@ RSpec.describe RateLimitChecker do let(:user) { create(:user) } let(:article) { create(:article, user_id: user.id) } - it ".limit_by_situation returns false for invalid case" do - expect(described_class.new(user).limit_by_situation("random-nothing")).to eq(false) - end - - it ".limit_by_situation 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_situation("comment_creation")).to eq(true) - end - - it ".limit_by_situation 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_situation("comment_creation")).to eq(false) - end - - it ". limit_by_situation 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_situation("published_article_creation")).to eq(true) - end - - it ".limit_by_situation returns false if published articles comment" do - create_list(:article, 2, user_id: user.id, published: true) - expect(described_class.new(user).limit_by_situation("published_article_creation")).to eq(false) - end - - it ".limit_by_email_recipient_address returns true if too many published articles at once" do - 10.times do - EmailMessage.create(to: user.email, sent_at: Time.current) + describe "#limit_by_action" do + it "returns false for invalid action" do + expect(described_class.new(user).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 + + it "triggers ping admin when too many comments" do + allow(RateLimitCheckerJob).to receive(:perform_later) + create_list(:comment, 10, user_id: user.id, commentable_id: article.id) + described_class.new(user).limit_by_action("comment_creation") + expect(RateLimitCheckerJob).to have_received(:perform_later).with(user.id, "comment_creation") + 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) + 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) + end + + it "returns false if published articles comment" do + create_list(:article, 2, user_id: user.id, published: true) + expect(described_class.new(user).limit_by_action("published_article_creation")).to eq(false) end - expect(described_class.new.limit_by_email_recipient_address(user.email)).to eq(true) end - it ".limit_by_email_recipient_address returns false if published articles comment" do - 2.times do - EmailMessage.create(to: user.email, sent_at: Time.current) + 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) } + 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) } + expect(described_class.new.limit_by_email_recipient_address(user.email)).to eq(false) end - expect(described_class.new.limit_by_email_recipient_address(user.email)).to eq(false) end end