FIX: Do not capture OAuth user on 2FA page (#27617)

If the `enforce_second_factor_on_external_auth` setting
is disabled and a user logs in with an OAuth method,
they don't automatically get redirected to /preferences/second-factor
on login. However, they can get there manually, and once there
they cannot leave.

This commit fixes the issue and allows them to leave
and also does some refactors to indicate to the client
what login method is used as a followup to
0e1102b332

app/assets/javascripts/discourse/app/routes/preferences-second-factor.js

@@ -38,14 +38,24 @@ export default class PreferencesSecondFactor extends RestrictedUserRoute {
   willTransition(transition) {
     super.willTransition(...arguments);
 
-    if (
+    // NOTE: Matches the should_enforce_2fa? and disqualified_from_2fa_enforcement
-      transition.targetName === "preferences.second-factor" ||
+    // methods in ApplicationController.
+    const enforcing2fa =
+      (this.siteSettings.enforce_second_factor === "staff" &&
+        this.currentUser.staff) ||
+      this.siteSettings.enforce_second_factor === "all";
+
+    const disqualifiedFrom2faEnforcement =
       !this.currentUser ||
       this.currentUser.is_anonymous ||
       this.currentUser.second_factor_enabled ||
-      (this.siteSettings.enforce_second_factor === "staff" &&
+      (!this.siteSettings.enforce_second_factor_on_external_auth &&
-        !this.currentUser.staff) ||
+        this.currentUser.login_method === "oauth");
-      this.siteSettings.enforce_second_factor === "no"
+
+    if (
+      transition.targetName === "preferences.second-factor" ||
+      disqualifiedFrom2faEnforcement ||
+      !enforcing2fa
     ) {
       return true;
     }

app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js

@@ -80,3 +80,53 @@ acceptance("Enforce Second Factor for unconfirmed session", function (needs) {
     );
   });
 });
+
+acceptance("Enforce second factor for OAuth logins", function (needs) {
+  needs.user();
+  needs.pretender((server, helper) => {
+    server.post("/u/second_factors.json", () => {
+      return helper.response({
+        success: "OK",
+        unconfirmed_session: "true",
+      });
+    });
+  });
+
+  test("as a user using local login (username + password) when enforce_second_factor_on_external_auth is false", async function (assert) {
+    updateCurrentUser({
+      moderator: false,
+      admin: false,
+      login_method: "local",
+    });
+    this.siteSettings.enforce_second_factor = "all";
+    this.siteSettings.enforce_second_factor_on_external_auth = false;
+
+    await visit("/u/eviltrout/preferences/second-factor");
+    await click(".home-logo-wrapper-outlet a");
+
+    assert.strictEqual(
+      currentRouteName(),
+      "preferences.second-factor",
+      "it does not let the user leave the second factor preferences"
+    );
+  });
+
+  test("as a user using oauth login when enforce_second_factor_on_external_auth is false", async function (assert) {
+    updateCurrentUser({
+      moderator: false,
+      admin: false,
+      login_method: "oauth",
+    });
+    this.siteSettings.enforce_second_factor = "all";
+    this.siteSettings.enforce_second_factor_on_external_auth = false;
+
+    await visit("/u/eviltrout/preferences/second-factor");
+    await click(".home-logo-wrapper-outlet a");
+
+    assert.strictEqual(
+      currentRouteName(),
+      "discovery.latest",
+      "it does let the user leave the second factor preferences"
+    );
+  });
+});

app/controllers/application_controller.rb

@@ -607,6 +607,11 @@ class ApplicationController < ActionController::Base
     RateLimiter.new(nil, "second-factor-min-#{user.username}", 6, 1.minute).performed! if user
   end
 
+  def login_method
+    return if current_user.anonymous?
+    secure_session["oauth"] == "true" ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL
+  end
+
   private
 
   def preload_anonymous_data
@@ -628,7 +633,7 @@ class ApplicationController < ActionController::Base
           current_user,
           scope: guardian,
           root: false,
-          navigation_menu_param: params[:navigation_menu],
+          login_method: login_method,
         ),
       ),
     )
@@ -905,7 +910,10 @@ class ApplicationController < ActionController::Base
 
   def disqualified_from_2fa_enforcement
     request.format.json? || is_api? || current_user.anonymous? ||
-      (!SiteSetting.enforce_second_factor_on_external_auth && secure_session["oauth"] == "true")
+      (
+        !SiteSetting.enforce_second_factor_on_external_auth &&
+          login_method == Auth::LOGIN_METHOD_OAUTH
+      )
   end
 
   def redirect_to_profile_if_required

app/controllers/session_controller.rb

@@ -652,7 +652,7 @@ class SessionController < ApplicationController
 
   def current
     if current_user.present?
-      render_serialized(current_user, CurrentUserSerializer)
+      render_serialized(current_user, CurrentUserSerializer, { login_method: login_method })
     else
       render body: nil, status: 404
     end

app/serializers/current_user_serializer.rb

@@ -76,7 +76,8 @@ class CurrentUserSerializer < BasicUserSerializer
              :use_experimental_topic_bulk_actions?,
              :use_admin_sidebar,
              :can_view_raw_email,
-             :use_glimmer_topic_list?
+             :use_glimmer_topic_list?,
+             :login_method
 
   delegate :user_stat, to: :object, private: true
   delegate :any_posts, :draft_count, :pending_posts_count, :read_faq?, to: :user_stat
@@ -92,6 +93,10 @@ class CurrentUserSerializer < BasicUserSerializer
     options[:include_status] = true
   end
 
+  def login_method
+    @options[:login_method]
+  end
+
   def groups
     owned_group_ids = GroupUser.where(user_id: id, owner: true).pluck(:group_id).to_set
 

lib/auth.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 module Auth
+  LOGIN_METHOD_OAUTH = "oauth"
+  LOGIN_METHOD_LOCAL = "local"
 end
 
 require "auth/auth_provider"