DEV: Allow `run_second_factor!` to be used before login (#25420)

In a handful of situations, we need to verify a user's 2fa credentials before `current_user` is assigned. For example: login, email_login and change-email confirmation. This commit adds an explicit `target_user:` parameter to the centralized 2fa system so that it can be used for those situations.

For safety and clarity, this new parameter only works for anon. If some user is logged in, and target_user is set to a different user, an exception will be raised.

app/controllers/application_controller.rb

@@ -1063,9 +1063,15 @@ class ApplicationController < ActionController::Base
       end
   end
 
-  def run_second_factor!(action_class, action_data = nil)
-    action = action_class.new(guardian, request, action_data)
-    manager = SecondFactor::AuthManager.new(guardian, action)
+  def run_second_factor!(action_class, action_data: nil, target_user: current_user)
+    if current_user && target_user != current_user
+      # Anon can run 2fa against another target, but logged-in users should not.
+      # This should be validated at the `run_second_factor!` call site.
+      raise "running 2fa against another user is not allowed"
+    end
+
+    action = action_class.new(guardian, request, opts: action_data, target_user: target_user)
+    manager = SecondFactor::AuthManager.new(guardian, action, target_user: target_user)
     yield(manager) if block_given?
     result = manager.run!(request, params, secure_session)
 

app/controllers/session_controller.rb

@@ -11,8 +11,6 @@ class SessionController < ApplicationController
 
   skip_before_action :check_xhr, only: %i[second_factor_auth_show]
 
-  requires_login only: %i[second_factor_auth_show second_factor_auth_perform]
-
   allow_in_staff_writes_only_mode :create
   allow_in_staff_writes_only_mode :email_login
 
@@ -47,8 +45,10 @@ class SessionController < ApplicationController
     result =
       run_second_factor!(
         SecondFactor::Actions::DiscourseConnectProvider,
+        action_data: {
           payload: payload,
           confirmed_2fa_during_login: confirmed_2fa_during_login,
+        },
       )
 
     if result.second_factor_auth_skipped?
@@ -136,8 +136,10 @@ class SessionController < ApplicationController
     skip_before_action :check_xhr, only: :test_second_factor_restricted_route
 
     def test_second_factor_restricted_route
+      target_user = User.find_by_username(params[:username]) || current_user
+      raise "user required" if !target_user
       result =
-        run_second_factor!(TestSecondFactorAction) do |manager|
+        run_second_factor!(TestSecondFactorAction, target_user: target_user) do |manager|
           manager.allow_backup_codes! if params[:allow_backup_codes]
         end
       if result.no_second_factors_enabled?
@@ -145,6 +147,15 @@ class SessionController < ApplicationController
       else
         render json: { result: "second_factor_auth_completed" }
       end
+    rescue StandardError => e
+      # Normally this would be checked by the consumer before calling `run_second_factor!`
+      # but since this is a test route, we allow passing a bad value into the API, catch the error
+      # and return a JSON response to assert against.
+      if e.message == "running 2fa against another user is not allowed"
+        render json: { result: "wrong user" }, status: 400
+      else
+        raise e
+      end
     end
   end
 
@@ -464,14 +475,18 @@ class SessionController < ApplicationController
   end
 
   def second_factor_auth_show
-    user = current_user
-
     nonce = params.require(:nonce)
     challenge = nil
     error_key = nil
+    user = nil
     status_code = 200
     begin
-      challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session)
+      challenge =
+        SecondFactor::AuthManager.find_second_factor_challenge(
+          nonce: nonce,
+          secure_session: secure_session,
+          target_user: current_user,
+        )
     rescue SecondFactor::BadChallenge => exception
       error_key = exception.error_translation_key
       status_code = exception.status_code
@@ -479,6 +494,7 @@ class SessionController < ApplicationController
 
     json = {}
     if challenge
+      user = User.find(challenge[:target_user_id])
       json.merge!(
         totp_enabled: user.totp_enabled?,
         backup_enabled: user.backup_codes_enabled?,
@@ -510,9 +526,16 @@ class SessionController < ApplicationController
     nonce = params.require(:nonce)
     challenge = nil
     error_key = nil
+    user = nil
     status_code = 200
     begin
-      challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session)
+      challenge =
+        SecondFactor::AuthManager.find_second_factor_challenge(
+          nonce: nonce,
+          secure_session: secure_session,
+          target_user: current_user,
+        )
+      user = User.find(challenge[:target_user_id])
     rescue SecondFactor::BadChallenge => exception
       error_key = exception.error_translation_key
       status_code = exception.status_code
