SECURITY: Respect topic permissions when loading bookmark metadata

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

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 6125bf9..da9ba48 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 f4831e1..c2184b3 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 d38bdab..b55e8eb 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 9f44975..1754c28 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 dbfecd5..11a0b7d 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 2fe99c3..c627abf 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
+

[... diff too long, it was truncated ...]

GitHub sha: 5db41cd5

Will this be backported too or is stable not affected?

Stable and Beta are not affected by this

ok. thanks. updating our instance with the other fix.