[deploy] Limit User Email Changes (#7746)

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
 

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
   # <https://developers.google.com/analytics/devguides/reporting/core/v4>

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

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

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
 

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
 

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").