FIX: Make deleted topic post bookmarks more resilient (#10619)

This PR ensures that new bookmarks cannot be created for deleted posts and topics, and also makes sure that if a bookmark was created and then the topic deleted that the show topic page does not error from trying to retrieve the bookmark reminder at.

lib/bookmark_manager.rb

@@ -8,10 +8,15 @@ class BookmarkManager
   end
 
   def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil, options: {})
-    post = Post.unscoped.includes(:topic).find(post_id)
+    post = Post.find_by(id: post_id)
     reminder_type = parse_reminder_type(reminder_type)
 
-    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_see_post?(post)
+    # no bookmarking deleted posts or topics
+    raise Discourse::InvalidAccess if post.blank? || post.topic.blank?
+
+    if !Guardian.new(@user).can_see_post?(post) || !Guardian.new(@user).can_see_topic?(post.topic)
+      raise Discourse::InvalidAccess
+    end
 
     bookmark = Bookmark.create(
       {

lib/topic_view.rb

@@ -356,7 +356,8 @@ class TopicView
   end
 
   def first_post_bookmark_reminder_at
-    @topic.first_post.bookmarks.where(user: @user).pluck_first(:reminder_at)
+    @topic.posts.with_deleted.where(post_number: 1).first
+      .bookmarks.where(user: @user).pluck_first(:reminder_at)
   end
 
   MAX_PARTICIPANTS = 24

spec/components/topic_view_spec.rb

@@ -349,6 +349,24 @@ describe TopicView do
       end
     end
 
+    context "#first_post_bookmark_reminder_at" do
+      let!(:user) { Fabricate(:user) }
+      let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) }
+
+      it "gets the first post bookmark reminder at for the user" do
+        expect(TopicView.new(topic.id, user).first_post_bookmark_reminder_at).to eq_time(bookmark1.reminder_at)
+      end
+
+      context "when the topic is deleted" do
+        it "gets the first post bookmark reminder at for the user" do
+          topic_view = TopicView.new(topic, user)
+          PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy
+          topic.reload
+          expect(topic_view.first_post_bookmark_reminder_at).to eq_time(bookmark1.reminder_at)
+        end
+      end
+    end
+
     context '.topic_user' do
       it 'returns nil when there is no user' do
         expect(TopicView.new(topic.id, nil).topic_user).to be_blank

spec/lib/bookmark_manager_spec.rb

@@ -21,6 +21,16 @@ RSpec.describe BookmarkManager do
       expect(bookmark.topic_id).to eq(post.topic_id)
     end
 
+    it "when topic is deleted it raises invalid access from guardian check" do
+      post.topic.trash!
+      expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
+    end
+
+    it "when post is deleted it raises invalid access from guardian check" do
+      post.trash!
+      expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
+    end
+
     it "updates the topic user bookmarked column to true if any post is bookmarked" do
       subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
       tu = TopicUser.find_by(user: user)