AO3-4729 Accept 5 abuse reports per profile per month (#2906)

* AO3-4729 Accept 5 abuse reports per profile per month

* Test that rate limiting can ignore different link protocols

* Use =~ where the MatchData returned by #match will not be used

app/models/abuse_report.rb

@@ -6,24 +6,29 @@ class AbuseReport < ApplicationRecord
   validates_presence_of :summary
   validates_presence_of :comment
   validates_presence_of :url
-  validate :work_is_not_over_reported
+  validate :url_is_not_over_reported
   validates_length_of :summary, maximum: ArchiveConfig.FEEDBACK_SUMMARY_MAX,
                                 too_long: ts('must be less than %{max}
                                              characters long.',
                                 max: ArchiveConfig.FEEDBACK_SUMMARY_MAX_DISPLAYED)
 
   scope :by_date, -> { order('created_at DESC') }
 
-  # if the URL ends like "works/123", add a / at the end
-  # if the URL contains "works/123?", remove the parameters and add a /
-  # work_is_not_over_reported uses the / so "/works/1234" isn't a match
+  # Clean work or profile URLs so we can prevent the same URLs from
+  # getting reported too many times.
+  # If the URL ends without a / at the end, add it:
+  # url_is_not_over_reported uses the / so "/works/1234" isn't a match
   # for "/works/123"
-  before_validation :clean_work_url, on: :create
-  def clean_work_url
-    if url.match(/(works\/\d+)$/)
-      self.url = url + '/'
-    elsif url.match(/(works\/\d+\?)/)
-      self.url = url.split('?').first + '/'
+  before_validation :clean_url, on: :create
+  def clean_url
+    # Work URLs: "works/123"
+    # Profile URLs: "users/username"
+    if url =~ /(works\/\d+)/ || url =~ /(users\/\w+)/
+      uri = Addressable::URI.parse url
+      uri.query = nil
+      uri.fragment = nil
+      uri.path += "/" unless uri.path.end_with? "/"
+      self.url = uri.to_s
     else
       url
     end
@@ -56,21 +61,31 @@ def send_report
   end
 
   # if the URL clearly belongs to a work (i.e. contains "/works/123")
-  # make sure it isn't reported more than ABUSE_REPORTS_PER_WORK_MAX times
-  # per month
-  def work_is_not_over_reported
-    if url.match(/\/works\/\d+/)
+  # or a user profile (i.e. contains "/users/username")
+  # make sure it isn't reported more than ABUSE_REPORTS_PER_WORK_MAX
+  # or ABUSE_REPORTS_PER_USER_MAX times per month
+  def url_is_not_over_reported
+    message = ts('URL has already been reported. To make sure the Abuse Team
+                 can handle reports quickly and efficiently, we limit the number
+                 of times a URL can be reported.')
+    if url =~ /\/works\/\d+/
       # use "/works/123/" to avoid matching chapter or external work ids
       work_params_only = url.match(/\/works\/\d+\//).to_s
       existing_reports_total = AbuseReport.where('created_at > ? AND
                                                  url LIKE ?',
                                                  1.month.ago,
                                                  "%#{work_params_only}%").count
       if existing_reports_total >= ArchiveConfig.ABUSE_REPORTS_PER_WORK_MAX
-        errors[:base] << ts('URL has already been reported. To make sure the
-                            Abuse Team can handle reports quickly and
-                            efficiently, we limit the number of times a URL can
-                            be reported.')
+        errors[:base] << message
+      end
+    elsif url =~ /\/users\/\w+/
+      user_params_only = url.match(/\/users\/\w+\//).to_s
+      existing_reports_total = AbuseReport.where('created_at > ? AND
+                                                 url LIKE ?',
+                                                 1.month.ago,
+                                                 "%#{user_params_only}%").count
+      if existing_reports_total >= ArchiveConfig.ABUSE_REPORTS_PER_USER_MAX
+        errors[:base] << message
       end
     end
   end

config/config.yml

@@ -136,6 +136,8 @@ IMPORT_MAX_WORKS: 25
 IMPORT_MAX_CHAPTERS: 200
 IMPORT_MAX_WORKS_BY_ARCHIVIST: 200
 
+# max number of abuse reports to accept for a given user profile URL
+ABUSE_REPORTS_PER_USER_MAX: 5
 # max number of abuse reports to accept for a given work URL
 ABUSE_REPORTS_PER_WORK_MAX: 5
 

lib/tasks/after_tasks.rake

@@ -432,10 +432,10 @@ namespace :After do
   end
 
 
-  desc "Clean up work URLs for abuse reports from the last month"
-  task(:clean_abuse_report_work_urls => :environment) do
+  desc "Clean up URLs for abuse reports from the last month"
+  task(:clean_abuse_report_urls => :environment) do
     AbuseReport.where("created_at > ?", 1.month.ago).each do |report|
-      report.clean_work_url
+      report.clean_url
       puts report.url
       report.save
     end

spec/models/abuse_report_spec.rb

@@ -84,36 +84,38 @@
     end
   end
 
+  shared_examples "enough already" do |url|
+    let(:report) { build(:abuse_report, url: url) }
+    it "can't be submitted" do
+      expect(report.save).to be_falsey
+      expect(report.errors[:base].first).to include("URL has already been reported.")
+    end
+  end
+
+  shared_examples "alright" do |url|
+    let(:report) { build(:abuse_report, url: url) }
+    it "can be submitted" do
+      expect(report.save).to be_truthy
+      expect(report.errors[:base]).to be_empty
+    end
+  end
+
   context "for a work reported the maximum number of times" do
     work_url = "http://archiveofourown.org/works/789"
 
     before do
       ArchiveConfig.ABUSE_REPORTS_PER_WORK_MAX.times do
         create(:abuse_report, url: work_url)
       end
-    end
-
-    shared_examples "enough already" do |url|
-      let(:report) { build(:abuse_report, url: url) }
-      it "can't be submitted" do
-        expect(AbuseReport.count).to eq(ArchiveConfig.ABUSE_REPORTS_PER_WORK_MAX)
-        expect(report.save).to be_falsey
-        expect(report.errors[:base].first).to include("URL has already been reported.")
-      end
-    end
-
-    shared_examples "alright" do |url|
-      let(:report) { build(:abuse_report, url: url) }
-      it "can be submitted" do
-        expect(AbuseReport.count).to eq(ArchiveConfig.ABUSE_REPORTS_PER_WORK_MAX)
-        expect(report.save).to be_truthy
-        expect(report.errors[:base]).to be_empty
-      end
+      expect(AbuseReport.count).to eq(ArchiveConfig.ABUSE_REPORTS_PER_WORK_MAX)
     end
 
     # obviously
     it_behaves_like "enough already", work_url
 
+    # the same work, different protocol
+    it_behaves_like "enough already", "https://archiveofourown.org/works/789"
+
     # the same work, with parameters/anchors
     it_behaves_like "enough already", "http://archiveofourown.org/works/789?smut=yes"
     it_behaves_like "enough already", "http://archiveofourown.org/works/789?smut=yes#timeline"
@@ -124,6 +126,10 @@
     # the same work, in a collection
     it_behaves_like "enough already", "http://archiveofourown.org/collections/rarepair/works/789"
 
+    # the same work, under users
+    it_behaves_like "enough already", "http://archiveofourown.org/users/author/works/789"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/coauthor/works/789"
+
     # the same work, subpages
     it_behaves_like "enough already", "http://archiveofourown.org/works/789/bookmarks"
     it_behaves_like "enough already", "http://archiveofourown.org/works/789/collections"
@@ -145,6 +151,68 @@
     it_behaves_like "alright", "http://archiveofourown.org/works/78"
     it_behaves_like "alright", "http://archiveofourown.org/works/7890"
     it_behaves_like "alright", "http://archiveofourown.org/external_works/789"
+
+    # unrelated
+    it_behaves_like "alright", "http://archiveofourown.org/users/someone"
+
+    context "a month later" do
+      before { Timecop.freeze(1.month.from_now) }
+      after { Timecop.return }
+
+      it_behaves_like "alright", work_url
+    end
+  end
+
+  context "for a user profile reported the maximum number of times" do
+    user_url = "http://archiveofourown.org/users/someone"
+
+    before do
+      ArchiveConfig.ABUSE_REPORTS_PER_USER_MAX.times do
+        create(:abuse_report, url: user_url)
+      end
+      expect(AbuseReport.count).to eq(ArchiveConfig.ABUSE_REPORTS_PER_USER_MAX)
+    end
+
+    # obviously
+    it_behaves_like "enough already", user_url
+
+    # the same user, different protocol
+    it_behaves_like "enough already", "https://archiveofourown.org/users/someone"
+
+    # the same user, with parameters/anchors
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone?sfw=yes"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone?sfw=yes#timeline"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone#timeline"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/?sfw=yes"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/#timeline"
+
+    # the same user, as admin (why admin?)
+    it_behaves_like "enough already", "http://archiveofourown.org/admin/users/someone"
+
+    # the same user, subpages
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/bookmarks"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/claims"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/pseuds/"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/pseuds/ghostwriter"
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/pseuds/g h o s t w r i t e r"
+
+    # the same user, Unicode in parameters
+    it_behaves_like "enough already", "http://archiveofourown.org/users/someone/inbox?utf8=✓&filters[read]=false"
+
+    # not the same user
+    it_behaves_like "alright", "http://archiveofourown.org/users/some"
+    it_behaves_like "alright", "http://archiveofourown.org/users/someoneelse"
+    it_behaves_like "alright", "http://archiveofourown.org/users/somebody"
+
+    # unrelated
+    it_behaves_like "alright", "http://archiveofourown.org/works/789"
+
+    context "a month later" do
+      before { Timecop.freeze(1.month.from_now) }
+      after { Timecop.return }
+
+      it_behaves_like "alright", user_url
+    end
   end
 
   context "for a URL that is not a work" do