FIX: Handle PG readonly mode in Auth::DefaultCurrentUserProvider.

Avoid writing to the DB when PG is in readonly mode.

lib/auth/default_current_user_provider.rb

@@ -290,7 +290,7 @@ class Auth::DefaultCurrentUserProvider
   end
 
   def should_update_last_seen?
-    return false if Discourse.pg_readonly_mode?
+    return false unless can_write?
 
     api = !!(@env[API_KEY_ENV]) || !!(@env[USER_API_KEY_ENV])
 
@@ -309,6 +309,7 @@ class Auth::DefaultCurrentUserProvider
         raise Discourse::InvalidAccess
       end
 
+      if can_write?
         api_key.update_columns(last_used_at: Time.zone.now)
 
         if client_id.present? && client_id != api_key.client_id
@@ -321,6 +322,7 @@ class Auth::DefaultCurrentUserProvider
 
           api_key.update_columns(client_id: client_id)
         end
+      end
 
       api_key.user
     end
@@ -347,7 +349,7 @@ class Auth::DefaultCurrentUserProvider
           SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user)
         end
 
-      if user
+      if user && can_write?
         api_key.update_columns(last_used_at: Time.zone.now)
       end
 
@@ -389,4 +391,8 @@ class Auth::DefaultCurrentUserProvider
     ).performed!
   end
 
+  def can_write?
+    @can_write ||= !Discourse.pg_readonly_mode?
+  end
+
 end

spec/components/auth/default_current_user_provider_spec.rb

@@ -3,6 +3,10 @@
 require 'rails_helper'
 
 describe Auth::DefaultCurrentUserProvider do
+  # careful using fab! here is can lead to an erratic test
+  # we want a distinct user object per test so last_seen_at is
+  # handled correctly
+  let(:user) { Fabricate(:user) }
 
   class TestProvider < Auth::DefaultCurrentUserProvider
     attr_reader :env
@@ -24,7 +28,6 @@ describe Auth::DefaultCurrentUserProvider do
 
   context "server header api" do
     it "raises for a revoked key" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!
       params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
       expect(
@@ -51,12 +54,15 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "finds a user for a correct per-user api key" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(user_id: user.id, created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key }
 
       good_provider = provider("/", params)
+
+      expect do
         expect(good_provider.current_user.id).to eq(user.id)
+      end.to change { api_key.reload.last_used_at }
+
       expect(good_provider.is_api?).to eq(true)
       expect(good_provider.is_user_api?).to eq(false)
       expect(good_provider.should_update_last_seen?).to eq(false)
@@ -75,7 +81,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "raises for a user pretending" do
-      user = Fabricate(:user)
       user2 = Fabricate(:user)
       api_key = ApiKey.create!(user_id: user.id, created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user2.username.downcase }
@@ -86,7 +91,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "raises for a user with a mismatching ip" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24'])
       params = {
         "HTTP_API_KEY" => api_key.key,
@@ -97,11 +101,9 @@ describe Auth::DefaultCurrentUserProvider do
       expect {
         provider("/", params).current_user
       }.to raise_error(Discourse::InvalidAccess)
-
     end
 
     it "allows a user with a matching ip" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24'])
       params = {
         "HTTP_API_KEY" => api_key.key,
@@ -121,18 +123,15 @@ describe Auth::DefaultCurrentUserProvider do
 
       found_user = provider("/", params).current_user
       expect(found_user.id).to eq(user.id)
-
     end
 
     it "finds a user for a correct system api key" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase }
       expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
     it "raises for a mismatched api_key header and param username" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key }
       expect {
@@ -141,7 +140,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "finds a user for a correct system api key with external id" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
       params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USER_EXTERNAL_ID" => "abc" }
@@ -149,7 +147,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "raises for a mismatched api_key header and param external id" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
       params = { "HTTP_API_KEY" => api_key.key }
@@ -159,14 +156,12 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "finds a user for a correct system api key with id" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USER_ID" => user.id }
       expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
     it "raises for a mismatched api_key header and param user id" do
-      user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
       params = { "HTTP_API_KEY" => api_key.key }
       expect {
@@ -174,6 +169,27 @@ describe Auth::DefaultCurrentUserProvider do
       }.to raise_error(Discourse::InvalidAccess)
     end
 
+    describe "when readonly mode is enabled due to postgres" do
+      before do
+        Discourse.enable_readonly_mode(Discourse::PG_READONLY_MODE_KEY)
+      end
+
+      after do
+        Discourse.disable_readonly_mode(Discourse::PG_READONLY_MODE_KEY)
+      end
+
+      it "should not update ApiKey#last_used_at" do
+        api_key = ApiKey.create!(user_id: user.id, created_by_id: -1)
+        params = { "HTTP_API_KEY" => api_key.key }
+
+        good_provider = provider("/", params)
+
+        expect do
+          expect(good_provider.current_user.id).to eq(user.id)
+        end.to_not change { api_key.reload.last_used_at }
+      end
+    end
+
     context "rate limiting" do
       before do
         RateLimiter.enable
@@ -188,7 +204,6 @@ describe Auth::DefaultCurrentUserProvider do
 
         freeze_time
 
-        user = Fabricate(:user)
         api_key = ApiKey.create!(created_by_id: -1)
         params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase }
         system_params = params.merge("HTTP_API_USERNAME" => "system")
@@ -217,18 +232,11 @@ describe Auth::DefaultCurrentUserProvider do
         api_key = ApiKey.create!(created_by_id: -1)
         params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase }
         provider("/", params).current_user