@@ -535,7 +558,7 @@ class SessionController < ApplicationController
     # they're redirected to the 2fa page and then uses the same method they've
     # disabled.
     second_factor_method = params[:second_factor_method].to_i
-    if !current_user.valid_second_factor_method_for_user?(second_factor_method)
+    if !user.valid_second_factor_method_for_user?(second_factor_method)
       raise Discourse::InvalidAccess.new
     end
     # and this happens if someone tries to use a 2FA method that's not accepted
@@ -546,8 +569,8 @@ class SessionController < ApplicationController
     end
 
     if !challenge[:successful]
-      rate_limit_second_factor!(current_user)
-      second_factor_auth_result = current_user.authenticate_second_factor(params, secure_session)
+      rate_limit_second_factor!(user)
+      second_factor_auth_result = user.authenticate_second_factor(params, secure_session)
       if second_factor_auth_result.ok
         challenge[:successful] = true
         challenge[:generated_at] += 1.minute.to_i

lib/second_factor/actions/base.rb

@@ -5,9 +5,10 @@ module SecondFactor::Actions
     include Rails.application.routes.url_helpers
     attr_reader :current_user, :guardian, :request
 
-    def initialize(guardian, request, opts = nil)
+    def initialize(guardian, request, target_user:, opts: nil)
       @guardian = guardian
       @current_user = guardian.user
+      @target_user = target_user
       @request = request
       @opts = HashWithIndifferentAccess.new(opts)
     end

lib/second_factor/auth_manager.rb

@@ -120,7 +120,7 @@ class SecondFactor::AuthManager
 
   attr_reader :allowed_methods
 
-  def self.find_second_factor_challenge(nonce, secure_session)
+  def self.find_second_factor_challenge(nonce:, secure_session:, target_user:)
     challenge_json = secure_session["current_second_factor_auth_challenge"]
     if challenge_json.blank?
       raise SecondFactor::BadChallenge.new(
@@ -137,16 +137,25 @@ class SecondFactor::AuthManager
             )
     end
 
+    if target_user && (challenge[:target_user_id] != target_user.id)
+      raise SecondFactor::BadChallenge.new(
+              "second_factor_auth.challenge_not_found",
+              status_code: 404,
+            )
+    end
+
     generated_at = challenge[:generated_at]
     if generated_at < MAX_CHALLENGE_AGE.ago.to_i
       raise SecondFactor::BadChallenge.new("second_factor_auth.challenge_expired", status_code: 401)
     end
+
     challenge
   end
 
-  def initialize(guardian, action)
+  def initialize(guardian, action, target_user:)
     @guardian = guardian
     @current_user = guardian.user
+    @target_user = target_user
     @action = action
     @allowed_methods =
       Set.new([UserSecondFactor.methods[:totp], UserSecondFactor.methods[:security_key]]).freeze
@@ -163,7 +172,7 @@ class SecondFactor::AuthManager
     elsif @action.skip_second_factor_auth?(params)
       data = @action.second_factor_auth_skipped!(params)
       create_result(:second_factor_auth_skipped, data)
-    elsif !allowed_methods.any? { |m| @current_user.valid_second_factor_method_for_user?(m) }
+    elsif !allowed_methods.any? { |m| @target_user.valid_second_factor_method_for_user?(m) }
       data = @action.no_second_factors_enabled!(params)
       create_result(:no_second_factor, data)
     else
@@ -185,6 +194,7 @@ class SecondFactor::AuthManager
       callback_params: callback_params,
       allowed_methods: allowed_methods.to_a,
       generated_at: Time.zone.now.to_i,
+      target_user_id: @target_user.id,
     }
     challenge[:description] = config[:description] if config[:description]
     challenge[:redirect_url] = config[:redirect_url] if config[:redirect_url].present?
@@ -193,7 +203,12 @@ class SecondFactor::AuthManager
   end
 
   def verify_second_factor_auth_completed(nonce, secure_session)
