FIX: token rotation not accounting for overlapping tokens correctly

also... freeze_time has no block form, correct all usages and specs

app/models/user_auth_token.rb

@@ -51,10 +51,11 @@ class UserAuthToken < ActiveRecord::Base
 
     token_expired =
        user_token &&
+       user_token.seen_at &&
        user_token.auth_token_seen &&
        user_token.prev_auth_token == token &&
        user_token.prev_auth_token != user_token.auth_token &&
-       user_token.rotated_at > 1.minute.ago
+       user_token.seen_at < 1.minute.ago
 
     if token_expired || !user_token
 
@@ -73,10 +74,14 @@ class UserAuthToken < ActiveRecord::Base
 
     if mark_seen && user_token && !user_token.auth_token_seen && user_token.auth_token == token
       # we must protect against concurrency issues here
-      changed_rows = UserAuthToken.where(id: user_token.id, auth_token: token).update_all(auth_token_seen: true)
+      changed_rows = UserAuthToken
+        .where(id: user_token.id, auth_token: token)
+        .update_all(auth_token_seen: true, seen_at: Time.zone.now)
+
       if changed_rows == 1
         # not doing a reload so we don't risk loading a rotated token
         user_token.auth_token_seen = true
+        user_token.seen_at = Time.zone.now
       end
 
       if SiteSetting.verbose_auth_token_logging
@@ -120,6 +125,7 @@ class UserAuthToken < ActiveRecord::Base
   UPDATE user_auth_tokens
   SET
     auth_token_seen = false,
+    seen_at = null,
     user_agent = :user_agent,
     client_ip = :client_ip,
     prev_auth_token = case when auth_token_seen then auth_token else prev_auth_token end,

db/migrate/20170215151505_add_seen_at_to_user_auth_token.rb

@@ -0,0 +1,10 @@
+class AddSeenAtToUserAuthToken < ActiveRecord::Migration
+  def up
+    add_column :user_auth_tokens, :seen_at, :datetime
+    ActiveRecord::Base.exec_sql "UPDATE user_auth_tokens SET seen_at = :now WHERE auth_token_seen", now: Time.zone.now
+  end
+
+  def down
+    remove_column :user_auth_tokens, :seen_at
+  end
+end

spec/models/topic_spec.rb

@@ -757,24 +757,29 @@ describe Topic do
 
       context 'topic was set to close when it was created' do
         it 'puts the autoclose duration in the moderator post' do
-          freeze_time(Time.new(2000,1,1)) do
-            @topic.created_at = 3.days.ago
-            @topic.update_status(status, true, @user)
-            expect(@topic.posts.last.raw).to include "closed after 3 days"
-          end
+          freeze_time(Time.new(2000,1,1))
+          @topic.created_at = 3.days.ago
+          @topic.update_status(status, true, @user)
+          expect(@topic.posts.last.raw).to include "closed after 3 days"
         end
       end
 
       context 'topic was set to close after it was created' do
         it 'puts the autoclose duration in the moderator post' do
-          freeze_time(Time.new(2000,1,1)) do
-            @topic.created_at = 7.days.ago
-            freeze_time(2.days.ago) do
-              @topic.set_auto_close(48)
-            end
-            @topic.update_status(status, true, @user)
-            expect(@topic.posts.last.raw).to include "closed after 2 days"
-          end
+
+          freeze_time(Time.new(2000,1,1))
+
+          @topic.created_at = 7.days.ago
+
+          freeze_time(2.days.ago)
+
+          @topic.set_auto_close(48)
+
+          freeze_time(2.days.from_now)
+
+          @topic.update_status(status, true, @user)
+          expect(@topic.posts.last.raw).to include "closed after 2 days"
+
         end
       end
     end

spec/models/topic_user_spec.rb

@@ -152,27 +152,28 @@ describe TopicUser do
 
   describe 'visited at' do
 
-    before do
-      TopicUser.track_visit!(topic.id, user.id)
-    end
-
     it 'set upon initial visit' do
-      freeze_time yesterday do
-        expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
-        expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
-      end
+      freeze_time yesterday
+
+      TopicUser.track_visit!(topic.id, user.id)
+
+      expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
+      expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
     end
 
     it 'updates upon repeat visit' do
-      today = yesterday.tomorrow
+      freeze_time yesterday
+
+      TopicUser.track_visit!(topic.id, user.id)
+
+      freeze_time Time.zone.now
+
+      TopicUser.track_visit!(topic.id, user.id)
+      # reload is a no go
+      topic_user = TopicUser.get(topic,user)
+      expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
+      expect(topic_user.last_visited_at.to_i).to eq(Time.zone.now.to_i)
 
