SECURITY: Respect topic permissions when loading bookmark metadata

Co-authored-by: Martin Brennan <martin@discourse.org>
Co-authored-by: Sam Saffron <sam.saffron@gmail.com>

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: {

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),
+      topic: post.topic,
-      post_id: post_id,
+      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)

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

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

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

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)

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"

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
+    it "it has nothing to serialize" do
-      serialized
+      expect(bookmark_list).to eq([])
-      expect(serialized.title).not_to eq(nil)
     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
+    it "it has nothing to serialize" do
-      serialized
+      expect(bookmark_list).to eq([])
-      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)
     end
   end
 
-  def serialized
-    described_class.new(BookmarkQuery.new(bookmark.user, {}).list_all.to_ary.last)
-  end
 end