From 5db41cd578dba75129c9bad24f2280845ef4db70 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 23 Mar 2020 11:04:39 +0000 Subject: [PATCH] SECURITY: Respect topic permissions when loading bookmark metadata Co-authored-by: Martin Brennan Co-authored-by: Sam Saffron --- app/controllers/users_controller.rb | 2 +- lib/bookmark_manager.rb | 11 +- lib/bookmark_query.rb | 16 ++- lib/guardian/bookmark_guardian.rb | 4 + spec/lib/bookmark_manager_spec.rb | 18 +++ spec/lib/bookmark_query_spec.rb | 105 +++++++++++++++++- spec/requests/users_controller_spec.rb | 5 + .../user_bookmark_serializer_spec.rb | 21 ++-- 8 files changed, 151 insertions(+), 31 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6125bf98366..da9ba484ee0 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1401,7 +1401,7 @@ class UsersController < ApplicationController respond_to do |format| format.json do - bookmarks = BookmarkQuery.new(user, params).list_all + bookmarks = BookmarkQuery.new(user: user, guardian: guardian, params: params).list_all if bookmarks.empty? render json: { diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index f4831e13503..c2184b394c0 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -8,12 +8,15 @@ class BookmarkManager end def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil) + post = Post.unscoped.includes(:topic).find(post_id) reminder_type = Bookmark.reminder_types[reminder_type.to_sym] if reminder_type.present? + raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_see_post?(post) + bookmark = Bookmark.create( user_id: @user.id, - topic_id: topic_id_for_post(post_id), - post_id: post_id, + topic: post.topic, + post: post, name: name, reminder_type: reminder_type, reminder_at: reminder_at, @@ -58,10 +61,6 @@ class BookmarkManager private - def topic_id_for_post(post_id) - Post.where(id: post_id).pluck_first(:topic_id) - end - def clear_at_desktop_cache_if_required return if user_has_any_pending_at_desktop_reminders? Discourse.redis.del(BookmarkReminderNotificationHandler::PENDING_AT_DESKTOP_KEY_PREFIX + @user.id.to_s) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index d38bdabf7c6..b55e8eb8b6a 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -18,18 +18,22 @@ class BookmarkQuery end end - def initialize(user, params = {}) + def initialize(user:, guardian: nil, params: {}) @user = user @params = params + @guardian = guardian || Guardian.new(@user) end def list_all results = user_bookmarks - .joins('INNER JOIN topics ON topics.id = bookmarks.topic_id') - .joins('INNER JOIN posts ON posts.id = bookmarks.post_id') - .joins('INNER JOIN users ON users.id = posts.user_id') .order('bookmarks.created_at DESC') + topics = Topic.listable_topics.secured(@guardian) + pms = Topic.private_messages_for_user(@user) + results = results.merge(topics.or(pms)) + + results = results.merge(Post.secured(@guardian)) + if @params[:limit] results = results.limit(@params[:limit]) end @@ -42,7 +46,7 @@ class BookmarkQuery BookmarkQuery.preload(results, self) - results + @guardian.filter_allowed_categories(results) end private @@ -51,5 +55,7 @@ class BookmarkQuery Bookmark.where(user: @user) .includes(topic: :tags) .includes(post: :user) + .references(:topic) + .references(:post) end end diff --git a/lib/guardian/bookmark_guardian.rb b/lib/guardian/bookmark_guardian.rb index 9f449757777..1754c280e23 100644 --- a/lib/guardian/bookmark_guardian.rb +++ b/lib/guardian/bookmark_guardian.rb @@ -4,4 +4,8 @@ module BookmarkGuardian def can_delete_bookmark?(bookmark) @user == bookmark.user end + + def can_create_bookmark?(bookmark) + can_see_topic?(bookmark.topic) + end end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index dbfecd565dd..11a0b7d8599 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -90,6 +90,24 @@ RSpec.describe BookmarkManager do expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future")) end end + + context "when the post is inaccessable for the user" do + before do + post.trash! + end + it "raises an invalid access error" do + expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess) + end + end + + context "when the topic is inaccessable for the user" do + before do + post.topic.update(category: Fabricate(:private_category, group: Fabricate(:group))) + end + it "raises an invalid access error" do + expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess) + end + end end describe ".destroy" do diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 2fe99c38930..c627abfc85e 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -8,11 +8,28 @@ RSpec.describe BookmarkQuery do fab!(:bookmark2) { Fabricate(:bookmark, user: user) } let(:params) { {} } - subject { described_class.new(user, params) } + def bookmark_query(user: nil, params: nil) + BookmarkQuery.new(user: user || self.user, params: params || self.params) + end + + before do + TopicUser.change(user.id, bookmark1.topic_id, total_msecs_viewed: 1) + TopicUser.change(user.id, bookmark2.topic_id, total_msecs_viewed: 1) + end describe "#list_all" do it "returns all the bookmarks for a user" do - expect(subject.list_all.count).to eq(2) + expect(bookmark_query.list_all.count).to eq(2) + end + + it "does not return deleted posts" do + bookmark1.post.trash! + expect(bookmark_query.list_all.count).to eq(1) + end + + it "does not return deleted topics" do + bookmark1.topic.trash! + expect(bookmark_query.list_all.count).to eq(1) end it "runs the on_preload block provided passing in bookmarks" do @@ -20,14 +37,92 @@ RSpec.describe BookmarkQuery do BookmarkQuery.on_preload do |bookmarks, bq| (preloaded_bookmarks << bookmarks).flatten end - subject.list_all + bookmark_query.list_all expect(preloaded_bookmarks.any?).to eq(true) end + context "for a whispered post" do + before do + bookmark1.post.update(post_type: Post.types[:whisper]) + end + context "when the user is moderator" do + it "does return the whispered post" do + user.update!(moderator: true) + expect(bookmark_query.list_all.count).to eq(2) + end + end + context "when the user is admin" do + it "does return the whispered post" do + user.update!(admin: true) + expect(bookmark_query.list_all.count).to eq(2) + end + end + context "when the user is not staff" do + it "does not return the whispered post" do + expect(bookmark_query.list_all.count).to eq(1) + end + end + end + + context "for a private message topic bookmark" do + let(:pm_topic) { Fabricate(:private_message_topic) } + before do + bookmark1.update(topic: pm_topic, post: Fabricate(:post, topic: pm_topic)) + TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1) + end + + context "when the user is a topic_allowed_user" do + before do + TopicAllowedUser.create(topic: pm_topic, user: user) + end + it "shows the user the bookmark in the PM" do + expect(bookmark_query.list_all.map(&:id).count).to eq(2) + end + end + + context "when the user is in a topic_allowed_group" do + before do + group = Fabricate(:group) + GroupUser.create(group: group, user: user) + TopicAllowedGroup.create(topic: pm_topic, group: group) + end + it "shows the user the bookmark in the PM" do + expect(bookmark_query.list_all.map(&:id).count).to eq(2) + end + end + + context "when the user is not a topic_allowed_user" do + it "does not show the user a bookmarked post in a PM where they are not an allowed user" do + expect(bookmark_query.list_all.map(&:id).count).to eq(1) + end + end + + context "when the user is not in a topic_allowed_group" do + it "does not show the user a bookmarked post in a PM where they are not in an allowed group" do + expect(bookmark_query.list_all.map(&:id).count).to eq(1) + end + end + end + + context "when the topic category is private" do + let(:group) { Fabricate(:group) } + before do + bookmark1.topic.update(category: Fabricate(:private_category, group: group)) + bookmark1.reload + end + it "does not show the user a post/topic in a private category they cannot see" do + expect(bookmark_query.list_all.map(&:id)).not_to include(bookmark1.id) + end + it "does show the user a post/topic in a private category they can see" do + GroupUser.create(user: user, group: group) + expect(bookmark_query.list_all.map(&:id)).to include(bookmark1.id) + end + end + context "when the limit param is provided" do let(:params) { { limit: 1 } } it "is respected" do - expect(subject.list_all.count).to eq(1) + expect(bookmark_query.list_all.count).to eq(1) end end @@ -41,7 +136,7 @@ RSpec.describe BookmarkQuery do it "preloads them" do Topic.expects(:preload_custom_fields) expect( - subject.list_all.find do |b| + bookmark_query.list_all.find do |b| b.topic_id = bookmark1.topic_id end.topic.custom_fields['test_field'] ).not_to eq(nil) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index b2cc43fedee..57a936aa333 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -4172,6 +4172,11 @@ describe UsersController do let!(:bookmark2) { Fabricate(:bookmark, user: user) } let!(:bookmark3) { Fabricate(:bookmark) } + before do + TopicUser.change(user.id, bookmark1.topic_id, total_msecs_viewed: 1) + TopicUser.change(user.id, bookmark2.topic_id, total_msecs_viewed: 1) + end + it "returns a list of serialized bookmarks for the user" do sign_in(user) get "/u/#{user.username}/bookmarks.json" diff --git a/spec/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb index 85acabf95cf..36b05abf7ba 100644 --- a/spec/serializers/user_bookmark_serializer_spec.rb +++ b/spec/serializers/user_bookmark_serializer_spec.rb @@ -6,9 +6,11 @@ RSpec.describe UserBookmarkSerializer do let(:user) { Fabricate(:user) } let(:post) { Fabricate(:post, user: user) } let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post, topic: post.topic) } + let(:bookmark_list) { BookmarkQuery.new(user: bookmark.user).list_all.to_ary } it "serializes all properties correctly" do - s = serialized + s = UserBookmarkSerializer.new(bookmark_list.last) + expect(s.id).to eq(bookmark.id) expect(s.created_at).to eq(bookmark.created_at) expect(s.topic_id).to eq(bookmark.topic_id) @@ -34,9 +36,8 @@ RSpec.describe UserBookmarkSerializer do bookmark.topic.trash! bookmark.reload end - it "still returns the topic title because the relationship is unscoped" do - serialized - expect(serialized.title).not_to eq(nil) + it "it has nothing to serialize" do + expect(bookmark_list).to eq([]) end end @@ -45,17 +46,9 @@ RSpec.describe UserBookmarkSerializer do bookmark.post.trash! bookmark.reload end - it "still returns the post number because the relationship is unscoped" do - serialized - expect(serialized.linked_post_number).not_to eq(nil) - end - it "still returns the post username" do - serialized - expect(serialized.username).not_to eq(nil) + it "it has nothing to serialize" do + expect(bookmark_list).to eq([]) end end - def serialized - described_class.new(BookmarkQuery.new(bookmark.user, {}).list_all.to_ary.last) - end end