DEV: Remove Discourse.redis.delete_prefixed (#22103)

This method is a huge footgun in production, since it calls
the Redis KEYS command. From the Redis documentation at
https://redis.io/commands/keys/:

> Warning: consider KEYS as a command that should only be used in
production environments with extreme care. It may ruin performance when
it is executed against large databases. This command is intended for
debugging and special operations, such as changing your keyspace layout.
Don't use KEYS in your regular application code.

Since we were only using `delete_prefixed` in specs (now that we
removed the usage in production in 24ec06ff85)
we can remove this and instead rely on `use_redis_snapshotting` on the
particular tests that need this kind of clearing functionality.
This commit is contained in:
Martin Brennan 2023-06-16 12:44:35 +10:00 committed by GitHub
parent e89c70643b
commit 9174716737
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
24 changed files with 225 additions and 224 deletions

View File

@ -91,8 +91,4 @@ class RandomTopicSelector
def self.cache_key(category = nil)
"random_topic_cache_#{category&.id}"
end
def self.clear_cache!
Discourse.redis.delete_prefixed(cache_key)
end
end

View File

@ -213,10 +213,6 @@ class DiscourseRedis
end
end
def delete_prefixed(prefix)
DiscourseRedis.ignore_readonly { keys("#{prefix}*").each { |k| Discourse.redis.del(k) } }
end
def reconnect
@redis._client.reconnect
end

View File

@ -23,11 +23,6 @@ class RateLimiter
@disabled
end
# Only used in test, only clears current namespace, does not clear globals
def self.clear_all!
Discourse.redis.delete_prefixed(RateLimiter.key_prefix)
end
def self.clear_all_global!
Discourse
.redis

View File

@ -70,14 +70,17 @@ RSpec.describe Chat::IncomingWebhooksController do
)
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
10.times { post "/chat/hooks/#{webhook.key}.json", params: valid_payload }
expect(response.status).to eq(200)
describe "rate limiting" do
use_redis_snapshotting
post "/chat/hooks/#{webhook.key}.json", params: valid_payload
expect(response.status).to eq(429)
it "rate limits" do
RateLimiter.enable
10.times { post "/chat/hooks/#{webhook.key}.json", params: valid_payload }
expect(response.status).to eq(200)
post "/chat/hooks/#{webhook.key}.json", params: valid_payload
expect(response.status).to eq(429)
end
end
end

View File

@ -2,10 +2,9 @@
# frozen_string_literal: true
RSpec.describe "rate limiter integration" do
before do
RateLimiter.enable
RateLimiter.clear_all!
end
before { RateLimiter.enable }
use_redis_snapshotting
it "will rate limit message bus requests once queueing" do
freeze_time
@ -44,15 +43,11 @@ RSpec.describe "rate limiter integration" do
expect(response.cookies.has_key?(name)).to eq(true)
expect(response.cookies[name]).to be_nil
end
RateLimiter.clear_all!
end
it "can cleanly limit requests and sets a Retry-After header" do
freeze_time
RateLimiter.clear_all!
admin = Fabricate(:admin)
api_key = Fabricate(:api_key, user: admin)

View File

@ -216,9 +216,9 @@ RSpec.describe Middleware::AnonymousCache do
describe "#force_anonymous!" do
before { RateLimiter.enable }
it "will revert to anonymous once we reach the limit" do
RateLimiter.clear_all!
use_redis_snapshotting
it "will revert to anonymous once we reach the limit" do
is_anon = false
app =

View File

@ -254,7 +254,6 @@ RSpec.describe Middleware::RequestTracker do
before do
RateLimiter.enable
RateLimiter.clear_all_global!
RateLimiter.clear_all!
@orig_logger = Rails.logger
Rails.logger = @fake_logger = FakeLogger.new
@ -264,6 +263,8 @@ RSpec.describe Middleware::RequestTracker do
freeze_time DateTime.parse("2021-01-01 01:00")
end
use_redis_snapshotting
after { Rails.logger = @orig_logger }
let :middleware do