-      freeze_time today do
-        TopicUser.track_visit!(topic.id, user.id)
-        # reload is a no go
-        topic_user = TopicUser.get(topic,user)
-        expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
-        expect(topic_user.last_visited_at.to_i).to eq(today.to_i)
-      end
     end
   end
 
@@ -180,29 +181,35 @@ describe TopicUser do
 
     context "without auto tracking" do
 
-      before do
-        TopicUser.update_last_read(user, topic.id, 1, 0)
-      end
-
       let(:topic_user) { TopicUser.get(topic,user) }
 
       it 'should create a new record for a visit' do
-        freeze_time yesterday do
-          expect(topic_user.last_read_post_number).to eq(1)
-          expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
-          expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
-        end
+        freeze_time yesterday
+
+        TopicUser.update_last_read(user, topic.id, 1, 0)
+
+        expect(topic_user.last_read_post_number).to eq(1)
+        expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
+        expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
       end
 
       it 'should update the record for repeat visit' do
-        freeze_time yesterday do
-          Fabricate(:post, topic: topic, user: user)
-          TopicUser.update_last_read(user, topic.id, 2, 0)
-          topic_user = TopicUser.get(topic,user)
-          expect(topic_user.last_read_post_number).to eq(2)
-          expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
-          expect(topic_user.first_visited_at.to_i).to eq(yesterday.to_i)
-        end
+
+        today = Time.zone.now
+        freeze_time Time.zone.now
+
+        TopicUser.update_last_read(user, topic.id, 1, 0)
+
+        tomorrow = 1.day.from_now
+        freeze_time tomorrow
+
+        Fabricate(:post, topic: topic, user: user)
+        TopicUser.update_last_read(user, topic.id, 2, 0)
+        topic_user = TopicUser.get(topic,user)
+
+        expect(topic_user.last_read_post_number).to eq(2)
+        expect(topic_user.last_visited_at.to_i).to eq(today.to_i)
+        expect(topic_user.first_visited_at.to_i).to eq(today.to_i)
       end
     end
 

spec/models/user_auth_token_spec.rb

@@ -110,16 +110,52 @@ describe UserAuthToken do
     expect(user_token.client_ip).to eq("1.1.2.4")
     expect(user_token.user_agent).to eq("a new user agent")
     expect(user_token.auth_token_seen).to eq(false)
+    expect(user_token.seen_at).to eq(nil)
     expect(user_token.prev_auth_token).to eq(prev_auth_token)
 
     # ability to auth using an old token
-    looked_up = UserAuthToken.lookup(unhashed_prev)
+    freeze_time
+
+    looked_up = UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true)
+    expect(looked_up.id).to eq(user_token.id)
+    expect(looked_up.auth_token_seen).to eq(true)
+    expect(looked_up.seen_at).to be_within(1.second).of(Time.zone.now)
+
+    looked_up = UserAuthToken.lookup(unhashed_prev, seen: true)
     expect(looked_up.id).to eq(user_token.id)
 
-    freeze_time(2.minute.from_now) do
-      looked_up = UserAuthToken.lookup(unhashed_prev)
-      expect(looked_up).to eq(nil)
-    end
+    freeze_time(2.minute.from_now)
+
+    looked_up = UserAuthToken.lookup(unhashed_prev)
+    expect(looked_up).to eq(nil)
+
+    rotated = user_token.rotate!(user_agent: "a new user agent", client_ip: "1.1.2.4")
+    expect(rotated).to eq(true)
+    user_token.reload
+    expect(user_token.seen_at).to eq(nil)
+  end
+
+  it "keeps prev token valid for 1 minute after it is confirmed" do
+
+    user = Fabricate(:user)
+
+    token = UserAuthToken.generate!(user_id: user.id,
+                                    user_agent: "some user agent",
+                                    client_ip: "1.1.2.3")
+
+    UserAuthToken.lookup(token.unhashed_auth_token, seen: true)
+
+    freeze_time(10.minutes.from_now)
+
+    prev_token = token.unhashed_auth_token
+
+    token.rotate!(user_agent: "firefox", client_ip: "1.1.1.1")
+
+    freeze_time(10.minutes.from_now)
+
+    expect(UserAuthToken.lookup(token.unhashed_auth_token, seen: true)).not_to eq(nil)
+    expect(UserAuthToken.lookup(prev_token, seen: true)).not_to eq(nil)
+
   end
 
   it "can correctly log auth tokens" do

spec/rails_helper.rb

@@ -139,6 +139,10 @@ Spork.prefork do
     datetime = DateTime.parse(now.to_s)
     time = Time.parse(now.to_s)
 
+    if block_given?
+      raise "Don't use a block with freeze_time"
+    end
+
     DateTime.stubs(:now).returns(datetime)
     Time.stubs(:now).returns(time)
   end