From be145ccf2fe117d97b36aa1e2fc500e44319d58c Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 19 Jan 2021 08:53:49 +1000 Subject: [PATCH] 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 | 4 ++ app/models/bookmark.rb | 8 ++++ app/models/user_bookmark_list.rb | 10 +++-- config/locales/server.en.yml | 1 + spec/models/user_bookmark_list_spec.rb | 27 +++++++++++ spec/requests/bookmarks_controller_spec.rb | 52 ++++++++++++++++++++++ 6 files changed, 99 insertions(+), 3 deletions(-) create mode 100644 spec/models/user_bookmark_list_spec.rb diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index 7d103ea5077..aa804d1d4e5 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/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], diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 1dc75828f78..f6bdb12a629 100644 --- a/app/models/bookmark.rb +++ b/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 @@ class Bookmark < ActiveRecord::Base 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 @@ class Bookmark < ActiveRecord::Base 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 diff --git a/app/models/user_bookmark_list.rb b/app/models/user_bookmark_list.rb index df885e609d4..59188ff3a06 100644 --- a/app/models/user_bookmark_list.rb +++ b/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 @@ class UserBookmarkList end def per_page - @per_page ||= PER_PAGE + @per_page ||= @params[:per_page] end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c4cd31e62c8..34af12f3eb5 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb new file mode 100644 index 00000000000..a6327c349da --- /dev/null +++ b/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 diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index 5ab699f9d0c..b2ac9f09929 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -12,6 +12,58 @@ describe BookmarksController do 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)