View File

@ -9,10 +9,9 @@ RSpec.describe PostActionCreator do
before { Group.refresh_automatic_groups! }
describe "rate limits" do
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
it "limits redo/undo" do
PostActionCreator.like(user, post)

View File

@ -293,12 +293,11 @@ RSpec.describe PostRevisor do
end
describe "revise wiki" do
before do
# There used to be a bug where wiki changes were considered posting "too similar"
# so this is enabled and checked
Discourse.redis.delete_prefixed("unique-post")
SiteSetting.unique_posts_mins = 10
end
# There used to be a bug where wiki changes were considered posting "too similar"
# so this is enabled and checked
use_redis_snapshotting
before { SiteSetting.unique_posts_mins = 10 }
it "allows the user to change it to a wiki" do
pc =
@ -780,10 +779,11 @@ RSpec.describe PostRevisor do
before do
RateLimiter.enable
RateLimiter.clear_all!
SiteSetting.editing_grace_period = 0
end
use_redis_snapshotting
it "triggers a rate limiter" do
EditRateLimiter.any_instance.expects(:performed!)
subject.revise!(changed_by, raw: "updated body")

View File

@ -221,7 +221,7 @@ RSpec.describe Invite do
3.times { Invite.generate(user, email: "test@example.com") }
end
after { RateLimiter.clear_all! }
use_redis_snapshotting
it "raises an error" do
expect { Invite.generate(user, email: "test@example.com") }.to raise_error(

View File

@ -789,7 +789,7 @@ RSpec.describe Topic do
Group.refresh_automatic_groups!
end
after { RateLimiter.clear_all! }
use_redis_snapshotting
context "when per day" do
before { SiteSetting.max_topic_invitations_per_day = 1 }
@ -2629,9 +2629,10 @@ RSpec.describe Topic do
SiteSetting.stubs(:client_settings_json).returns(SiteSetting.client_settings_json_uncached)
RateLimiter.stubs(:rate_limit_create_topic).returns(100)
RateLimiter.enable
RateLimiter.clear_all!
end
use_redis_snapshotting
it "limits new users to max_topics_in_first_day and max_posts_in_first_day" do
start = Time.now.tomorrow.beginning_of_day
@ -2683,7 +2684,7 @@ RSpec.describe Topic do
RateLimiter.enable
end
after { RateLimiter.clear_all! }
use_redis_snapshotting
it "limits according to max_personal_messages_per_day" do
Group.refresh_automatic_groups!

View File

@ -5,12 +5,11 @@ RSpec.describe "RequestTracker in multisite", type: :multisite do
global_setting :skip_per_ip_rate_limit_trust_level, 2
RateLimiter.enable
test_multisite_connection("default") { RateLimiter.clear_all! }
test_multisite_connection("second") { RateLimiter.clear_all! }
RateLimiter.clear_all_global!
end
use_redis_snapshotting
def call(env, &block)
Middleware::RequestTracker.new(block).call(env)
end

View File

@ -154,10 +154,9 @@ RSpec.describe Admin::BackupsController do
end
context "with rate limiting enabled" do
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
after { RateLimiter.disable }

View File

@ -756,10 +756,9 @@ RSpec.describe ApplicationController do
after { I18n.reload! }
context "with rate limits" do
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
it "serves a LimitExceeded error in the preferred locale" do
SiteSetting.max_likes_per_day = 1
@ -974,10 +973,9 @@ RSpec.describe ApplicationController do
describe "Discourse-Rate-Limit-Error-Code header" do
fab!(:admin) { Fabricate(:admin) }
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
it "is included when API key is rate limited" do
global_setting :max_admin_api_reqs_per_minute, 1
@ -1021,10 +1019,9 @@ RSpec.describe ApplicationController do
end
describe "crawlers in slow_down_crawler_user_agents site setting" do
before do
RateLimiter.enable
RateLimiter.clear_all!
end
before { RateLimiter.enable }
use_redis_snapshotting
it "are rate limited" do
SiteSetting.slow_down_crawler_rate = 128

View File

@ -9,10 +9,11 @@ RSpec.describe BookmarksController do
before { sign_in(current_user) }
describe "#create" do
use_redis_snapshotting
it "rate limits creates" do
SiteSetting.max_bookmarks_per_day = 1
RateLimiter.enable
RateLimiter.clear_all!
post "/bookmarks.json",
params: {

View File

@ -2608,9 +2608,10 @@ RSpec.describe GroupsController do
end
context "when rate limited" do
use_redis_snapshotting
it "rate limits anon searches per user" do
RateLimiter.enable
RateLimiter.clear_all!
5.times { post "/groups/#{group.id}/test_email_settings.json", params: params }
post "/groups/#{group.id}/test_email_settings.json", params: params

View File

@ -554,20 +554,6 @@ RSpec.describe InvitesController do
expect(Jobs::InviteEmail.jobs.size).to eq(0)
end
it "can send invite email" do
sign_in(user)
RateLimiter.enable
RateLimiter.clear_all!
invite = Fabricate(:invite, invited_by: user, email: "test@example.com")
expect { put "/invites/#{invite.id}", params: { send_email: true } }.to change {
RateLimiter.new(user, "resend-invite-per-hour", 10, 1.hour).remaining
}.by(-1)
expect(response.status).to eq(200)
expect(Jobs::InviteEmail.jobs.size).to eq(1)
end
it "cannot create duplicated invites" do
Fabricate(:invite, invited_by: admin, email: "test2@example.com")
@ -575,6 +561,24 @@ RSpec.describe InvitesController do
expect(response.status).to eq(409)
end
describe "rate limiting" do
before { RateLimiter.enable }
use_redis_snapshotting
it "can send invite email" do
sign_in(user)
invite = Fabricate(:invite, invited_by: user, email: "test@example.com")
expect { put "/invites/#{invite.id}", params: { send_email: true } }.to change {
RateLimiter.new(user, "resend-invite-per-hour", 10, 1.hour).remaining
}.by(-1)
expect(response.status).to eq(200)
expect(Jobs::InviteEmail.jobs.size).to eq(1)
end
end
context "when providing an email belonging to an existing user" do
subject(:update_invite) { put "/invites/#{invite.id}.json", params: { email: admin.email } }
@ -1361,7 +1365,12 @@ RSpec.describe InvitesController do
describe "#resend_all_invites" do
let(:admin) { Fabricate(:admin) }
before { SiteSetting.invite_expiry_days = 30 }
before do
SiteSetting.invite_expiry_days = 30
RateLimiter.enable
end
use_redis_snapshotting
it "resends all non-redeemed invites by a user" do
freeze_time
@ -1384,8 +1393,6 @@ RSpec.describe InvitesController do
it "errors if admins try to exceed limit of one bulk invite per day" do
sign_in(admin)
RateLimiter.enable
RateLimiter.clear_all!
start = Time.now
freeze_time(start)

View File

@ -228,6 +228,10 @@ RSpec.describe SearchController do
end
context "when rate limited" do
before { RateLimiter.enable }
use_redis_snapshotting
def unlimited_request(ip_address = "1.2.3.4")
get "/search/query.json", params: { term: "wookie" }, env: { REMOTE_ADDR: ip_address }
@ -246,8 +250,6 @@ RSpec.describe SearchController do
it "rate limits anon searches per user" do
SiteSetting.rate_limit_search_anon_user_per_second = 2
SiteSetting.rate_limit_search_anon_user_per_minute = 3
RateLimiter.enable
RateLimiter.clear_all!
start = Time.now
freeze_time start
@ -268,8 +270,6 @@ RSpec.describe SearchController do
it "rate limits anon searches globally" do
SiteSetting.rate_limit_search_anon_global_per_second = 2
SiteSetting.rate_limit_search_anon_global_per_minute = 3
RateLimiter.enable
RateLimiter.clear_all!
t = Time.now
freeze_time t
@ -289,8 +289,6 @@ RSpec.describe SearchController do
it "rate limits logged in searches" do
SiteSetting.rate_limit_search_user = 3
RateLimiter.enable
RateLimiter.clear_all!
3.times do
get "/search/query.json", params: { term: "wookie" }
@ -367,6 +365,10 @@ RSpec.describe SearchController do
end
context "when rate limited" do
before { RateLimiter.enable }
use_redis_snapshotting
def unlimited_request(ip_address = "1.2.3.4")
get "/search.json", params: { q: "wookie" }, env: { REMOTE_ADDR: ip_address }
@ -385,8 +387,6 @@ RSpec.describe SearchController do
it "rate limits anon searches per user" do
SiteSetting.rate_limit_search_anon_user_per_second = 2
SiteSetting.rate_limit_search_anon_user_per_minute = 3
RateLimiter.enable
RateLimiter.clear_all!
t = Time.now
freeze_time t
@ -405,8 +405,6 @@ RSpec.describe SearchController do
it "rate limits anon searches globally" do
SiteSetting.rate_limit_search_anon_global_per_second = 2
SiteSetting.rate_limit_search_anon_global_per_minute = 3
RateLimiter.enable
RateLimiter.clear_all!
t = Time.now
freeze_time t
@ -426,8 +424,6 @@ RSpec.describe SearchController do
it "rate limits searches" do
SiteSetting.rate_limit_search_user = 3
RateLimiter.enable
RateLimiter.clear_all!
3.times do
get "/search.json", params: { q: "bantha" }

View File

@ -2350,10 +2350,12 @@ RSpec.describe SessionController do
end
context "when rate limited" do
before { RateLimiter.enable }
use_redis_snapshotting
it "rate limits login" do
SiteSetting.max_logins_per_ip_per_hour = 2
RateLimiter.enable
RateLimiter.clear_all!
EmailToken.confirm(email_token.token)
2.times do
@ -2371,9 +2373,6 @@ RSpec.describe SessionController do
end
it "rate limits second factor attempts by IP" do
RateLimiter.enable
RateLimiter.clear_all!
6.times do |x|
post "/session.json",
params: {
@ -2400,8 +2399,6 @@ RSpec.describe SessionController do
end
it "rate limits second factor attempts by login" do
RateLimiter.enable
RateLimiter.clear_all!
EmailToken.confirm(email_token.token)
6.times do |x|
@ -2644,44 +2641,47 @@ RSpec.describe SessionController do
)
end
it "should correctly rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
describe "rate limiting" do
before { RateLimiter.enable }
user = Fabricate(:user)
use_redis_snapshotting
it "should correctly rate limits" do
user = Fabricate(:user)
3.times do
post "/session/forgot_password.json", params: { login: user.username }
expect(response.status).to eq(200)
expect(response.parsed_body["error"]).not_to be_present
end
3.times do
post "/session/forgot_password.json", params: { login: user.username }
expect(response.status).to eq(200)
expect(response.parsed_body["error"]).not_to be_present
end
expect(response.status).to eq(422)
post "/session/forgot_password.json", params: { login: user.username }
expect(response.status).to eq(422)
3.times do
post "/session/forgot_password.json",
params: {
login: user.username,
},
headers: {
"REMOTE_ADDR" => "10.1.1.1",
}
expect(response.status).to eq(200)
expect(response.parsed_body["error"]).not_to be_present
end
3.times do
post "/session/forgot_password.json",
params: {
login: user.username,
},
headers: {
"REMOTE_ADDR" => "10.1.1.1",
"REMOTE_ADDR" => "100.1.1.1",
}
expect(response.status).to eq(200)
expect(response.parsed_body["error"]).not_to be_present
# not allowed, max 6 a day
expect(response.status).to eq(422)
end
post "/session/forgot_password.json",
params: {
login: user.username,
},
headers: {
"REMOTE_ADDR" => "100.1.1.1",
}
# not allowed, max 6 a day
expect(response.status).to eq(422)
end
context "for a non existant username" do

View File

@ -22,23 +22,26 @@ RSpec.describe SlugsController do
expect(response.parsed_body["slug"]).to eq(Slug.for(name, ""))
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
stub_const(SlugsController, "MAX_SLUG_GENERATIONS_PER_MINUTE", 1) do
post "/slugs.json?name=#{name}"
post "/slugs.json?name=#{name}"
end
expect(response.status).to eq(429)
end
it "requires name" do
post "/slugs.json"
expect(response.status).to eq(400)
end
describe "rate limiting" do
before { RateLimiter.enable }
use_redis_snapshotting
it "rate limits" do
stub_const(SlugsController, "MAX_SLUG_GENERATIONS_PER_MINUTE", 1) do
post "/slugs.json?name=#{name}"
post "/slugs.json?name=#{name}"
end
expect(response.status).to eq(429)
end
end
context "when user is not TL4 or higher" do
before { current_user.change_trust_level!(1) }

View File

@ -776,25 +776,29 @@ RSpec.describe UploadsController do
expect(result["url"]).not_to include("&x-amz-meta-blah=wontbeincluded")
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
SiteSetting.max_presigned_put_per_minute = 1
describe "rate limiting" do
before { RateLimiter.enable }
post "/uploads/generate-presigned-put.json",
params: {
file_name: "test.png",
type: "card_background",
file_size: 1024,
}
post "/uploads/generate-presigned-put.json",
params: {
file_name: "test.png",
type: "card_background",
file_size: 1024,
}
use_redis_snapshotting
expect(response.status).to eq(429)
it "rate limits" do
SiteSetting.max_presigned_put_per_minute = 1
post "/uploads/generate-presigned-put.json",
params: {
file_name: "test.png",
type: "card_background",
file_size: 1024,
}
post "/uploads/generate-presigned-put.json",
params: {
file_name: "test.png",
type: "card_background",
file_size: 1024,
}
expect(response.status).to eq(429)
end
end
end
@ -944,27 +948,31 @@ RSpec.describe UploadsController do
expect(response.status).to eq(200)
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
SiteSetting.max_create_multipart_per_minute = 1
describe "rate limiting" do
before { RateLimiter.enable }
stub_create_multipart_request
post "/uploads/create-multipart.json",
params: {
file_name: "test.png",
upload_type: "composer",
file_size: 1024,
}
expect(response.status).to eq(200)
use_redis_snapshotting
post "/uploads/create-multipart.json",
params: {
file_name: "test.png",
upload_type: "composer",
file_size: 1024,
}
expect(response.status).to eq(429)
it "rate limits" do
SiteSetting.max_create_multipart_per_minute = 1
stub_create_multipart_request
post "/uploads/create-multipart.json",
params: {
file_name: "test.png",
upload_type: "composer",
file_size: 1024,
}
expect(response.status).to eq(200)
post "/uploads/create-multipart.json",
params: {
file_name: "test.png",
upload_type: "composer",
file_size: 1024,
}
expect(response.status).to eq(429)
end
end
end
@ -1117,27 +1125,31 @@ RSpec.describe UploadsController do
)
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
SiteSetting.max_batch_presign_multipart_per_minute = 1
describe "rate limiting" do
before { RateLimiter.enable }
stub_list_multipart_request
post "/uploads/batch-presign-multipart-parts.json",
params: {
unique_identifier: external_upload_stub.unique_identifier,
part_numbers: [1, 2, 3],
}
use_redis_snapshotting
expect(response.status).to eq(200)
it "rate limits" do
SiteSetting.max_batch_presign_multipart_per_minute = 1
post "/uploads/batch-presign-multipart-parts.json",
params: {
unique_identifier: external_upload_stub.unique_identifier,
part_numbers: [1, 2, 3],
}
stub_list_multipart_request
post "/uploads/batch-presign-multipart-parts.json",
params: {
unique_identifier: external_upload_stub.unique_identifier,
part_numbers: [1, 2, 3],
}
expect(response.status).to eq(429)
expect(response.status).to eq(200)
post "/uploads/batch-presign-multipart-parts.json",
params: {
unique_identifier: external_upload_stub.unique_identifier,
part_numbers: [1, 2, 3],
}
expect(response.status).to eq(429)
end
end
end
@ -1340,23 +1352,27 @@ RSpec.describe UploadsController do
expect(result[:upload]).to eq(JSON.parse(UploadSerializer.new(upload).to_json)[:upload])
end
it "rate limits" do
RateLimiter.enable
RateLimiter.clear_all!
SiteSetting.max_complete_multipart_per_minute = 1
describe "rate limiting" do
before { RateLimiter.enable }
post "/uploads/complete-multipart.json",
params: {
unique_identifier: "blah",
parts: [{ part_number: 1, etag: "test1" }, { part_number: 2, etag: "test2" }],
}
post "/uploads/complete-multipart.json",
params: {
unique_identifier: "blah",
parts: [{ part_number: 1, etag: "test1" }, { part_number: 2, etag: "test2" }],
}
use_redis_snapshotting
expect(response.status).to eq(429)
it "rate limits" do
SiteSetting.max_complete_multipart_per_minute = 1
post "/uploads/complete-multipart.json",
params: {
unique_identifier: "blah",
parts: [{ part_number: 1, etag: "test1" }, { part_number: 2, etag: "test2" }],
}
post "/uploads/complete-multipart.json",
params: {
unique_identifier: "blah",
parts: [{ part_number: 1, etag: "test1" }, { part_number: 2, etag: "test2" }],
}
expect(response.status).to eq(429)
end
end
end

View File

@ -327,10 +327,9 @@ RSpec.describe UsersController do
end
context "with rate limiting" do
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
it "rate limits reset passwords" do
freeze_time
@ -4179,6 +4178,8 @@ RSpec.describe UsersController do
end
context "with a session variable" do
use_redis_snapshotting
it "raises an error with an invalid session value" do
post_user
@ -4250,7 +4251,6 @@ RSpec.describe UsersController do
it "tells the user to slow down after many requests" do
RateLimiter.enable
RateLimiter.clear_all!
freeze_time
user = post_user
@ -4352,7 +4352,6 @@ RSpec.describe UsersController do
it "tells the user to slow down after many requests" do
RateLimiter.enable
RateLimiter.clear_all!
freeze_time
user = inactive_user
@ -5279,6 +5278,8 @@ RSpec.describe UsersController do
describe "#enable_second_factor_totp" do
before { sign_in(user1) }
use_redis_snapshotting
def create_totp
stub_secure_session_confirmed
post "/users/create_second_factor_totp.json"
@ -5301,7 +5302,6 @@ RSpec.describe UsersController do
it "rate limits by IP address" do
RateLimiter.enable
RateLimiter.clear_all!
create_totp
staged_totp_key = read_secure_session["staged-totp-#{user1.id}"]
@ -5320,7 +5320,6 @@ RSpec.describe UsersController do
it "rate limits by username" do
RateLimiter.enable
RateLimiter.clear_all!
create_totp
staged_totp_key = read_secure_session["staged-totp-#{user1.id}"]

View File

@ -118,10 +118,9 @@ RSpec.describe UsersEmailController do
end
context "with rate limiting" do
before do
RateLimiter.clear_all!
RateLimiter.enable
end
before { RateLimiter.enable }
use_redis_snapshotting
it "rate limits by IP" do
freeze_time

View File

@ -8,10 +8,8 @@ RSpec.describe TopicViewSerializer do
JSON.parse(MultiJson.dump(serializer)).deep_symbolize_keys!
end
before do
# ensure no suggested ids are cached cause that can muck up suggested
RandomTopicSelector.clear_cache!
end
# Ensure no suggested ids are cached cause that can muck up suggested
use_redis_snapshotting
fab!(:topic) { Fabricate(:topic) }
fab!(:user) { Fabricate(:user) }