Remove rack attack throttle for admin users in API requests (#18612)

* remove rack attack throttle for admin users

* Add tests

* Optimize cache admin secrets for 24h with invalidation when needed

* Make ApiSecret callback only execute on create

app/models/api_secret.rb

@@ -9,6 +9,8 @@ class ApiSecret < ApplicationRecord
   validates :description, presence: true, length: { maximum: 300 }
   validate :user_api_secret_count
 
+  after_create_commit :clear_rack_attack_cache
+
   private
 
   def user_api_secret_count
@@ -16,4 +18,14 @@ class ApiSecret < ApplicationRecord
 
     errors.add(:user, I18n.t("models.api_secret.api_limit_reached"))
   end
+
+  def clear_rack_attack_cache
+    admin_api_secret = User.joins(:roles)
+      .exists?(roles: { name: Rack::Attack::ADMIN_ROLES }, users: { id: user_id })
+    return unless admin_api_secret
+
+    # ApiSecret that belongs to an admin should clear the Rack::Attack
+    # cache so they can bypass API throttling immediately
+    Rails.cache.delete(Rack::Attack::ADMIN_API_CACHE_KEY)
+  end
 end

app/services/moderator/manage_activity_and_roles.rb

@@ -99,6 +99,11 @@ module Moderator
       check_super_admin
       remove_negative_roles
       user.add_role(role)
+
+      # Clear cache key if the elevated role matches Rack::Attack bypass roles
+      return unless Rack::Attack::ADMIN_ROLES.include?(role.to_s)
+
+      Rails.cache.delete(Rack::Attack::ADMIN_API_CACHE_KEY)
     end
 
     def check_super_admin

config/initializers/rack_attack.rb

@@ -2,6 +2,26 @@ Rack::Attack.throttled_response_retry_after_header = true
 
 module Rack
   class Attack
+    ADMIN_API_CACHE_KEY = "rack_attack_admin_api_keys".freeze
+    ADMIN_ROLES = %w[admin super_admin tech_admin].freeze
+
+    # Method that checks API Key from the request and returns true if it
+    # belongs to an admin, false otherwise
+    def self.admin_api_key?(request)
+      api_key = request.env["HTTP_API_KEY"]
+      return false if api_key.nil?
+
+      # Admin API Secrets are cached to avoid making DB queries on each request
+      admin_keys = Rails.cache.fetch(ADMIN_API_CACHE_KEY, expires_in: 24.hours) do
+        ApiSecret.joins(user: :roles)
+          .where(roles: { name: ADMIN_ROLES })
+          .group("api_secrets.id")
+          .pluck(:secret)
+      end
+
+      admin_keys.include?(api_key)
+    end
+
     class Request < ::Rack::Request
       def track_and_return_ip
         if ApplicationConfig["FASTLY_API_KEY"].present?
@@ -20,19 +40,23 @@ module Rack
     end
 
     throttle("api_throttle", limit: 3, period: 1) do |request|
-      if request.path.starts_with?("/api/") && request.get?
+      api_endpoint = request.path.starts_with?("/api/")
+      if api_endpoint && request.get? && !admin_api_key?(request)
         request.track_and_return_ip
       end
     end
 
     throttle("api_write_throttle", limit: 1, period: 1) do |request|
-      if request.path.starts_with?("/api/") && (request.put? || request.post? || request.delete?)
+      api_endpoint = request.path.starts_with?("/api/")
+      if api_endpoint && (request.put? || request.post? || request.delete?)
         Honeycomb.add_field("user_api_key", request.env["HTTP_API_KEY"])
-        ip_address = request.track_and_return_ip
-        if request.env["HTTP_API_KEY"].present?
-          "#{ip_address}-#{request.env['HTTP_API_KEY']}"
-        elsif ip_address.present?
-          ip_address
+        unless admin_api_key?(request)
+          ip_address = request.track_and_return_ip
+          if request.env["HTTP_API_KEY"].present?
+            "#{ip_address}-#{request.env['HTTP_API_KEY']}"
+          elsif ip_address.present?
+            ip_address
+          end
         end
       end
     end

spec/initializers/rack_attack_spec.rb

@@ -50,6 +50,20 @@ describe Rack, ".attack", type: :request, throttle: true do
         expect(Honeycomb).to have_received(:add_field).with("fastly_client_ip", "1.1.1.1").exactly(2).times
       end
     end
+
+    it "doesn't throttle when API key provided belongs to admin" do
+      admin_api_key = create(:api_secret, user: create(:user, :admin))
+
+      Timecop.freeze do
+        headers = { "HTTP_FASTLY_CLIENT_IP" => "5.6.7.8", "api-key" => admin_api_key.secret }
+        valid_responses = Array.new(10).map do
+          get api_articles_path, headers: headers
+        end
+
+        valid_responses.each { |r| expect(r).not_to eq(429) }
+        expect(Honeycomb).to have_received(:add_field).with("fastly_client_ip", "5.6.7.8").exactly(10).times
+      end
+    end
   end
 
   describe "api_write_throttle" do
@@ -100,6 +114,26 @@ describe Rack, ".attack", type: :request, throttle: true do
         expect(Honeycomb).to have_received(:add_field).with("fastly_client_ip", "1.1.1.1").exactly(2).times
       end
     end
+
+    it "doesn't throttle api write endpoints when API key provided belongs to admin" do
+      admin_api_key = create(:api_secret, user: create(:user, :admin))
+      params = { article: { body_markdown: "", title: Faker::Book.title } }.to_json
+      admin_headers = {
+        "api-key" => admin_api_key.secret,
+        "content-type" => "application/json",
+        "HTTP_FASTLY_CLIENT_IP" => "5.6.7.8"
+      }
+
+      Timecop.freeze do
+        valid_responses = Array.new(10).map do
+          post api_articles_path, params: params, headers: admin_headers
+        end
+
+        valid_responses.each { |r| expect(r).not_to eq(429) }
+        expect(Honeycomb).to have_received(:add_field).with("fastly_client_ip", "5.6.7.8").exactly(10).times
+        expect(Honeycomb).to have_received(:add_field).with("user_api_key", admin_api_key.secret).exactly(10).times
+      end
+    end
   end
 
   describe "tag_throttle" do

spec/models/api_secret_spec.rb

@@ -17,4 +17,29 @@ RSpec.describe ApiSecret, type: :model do
       expect(invalid_secret.errors.full_messages.join).to include("limit of 10 per user has been reached")
     end
   end
+
+  describe "Rack::Attack cache invalidation optimization" do
+    before do
+      cache_db = ActiveSupport::Cache.lookup_store(:redis_cache_store)
+      allow(Rails).to receive(:cache) { cache_db }
+
+      allow(Rails.cache).to receive(:delete)
+      allow(Rails.cache).to receive(:delete)
+        .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+    end
+
+    context "when ApiSecret is created" do
+      it "clears the cache if it belongs to an admin" do
+        create(:api_secret, user: create(:user, :admin))
+        expect(Rails.cache).to have_received(:delete)
+          .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+      end
+
+      it "doesn't clear the cache if it belongs to an admin" do
+        create(:api_secret)
+        expect(Rails.cache).not_to have_received(:delete)
+          .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+      end
+    end
+  end
 end

spec/services/moderator/manage_activity_and_roles_spec.rb

@@ -84,6 +84,40 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
     expect(user.roles.count).to eq(0)
   end
 
+  describe "Rack::Attack cache invalidation optimization" do
+    before do
+      cache_db = ActiveSupport::Cache.lookup_store(:redis_cache_store)
+      allow(Rails).to receive(:cache) { cache_db }
+
+      allow(Rails.cache).to receive(:delete)
+      allow(Rails.cache).to receive(:delete)
+        .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+    end
+
+    it "clears Rack::Attack cache if assigned admin role to user" do
+      described_class.handle_user_roles(
+        admin: admin,
+        user: user,
+        user_params: { note_for_current_role: "Upgrading to tech admin", user_status: "Super Admin" },
+      )
+
+      expect(Rails.cache).to have_received(:delete)
+        .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+    end
+
+    it "doesn't Rack::Attack cache if assigned non-admin role to user" do
+      user.add_role(:comment_suspended)
+      described_class.handle_user_roles(
+        admin: admin,
+        user: user,
+        user_params: { note_for_current_role: "Upgrading to trusted user", user_status: "Good standing" },
+      )
+
+      expect(Rails.cache).not_to have_received(:delete)
+        .with(Rack::Attack::ADMIN_API_CACHE_KEY)
+    end
+  end
+
   context "when not super admin" do
     before do
       admin.remove_role(:super_admin)