From cada172981d6c3af9391ced1dd423a938edce18f Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 27 Jun 2024 10:27:49 +1000 Subject: [PATCH] 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 0e1102b3326ace878c357301e108154051d37c31 --- .../app/routes/preferences-second-factor.js | 20 ++++++-- .../acceptance/enforce-second-factor-test.js | 50 +++++++++++++++++++ app/controllers/application_controller.rb | 12 ++++- app/controllers/session_controller.rb | 2 +- app/serializers/current_user_serializer.rb | 7 ++- lib/auth.rb | 2 + 6 files changed, 84 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js b/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js index c9845d4628d..d6e78fafb5d 100644 --- a/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js +++ b/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 ( - transition.targetName === "preferences.second-factor" || + // NOTE: Matches the should_enforce_2fa? and disqualified_from_2fa_enforcement + // 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.currentUser.staff) || - this.siteSettings.enforce_second_factor === "no" + (!this.siteSettings.enforce_second_factor_on_external_auth && + this.currentUser.login_method === "oauth"); + + if ( + transition.targetName === "preferences.second-factor" || + disqualifiedFrom2faEnforcement || + !enforcing2fa ) { return true; } diff --git a/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js b/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js index 4ef47d8524d..4cdfca2be29 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js +++ b/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" + ); + }); +}); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 482e672f0e3..372b8923c7d 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 4f661afc427..15928ec2bad 100644 --- a/app/controllers/session_controller.rb +++ b/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 diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 3d1a6e9ae43..4962bc0b002 100644 --- a/app/serializers/current_user_serializer.rb +++ b/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 diff --git a/lib/auth.rb b/lib/auth.rb index fec087fd031..d96babe0b8e 100644 --- a/lib/auth.rb +++ b/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"