From 431bd84dec37ce351a0c154528ca0d4b815413d4 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Mon, 7 Sep 2020 14:52:14 +1000
Subject: [PATCH] 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            |  9 +++++++--
 lib/topic_view.rb                  |  3 ++-
 spec/components/topic_view_spec.rb | 18 ++++++++++++++++++
 spec/lib/bookmark_manager_spec.rb  | 10 ++++++++++
 4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb
index 773713a1fe7..ba5c42a040d 100644
--- a/lib/bookmark_manager.rb
+++ b/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(
       {
diff --git a/lib/topic_view.rb b/lib/topic_view.rb
index 451f483f8fd..bcc4ac2169c 100644
--- a/lib/topic_view.rb
+++ b/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
diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb
index 508ba36fc6d..7600b9e50f3 100644
--- a/spec/components/topic_view_spec.rb
+++ b/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
diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb
index 0ccd35679d0..bd0836329c9 100644
--- a/spec/lib/bookmark_manager_spec.rb
+++ b/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)