From a8a1f5e75945c8f8d1757b1865828ae06d2767bd Mon Sep 17 00:00:00 2001 From: "zhanfeng.zeng" Date: Mon, 19 May 2025 14:59:56 +0800 Subject: [PATCH 1/5] FEATURE: Provide generic modifier interface for rate limiting. --- .../discourse_solved/answer_controller.rb | 7 ++ spec/requests/answer_controller_spec.rb | 95 +++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 spec/requests/answer_controller_spec.rb diff --git a/app/controllers/discourse_solved/answer_controller.rb b/app/controllers/discourse_solved/answer_controller.rb index ced07b04..0ca76546 100644 --- a/app/controllers/discourse_solved/answer_controller.rb +++ b/app/controllers/discourse_solved/answer_controller.rb @@ -35,6 +35,13 @@ def unaccept def limit_accepts return if current_user.staff? + run_rate_limiter = + DiscoursePluginRegistry.apply_modifier( + :solved_answers_controller_run_rate_limiter, + true, + current_user, + ) + return if !run_rate_limiter RateLimiter.new(nil, "accept-hr-#{current_user.id}", 20, 1.hour).performed! RateLimiter.new(nil, "accept-min-#{current_user.id}", 4, 30.seconds).performed! end diff --git a/spec/requests/answer_controller_spec.rb b/spec/requests/answer_controller_spec.rb new file mode 100644 index 00000000..8f5118ec --- /dev/null +++ b/spec/requests/answer_controller_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe DiscourseSolved::AnswerController do + fab!(:user) + fab!(:staff_user) { Fabricate(:admin) } + fab!(:category) + fab!(:topic) { Fabricate(:topic, category: category) } + fab!(:p) { Fabricate(:post, topic: topic) } + fab!(:solution_post) { Fabricate(:post, topic: topic) } + + before do + SiteSetting.solved_enabled = true + SiteSetting.allow_solved_on_all_topics = true + category.custom_fields[DiscourseSolved::ENABLE_ACCEPTED_ANSWERS_CUSTOM_FIELD] = "true" + category.save_custom_fields + + # Give permission to accept solutions + user.update!(trust_level: 1) + + # Make user the topic creator so they can accept answers + topic.update!(user_id: user.id) + end + + describe "#accept" do + context "with default rate limiting" do + it "applies rate limits to regular users" do + sign_in(user) + + # First request should succeed + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + + # Second request should be rate limited + RateLimiter.any_instance.expects(:performed!).raises(RateLimiter::LimitExceeded.new(60)) + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(429) + end + + it "does not apply rate limits to staff" do + sign_in(staff_user) + + # Staff can make multiple requests without hitting limits + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + end + end + + context "with plugin modifier" do + it "allows plugins to bypass rate limiting" do + sign_in(user) + + # Register a modifier that disables rate limiting + DiscoursePluginRegistry.register_modifier( + :solved_answers_controller_run_rate_limiter, + ) do |_, _| + false # Skip rate limiting + end + + # Multiple requests should succeed without rate limiting + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + + # Clean up + DiscoursePluginRegistry.unregister_modifier(:solved_answers_controller_run_rate_limiter) + end + end + end + + describe "#unaccept" do + before do + # Setup an accepted solution + sign_in(user) + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + sign_out + end + + it "applies rate limits to regular users" do + sign_in(user) + + # Should be rate limited + RateLimiter.any_instance.expects(:performed!).raises(RateLimiter::LimitExceeded.new(60)) + post "/solution/unaccept.json", params: { id: solution_post.id } + expect(response.status).to eq(429) + end + end +end From acbad996f8c9ff4954b4fbe6542f59d4d0c7911b Mon Sep 17 00:00:00 2001 From: zhanfengzeng Date: Mon, 19 May 2025 21:57:32 +0800 Subject: [PATCH 2/5] FIX: Fix spec use cases --- spec/requests/answer_controller_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/requests/answer_controller_spec.rb b/spec/requests/answer_controller_spec.rb index 8f5118ec..534e81d7 100644 --- a/spec/requests/answer_controller_spec.rb +++ b/spec/requests/answer_controller_spec.rb @@ -55,10 +55,12 @@ sign_in(user) # Register a modifier that disables rate limiting + plugin_instance = DiscoursePluginRegistry.new DiscoursePluginRegistry.register_modifier( - :solved_answers_controller_run_rate_limiter, + plugin_instance, + :solved_answers_controller_run_rate_limiter ) do |_, _| - false # Skip rate limiting + false end # Multiple requests should succeed without rate limiting From 4cdc5c64d5d2814c373f0b96bd2e6e5ba003b489 Mon Sep 17 00:00:00 2001 From: "zhanfeng.zeng" Date: Tue, 20 May 2025 10:04:30 +0800 Subject: [PATCH 3/5] FIX: Correct spec execution error. --- spec/requests/answer_controller_spec.rb | 46 ++++++++++++------------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/spec/requests/answer_controller_spec.rb b/spec/requests/answer_controller_spec.rb index 534e81d7..21b02e53 100644 --- a/spec/requests/answer_controller_spec.rb +++ b/spec/requests/answer_controller_spec.rb @@ -51,31 +51,29 @@ end context "with plugin modifier" do - it "allows plugins to bypass rate limiting" do - sign_in(user) - - # Register a modifier that disables rate limiting - plugin_instance = DiscoursePluginRegistry.new - DiscoursePluginRegistry.register_modifier( - plugin_instance, - :solved_answers_controller_run_rate_limiter - ) do |_, _| - false - end - - # Multiple requests should succeed without rate limiting - post "/solution/accept.json", params: { id: solution_post.id } - expect(response.status).to eq(200) - - post "/solution/accept.json", params: { id: solution_post.id } - expect(response.status).to eq(200) - - # Clean up - DiscoursePluginRegistry.unregister_modifier(:solved_answers_controller_run_rate_limiter) - end - end + it "allows plugins to bypass rate limiting" do + sign_in(user) + # Store the block in a variable so we can reference it for unregistration + block = ->(_, _) { false } + # Register modifier with proper parameters - plugin instance (self) and name + DiscoursePluginRegistry.register_modifier( + self, + :solved_answers_controller_run_rate_limiter, + &block + ) + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) + # Unregister with the same plugin instance and block + DiscoursePluginRegistry.unregister_modifier( + self, # plugin_instance parameter + :solved_answers_controller_run_rate_limiter, # name parameter + &block # same block used for registration + ) + end + end end - describe "#unaccept" do before do # Setup an accepted solution From a7bb1b12d0a162af730549a1e10025c06af0e8df Mon Sep 17 00:00:00 2001 From: "zhanfeng.zeng" Date: Wed, 21 May 2025 11:15:48 +0800 Subject: [PATCH 4/5] FIX: Fix the issue of spec case execution failure. --- spec/requests/answer_controller_spec.rb | 61 +++++++++++++------------ 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/spec/requests/answer_controller_spec.rb b/spec/requests/answer_controller_spec.rb index 21b02e53..746d6515 100644 --- a/spec/requests/answer_controller_spec.rb +++ b/spec/requests/answer_controller_spec.rb @@ -28,11 +28,7 @@ it "applies rate limits to regular users" do sign_in(user) - # First request should succeed - post "/solution/accept.json", params: { id: solution_post.id } - expect(response.status).to eq(200) - - # Second request should be rate limited + # Should be rate limited RateLimiter.any_instance.expects(:performed!).raises(RateLimiter::LimitExceeded.new(60)) post "/solution/accept.json", params: { id: solution_post.id } expect(response.status).to eq(429) @@ -41,38 +37,43 @@ it "does not apply rate limits to staff" do sign_in(staff_user) - # Staff can make multiple requests without hitting limits post "/solution/accept.json", params: { id: solution_post.id } expect(response.status).to eq(200) + end + end + + context "with plugin modifier" do + it "allows plugins to bypass rate limiting" do + sign_in(user) + # Create a mock plugin instance with enabled? method + plugin_instance = Object.new + def plugin_instance.enabled? + true + end + # Store the block in a variable so we can reference it for unregistration + block = ->(_, _) { false } + + # Register modifier with proper parameters - using mock plugin instance instead of self + DiscoursePluginRegistry.register_modifier( + plugin_instance, + :solved_answers_controller_run_rate_limiter, + &block + ) + + post "/solution/accept.json", params: { id: solution_post.id } + expect(response.status).to eq(200) post "/solution/accept.json", params: { id: solution_post.id } expect(response.status).to eq(200) + + # Unregister with the same plugin instance and block + DiscoursePluginRegistry.unregister_modifier( + plugin_instance, # Using the same mock plugin instance + :solved_answers_controller_run_rate_limiter, + &block + ) end end - - context "with plugin modifier" do - it "allows plugins to bypass rate limiting" do - sign_in(user) - # Store the block in a variable so we can reference it for unregistration - block = ->(_, _) { false } - # Register modifier with proper parameters - plugin instance (self) and name - DiscoursePluginRegistry.register_modifier( - self, - :solved_answers_controller_run_rate_limiter, - &block - ) - post "/solution/accept.json", params: { id: solution_post.id } - expect(response.status).to eq(200) - post "/solution/accept.json", params: { id: solution_post.id } - expect(response.status).to eq(200) - # Unregister with the same plugin instance and block - DiscoursePluginRegistry.unregister_modifier( - self, # plugin_instance parameter - :solved_answers_controller_run_rate_limiter, # name parameter - &block # same block used for registration - ) - end - end end describe "#unaccept" do before do From 639e01212d3632d2c15ada299e9c854c4529fa3e Mon Sep 17 00:00:00 2001 From: "zhanfeng.zeng" Date: Thu, 22 May 2025 14:33:57 +0800 Subject: [PATCH 5/5] TEST: use Plugin::Instance instead of mock object in rate limiter tests. --- spec/requests/answer_controller_spec.rb | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/spec/requests/answer_controller_spec.rb b/spec/requests/answer_controller_spec.rb index 746d6515..4861080f 100644 --- a/spec/requests/answer_controller_spec.rb +++ b/spec/requests/answer_controller_spec.rb @@ -45,20 +45,12 @@ context "with plugin modifier" do it "allows plugins to bypass rate limiting" do sign_in(user) - # Create a mock plugin instance with enabled? method - plugin_instance = Object.new - def plugin_instance.enabled? - true - end - - # Store the block in a variable so we can reference it for unregistration - block = ->(_, _) { false } - - # Register modifier with proper parameters - using mock plugin instance instead of self - DiscoursePluginRegistry.register_modifier( - plugin_instance, + # Create a plugin instance and register a modifier + plugin_instance = Plugin::Instance.new + modifier_block = Proc.new { |_, _| false } + plugin_instance.register_modifier( :solved_answers_controller_run_rate_limiter, - &block + &modifier_block ) post "/solution/accept.json", params: { id: solution_post.id } @@ -66,11 +58,11 @@ def plugin_instance.enabled? post "/solution/accept.json", params: { id: solution_post.id } expect(response.status).to eq(200) - # Unregister with the same plugin instance and block + # Unregister the modifier using DiscoursePluginRegistry DiscoursePluginRegistry.unregister_modifier( - plugin_instance, # Using the same mock plugin instance + plugin_instance, :solved_answers_controller_run_rate_limiter, - &block + &modifier_block ) end end