FIX: disable storing invalid post and topic timing when sent from client (#26683)

This ensures we only ever store correct post and topic timing when the client
notifies.

Previous to this change we would blindly trust the client.

Additionally this has error correction code that will correct the last seen
post number when you visit a topic with incorrect timings.

app/models/post_timing.rb

@@ -148,6 +148,16 @@ class PostTiming < ActiveRecord::Base
   MAX_READ_TIME_PER_BATCH = 60 * 1000.0
 
   def self.process_timings(current_user, topic_id, topic_time, timings, opts = {})
+    lookup_column = current_user.whisperer? ? "highest_staff_post_number" : "highest_post_number"
+    highest_post_number = DB.query_single(<<~SQL, topic_id: topic_id).first
+          SELECT #{lookup_column}
+          FROM topics
+          WHERE id = :topic_id
+        SQL
+
+    # does not exist log nothing
+    return if highest_post_number.nil?
+
     UserStat.update_time_read!(current_user.id)
 
     max_time_per_post = ((Time.now - current_user.created_at) * 1000.0)
@@ -163,6 +173,7 @@ class PostTiming < ActiveRecord::Base
       i -= 1
       timings[i][1] = max_time_per_post if timings[i][1] > max_time_per_post
       timings.delete_at(i) if timings[i][0] < 1
+      timings.delete_at(i) if timings[i][0] > highest_post_number
     end
 
     timings.each_with_index do |(post_number, time), index|

app/models/topic_user.rb

@@ -308,7 +308,14 @@ class TopicUser < ActiveRecord::Base
     UPDATE_TOPIC_USER_SQL = <<~SQL
       UPDATE topic_users
       SET
-        last_read_post_number = GREATEST(:post_number, tu.last_read_post_number),
+        last_read_post_number =
+          LEAST(
+            CASE WHEN :whisperer
+              THEN highest_staff_post_number
+              ELSE highest_post_number END
+            ,
+            GREATEST(:post_number, tu.last_read_post_number)
+          ),
         total_msecs_viewed = LEAST(tu.total_msecs_viewed + :msecs,86400000),
         notification_level =
            case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) >
@@ -357,6 +364,7 @@ class TopicUser < ActiveRecord::Base
         msecs: msecs,
         tracking: notification_levels[:tracking],
         threshold: SiteSetting.default_other_auto_track_topics_after_msecs,
+        whisperer: user.whisperer?,
       }
 
       rows = DB.query(UPDATE_TOPIC_USER_SQL, args)

spec/models/post_timing_spec.rb

@@ -83,6 +83,7 @@ RSpec.describe PostTiming do
 
       (2..5).each { |i| Fabricate(:post, topic: post.topic, post_number: i) }
       user2 = Fabricate(:coding_horror, created_at: 1.day.ago)
+      Topic.reset_highest(post.topic.id)
 
       PostActionCreator.like(user2, post)
 

spec/models/topic_user_spec.rb

@@ -228,6 +228,12 @@ RSpec.describe TopicUser do
         today = Time.zone.now
         freeze_time Time.zone.now
 
+        # ensure data model is correct for the test
+        # logging an update to a row that does not exist
+        # is not supported
+        _post1 = Fabricate(:post, topic: topic)
+        _post2 = Fabricate(:post, topic: topic)
+
         TopicUser.update_last_read(user, topic.id, 1, 1, 0)
 
         tomorrow = 1.day.from_now

spec/requests/topics_controller_spec.rb

@@ -4626,6 +4626,62 @@ RSpec.describe TopicsController do
   describe "#timings" do
     fab!(:post_1) { Fabricate(:post, user: post_author1, topic: topic) }
 
+    before do
+      # admins
+      SiteSetting.whispers_allowed_groups = "1"
+    end
+
+    let(:whisper) do
+      Fabricate(:post, user: post_author1, topic: topic, post_type: Post.types[:whisper])
+    end
+
+    it "should gracefully handle invalid timings sent in" do
+      sign_in(user)
+      params = {
+        topic_id: topic.id,
+        topic_time: 5,
+        timings: {
+          post_1.post_number => 2,
+          whisper.post_number => 2,
+          1000 => 100,
+        },
+      }
+
+      post "/topics/timings.json", params: params
+      expect(response.status).to eq(200)
+
+      tu = TopicUser.find_by(user: user, topic: topic)
+      expect(tu.last_read_post_number).to eq(post_1.post_number)
+
+      # lets also test timing recovery here
+      tu.update!(last_read_post_number: 999)
+
+      post "/topics/timings.json", params: params
+
+      tu = TopicUser.find_by(user: user, topic: topic)
+      expect(tu.last_read_post_number).to eq(post_1.post_number)
+    end
+
+    it "should gracefully handle invalid timings sent in from staff" do
+      sign_in(admin)
+
+      post "/topics/timings.json",
+           params: {
+             topic_id: topic.id,
+             topic_time: 5,
+             timings: {
+               post_1.post_number => 2,
+               whisper.post_number => 2,
+               1000 => 100,
+             },
+           }
+
+      expect(response.status).to eq(200)
+
+      tu = TopicUser.find_by(user: admin, topic: topic)
+      expect(tu.last_read_post_number).to eq(whisper.post_number)
+    end
+
     it "should record the timing" do
       sign_in(user)
 
@@ -5382,7 +5438,7 @@ RSpec.describe TopicsController do
 
       it "resets bump correctly" do
         post1 = Fabricate(:post, user: post_author1, topic: topic, created_at: 2.days.ago)
-        post2 = Fabricate(:post, user: post_author1, topic: topic, created_at: 1.day.ago)
+        _post2 = Fabricate(:post, user: post_author1, topic: topic, created_at: 1.day.ago)
 
         put "/t/#{topic.id}/reset-bump-date/#{post1.id}.json"
         expect(response.status).to eq(200)