FIX: store information about the login method in the database. (#28054)

Previously in these 2 PRs, we introduced a new site setting `SiteSetting.enforce_second_factor_on_external_auth`.

https://github.com/discourse/discourse/pull/27547
https://github.com/discourse/discourse/pull/27674

When disabled, it should enforce 2FA for local login with username and password and skip the requirement when authenticating with oauth2.

We stored information about the login method in a secure session but it is not reliable. Therefore, information about the login method is moved to the database.

app/controllers/application_controller.rb

@@ -609,7 +609,7 @@ class ApplicationController < ActionController::Base
 
   def login_method
     return if current_user.anonymous?
-    secure_session["oauth"] == "true" ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL
+    current_user.authenticated_with_oauth ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL
   end
 
   private

app/controllers/session_controller.rb

@@ -370,7 +370,6 @@ class SessionController < ApplicationController
     return render(json: @second_factor_failure_payload) if !second_factor_auth_result.ok
 
     if user.active && user.email_confirmed?
-      secure_session["oauth"] = false if !SiteSetting.persistent_sessions
       login(user, second_factor_auth_result)
     else
       not_activated(user)

app/controllers/users/omniauth_callbacks_controller.rb

@@ -86,7 +86,6 @@ class Users::OmniauthCallbacksController < ApplicationController
 
     cookies["_bypass_cache"] = true
     cookies[:authentication_data] = { value: client_hash.to_json, path: Discourse.base_path("/") }
-    secure_session.set("oauth", true, expires: SiteSetting.maximum_session_age.hours)
     redirect_to @origin
   end
 
@@ -183,7 +182,7 @@ class Users::OmniauthCallbacksController < ApplicationController
         return
       end
 
-      log_on_user(user)
+      log_on_user(user, { authenticated_with_oauth: true })
       Invite.invalidate_for_email(user.email) # invite link can't be used to log in anymore
       session[:authentication] = nil # don't carry around old auth info, perhaps move elsewhere
       @auth_result.authenticated = true

app/models/user.rb

@@ -253,6 +253,9 @@ class User < ActiveRecord::Base
   # Cache for user custom fields. Currently it is used to display quick search results
   attr_accessor :custom_data
 
+  # Information if user was authenticated with OAuth
+  attr_accessor :authenticated_with_oauth
+
   scope :with_email,
         ->(email) { joins(:user_emails).where("lower(user_emails.email) IN (?)", email) }
 

app/models/user_auth_token.rb

@@ -78,7 +78,8 @@ class UserAuthToken < ActiveRecord::Base
     client_ip: nil,
     path: nil,
     staff: nil,
-    impersonate: false
+    impersonate: false,
+    authenticated_with_oauth: false
   )
     token = SecureRandom.hex(16)
     hashed_token = hash_token(token)
@@ -90,6 +91,7 @@ class UserAuthToken < ActiveRecord::Base
         auth_token: hashed_token,
         prev_auth_token: hashed_token,
         rotated_at: Time.zone.now,
+        authenticated_with_oauth: !!authenticated_with_oauth,
       )
     user_auth_token.unhashed_auth_token = token
 
@@ -278,17 +280,18 @@ end
 #
 # Table name: user_auth_tokens
 #
-#  id              :integer          not null, primary key
+#  id                       :integer          not null, primary key
-#  user_id         :integer          not null
+#  user_id                  :integer          not null
-#  auth_token      :string           not null
+#  auth_token               :string           not null
-#  prev_auth_token :string           not null
+#  prev_auth_token          :string           not null
-#  user_agent      :string
+#  user_agent               :string
-#  auth_token_seen :boolean          default(FALSE), not null
+#  auth_token_seen          :boolean          default(FALSE), not null
-#  client_ip       :inet
+#  client_ip                :inet
-#  rotated_at      :datetime         not null
+#  rotated_at               :datetime         not null
-#  created_at      :datetime         not null
+#  created_at               :datetime         not null
-#  updated_at      :datetime         not null
+#  updated_at               :datetime         not null
-#  seen_at         :datetime
+#  seen_at                  :datetime
+#  authenticated_with_oauth :boolean          default(FALSE)
 #
 # Indexes
 #

db/migrate/20240724021732_add_login_method_to_user_auth_tokens.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddLoginMethodToUserAuthTokens < ActiveRecord::Migration[7.1]
+  def change
+    add_column :user_auth_tokens, :authenticated_with_oauth, :boolean, default: false
+  end
+end

lib/auth/default_current_user_provider.rb

@@ -140,6 +140,7 @@ class Auth::DefaultCurrentUserProvider
           end
 
         current_user = @user_token.try(:user)
+        current_user.authenticated_with_oauth = @user_token.authenticated_with_oauth if current_user
       end
 
       if !current_user
@@ -267,6 +268,7 @@ class Auth::DefaultCurrentUserProvider
         client_ip: @request.ip,
         staff: user.staff?,
         impersonate: opts[:impersonate],
+        authenticated_with_oauth: opts[:authenticated_with_oauth],
       )
 
     set_auth_cookie!(@user_token.unhashed_auth_token, user, cookie_jar)

spec/requests/application_controller_spec.rb

@@ -152,8 +152,8 @@ RSpec.describe ApplicationController do
 
     it "should redirect users when enforce_second_factor is 'all' and authenticated via oauth" do
       SiteSetting.enforce_second_factor = "all"
-      write_secure_session("oauth", true)
       sign_in(user)
+      user.user_auth_tokens.last.update(authenticated_with_oauth: true)
 
       get "/"
       expect(response).to redirect_to("/u/#{user.username}/preferences/second-factor")
@@ -162,8 +162,8 @@ RSpec.describe ApplicationController do
     it "should not redirect users when enforce_second_factor is 'all', authenticated via oauth but enforce_second_factor_on_external_auth is false" do
       SiteSetting.enforce_second_factor = "all"
       SiteSetting.enforce_second_factor_on_external_auth = false
-      write_secure_session("oauth", true)
       sign_in(user)
+      user.user_auth_tokens.last.update(authenticated_with_oauth: true)
 
       get "/"
       expect(response.status).to eq(200)

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -236,11 +236,6 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(data["email_valid"]).to eq(true)
         expect(data["can_edit_username"]).to eq(true)
         expect(data["destination_url"]).to eq(destination_url)
-        expect(read_secure_session["oauth"]).to eq("true")
-        expect(Discourse.redis.ttl("#{session[:secure_session_id]}oauth")).to be_between(
-          SiteSetting.maximum_session_age.hours.seconds - 10,
-          SiteSetting.maximum_session_age.hours.seconds,
-        )
       end
 
       it "should return the right response for staged users" do
@@ -402,6 +397,7 @@ RSpec.describe Users::OmniauthCallbacksController do
 
         user.reload
         expect(user.email_confirmed?).to eq(true)
+        expect(user.user_auth_tokens.last.authenticated_with_oauth).to be true
       end
 
       it "should return the authenticated response with the correct path for subfolders" do

spec/requests/session_controller_spec.rb

@@ -2010,6 +2010,7 @@ RSpec.describe SessionController do
 
           expect(session[:current_user_id]).to eq(user.id)
           expect(user.user_auth_tokens.count).to eq(1)
+          expect(user.user_auth_tokens.last.authenticated_with_oauth).to be false
           unhashed_token = decrypt_auth_cookie(cookies[:_t])[:token]
           expect(UserAuthToken.hash_token(unhashed_token)).to eq(
             user.user_auth_tokens.first.auth_token,