From: Andy Allan Date: Thu, 31 Aug 2023 09:02:18 +0000 (+0100) Subject: Merge pull request #4211 from tomhughes/changeset-comment-cleanup X-Git-Tag: live~649 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/cdfd617eba9d5f9291f746f2e904385521ad2b37?hp=8e3af42fe1f60480af83c2c7d40ab6289bf24508 Merge pull request #4211 from tomhughes/changeset-comment-cleanup Address review comments for changeset comment limiting --- diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb index a9e80630e..bb77e1106 100644 --- a/app/controllers/api/changeset_comments_controller.rb +++ b/app/controllers/api/changeset_comments_controller.rb @@ -17,7 +17,7 @@ module Api # Check the arguments are sane raise OSM::APIBadUserInput, "No id was given" unless params[:id] raise OSM::APIBadUserInput, "No text was given" if params[:text].blank? - raise OSM::APIRateLimitExceeded if current_user.changeset_comments.where("created_at >= ?", Time.now.utc - 1.hour).count >= current_user.max_changeset_comments_per_hour + raise OSM::APIRateLimitExceeded if rate_limit_exceeded? # Extract the arguments id = params[:id].to_i @@ -99,5 +99,15 @@ module Api format.json end end + + private + + ## + # Check if the current user has exceed the rate limit for comments + def rate_limit_exceeded? + recent_comments = current_user.changeset_comments.where("created_at >= ?", Time.now.utc - 1.hour).count + + recent_comments >= current_user.max_changeset_comments_per_hour + end end end diff --git a/app/models/user.rb b/app/models/user.rb index fe2a98d61..3eb03a2fe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -397,14 +397,14 @@ class User < ApplicationRecord def max_changeset_comments_per_hour if moderator? - 36000 + Settings.moderator_changeset_comments_per_hour else previous_comments = changeset_comments.limit(200).count active_reports = issues.with_status(:open).sum(:reports_count) max_comments = previous_comments / 200.0 * Settings.max_changeset_comments_per_hour - max_comments = max_comments.floor.clamp(Settings.min_changeset_comments_per_hour, Settings.max_changeset_comments_per_hour) + max_comments = max_comments.floor.clamp(Settings.initial_changeset_comments_per_hour, Settings.max_changeset_comments_per_hour) max_comments /= 2**active_reports - max_comments.floor.clamp(1, Settings.max_changeset_comments_per_hour) + max_comments.floor.clamp(Settings.min_changeset_comments_per_hour, Settings.max_changeset_comments_per_hour) end end diff --git a/config/settings.yml b/config/settings.yml index 5330ed678..3ea298efc 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -56,8 +56,10 @@ max_messages_per_hour: 60 # Rate limit for friending max_friends_per_hour: 60 # Rate limit for changeset comments -min_changeset_comments_per_hour: 6 +min_changeset_comments_per_hour: 1 +initial_changeset_comments_per_hour: 6 max_changeset_comments_per_hour: 60 +moderator_changeset_comments_per_hour: 36000 # Domain for handling message replies #messages_domain: "messages.openstreetmap.org" # MaxMind GeoIPv2 database diff --git a/config/settings/test.yml b/config/settings/test.yml index 64039e362..5f0025925 100644 --- a/config/settings/test.yml +++ b/config/settings/test.yml @@ -19,3 +19,6 @@ avatar_storage: "test" trace_file_storage: "test" trace_image_storage: "test" trace_icon_storage: "test" +# Lower some rate limits for testing +max_changeset_comments_per_hour: 30 +moderator_changeset_comments_per_hour: 60 diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 624b8a358..e25926c78 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -133,15 +133,80 @@ module Api end ## - # create comment rate limit - def test_create_comment_rate_limit + # create comment rate limit for new users + def test_create_comment_new_user_rate_limit changeset = create(:changeset, :closed) user = create(:user) auth_header = basic_authorization_header user.email, "test" - assert_difference "ChangesetComment.count", Settings.min_changeset_comments_per_hour do - 1.upto(Settings.min_changeset_comments_per_hour) do |count| + assert_difference "ChangesetComment.count", Settings.initial_changeset_comments_per_hour do + 1.upto(Settings.initial_changeset_comments_per_hour) do |count| + post changeset_comment_path(:id => changeset, :text => "Comment #{count}"), :headers => auth_header + assert_response :success + end + end + + assert_no_difference "ChangesetComment.count" do + post changeset_comment_path(:id => changeset, :text => "One comment too many"), :headers => auth_header + assert_response :too_many_requests + end + end + + ## + # create comment rate limit for experienced users + def test_create_comment_experienced_user_rate_limit + changeset = create(:changeset, :closed) + user = create(:user) + create_list(:changeset_comment, 200, :author_id => user.id, :created_at => Time.now.utc - 1.day) + + auth_header = basic_authorization_header user.email, "test" + + assert_difference "ChangesetComment.count", Settings.max_changeset_comments_per_hour do + 1.upto(Settings.max_changeset_comments_per_hour) do |count| + post changeset_comment_path(:id => changeset, :text => "Comment #{count}"), :headers => auth_header + assert_response :success + end + end + + assert_no_difference "ChangesetComment.count" do + post changeset_comment_path(:id => changeset, :text => "One comment too many"), :headers => auth_header + assert_response :too_many_requests + end + end + + ## + # create comment rate limit for reported users + def test_create_comment_reported_user_rate_limit + changeset = create(:changeset, :closed) + user = create(:user) + create(:issue_with_reports, :reportable => user, :reported_user => user) + + auth_header = basic_authorization_header user.email, "test" + + assert_difference "ChangesetComment.count", Settings.initial_changeset_comments_per_hour / 2 do + 1.upto(Settings.initial_changeset_comments_per_hour / 2) do |count| + post changeset_comment_path(:id => changeset, :text => "Comment #{count}"), :headers => auth_header + assert_response :success + end + end + + assert_no_difference "ChangesetComment.count" do + post changeset_comment_path(:id => changeset, :text => "One comment too many"), :headers => auth_header + assert_response :too_many_requests + end + end + + ## + # create comment rate limit for moderator users + def test_create_comment_moderator_user_rate_limit + changeset = create(:changeset, :closed) + user = create(:moderator_user) + + auth_header = basic_authorization_header user.email, "test" + + assert_difference "ChangesetComment.count", Settings.moderator_changeset_comments_per_hour do + 1.upto(Settings.moderator_changeset_comments_per_hour) do |count| post changeset_comment_path(:id => changeset, :text => "Comment #{count}"), :headers => auth_header assert_response :success end diff --git a/test/factories/issues.rb b/test/factories/issues.rb index c575c3398..bb6b2dd52 100644 --- a/test/factories/issues.rb +++ b/test/factories/issues.rb @@ -6,5 +6,16 @@ FactoryBot.define do # Default to assigning to an administrator assigned_role { "administrator" } + + # Optionally create some reports for this issue + factory :issue_with_reports do + transient do + reports_count { 1 } + end + + after(:create) do |issue, evaluator| + create_list(:report, evaluator.reports_count, :issue => issue) + end + end end end