-
       end
     end
-
   end
 
   describe "#current_user" do
-    # careful using fab! here is can lead to an erratic test
-    # we want a distinct user object per test so last_seen_at is
-    # handled correctly
-    let!(:user) { Fabricate(:user) }
-
     let(:unhashed_token) do
       new_provider = provider('/')
       cookies = {}
@@ -271,7 +279,7 @@ describe Auth::DefaultCurrentUserProvider do
         Discourse.disable_readonly_mode(Discourse::PG_READONLY_MODE_KEY)
       end
 
-      it "should not update last seen at" do
+      it "should not update User#last_seen_at" do
         provider2 = provider("/", "HTTP_COOKIE" => "_t=#{unhashed_token}")
         u = provider2.current_user
         u.reload
@@ -301,7 +309,6 @@ describe Auth::DefaultCurrentUserProvider do
   end
 
   it "should update last seen for API calls with Discourse-Present header" do
-    user = Fabricate(:user)
     api_key = ApiKey.create!(user_id: user.id, created_by_id: -1)
     params = { :method => "POST",
                "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest",
@@ -314,7 +321,6 @@ describe Auth::DefaultCurrentUserProvider do
 
   it "correctly rotates tokens" do
     SiteSetting.maximum_session_age = 3
-    user = Fabricate(:user)
     @provider = provider('/')
     cookies = {}
     @provider.log_on_user(user, {}, cookies)
@@ -376,7 +382,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "fires event when updating last seen" do
-      user = Fabricate(:user)
       @provider = provider('/')
       cookies = {}
       @provider.log_on_user(user, {}, cookies)
@@ -388,7 +393,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "does not fire an event when last seen does not update" do
-      user = Fabricate(:user)
       @provider = provider('/')
       cookies = {}
       @provider.log_on_user(user, {}, cookies)
@@ -411,7 +415,6 @@ describe Auth::DefaultCurrentUserProvider do
     end
 
     it "can only try 10 bad cookies a minute" do
-      user = Fabricate(:user)
       token = UserAuthToken.generate!(user_id: user.id)
 
       provider('/').log_on_user(user, {}, {})
@@ -450,8 +453,6 @@ describe Auth::DefaultCurrentUserProvider do
   end
 
   it "logging on user always creates a new token" do
-    user = Fabricate(:user)
-
     provider('/').log_on_user(user, {}, {})
     provider('/').log_on_user(user, {}, {})
 
@@ -459,8 +460,6 @@ describe Auth::DefaultCurrentUserProvider do
   end
 
   it "cleans up old sessions when a user logs in" do
-    user = Fabricate(:user)
-
     yesterday = 1.day.ago
 
     UserAuthToken.insert_all((1..(UserAuthToken::MAX_SESSION_COUNT + 2)).to_a.map do |i|
@@ -489,7 +488,6 @@ describe Auth::DefaultCurrentUserProvider do
     SiteSetting.force_https = false
     SiteSetting.same_site_cookies = "Lax"
 
-    user = Fabricate(:user)
     cookies = {}
     provider('/').log_on_user(user, {}, cookies)
 
@@ -509,7 +507,6 @@ describe Auth::DefaultCurrentUserProvider do
 
   it "correctly expires session" do
     SiteSetting.maximum_session_age = 2
-    user = Fabricate(:user)
     token = UserAuthToken.generate!(user_id: user.id)
 
     provider('/').log_on_user(user, {}, {})
@@ -521,10 +518,10 @@ describe Auth::DefaultCurrentUserProvider do
   end
 
   it "always unstage users" do
-    staged_user = Fabricate(:user, staged: true)
+    user.update!(staged: true)
-    provider("/").log_on_user(staged_user, {}, {})
+    provider("/").log_on_user(user, {}, {})
-    staged_user.reload
+    user.reload
-    expect(staged_user.staged).to eq(false)
+    expect(user.staged).to eq(false)
   end
 
   context "user api" do
@@ -568,7 +565,10 @@ describe Auth::DefaultCurrentUserProvider do
 
       good_provider = provider("/", params)
 
+      expect do
         expect(good_provider.current_user.id).to eq(user.id)
+      end.to change { api_key.reload.last_used_at }
+
       expect(good_provider.is_api?).to eq(false)
       expect(good_provider.is_user_api?).to eq(true)
       expect(good_provider.should_update_last_seen?).to eq(false)
@@ -582,7 +582,30 @@ describe Auth::DefaultCurrentUserProvider do
       expect {
         provider("/", params).current_user
       }.to raise_error(Discourse::InvalidAccess)
+    end
 
+
+    describe "when readonly mode is enabled due to postgres" do
+      before do
+        Discourse.enable_readonly_mode(Discourse::PG_READONLY_MODE_KEY)
+      end
+
+      after do
+        Discourse.disable_readonly_mode(Discourse::PG_READONLY_MODE_KEY)
+      end
+
+      it 'should not update ApiKey#last_used_at' do
+        params = {
+          "REQUEST_METHOD" => "GET",
+          "HTTP_USER_API_KEY" => api_key.key,
+        }
+
+        good_provider = provider("/", params)
+
+        expect do
+          expect(good_provider.current_user.id).to eq(user.id)
+        end.to_not change { api_key.reload.last_used_at }
+      end
     end
 
     context "rate limiting" do
@@ -630,7 +653,6 @@ describe Auth::DefaultCurrentUserProvider do
         expect {
           provider("/", params).current_user
         }.to raise_error(RateLimiter::LimitExceeded)
-
       end
     end
   end