From 96aefa873a13ee87e6b3f160c2e60625c9546648 Mon Sep 17 00:00:00 2001 From: Molly Struve Date: Mon, 11 May 2020 10:42:37 -0500 Subject: [PATCH] [deploy] Limit User Email Changes (#7746) --- app/helpers/rate_limit_checker_helper.rb | 5 ++ app/models/site_config.rb | 1 + app/models/user.rb | 14 +++++ app/services/rate_limit_checker.rb | 1 + spec/initializers/rack/attack_spec.rb | 5 +- spec/models/user_spec.rb | 17 ++++-- spec/services/rate_limit_checker_spec.rb | 75 ++++++++---------------- 7 files changed, 59 insertions(+), 59 deletions(-) diff --git a/app/helpers/rate_limit_checker_helper.rb b/app/helpers/rate_limit_checker_helper.rb index dfacd1c5d..8c4651f36 100644 --- a/app/helpers/rate_limit_checker_helper.rb +++ b/app/helpers/rate_limit_checker_helper.rb @@ -39,6 +39,11 @@ module RateLimitCheckerHelper min: 1, placeholder: 10, description: "The number of times a user can react in a 30 second period" + }, + rate_limit_send_email_confirmation: { + min: 1, + placeholder: 2, + description: "The number of times we will send a confirmation email to a user in a 2 minute period" } }.freeze diff --git a/app/models/site_config.rb b/app/models/site_config.rb index a8f1be2c0..fddd31322 100644 --- a/app/models/site_config.rb +++ b/app/models/site_config.rb @@ -81,6 +81,7 @@ class SiteConfig < RailsSettings::Base field :rate_limit_image_upload, type: :integer, default: 9 field :rate_limit_email_recipient, type: :integer, default: 5 field :rate_limit_article_update, type: :integer, default: 150 + field :rate_limit_send_email_confirmation, type: :integer, default: 2 # Google Analytics Reporting API v4 # diff --git a/app/models/user.rb b/app/models/user.rb index 55becabf9..73372c7f9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -140,6 +140,7 @@ class User < ApplicationRecord validate :unique_including_orgs_and_podcasts, if: :username_changed? validate :validate_feed_url, if: :feed_url_changed? validate :validate_mastodon_url + validate :can_send_confirmation_email alias_attribute :positive_reactions_count, :reactions_count alias_attribute :subscribed_to_welcome_notifications?, :welcome_notifications @@ -450,6 +451,10 @@ class User < ApplicationRecord search_score end + def rate_limiter + RateLimitChecker.new(self) + end + private def estimate_default_language @@ -604,4 +609,13 @@ class User < ApplicationRecord def index_roles(_role) index_to_elasticsearch_inline end + + def can_send_confirmation_email + return if changes[:email].blank? + + rate_limiter.track_limit_by_action(:send_email_confirmation) + rate_limiter.check_limit!(:send_email_confirmation) + rescue RateLimitChecker::LimitReached => e + errors.add(:email, "confirmation could not be sent. #{e.message}") + end end diff --git a/app/services/rate_limit_checker.rb b/app/services/rate_limit_checker.rb index bbe3378f5..cbf94c089 100644 --- a/app/services/rate_limit_checker.rb +++ b/app/services/rate_limit_checker.rb @@ -4,6 +4,7 @@ class RateLimitChecker # retry_after values are the seconds until a user can retry an action ACTION_LIMITERS = { article_update: { retry_after: 30 }, + send_email_confirmation: { retry_after: 120 }, image_upload: { retry_after: 30 }, listing_creation: { retry_after: 60 }, published_article_creation: { retry_after: 30 }, diff --git a/spec/initializers/rack/attack_spec.rb b/spec/initializers/rack/attack_spec.rb index 8c61db2e5..144700d39 100644 --- a/spec/initializers/rack/attack_spec.rb +++ b/spec/initializers/rack/attack_spec.rb @@ -2,10 +2,9 @@ require "rails_helper" describe Rack::Attack, type: :request, throttle: true do before do - redis_url = "redis://localhost:6379" - cache_db = ActiveSupport::Cache::RedisStore.new(redis_url) + cache_db = ActiveSupport::Cache.lookup_store(:redis_cache_store) allow(Rails).to receive(:cache) { cache_db } - cache_db.data.flushdb + cache_db.redis.flushdb allow(Honeycomb).to receive(:add_field) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a44b18849..9f6ab3512 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -177,28 +177,37 @@ RSpec.describe User, type: :model do it "validates username against reserved words" do user = build(:user, username: "readinglist") expect(user).not_to be_valid - expect(user.errors[:username].to_s.include?("reserved")).to be true + expect(user.errors[:username].to_s).to include("reserved") end it "takes organization slug into account" do create(:organization, slug: "lightalloy") user = build(:user, username: "lightalloy") expect(user).not_to be_valid - expect(user.errors[:username].to_s.include?("taken")).to be true + expect(user.errors[:username].to_s).to include("taken") end it "takes podcast slug into account" do create(:podcast, slug: "lightpodcast") user = build(:user, username: "lightpodcast") expect(user).not_to be_valid - expect(user.errors[:username].to_s.include?("taken")).to be true + expect(user.errors[:username].to_s).to include("taken") end it "takes page slug into account" do create(:page, slug: "page_yo") user = build(:user, username: "page_yo") expect(user).not_to be_valid - expect(user.errors[:username].to_s.include?("taken")).to be true + expect(user.errors[:username].to_s).to include("taken") + end + + it "validates can_send_confirmation_email" do + user = build(:user) + limiter = RateLimitChecker.new(user) + allow(user).to receive(:rate_limiter).and_return(limiter) + allow(limiter).to receive(:limit_by_action).and_return(true) + expect(user).not_to be_valid + expect(user.errors[:email].to_s).to include("confirmation could not be sent. Rate limit reached") end end diff --git a/spec/services/rate_limit_checker_spec.rb b/spec/services/rate_limit_checker_spec.rb index 0545f2499..3c2594fd1 100644 --- a/spec/services/rate_limit_checker_spec.rb +++ b/spec/services/rate_limit_checker_spec.rb @@ -5,11 +5,34 @@ RSpec.describe RateLimitChecker, type: :service do let(:article) { create(:article, user: user) } let(:rate_limit_checker) { described_class.new(user) } + def cache_key(action) + rate_limit_checker.send("limit_cache_key", action) + end + describe "#limit_by_action" do it "returns false for invalid action" do expect(rate_limit_checker.limit_by_action("random-nothing")).to be(false) end + # published_article_creation limit we check against the database rather than our cache + RateLimitChecker::ACTION_LIMITERS.except(:published_article_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), + ).and_return(SiteConfig.public_send("rate_limit_#{action}") + 1) + + expect(rate_limit_checker.limit_by_action(action)).to be(true) + end + + it "returns false if #{action} limit has NOT been reached" do + allow(Rails.cache).to receive(:read).with( + cache_key(action), + ).and_return(SiteConfig.public_send("rate_limit_#{action}")) + + expect(rate_limit_checker.limit_by_action(action)).to be(false) + end + end + context "when creating comments" do before do allow(SiteConfig).to receive(:rate_limit_comment_creation).and_return(1) @@ -20,14 +43,6 @@ RSpec.describe RateLimitChecker, type: :service do expect(rate_limit_checker.limit_by_action("comment_creation")).to be(true) end - it "sends a slack message if a user leaves too any messages" do - create_list(:comment, 2, user_id: user.id, commentable: article) - - sidekiq_assert_enqueued_with(job: Slack::Messengers::Worker) do - rate_limit_checker.limit_by_action("comment_creation") - end - end - it "returns false if allowed comment" do expect(rate_limit_checker.limit_by_action("comment_creation")).to be(false) end @@ -67,50 +82,6 @@ RSpec.describe RateLimitChecker, type: :service do expect(described_class.new(user).limit_by_action("published_article_creation")).to be(false) end - it "returns true if a user has uploaded too many images" do - allow(Rails.cache). - to receive(:read).with("#{user.id}_image_upload"). - and_return(SiteConfig.rate_limit_image_upload + 1) - - expect(rate_limit_checker.limit_by_action("image_upload")).to be(true) - end - - it "returns false if a user hasn't uploaded too many images" do - allow(Rails.cache). - to receive(:read).with("#{user.id}_image_upload"). - and_return(SiteConfig.rate_limit_image_upload) - - expect(rate_limit_checker.limit_by_action("image_upload")).to be(false) - end - - it "returns true if a user has updated too many articles" do - allow(Rails.cache). - to receive(:read).with("#{user.id}_article_update"). - and_return(SiteConfig.rate_limit_article_update + 1) - - expect(rate_limit_checker.limit_by_action("article_update")).to be(true) - end - - it "returns false if a user hasn't updated too many articles" do - allow(Rails.cache). - to receive(:read).with("#{user.id}_article_update"). - and_return(SiteConfig.rate_limit_article_update) - - expect(rate_limit_checker.limit_by_action("article_update")).to be(false) - end - - it "returns true if user has created too many organizations" do - allow(Rails.cache). - to receive(:read).with("#{user.id}_organization_creation"). - and_return(SiteConfig.rate_limit_organization_creation + 1) - - expect(rate_limit_checker.limit_by_action("organization_creation")).to be(true) - end - - it "returns false if organization_creation limit has not been reached" do - expect(described_class.new(user).limit_by_action("organization_creation")).to be(false) - end - it "logs a rate limit hit to datadog" do allow(Rails.cache). to receive(:read).with("#{user.id}_organization_creation").