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 a9e80630e22226447bc9797f363ac2b854801987..bb77e1106609b2cea7b0190a61fcb160fd4032b0 100644 (file)
--- 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 fe2a98d61e31f6af42e6a085bdcf19e93a21d546..3eb03a2fe4ae93bfb2010b359265f169b8315f79 100644 (file)
--- 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 5330ed678c47333acd0e5ca388709da14100227b..3ea298efcb4378f4073397d02d301d785c772ff2 100644 (file)
--- 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 64039e362c4f8c879c0f419e3ddd4bd04a64717b..5f00259256d6ef81873ad323810657d9ce289c06 100644 (file)
--- 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 624b8a35808645c1964c3b7fd40aa5fa5f46b262..e25926c78e41dfcf5bb691f268101256a83c3ce3 100644 (file)
--- 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 c575c3398e3fe16108329098316eb2430cb822d4..bb6b2dd5222d517fbe95ca35a812991dc95d2b38 100644 (file)
--- 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