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

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

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

diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index b97b866..f20b056 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/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,17 +309,19 @@ class Auth::DefaultCurrentUserProvider
         raise Discourse::InvalidAccess
       end
 
-      api_key.update_columns(last_used_at: Time.zone.now)
+      if can_write?
+        api_key.update_columns(last_used_at: Time.zone.now)
 
-      if client_id.present? && client_id != api_key.client_id
+        if client_id.present? && client_id != api_key.client_id
 
-        # invalidate old dupe api key for client if needed
-        UserApiKey
-          .where(client_id: client_id, user_id: api_key.user_id)
-          .where('id <> ?', api_key.id)
-          .destroy_all
+          # invalidate old dupe api key for client if needed
+          UserApiKey
+            .where(client_id: client_id, user_id: api_key.user_id)
+            .where('id <> ?', api_key.id)
+            .destroy_all
 
-        api_key.update_columns(client_id: client_id)
+          api_key.update_columns(client_id: client_id)
+        end
       end
 
       api_key.user
@@ -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
diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb
index 8c14233..94e1a29 100644
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/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(good_provider.current_user.id).to eq(user.id)
+
+      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")

[... diff too long, it was truncated ...]

GitHub sha: 94fced21