FIX: Add bookmark limits (#11725)

Adds a bookmark search per page limit, a total bookmark creation limit, and a rate limit per day for bookmark creation.

app/controllers/bookmarks_controller.rb

@@ -6,6 +6,10 @@ class BookmarksController < ApplicationController
   def create
     params.require(:post_id)
 
+    RateLimiter.new(
+      current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i
+    ).performed!
+
     bookmark_manager = BookmarkManager.new(current_user)
     bookmark = bookmark_manager.create(
       post_id: params[:post_id],

app/models/bookmark.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class Bookmark < ActiveRecord::Base
+  BOOKMARK_LIMIT = 2000
+
   self.ignored_columns = [
     "delete_when_reminder_sent" # TODO(2021-07-22): remove
   ]
@@ -37,6 +39,7 @@ def self.auto_delete_preferences
 
   validate :unique_per_post_for_user
   validate :ensure_sane_reminder_at_time
+  validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
 
   # we don't care whether the post or topic is deleted,
@@ -65,6 +68,11 @@ def ensure_sane_reminder_at_time
     end
   end
 
+  def bookmark_limit_not_reached
+    return if user.bookmarks.count < BOOKMARK_LIMIT
+    self.errors.add(:base, I18n.t("bookmarks.errors.too_many", user_bookmarks_url: "#{Discourse.base_url}/my/activity/bookmarks"))
+  end
+
   def no_reminder?
     self.reminder_at.blank? && self.reminder_type.blank?
   end

app/models/user_bookmark_list.rb

@@ -5,13 +5,17 @@ class UserBookmarkList
 
   PER_PAGE = 20
 
-  attr_reader :bookmarks
+  attr_reader :bookmarks, :per_page
   attr_accessor :more_bookmarks_url
 
   def initialize(user:, guardian:, params:)
     @user = user
     @guardian = guardian
-    @params = params.merge(per_page: PER_PAGE)
+    @params = params
+
+    @params.merge!(per_page: PER_PAGE) if params[:per_page].blank?
+    @params[:per_page] = PER_PAGE if @params[:per_page] > PER_PAGE
+
     @bookmarks = []
   end
 
@@ -21,6 +25,6 @@ def load
   end
 
   def per_page
-    @per_page ||= PER_PAGE
+    @per_page ||= @params[:per_page]
   end
 end

config/locales/server.en.yml

@@ -413,6 +413,7 @@ en:
   bookmarks:
     errors:
       already_bookmarked_post: "You cannot bookmark the same post twice."
+      too_many: "Sorry, you have too many bookmarks, visit %{user_bookmarks_url} to remove some."
       cannot_set_past_reminder: "You cannot set a bookmark reminder in the past."
       cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future."
       time_must_be_provided: "time must be provided for all reminders"

spec/models/user_bookmark_list_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe UserBookmarkList do
+  let(:params) { {} }
+  fab!(:user) { Fabricate(:user) }
+  let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) }
+
+  before do
+    22.times do
+      Fabricate(:bookmark, user: user)
+    end
+  end
+
+  it "defaults to 20 per page" do
+    expect(list.per_page).to eq(20)
+  end
+
+  context "when the per_page param is too high" do
+    let(:params) { { per_page: 1000 } }
+
+    it "does not allow more than X bookmarks to be requested per page" do
+      expect(list.load.count).to eq(20)
+    end
+  end
+end

spec/requests/bookmarks_controller_spec.rb

@@ -12,6 +12,58 @@
   end
 
   describe "#create" do
+    it "rate limits creates" do
+      SiteSetting.max_bookmarks_per_day = 1
+      RateLimiter.enable
+      RateLimiter.clear_all!
+
+      post "/bookmarks.json", params: {
+        post_id: bookmark_post.id,
+        reminder_type: "tomorrow",
+        reminder_at: (Time.zone.now + 1.day).iso8601
+      }
+
+      expect(response.status).to eq(200)
+
+      post "/bookmarks.json", params: {
+        post_id: Fabricate(:post).id
+      }
+      expect(response.status).to eq(429)
+      expect(response.parsed_body['errors']).to include(
+        I18n.t("rate_limiter.by_type.create_bookmark", time_left: "24 hours")
+      )
+    end
+
+    context "if the user reached the max bookmark limit" do
+      before do
+        @old_constant = Bookmark::BOOKMARK_LIMIT
+        Bookmark.send(:remove_const, "BOOKMARK_LIMIT")
+        Bookmark.const_set("BOOKMARK_LIMIT", 1)
+      end
+
+      it "returns failed JSON with a 400 error" do
+        post "/bookmarks.json", params: {
+          post_id: bookmark_post.id,
+          reminder_type: "tomorrow",
+          reminder_at: (Time.zone.now + 1.day).iso8601
+        }
+        post "/bookmarks.json", params: {
+          post_id: Fabricate(:post).id
+        }
+
+        expect(response.status).to eq(400)
+        user_bookmarks_url = "#{Discourse.base_url}/my/activity/bookmarks"
+        expect(response.parsed_body['errors']).to include(
+          I18n.t("bookmarks.errors.too_many", user_bookmarks_url: user_bookmarks_url)
+        )
+      end
+
+      after do
+        Bookmark.send(:remove_const, "BOOKMARK_LIMIT")
+        Bookmark.const_set("BOOKMARK_LIMIT", @old_constant)
+      end
+    end
+
     context "if the user already has bookmarked the post" do
       before do
         Fabricate(:bookmark, post: bookmark_post, user: bookmark_user)