-    challenge = self.class.find_second_factor_challenge(nonce, secure_session)
+    challenge =
+      self.class.find_second_factor_challenge(
+        nonce: nonce,
+        secure_session: secure_session,
+        target_user: @target_user,
+      )
     if !challenge[:successful]
       raise SecondFactor::BadChallenge.new(
               "second_factor_auth.challenge_not_completed",

spec/lib/second_factor/actions/discourse_connect_provider_spec.rb

@@ -33,7 +33,12 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
 
   def create_instance(user, request = nil, opts = nil)
     request ||= create_request
-    SecondFactor::Actions::DiscourseConnectProvider.new(Guardian.new(user), request, opts)
+    SecondFactor::Actions::DiscourseConnectProvider.new(
+      Guardian.new(user),
+      request,
+      opts: opts,
+      target_user: user,
+    )
   end
 
   describe "#skip_second_factor_auth?" do

spec/lib/second_factor/actions/grant_admin_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe SecondFactor::Actions::GrantAdmin do
 
   def create_instance(user, request = nil)
     request ||= create_request
-    SecondFactor::Actions::GrantAdmin.new(Guardian.new(user), request)
+    SecondFactor::Actions::GrantAdmin.new(Guardian.new(user), request, target_user: user)
   end
 
   describe "#no_second_factors_enabled!" do

spec/lib/second_factor/auth_manager_spec.rb

@@ -10,12 +10,12 @@ RSpec.describe SecondFactor::AuthManager do
   end
 
   def create_manager(action)
-    SecondFactor::AuthManager.new(guardian, action)
+    SecondFactor::AuthManager.new(guardian, action, target_user: user)
   end
 
   def create_action(request = nil)
     request ||= create_request
-    TestSecondFactorAction.new(guardian, request)
+    TestSecondFactorAction.new(guardian, request, target_user: user)
   end
 
   def stage_challenge(successful:)

spec/requests/session_controller_spec.rb

@@ -2844,19 +2844,43 @@ RSpec.describe SessionController do
   describe "#second_factor_auth_show" do
     let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) }
 
+    it "can work for anon" do
+      post "/session/2fa/test-action?username=#{user.username}", xhr: true
+      expect(response.status).to eq(403)
+
+      nonce = response.parsed_body["second_factor_challenge_nonce"]
+      get "/session/2fa.json", params: { nonce: nonce }
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["error"]).not_to be_present
+    end
+
+    it "throws an error if logged in to a different user" do
+      sign_in user
+      other_user = Fabricate(:user)
+      post "/session/2fa/test-action?username=#{other_user.username}", xhr: true
+
+      expect(response.status).to eq(400)
+      expect(response.parsed_body["result"]).to eq("wrong user")
+    end
+
+    context "when logged in" do
       before { sign_in(user) }
 
       it "returns 404 if there is no challenge for the given nonce" do
         get "/session/2fa.json", params: { nonce: "asdasdsadsad" }
         expect(response.status).to eq(404)
-      expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found"))
+        expect(response.parsed_body["error"]).to eq(
+          I18n.t("second_factor_auth.challenge_not_found"),
+        )
       end
 
       it "returns 404 if the nonce does not match the challenge nonce" do
         post "/session/2fa/test-action"
         get "/session/2fa.json", params: { nonce: "wrongnonce" }
         expect(response.status).to eq(404)
-      expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found"))
+        expect(response.parsed_body["error"]).to eq(
+          I18n.t("second_factor_auth.challenge_not_found"),
+        )
       end
 
       it "returns 401 if the challenge nonce has expired" do
@@ -2913,10 +2937,42 @@ RSpec.describe SessionController do
         )
       end
     end
+  end
 
   describe "#second_factor_auth_perform" do
     let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) }
 
+    it "works as anon" do
+      post "/session/2fa/test-action?username=#{user.username}", xhr: true
+      nonce = response.parsed_body["second_factor_challenge_nonce"]
+
+      token = ROTP::TOTP.new(user_second_factor.data).now
+      post "/session/2fa.json",
+           params: {
+             nonce: nonce,
+             second_factor_method: UserSecondFactor.methods[:totp],
+             second_factor_token: token,
+           }
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["error"]).not_to be_present
+
+      post "/session/2fa/test-action?username=#{user.username}",
+           params: {
+             second_factor_nonce: nonce,
+           }
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["error"]).not_to be_present
+      expect(response.parsed_body["result"]).to eq("second_factor_auth_completed")
+    end
+
+    it "prevents use by different user" do
+      other_user = Fabricate(:user)
+
+      post "/session/2fa/test-action?username=#{user.username}", xhr: true
+      expect(response.status).to eq(403)
+    end
+
+    context "when signed in" do
       before { sign_in(user) }
 
       it "returns 401 if the challenge nonce has expired" do
@@ -3007,6 +3063,7 @@ RSpec.describe SessionController do
         expect(response.status).to eq(401)
       end
     end
+  end
 
   describe "#passkey_challenge" do
     it "returns a challenge for an anonymous user" do