From 616de8323266da3e302a1ca47cdb03acd66f99a9 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Tue, 3 May 2022 08:50:56 +1000
Subject: [PATCH] FIX: avoid concurrent usage of AR models (#16596)

Flagged by the truffle team at: https://meta.discourse.org/t/thread-unsafe-current-user-usage-in-auth-defaultcurrentuserprovider/225671

This usage of AR is unsafe currently, as AR models are not safe for concurrent usage

Introduces a new query potentially every minute which should be acceptable.
---
 app/models/user.rb                        | 47 +++++++++++++++--------
 lib/auth/default_current_user_provider.rb | 11 ++++--
 2 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 3a5f3837529..f9099ee2bc0 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -752,12 +752,17 @@ class User < ActiveRecord::Base
     end
   end
 
-  def update_ip_address!(new_ip_address)
-    unless ip_address == new_ip_address || new_ip_address.blank?
-      update_column(:ip_address, new_ip_address)
+  def self.update_ip_address!(user_id, new_ip:, old_ip:)
+    unless old_ip == new_ip || new_ip.blank?
+
+      DB.exec(<<~SQL, user_id: user_id, ip_address: new_ip)
+        UPDATE users
+        SET ip_address = :ip_address
+        WHERE id = :user_id
+      SQL
 
       if SiteSetting.keep_old_ip_address_count > 0
-        DB.exec(<<~SQL, user_id: self.id, ip_address: new_ip_address, current_timestamp: Time.zone.now)
+        DB.exec(<<~SQL, user_id: user_id, ip_address: new_ip, current_timestamp: Time.zone.now)
         INSERT INTO user_ip_address_histories (user_id, ip_address, created_at, updated_at)
         VALUES (:user_id, :ip_address, :current_timestamp, :current_timestamp)
         ON CONFLICT (user_id, ip_address)
@@ -765,7 +770,7 @@ class User < ActiveRecord::Base
           UPDATE SET updated_at = :current_timestamp
         SQL
 
-        DB.exec(<<~SQL, user_id: self.id, offset: SiteSetting.keep_old_ip_address_count)
+        DB.exec(<<~SQL, user_id: user_id, offset: SiteSetting.keep_old_ip_address_count)
         DELETE FROM user_ip_address_histories
         WHERE id IN (
           SELECT
@@ -780,24 +785,36 @@ class User < ActiveRecord::Base
     end
   end
 
-  def last_seen_redis_key(now)
+  def update_ip_address!(new_ip_address)
+    User.update_ip_address!(id, new_ip: new_ip_address, old_ip: ip_address)
+  end
+
+  def self.last_seen_redis_key(user_id, now)
     now_date = now.to_date
-    "user:#{id}:#{now_date}"
+    "user:#{user_id}:#{now_date}"
+  end
+
+  def last_seen_redis_key(now)
+    User.last_seen_redis_key(id, now)
   end
 
   def clear_last_seen_cache!(now = Time.zone.now)
     Discourse.redis.del(last_seen_redis_key(now))
   end
 
-  def update_last_seen!(now = Time.zone.now)
-    redis_key = last_seen_redis_key(now)
+  def self.should_update_last_seen?(user_id, now = Time.zone.now)
+    return true if SiteSetting.active_user_rate_limit_secs <= 0
 
-    if SiteSetting.active_user_rate_limit_secs > 0
-      return if !Discourse.redis.set(
-        redis_key, "1",
-        nx: true,
-        ex: SiteSetting.active_user_rate_limit_secs
-      )
+    Discourse.redis.set(
+      last_seen_redis_key(user_id, now), "1",
+      nx: true,
+      ex: SiteSetting.active_user_rate_limit_secs
+    )
+  end
+
+  def update_last_seen!(now = Time.zone.now, force: false)
+    if !force
+      return if !User.should_update_last_seen?(self.id, now)
     end
 
     update_previous_visit(now)
diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 8f10226254a..9c3dd60d5e1 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -217,12 +217,17 @@ class Auth::DefaultCurrentUserProvider
     end
 
     if current_user && should_update_last_seen?
-      u = current_user
       ip = request.ip
+      user_id = current_user.id
+      old_ip = current_user.ip_address
 
       Scheduler::Defer.later "Updating Last Seen" do
-        u.update_last_seen!
-        u.update_ip_address!(ip)
+        if User.should_update_last_seen?(user_id)
+          if u = User.find_by(id: user_id)
+            u.update_last_seen!(Time.zone.now, force: true)
+          end
+        end
+        User.update_ip_address!(user_id, new_ip: ip, old_ip: old_ip)
       end
     end