FEATURE: Add 2FA support to the Discourse Connect Provider protocol (#16386)

Discourse has the Discourse Connect Provider protocol that makes it possible to
use a Discourse instance as an identity provider for external sites. As a
natural extension to this protocol, this PR adds a new feature that makes it
possible to use Discourse as a 2FA provider as well as an identity provider.

The rationale for this change is that it's very difficult to implement 2FA
support in a website and if you have multiple websites that need to have 2FA,
it's unrealistic to build and maintain a separate 2FA implementation for each
one. But with this change, you can piggyback on Discourse to take care of all
the 2FA details for you for as many sites as you wish.

To use Discourse as a 2FA provider, you'll need to follow this guide:
https://meta.discourse.org/t/-/32974. It walks you through what you need to
implement on your end/site and how to configure your Discourse instance. Once
you're done, there is only one additional thing you need to do which is to
include `require_2fa=true` in the payload that you send to Discourse.

When Discourse sees `require_2fa=true`, it'll prompt the user to confirm their
2FA using whatever methods they've enabled (TOTP or security keys), and once
they confirm they'll be redirected back to the return URL you've configured and
the payload will contain `confirmed_2fa=true`. If the user has no 2FA methods
enabled however, the payload will not contain `confirmed_2fa`, but it will
contain `no_2fa_methods=true`.

You'll need to be careful to re-run all the security checks and ensure the user
can still access the resource on your site after they return from Discourse.
This is very important because there's nothing that guarantees the user that
will come back from Discourse after they confirm 2FA is the same user that
you've redirected to Discourse.

Internal ticket: t62183.

app/assets/javascripts/discourse/app/controllers/second-factor-auth.js

@@ -194,7 +194,11 @@ export default Controller.extend({
           type: response.callback_method,
           data: { second_factor_nonce: this.nonce },
         })
-          .then(() => DiscourseURL.routeTo(response.redirect_path))
+          .then((callbackResponse) => {
+            const redirectUrl =
+              callbackResponse.redirect_url || response.redirect_url;
+            DiscourseURL.routeTo(redirectUrl);
+          })
           .catch((error) => this.displayError(extractError(error)));
       })
       .catch((error) => {

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

@@ -87,7 +87,7 @@ acceptance("Second Factor Auth Page", function (needs) {
             ok: true,
             callback_method: "PUT",
             callback_path: "/callback-path",
-            redirect_path: "/",
+            redirect_url: "/",
           },
         ];
       }
@@ -291,7 +291,7 @@ acceptance("Second Factor Auth Page", function (needs) {
     assert.equal(
       currentURL(),
       "/",
-      "user has been redirected to the redirect_path"
+      "user has been redirected to the redirect_url"
     );
     assert.equal(callbackCount, 1, "callback request has been performed");
   });

app/controllers/application_controller.rb

@@ -251,9 +251,13 @@ class ApplicationController < ActionController::Base
   end
 
   rescue_from SecondFactor::AuthManager::SecondFactorRequired do |e|
+    if request.xhr?
       render json: {
         second_factor_challenge_nonce: e.nonce
       }, status: 403
+    else
+      redirect_to session_2fa_path(nonce: e.nonce)
+    end
   end
 
   rescue_from SecondFactor::BadChallenge do |e|
@@ -482,6 +486,11 @@ class ApplicationController < ActionController::Base
   end
 
   def guardian
+    # sometimes we log on a user in the middle of a request so we should throw
+    # away the cached guardian instance when we do that
+    if (@guardian&.user).blank? && current_user.present?
+      @guardian = Guardian.new(current_user, request)
+    end
     @guardian ||= Guardian.new(current_user, request)
   end
 
@@ -978,13 +987,15 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def run_second_factor!(action_class)
+  def run_second_factor!(action_class, action_data = nil)
-    action = action_class.new(guardian)
+    action = action_class.new(guardian, request, action_data)
     manager = SecondFactor::AuthManager.new(guardian, action)
     yield(manager) if block_given?
     result = manager.run!(request, params, secure_session)
 
-    if !result.no_second_factors_enabled? && !result.second_factor_auth_completed?
+    if !result.no_second_factors_enabled? &&
+      !result.second_factor_auth_completed? &&
+      !result.second_factor_auth_skipped?
       # should never happen, but I want to know if somehow it does! (osama)
       raise "2fa process ended up in a bad state!"
     end

app/controllers/session_controller.rb

@@ -39,77 +39,56 @@ class SessionController < ApplicationController
     end
   end
 
-  def sso_provider(payload = nil)
+  def sso_provider(payload = nil, confirmed_2fa_during_login = false)
-    if SiteSetting.enable_discourse_connect_provider
+    if !SiteSetting.enable_discourse_connect_provider
-      begin
+      render body: nil, status: 404
-        if !payload
-          params.require(:sso)
-          payload = request.query_string
-        end
-        sso = DiscourseConnectProvider.parse(payload)
-      rescue DiscourseConnectProvider::BlankSecret
-        render plain: I18n.t("discourse_connect.missing_secret"), status: 400
-        return
-      rescue DiscourseConnectProvider::ParseError => e
-        if SiteSetting.verbose_discourse_connect_logging
-          Rails.logger.warn("Verbose SSO log: Signature parse error\n\n#{e.message}\n\n#{sso&.diagnostics}")
-        end
-
-        # Do NOT pass the error text to the client, it would give them the correct signature
-        render plain: I18n.t("discourse_connect.login_error"), status: 422
       return
     end
 
-      if sso.return_sso_url.blank?
+    result = run_second_factor!(
-        render plain: "return_sso_url is blank, it must be provided", status: 400
+      SecondFactor::Actions::DiscourseConnectProvider,
-        return
+      payload: payload,
-      end
+      confirmed_2fa_during_login: confirmed_2fa_during_login
+    )
 
-      if sso.logout
+    if result.second_factor_auth_skipped?
-        params[:return_url] = sso.return_sso_url
+      data = result.data
+      if data[:logout]
+        params[:return_url] = data[:return_sso_url]
         destroy
         return
       end
 
-      if current_user
+      if data[:no_current_user]
-        sso.name = current_user.name
+        cookies[:sso_payload] = payload || request.query_string
-        sso.username = current_user.username
+        redirect_to path('/login')
-        sso.email = current_user.email
+        return
-        sso.external_id = current_user.id.to_s
-        sso.admin = current_user.admin?
-        sso.moderator = current_user.moderator?
-        sso.groups = current_user.groups.pluck(:name).join(",")
-
-        if current_user.uploaded_avatar.present?
-          base_url = Discourse.store.external? ? "#{Discourse.store.absolute_base_url}/" : Discourse.base_url
-          avatar_url = "#{base_url}#{Discourse.store.get_path_for_upload(current_user.uploaded_avatar)}"
-          sso.avatar_url = UrlHelper.absolute Discourse.store.cdn_url(avatar_url)
-        end
-
-        if current_user.user_profile.profile_background_upload.present?
-          sso.profile_background_url = UrlHelper.absolute(upload_cdn_path(
-            current_user.user_profile.profile_background_upload.url
-          ))
-        end
-
-        if current_user.user_profile.card_background_upload.present?
-          sso.card_background_url = UrlHelper.absolute(upload_cdn_path(
-            current_user.user_profile.card_background_upload.url
-          ))
       end
 
       if request.xhr?
-          cookies[:sso_destination_url] = sso.to_url(sso.return_sso_url)
+        # for the login modal
+        cookies[:sso_destination_url] = data[:sso_redirect_url]
       else
-          redirect_to sso.to_url(sso.return_sso_url)
+        redirect_to data[:sso_redirect_url]
       end
+    elsif result.no_second_factors_enabled?
+      if request.xhr?
+        # for the login modal
+        cookies[:sso_destination_url] = result.data[:sso_redirect_url]
       else
-        cookies[:sso_payload] = request.query_string
+        redirect_to result.data[:sso_redirect_url]
-        redirect_to path('/login')
       end
-    else
+    elsif result.second_factor_auth_completed?
-      render body: nil, status: 404
+      redirect_url = result.data[:sso_redirect_url]
+      render json: success_json.merge(redirect_url: redirect_url)
     end
+  rescue DiscourseConnectProvider::BlankSecret
+    render plain: I18n.t("discourse_connect.missing_secret"), status: 400
+  rescue DiscourseConnectProvider::ParseError => e
+    # Do NOT pass the error text to the client, it would give them the correct signature
+    render plain: I18n.t("discourse_connect.login_error"), status: 422
+  rescue DiscourseConnectProvider::BlankReturnUrl
+    render plain: "return_sso_url is blank, it must be provided", status: 400
   end
 
   # For use in development mode only when login options could be limited or disabled.
@@ -334,11 +313,12 @@ class SessionController < ApplicationController
       return render json: payload
     end
 
-    if !authenticate_second_factor(user)
+    second_factor_auth_result = authenticate_second_factor(user)
+    if !second_factor_auth_result.ok
       return render(json: @second_factor_failure_payload)
     end
 
-    (user.active && user.email_confirmed?) ? login(user) : not_activated(user)
+    (user.active && user.email_confirmed?) ? login(user, second_factor_auth_result) : not_activated(user)
   end
 
   def email_login_info
@@ -388,7 +368,7 @@ class SessionController < ApplicationController
 
     rate_limit_second_factor!(user)
 
-    if user.present? && !authenticate_second_factor(user)
+    if user.present? && !authenticate_second_factor(user).ok
       return render(json: @second_factor_failure_payload)
     end
 
@@ -534,7 +514,7 @@ class SessionController < ApplicationController
       ok: true,
       callback_method: challenge[:callback_method],
       callback_path: challenge[:callback_path],
-      redirect_path: challenge[:redirect_path]
+      redirect_url: challenge[:redirect_url]
     }, status: 200
   end
 
@@ -651,10 +631,10 @@ class SessionController < ApplicationController
         failure_payload.merge!(Webauthn.allowed_credentials(user, secure_session))
       end
       @second_factor_failure_payload = failed_json.merge(failure_payload)
-      return false
+      return second_factor_authentication_result
     end
 
-    true
+    second_factor_authentication_result
   end
 
   def login_error_check(user)
@@ -706,13 +686,18 @@ class SessionController < ApplicationController
     }
   end
 
-  def login(user)
+  def login(user, second_factor_auth_result)
     session.delete(ACTIVATE_USER_KEY)
     user.update_timezone_if_missing(params[:timezone])
     log_on_user(user)
 
     if payload = cookies.delete(:sso_payload)
-      sso_provider(payload)
+      confirmed_2fa_during_login = (
+        second_factor_auth_result&.ok &&
+        second_factor_auth_result.used_2fa_method.present? &&
+        second_factor_auth_result.used_2fa_method != UserSecondFactor.methods[:backup_codes]
+      )
+      sso_provider(payload, confirmed_2fa_during_login)
     else
       render_serialized(user, UserSerializer)
     end

app/models/concerns/second_factor_manager.rb

@@ -6,7 +6,14 @@ module SecondFactorManager
   extend ActiveSupport::Concern
 
   SecondFactorAuthenticationResult = Struct.new(
-    :ok, :error, :reason, :backup_enabled, :security_key_enabled, :totp_enabled, :multiple_second_factor_methods
+    :ok,
+    :error,
+    :reason,
+    :backup_enabled,
+    :security_key_enabled,
+    :totp_enabled,
+    :multiple_second_factor_methods,
+    :used_2fa_method,
   )
 
   def create_totp(opts = {})
@@ -112,11 +119,26 @@ module SecondFactorManager
 
     case second_factor_method
     when UserSecondFactor.methods[:totp]
-      return authenticate_totp(second_factor_token) ? ok_result : invalid_totp_or_backup_code_result
+      if authenticate_totp(second_factor_token)
+        ok_result.used_2fa_method = UserSecondFactor.methods[:totp]
+        return ok_result
+      else
+        return invalid_totp_or_backup_code_result
+      end
     when UserSecondFactor.methods[:backup_codes]
-      return authenticate_backup_code(second_factor_token) ? ok_result : invalid_totp_or_backup_code_result
+      if authenticate_backup_code(second_factor_token)
+        ok_result.used_2fa_method = UserSecondFactor.methods[:backup_codes]
+        return ok_result
+      else
+        return invalid_totp_or_backup_code_result
+      end
     when UserSecondFactor.methods[:security_key]
-      return authenticate_security_key(secure_session, second_factor_token) ? ok_result : invalid_security_key_result
+      if authenticate_security_key(secure_session, second_factor_token)
+        ok_result.used_2fa_method = UserSecondFactor.methods[:security_key]
+        return ok_result
+      else
+        return invalid_security_key_result
+      end
     end
 
     # if we have gotten down to this point without being

config/locales/server.en.yml

@@ -2590,6 +2590,8 @@ en:
     actions:
       grant_admin:
         description: "For additional security measures, you need to confirm your 2FA before %{username} is granted admin access."
+      discourse_connect_provider:
+        description: "%{hostname} has requested that you confirm your 2FA. You'll be redirected back to the site once you confirm your 2FA."
   admin:
     email:
       sent_test: "sent!"

lib/discourse_connect_base.rb

@@ -11,23 +11,26 @@ class DiscourseConnectBase
     avatar_url
     bio
     card_background_url
+    confirmed_2fa
     email
     external_id
     groups
     locale
     locale_force_update
+    location
     logout
     name
+    no_2fa_methods
     nonce
     profile_background_url
     remove_groups
+    require_2fa
     require_activation
     return_sso_url
     suppress_welcome_message
     title
     username
     website
-    location
   }
 
   FIXNUMS = []
@@ -35,9 +38,12 @@ class DiscourseConnectBase
   BOOLS = %i{
     admin
     avatar_force_update
+    confirmed_2fa
     locale_force_update
     logout
     moderator
+    no_2fa_methods
+    require_2fa
     require_activation
     suppress_welcome_message
   }

lib/discourse_connect_provider.rb

@@ -2,6 +2,7 @@
 
 class DiscourseConnectProvider < DiscourseConnectBase
   class BlankSecret < RuntimeError; end
+  class BlankReturnUrl < RuntimeError; end
 
   def self.parse(payload, sso_secret = nil)
     set_return_sso_url(payload)

lib/second_factor/actions/base.rb

@@ -3,11 +3,21 @@
 module SecondFactor::Actions
   class Base
     include Rails.application.routes.url_helpers
-    attr_reader :current_user, :guardian
+    attr_reader :current_user, :guardian, :request
 
-    def initialize(guardian)
+    def initialize(guardian, request, opts = nil)
       @guardian = guardian
       @current_user = guardian.user
+      @request = request
+      @opts = HashWithIndifferentAccess.new(opts)
+    end
+
+    def skip_second_factor_auth?(params)
+      false
+    end
+
+    def second_factor_auth_skipped!(params)
+      raise NotImplementedError.new
     end
 
     def no_second_factors_enabled!(params)

lib/second_factor/actions/discourse_connect_provider.rb

@@ -0,0 +1,95 @@
+# frozen_string_literal: true
+
+module SecondFactor::Actions
+  class DiscourseConnectProvider < Base
+    def skip_second_factor_auth?(params)
+      sso = get_sso(payload(params))
+      !current_user || sso.logout || !sso.require_2fa || @opts[:confirmed_2fa_during_login]
+    end
+
+    def second_factor_auth_skipped!(params)
+      sso = get_sso(payload(params))
+      return { logout: true, return_sso_url: sso.return_sso_url } if sso.logout
+      return { no_current_user: true } if !current_user
+      populate_user_data(sso)
+      sso.confirmed_2fa = true if @opts[:confirmed_2fa_during_login]
+      { sso_redirect_url: sso.to_url(sso.return_sso_url) }
+    end
+
+    def no_second_factors_enabled!(params)
+      sso = get_sso(payload(params))
+      populate_user_data(sso)
+      sso.no_2fa_methods = true
+      { sso_redirect_url: sso.to_url(sso.return_sso_url) }
+    end
+
+    def second_factor_auth_required!(params)
+      pl = payload(params)
+      sso = get_sso(pl)
+      hostname = URI(sso.return_sso_url).hostname
+      {
+        callback_params: { payload: pl },
+        callback_path: session_sso_provider_path,
+        callback_method: "GET",
+        description: I18n.t(
+          "second_factor_auth.actions.discourse_connect_provider.description",
+          hostname: hostname,
+        )
+      }
+    end
+
+    def second_factor_auth_completed!(callback_params)
+      sso = get_sso(callback_params[:payload])
+      populate_user_data(sso)
+      sso.confirmed_2fa = true
+      { sso_redirect_url: sso.to_url(sso.return_sso_url) }
+    end
+
+    private
+
+    def payload(params)
+      return @opts[:payload] if @opts[:payload]
+      params.require(:sso)
+      request.query_string
+    end
+
+    def populate_user_data(sso)
+      sso.name = current_user.name
+      sso.username = current_user.username
+      sso.email = current_user.email
+      sso.external_id = current_user.id.to_s
+      sso.admin = current_user.admin?
+      sso.moderator = current_user.moderator?
+      sso.groups = current_user.groups.pluck(:name).join(",")
+
+      if current_user.uploaded_avatar.present?
+        base_url = Discourse.store.external? ? "#{Discourse.store.absolute_base_url}/" : Discourse.base_url
+        avatar_url = "#{base_url}#{Discourse.store.get_path_for_upload(current_user.uploaded_avatar)}"
+        sso.avatar_url = UrlHelper.absolute Discourse.store.cdn_url(avatar_url)
+      end
+
+      if current_user.user_profile.profile_background_upload.present?
+        sso.profile_background_url = UrlHelper.absolute(GlobalPath.upload_cdn_path(
+          current_user.user_profile.profile_background_upload.url
+        ))
+      end
+
+      if current_user.user_profile.card_background_upload.present?
+        sso.card_background_url = UrlHelper.absolute(GlobalPath.upload_cdn_path(
+          current_user.user_profile.card_background_upload.url
+        ))
+      end
+    end
+
+    def get_sso(payload)
+      sso = ::DiscourseConnectProvider.parse(payload)
+      raise ::DiscourseConnectProvider::BlankReturnUrl.new if sso.return_sso_url.blank?
+      sso
+    rescue ::DiscourseConnectProvider::ParseError => e
+      if SiteSetting.verbose_discourse_connect_logging
+        Rails.logger.warn("Verbose SSO log: Signature parse error\n\n#{e.message}\n\n#{sso&.diagnostics}")
+      end
+      raise
+    end
+  end
+end

lib/second_factor/actions/grant_admin.rb

@@ -5,6 +5,7 @@ module SecondFactor::Actions
     def no_second_factors_enabled!(params)
       user = find_user(params[:user_id])
       AdminConfirmation.new(user, current_user).create_confirmation
+      nil
     end
 
     def second_factor_auth_required!(params)
@@ -15,7 +16,7 @@ module SecondFactor::Actions
       )
       {
         callback_params: { user_id: user.id },
-        redirect_path: admin_user_show_path(id: user.id, username: user.username),
+        redirect_url: admin_user_show_path(id: user.id, username: user.username),
         description: description
       }
     end
@@ -24,6 +25,7 @@ module SecondFactor::Actions
       user = find_user(callback_params[:user_id])
       user.grant_admin!
       StaffActionLogger.new(current_user).log_grant_admin(user)
+      nil
     end
 
     private

lib/second_factor/auth_manager.rb

@@ -27,7 +27,7 @@ To use the auth manager for requiring 2fa for an action, it needs to be invoked
 from the controller action using the `run_second_factor!` method which is
 available in all controllers. This method takes a single argument which is a
 class that inherits from the `SecondFactor::Actions::Base` class and implements
-the following methods:
+at least the following methods:
 
 1. no_second_factors_enabled!(params):
   This method corresponds to outcome (1) above, i.e. it's called when the user
@@ -48,9 +48,8 @@ the following methods:
   finish the action once 2fa is completed. Everything in this Hash must be
   serializable to JSON.
 
-  :redirect_path => relative subfolder-aware path that the user should be
+  :redirect_url => where the user should be redirected after they confirm 2fa.
-  redirected to after the action is finished. When this key is omitted, the
+  A relative path (must be subfolder-aware) is a valid value for this key.
-  redirect path is set to the homepage (/).
 
   :description => optional action-specific description message that's shown on
   the 2FA page.
@@ -68,6 +67,20 @@ the following methods:
   The `callback_params` param of this method is the `callback_params` Hash from
   the return value of the previous method.
 
+There are 2 additionals methods in the base class that can be overridden, but
+they're optional:
+
+4. skip_second_factor_auth?(params):
+  This method returns false by default. As the name implies, this method can be
+  used to skip the 2FA for the action entirely. For example, if your action
+  deletes a user, then you may want to require 2FA only if the deleted user has
+  more than a specific number of posts. If you override this method in your
+  action, you must implement the following method as well.
+
+5. second_factor_auth_skipped!(params):
+  This method is called when the `skip_second_factor_auth?` method above
+  returns true.
+
 If there are permission/security checks that the current user must pass in
 order to perform the 2fa-protected action, it's important to run the checks in
 all of the 3 methods of the action class and raise errors if the user doesn't
@@ -79,14 +92,19 @@ which is an instance of `SecondFactor::AuthManagerResult`, can be used to know
 which outcome the auth manager has picked and render a different response based
 on the outcome.
 
+The results object also has a `data` method that returns the return value of
+the hook/method of your action class. For example, if
+`second_factor_auth_required!` is called and it returns a hash object, you can
+get that hash object by calling the `data` method of the results object.
+
 For a real example where the auth manager is used, please refer to:
 
-* `SecondFactor::Actions::GrantAdmin` action class. This is a class that
+* The `lib/second_factor/actions` directory where all existing actions live.
-  inherits `SecondFactor::Actions::Base` and implements the 3 methods mentioned
-  above.
 
 * `Admin::UsersController#grant_admin` controller action.
 
+* `SessionController#sso_provider` controller action.
+
 =end
 
 class SecondFactor::AuthManager
@@ -144,12 +162,15 @@ class SecondFactor::AuthManager
   end
 
   def run!(request, params, secure_session)
-    if !allowed_methods.any? { |m| @current_user.valid_second_factor_method_for_user?(m) }
+    if nonce = params[:second_factor_nonce].presence
-      @action.no_second_factors_enabled!(params)
+      data = verify_second_factor_auth_completed(nonce, secure_session)
-      create_result(:no_second_factor)
+      create_result(:second_factor_auth_completed, data)
-    elsif nonce = params[:second_factor_nonce].presence
+    elsif @action.skip_second_factor_auth?(params)
-      verify_second_factor_auth_completed(nonce, secure_session)
+      data = @action.second_factor_auth_skipped!(params)
-      create_result(:second_factor_auth_completed)
+      create_result(:second_factor_auth_skipped, data)
+    elsif !allowed_methods.any? { |m| @current_user.valid_second_factor_method_for_user?(m) }
+      data = @action.no_second_factors_enabled!(params)
+      create_result(:no_second_factor, data)
     else
       nonce = initiate_second_factor_auth(params, secure_session, request)
       raise SecondFactorRequired.new(nonce: nonce)
@@ -162,19 +183,20 @@ class SecondFactor::AuthManager
     config = @action.second_factor_auth_required!(params)
     nonce = SecureRandom.alphanumeric(32)
     callback_params = config[:callback_params] || {}
-    redirect_path = config[:redirect_path] || GlobalPath.path("").presence || "/"
     challenge = {
       nonce: nonce,
-      callback_method: request.request_method,
+      callback_method: config[:callback_method] || request.request_method,
-      callback_path: request.path,
+      callback_path: config[:callback_path] || request.path,
       callback_params: callback_params,
-      redirect_path: redirect_path,
       allowed_methods: allowed_methods.to_a,
       generated_at: Time.zone.now.to_i
     }
     if config[:description]
       challenge[:description] = config[:description]
     end
+    if config[:redirect_url].present?
+      challenge[:redirect_url] = config[:redirect_url]
+    end
     secure_session["current_second_factor_auth_challenge"] = challenge.to_json
     nonce
   end
@@ -190,7 +212,8 @@ class SecondFactor::AuthManager
 
     secure_session["current_second_factor_auth_challenge"] = nil
     callback_params = challenge[:callback_params]
-    @action.second_factor_auth_completed!(callback_params)
+    data = @action.second_factor_auth_completed!(callback_params)
+    data
   end
 
   def add_method(id)
@@ -201,7 +224,7 @@ class SecondFactor::AuthManager
     end
   end
 
-  def create_result(status)
+  def create_result(status, data = nil)
-    SecondFactor::AuthManagerResult.new(status)
+    SecondFactor::AuthManagerResult.new(status, data)
   end
 end

lib/second_factor/auth_manager_result.rb

@@ -4,15 +4,18 @@ class SecondFactor::AuthManagerResult
   STATUSES = {
     no_second_factor: 1,
     second_factor_auth_completed: 2,
+    second_factor_auth_skipped: 3,
   }.freeze
 
   private_constant :STATUSES
+  attr_reader :data
 
-  def initialize(status)
+  def initialize(status, data)
     if !STATUSES.key?(status)
       raise ArgumentError.new("#{status.inspect} is not a valid status. Allowed statuses: #{STATUSES.inspect}")
     end
     @status_id = STATUSES[status]
+    @data = data
   end
 
   def no_second_factors_enabled?
@@ -22,4 +25,8 @@ class SecondFactor::AuthManagerResult
   def second_factor_auth_completed?
     @status_id == STATUSES[:second_factor_auth_completed]
   end
+
+  def second_factor_auth_skipped?
+    @status_id == STATUSES[:second_factor_auth_skipped]
+  end
 end

spec/lib/concern/second_factor_manager_spec.rb

@@ -172,6 +172,10 @@ RSpec.describe SecondFactorManager do
       it "returns OK, because it doesn't need to authenticate" do
         expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
       end
+
+      it "keeps used_2fa_method nil because no authentication is done" do
+        expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(nil)
+      end
     end
 
     context "when only security key is enabled" do
@@ -186,6 +190,10 @@ RSpec.describe SecondFactorManager do
         it "returns OK" do
           expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
         end
+
+        it "sets used_2fa_method to security keys" do
+          expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:security_key])
+        end
       end
 
       context "when security key params are invalid" do
@@ -204,6 +212,7 @@ RSpec.describe SecondFactorManager do
           result = user.authenticate_second_factor(params, secure_session)
           expect(result.ok).to eq(false)
           expect(result.error).to eq(I18n.t("webauthn.validation.not_found_error"))
+          expect(result.used_2fa_method).to eq(nil)
         end
       end
     end
@@ -223,6 +232,10 @@ RSpec.describe SecondFactorManager do
         it "returns OK" do
           expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
         end
+
+        it "sets used_2fa_method to totp" do
+          expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:totp])
+        end
       end
 
       context "when totp is invalid" do
@@ -236,6 +249,7 @@ RSpec.describe SecondFactorManager do
           result = user.authenticate_second_factor(params, secure_session)
           expect(result.ok).to eq(false)
           expect(result.error).to eq(I18n.t("login.invalid_second_factor_code"))
+          expect(result.used_2fa_method).to eq(nil)
         end
       end
     end
@@ -254,6 +268,7 @@ RSpec.describe SecondFactorManager do
           result = user.authenticate_second_factor(params, secure_session)
           expect(result.ok).to eq(false)
           expect(result.error).to eq(I18n.t("login.invalid_second_factor_method"))
+          expect(result.used_2fa_method).to eq(nil)
         end
       end
 
@@ -273,6 +288,10 @@ RSpec.describe SecondFactorManager do
             expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
           end
 
+          it "sets used_2fa_method to totp" do
+            expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:totp])
+          end
+
           context "when the user does not have TOTP enabled" do
             let(:token) { 'test' }
             before do
@@ -283,6 +302,7 @@ RSpec.describe SecondFactorManager do
               result = user.authenticate_second_factor(params, secure_session)
               expect(result.ok).to eq(false)
               expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+              expect(result.used_2fa_method).to eq(nil)
             end
           end
         end
@@ -302,6 +322,10 @@ RSpec.describe SecondFactorManager do
             expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
           end
 
+          it "sets used_2fa_method to security keys" do
+            expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:security_key])
+          end
+
           context "when the user does not have security keys enabled" do
             before do
               user.security_keys.destroy_all
@@ -311,6 +335,7 @@ RSpec.describe SecondFactorManager do
               result = user.authenticate_second_factor(params, secure_session)
               expect(result.ok).to eq(false)
               expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+              expect(result.used_2fa_method).to eq(nil)
             end
           end
         end
@@ -332,6 +357,10 @@ RSpec.describe SecondFactorManager do
             it "validates codes OK" do
               expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
             end
+
+            it "sets used_2fa_method to backup codes" do
+              expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:backup_codes])
+            end
           end
 
           context "when backup codes disabled" do
@@ -343,6 +372,7 @@ RSpec.describe SecondFactorManager do
               result = user.authenticate_second_factor(params, secure_session)
               expect(result.ok).to eq(false)
               expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+              expect(result.used_2fa_method).to eq(nil)
             end
           end
         end
@@ -354,6 +384,10 @@ RSpec.describe SecondFactorManager do
         it "validates the security key OK" do
           expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
         end
+
+        it "sets used_2fa_method to security keys" do
+          expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:security_key])
+        end
       end
 
       context "when totp params are provided" do
@@ -367,6 +401,10 @@ RSpec.describe SecondFactorManager do
         it "validates totp OK" do
           expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
         end
+
+        it "sets used_2fa_method to totp" do
+          expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(UserSecondFactor.methods[:totp])
+        end
       end
     end
   end

spec/lib/second_factor/actions/discourse_connect_provider_spec.rb

@@ -0,0 +1,247 @@
+# frozen_string_literal: true
+
+describe SecondFactor::Actions::DiscourseConnectProvider do
+  fab!(:user) { Fabricate(:user) }
+  sso_secret = "mysecretmyprecious"
+  let!(:sso) do
+    sso = ::DiscourseConnectProvider.new
+    sso.nonce = "mysecurenonce"
+    sso.return_sso_url = "http://hobbit.shire.com/sso"
+    sso.sso_secret = sso_secret
+    sso.require_2fa = true
+    sso
+  end
+
+  before do
+    SiteSetting.enable_discourse_connect_provider = true
+    SiteSetting.discourse_connect_provider_secrets = "hobbit.shire.com|#{sso_secret}"
+  end
+
+  def params(hash)
+    ActionController::Parameters.new(hash)
+  end
+
+  def create_request(query_string)
+    ActionDispatch::TestRequest.create({
+      "REQUEST_METHOD" => "GET",
+      "PATH_INFO" => "/",
+      "QUERY_STRING" => query_string
+    })
+  end
+
+  def params_from_payload(payload)
+    ActionController::Parameters.new(Rack::Utils.parse_query(payload))
+  end
+
+  def create_instance(user, request = nil, opts = nil)
+    request ||= create_request
+    SecondFactor::Actions::DiscourseConnectProvider.new(Guardian.new(user), request, opts)
+  end
+
+  describe "#skip_second_factor_auth?" do
+    it "returns true if there's no current_user" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(nil, request)
+      expect(action.skip_second_factor_auth?(params)).to eq(true)
+    end
+
+    it "returns true if SSO is for logout" do
+      sso.logout = true
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      expect(action.skip_second_factor_auth?(params)).to eq(true)
+    end
+
+    it "returns true if SSO doesn't require 2fa" do
+      sso.require_2fa = false
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      expect(action.skip_second_factor_auth?(params)).to eq(true)
+    end
+
+    it "returns true if 2fa has been confirmed during login" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request, confirmed_2fa_during_login: true)
+      expect(action.skip_second_factor_auth?(params)).to eq(true)
+    end
+
+    it "returns falsey value otherwise" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      expect(action.skip_second_factor_auth?(params)).to be_falsey
+    end
+  end
+
+  describe "#second_factor_auth_skipped!" do
+    before { sso.require_2fa = false }
+
+    it "returns a hash with logout: true and return_sso_url without no payload if the SSO is for logout" do
+      sso.logout = true
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      expect(action.second_factor_auth_skipped!(params)).to eq({
+        logout: true,
+        return_sso_url: "http://hobbit.shire.com/sso"
+      })
+    end
+
+    it "returns a hash with no_current_user: true if there's no current_user" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(nil, request)
+      expect(action.second_factor_auth_skipped!(params)).to eq({
+        no_current_user: true
+      })
+    end
+
+    it "returns sso_redirect_url to the SSO website with payload that indicates confirmed 2FA if confirmed_2fa_during_login is true" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request, confirmed_2fa_during_login: true)
+      output = action.second_factor_auth_skipped!(params)
+      expect(output.keys).to contain_exactly(:sso_redirect_url)
+      expect(output[:sso_redirect_url]).to start_with("http://hobbit.shire.com/sso")
+      response_payload = ::DiscourseConnectProvider.parse(URI(output[:sso_redirect_url]).query)
+      expect(response_payload.confirmed_2fa).to eq(true)
+      expect(response_payload.no_2fa_methods).to eq(nil)
+      expect(response_payload.username).to eq(user.username)
+      expect(response_payload.email).to eq(user.email)
+    end
+
+    it "returns sso_redirect_url to the SSO website with payload that doesn't indicate confirmed 2FA" do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      output = action.second_factor_auth_skipped!(params)
+      expect(output.keys).to contain_exactly(:sso_redirect_url)
+      expect(output[:sso_redirect_url]).to start_with("http://hobbit.shire.com/sso")
+      response_payload = ::DiscourseConnectProvider.parse(URI(output[:sso_redirect_url]).query)
+      expect(response_payload.confirmed_2fa).to eq(nil)
+      expect(response_payload.no_2fa_methods).to eq(nil)
+      expect(response_payload.username).to eq(user.username)
+      expect(response_payload.email).to eq(user.email)
+    end
+
+    it "prioritizes the SSO logout case over the no current_user case" do
+      sso.logout = true
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(nil, request)
+      expect(action.second_factor_auth_skipped!(params)).to eq({
+        logout: true,
+        return_sso_url: "http://hobbit.shire.com/sso"
+      })
+    end
+  end
+
+  describe "#no_second_factors_enabled!" do
+    let(:output) do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      action.no_second_factors_enabled!(params)
+    end
+
+    let(:response_payload) do
+      ::DiscourseConnectProvider.parse(URI(output[:sso_redirect_url]).query)
+    end
+
+    it "returns a hash with just sso_redirect_url" do
+      expect(output.keys).to contain_exactly(:sso_redirect_url)
+    end
+
+    it "the sso_redirect_url is the SSO site" do
+      expect(output[:sso_redirect_url]).to start_with("http://hobbit.shire.com/sso")
+    end
+
+    it "the response payload indicates the user has no 2fa methods" do
+      expect(response_payload.no_2fa_methods).to eq(true)
+    end
+
+    it "the response payload of the sso_redirect_url doesn't indicate the user has confirmed 2fa" do
+      expect(response_payload.confirmed_2fa).to eq(nil)
+    end
+
+    it "the response payload contains the user details" do
+      expect(response_payload.username).to eq(user.username)
+      expect(response_payload.email).to eq(user.email)
+    end
+  end
+
+  describe "#second_factor_auth_required!" do
+    let(:output) do
+      request = create_request(sso.payload)
+      params = params_from_payload(sso.payload)
+      action = create_instance(user, request)
+      action.second_factor_auth_required!(params)
+    end
+
+    it "includes the payload in the callback_params" do
+      expect(output[:callback_params]).to eq({ payload: sso.payload })
+    end
+
+    it "sets the callback_path to the SSO provider endpoint" do
+      expect(output[:callback_path]).to eq("/session/sso_provider")
+    end
+
+    it "sets the callback_method to the HTTP method of SSO provider endpoint" do
+      expect(output[:callback_method]).to eq("GET")
+    end
+
+    it "includes a description" do
+      expect(output[:description]).to eq(I18n.t(
+        "second_factor_auth.actions.discourse_connect_provider.description",
+        hostname: "hobbit.shire.com",
+      ))
+    end
+  end
+
+  describe "#second_factor_auth_completed!" do
+    let(:output) do
+      request = create_request("")
+      params = params_from_payload("")
+      action = create_instance(user, request)
+      action.second_factor_auth_completed!(payload: sso.payload)
+    end
+
+    let(:response_payload) do
+      ::DiscourseConnectProvider.parse(URI(output[:sso_redirect_url]).query)
+    end
+
+    it "gets the payload from callback_params and not the request params" do
+      wrong_sso = ::DiscourseConnectProvider.new
+      wrong_sso.nonce = "mysecurenonceWRONG"
+      wrong_sso.return_sso_url = "http://wrong.shire.com/sso"
+      wrong_sso.sso_secret = "mysecretmypreciousWRONG"
+      wrong_sso.require_2fa = true
+
+      request = create_request(wrong_sso.payload)
+      params = params_from_payload(wrong_sso.payload)
+      action = create_instance(user, request)
+      redirect_url = action.second_factor_auth_completed!(payload: sso.payload)[:sso_redirect_url]
+      response_payload = ::DiscourseConnectProvider.parse(URI(redirect_url).query)
+      expect(response_payload.return_sso_url).to eq("http://hobbit.shire.com/sso")
+      expect(response_payload.nonce).to eq("mysecurenonce")
+      expect(response_payload.sso_secret).to eq(sso_secret)
+    end
+
+    it "the response payload of the sso_redirect_url indicates the user has confirmed 2fa" do
+      expect(response_payload.confirmed_2fa).to eq(true)
+    end
+
+    it "the response payload of the sso_redirect_url doesn't include no_2fa_methods" do
+      expect(response_payload.no_2fa_methods).to eq(nil)
+    end
+
+    it "the response payload contains the user details" do
+      expect(response_payload.username).to eq(user.username)
+      expect(response_payload.email).to eq(user.email)
+    end
+  end
+end

spec/lib/second_factor/actions/grant_admin_spec.rb

@@ -18,8 +18,16 @@ describe SecondFactor::Actions::GrantAdmin do
     ActionController::Parameters.new(hash)
   end
 
-  def create_instance(user)
+  def create_request(request_method: "GET", path: "/")
-    SecondFactor::Actions::GrantAdmin.new(Guardian.new(user))
+    ActionDispatch::TestRequest.create({
+      "REQUEST_METHOD" => request_method,
+      "PATH_INFO" => path
+    })
+  end
+
+  def create_instance(user, request = nil)
+    request ||= create_request
+    SecondFactor::Actions::GrantAdmin.new(Guardian.new(user), request)
   end
 
   describe "#no_second_factors_enabled!" do
@@ -40,11 +48,11 @@ describe SecondFactor::Actions::GrantAdmin do
   end
 
   describe "#second_factor_auth_required!" do
-    it "returns a hash with callback_params, redirect_path and a description" do
+    it "returns a hash with callback_params, redirect_url and a description" do
       instance = create_instance(admin)
       hash = instance.second_factor_auth_required!(params({ user_id: user.id }))
       expect(hash[:callback_params]).to eq({ user_id: user.id })
-      expect(hash[:redirect_path]).to eq("/admin/users/#{user.id}/#{user.username}")
+      expect(hash[:redirect_url]).to eq("/admin/users/#{user.id}/#{user.username}")
       expect(hash[:description]).to eq(
         I18n.t(
           "second_factor_auth.actions.grant_admin.description",

spec/lib/second_factor/auth_manager_spec.rb

@@ -16,12 +16,17 @@ describe SecondFactor::AuthManager do
     SecondFactor::AuthManager.new(guardian, action)
   end
 
-  def create_action
+  def create_action(request = nil)
-    TestSecondFactorAction.new(guardian)
+    request ||= create_request
+    TestSecondFactorAction.new(guardian, request)
   end
 
   def stage_challenge(successful:)
-    action = create_action
+    request = create_request(
+      request_method: "POST",
+      path: "/abc/xyz"
+    )
+    action = create_action(request)
     action.expects(:no_second_factors_enabled!).never
     action
       .expects(:second_factor_auth_required!)
@@ -29,23 +34,21 @@ describe SecondFactor::AuthManager do
       .returns({ callback_params: { call_me_back: 4314 } })
       .once
     manager = create_manager(action)
-    request = create_request(
-      request_method: "POST",
-      path: "/abc/xyz"
-    )
     secure_session = {}
-    begin
+    expect {
       manager.run!(request, { random_param: 'hello' }, secure_session)
-    rescue SecondFactor::AuthManager::SecondFactorRequired
+    }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) do |ex|
-      # expected
+      expect(ex.nonce).to be_present
     end
 
     challenge = JSON
       .parse(secure_session["current_second_factor_auth_challenge"])
       .deep_symbolize_keys
 
-    challenge[:successful] = successful
+    if successful
+      challenge[:successful] = true
       secure_session["current_second_factor_auth_challenge"] = challenge.to_json
+    end
     [challenge[:nonce], secure_session]
   end
 
@@ -76,32 +79,26 @@ describe SecondFactor::AuthManager do
         manager = create_manager(action)
         manager.run!(create_request, { hello_world: 331 }, {})
       end
-
-      it 'calls the no_second_factors_enabled! method of the action even if a nonce is present in the params' do
-        action = create_action
-        params = { second_factor_nonce: SecureRandom.hex }
-        action.expects(:no_second_factors_enabled!).with(params).once
-        action.expects(:second_factor_auth_required!).never
-        action.expects(:second_factor_auth_completed!).never
-        manager = create_manager(action)
-        manager.run!(create_request, params, {})
-      end
     end
 
     it "initiates the 2FA process and stages a challenge in secure session when there is no nonce in params" do
-      action = create_action
-      action.expects(:no_second_factors_enabled!).never
-      action
-        .expects(:second_factor_auth_required!)
-        .with({ expect_me: 131 })
-        .returns({ callback_params: { call_me_back: 4314 }, redirect_path: "/gg", description: "hello world!" })
-        .once
-      action.expects(:second_factor_auth_completed!).never
-      manager = create_manager(action)
       request = create_request(
         request_method: "POST",
         path: "/abc/xyz"
       )
+      action = create_action(request)
+      action.expects(:no_second_factors_enabled!).never
+      action
+        .expects(:second_factor_auth_required!)
+        .with({ expect_me: 131 })
+        .returns(
+          callback_params: { call_me_back: 4314 },
+          redirect_url: "/gg",
+          description: "hello world!"
+        )
+        .once
+      action.expects(:second_factor_auth_completed!).never
+      manager = create_manager(action)
       secure_session = {}
       expect {
         manager.run!(request, { expect_me: 131 }, secure_session)
@@ -111,61 +108,46 @@ describe SecondFactor::AuthManager do
       expect(challenge[:nonce]).to be_present
       expect(challenge[:callback_method]).to eq("POST")
       expect(challenge[:callback_path]).to eq("/abc/xyz")
-      expect(challenge[:redirect_path]).to eq("/gg")
+      expect(challenge[:redirect_url]).to eq("/gg")
       expect(challenge[:allowed_methods]).to eq(manager.allowed_methods.to_a)
       expect(challenge[:callback_params]).to eq({ call_me_back: 4314 })
       expect(challenge[:description]).to eq("hello world!")
     end
 
-    it "sets the redirect_path to the root path if second_factor_auth_required! doesn't specify a redirect_path" do
+    it "prefers callback_method and callback_path from the output of the action's second_factor_auth_required! method if they're present" do
-      action = create_action
-      action.expects(:no_second_factors_enabled!).never
-      action
-        .expects(:second_factor_auth_required!)
-        .with({ expect_me: 131 })
-        .returns({ callback_params: { call_me_back: 4314 } })
-        .once
-      action.expects(:second_factor_auth_completed!).never
-      manager = create_manager(action)
       request = create_request(
         request_method: "POST",
         path: "/abc/xyz"
       )
-      secure_session = {}
+      action = create_action(request)
-      expect {
-        manager.run!(request, { expect_me: 131 }, secure_session)
-      }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired)
-      json = secure_session["current_second_factor_auth_challenge"]
-      challenge = JSON.parse(json).deep_symbolize_keys
-      expect(challenge[:redirect_path]).to eq("/")
-
-      set_subfolder("/community")
-      action = create_action
-      action.expects(:no_second_factors_enabled!).never
       action
         .expects(:second_factor_auth_required!)
-        .with({ expect_me: 131 })
+        .with({})
-        .returns({ callback_params: { call_me_back: 4314 } })
+        .returns(
-        .once
+          callback_params: { call_me_back: 4314 },
-      action.expects(:second_factor_auth_completed!).never
+          callback_method: "PUT",
-      manager = create_manager(action)
+          callback_path: "/test/443"
-      request = create_request(
-        request_method: "POST",
-        path: "/abc/xyz"
         )
+        .once
+      manager = create_manager(action)
       secure_session = {}
       expect {
-        manager.run!(request, { expect_me: 131 }, secure_session)
+        manager.run!(request, {}, secure_session)
       }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired)
       json = secure_session["current_second_factor_auth_challenge"]
       challenge = JSON.parse(json).deep_symbolize_keys
-      expect(challenge[:redirect_path]).to eq("/community")
+      expect(challenge[:callback_method]).to eq("PUT")
+      expect(challenge[:callback_path]).to eq("/test/443")
     end
 
     it "calls the second_factor_auth_completed! method of the action if the challenge is successful and not expired" do
       nonce, secure_session = stage_challenge(successful: true)
 
-      action = create_action
+      request = create_request(
+        request_method: "POST",
+        path: "/abc/xyz"
+      )
+      action = create_action(request)
 
       action.expects(:no_second_factors_enabled!).never
       action.expects(:second_factor_auth_required!).never
@@ -174,25 +156,21 @@ describe SecondFactor::AuthManager do
         .with({ call_me_back: 4314 })
         .once
       manager = create_manager(action)
-      request = create_request(
-        request_method: "POST",
-        path: "/abc/xyz"
-      )
       manager.run!(request, { second_factor_nonce: nonce }, secure_session)
     end
 
     it "does not call the second_factor_auth_completed! method of the action if the challenge is not marked successful" do
       nonce, secure_session = stage_challenge(successful: false)
 
-      action = create_action
-      action.expects(:no_second_factors_enabled!).never
-      action.expects(:second_factor_auth_required!).never
-      action.expects(:second_factor_auth_completed!).never
-      manager = create_manager(action)
       request = create_request(
         request_method: "POST",
         path: "/abc/xyz"
       )
+      action = create_action(request)
+      action.expects(:no_second_factors_enabled!).never
+      action.expects(:second_factor_auth_required!).never
+      action.expects(:second_factor_auth_completed!).never
+      manager = create_manager(action)
       expect {
         manager.run!(request, { second_factor_nonce: nonce }, secure_session)
       }.to raise_error(SecondFactor::BadChallenge) do |ex|
@@ -203,15 +181,15 @@ describe SecondFactor::AuthManager do
     it "does not call the second_factor_auth_completed! method of the action if the challenge is expired" do
       nonce, secure_session = stage_challenge(successful: true)
 
-      action = create_action
-      action.expects(:no_second_factors_enabled!).never
-      action.expects(:second_factor_auth_required!).never
-      action.expects(:second_factor_auth_completed!).never
-      manager = create_manager(action)
       request = create_request(
         request_method: "POST",
         path: "/abc/xyz"
       )
+      action = create_action(request)
+      action.expects(:no_second_factors_enabled!).never
+      action.expects(:second_factor_auth_required!).never
+      action.expects(:second_factor_auth_completed!).never
+      manager = create_manager(action)
 
       freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now
       expect {
@@ -220,5 +198,69 @@ describe SecondFactor::AuthManager do
         expect(ex.error_translation_key).to eq("second_factor_auth.challenge_expired")
       end
     end
+
+    it "calls second_factor_auth_skipped! if skip_second_factor_auth? return true" do
+      action = create_action
+      params = { a: 1 }
+      action.expects(:skip_second_factor_auth?).with(params).returns(true).once
+      action.expects(:second_factor_auth_skipped!).with(params).once
+      action.expects(:no_second_factors_enabled!).never
+      action.expects(:second_factor_auth_required!).never
+      action.expects(:second_factor_auth_completed!).never
+      manager = create_manager(action)
+      manager.run!(action.request, params, {})
+    end
+
+    it "doesn't call second_factor_auth_skipped! if skip_second_factor_auth? return false" do
+      action = create_action
+      params = { a: 1 }
+      action.expects(:skip_second_factor_auth?).with(params).returns(false).once
+      action.expects(:second_factor_auth_skipped!).never
+      action.expects(:no_second_factors_enabled!).never
+      action.expects(:second_factor_auth_required!).with(params).returns({}).once
+      action.expects(:second_factor_auth_completed!).never
+      manager = create_manager(action)
+      expect {
+        manager.run!(action.request, params, {})
+      }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) do |ex|
+        expect(ex.nonce).to be_present
+      end
+    end
+
+    context "returned results object" do
+      it "has the correct status and contains the return value of the action hook that's called" do
+        action = create_action
+        action.expects(:skip_second_factor_auth?).with({}).returns(true).once
+        action.expects(:second_factor_auth_skipped!).with({}).returns("yeah whatever").once
+        manager = create_manager(action)
+        results = manager.run!(action.request, {}, {})
+        expect(results.data).to eq("yeah whatever")
+        expect(results.second_factor_auth_skipped?).to eq(true)
+
+        nonce, secure_session = stage_challenge(successful: true)
+        request = create_request(
+          request_method: "POST",
+          path: "/abc/xyz"
+        )
+        action = create_action(request)
+        action
+          .expects(:second_factor_auth_completed!)
+          .with({ call_me_back: 4314 })
+          .returns({ eviltrout: "goodbye :(" })
+          .once
+        manager = create_manager(action)
+        results = manager.run!(request, { second_factor_nonce: nonce }, secure_session)
+        expect(results.data).to eq({ eviltrout: "goodbye :(" })
+        expect(results.second_factor_auth_completed?).to eq(true)
+
+        user_totp.destroy!
+        action = create_action
+        action.expects(:no_second_factors_enabled!).with({}).returns("NOTHING WORKS").once
+        manager = create_manager(action)
+        results = manager.run!(action.request, {}, {})
+        expect(results.data).to eq("NOTHING WORKS")
+        expect(results.no_second_factors_enabled?).to eq(true)
+      end
+    end
   end
 end

spec/requests/admin/users_controller_spec.rb

@@ -384,7 +384,7 @@ RSpec.describe Admin::UsersController do
     it 'asks the acting admin for second factor if it is enabled' do
       Fabricate(:user_second_factor_totp, user: admin)
 
-      put "/admin/users/#{another_user.id}/grant_admin.json"
+      put "/admin/users/#{another_user.id}/grant_admin.json", xhr: true
 
       expect(response.parsed_body["second_factor_challenge_nonce"]).to be_present
       expect(another_user.reload.admin).to eq(false)
@@ -393,7 +393,7 @@ RSpec.describe Admin::UsersController do
     it 'grants admin if second factor is correct' do
       user_second_factor = Fabricate(:user_second_factor_totp, user: admin)
 
-      put "/admin/users/#{another_user.id}/grant_admin.json"
+      put "/admin/users/#{another_user.id}/grant_admin.json", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       expect(nonce).to be_present
       expect(another_user.reload.admin).to eq(false)
@@ -408,7 +408,7 @@ RSpec.describe Admin::UsersController do
       expect(res["ok"]).to eq(true)
       expect(res["callback_method"]).to eq("PUT")
       expect(res["callback_path"]).to eq("/admin/users/#{another_user.id}/grant_admin.json")
-      expect(res["redirect_path"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}")
+      expect(res["redirect_url"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}")
       expect(another_user.reload.admin).to eq(false)
 
       put res["callback_path"], params: {
@@ -421,7 +421,7 @@ RSpec.describe Admin::UsersController do
     it 'does not grant admin if second factor auth is not successful' do
       user_second_factor = Fabricate(:user_second_factor_totp, user: admin)
 
-      put "/admin/users/#{another_user.id}/grant_admin.json"
+      put "/admin/users/#{another_user.id}/grant_admin.json", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       expect(nonce).to be_present
       expect(another_user.reload.admin).to eq(false)
@@ -446,7 +446,7 @@ RSpec.describe Admin::UsersController do
     it 'does not grant admin if the acting admin loses permission in the middle of the process' do
       user_second_factor = Fabricate(:user_second_factor_totp, user: admin)
 
-      put "/admin/users/#{another_user.id}/grant_admin.json"
+      put "/admin/users/#{another_user.id}/grant_admin.json", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       expect(nonce).to be_present
       expect(another_user.reload.admin).to eq(false)
@@ -461,7 +461,7 @@ RSpec.describe Admin::UsersController do
       expect(res["ok"]).to eq(true)
       expect(res["callback_method"]).to eq("PUT")
       expect(res["callback_path"]).to eq("/admin/users/#{another_user.id}/grant_admin.json")
-      expect(res["redirect_path"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}")
+      expect(res["redirect_url"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}")
       expect(another_user.reload.admin).to eq(false)
 
       admin.update!(admin: false)
@@ -476,7 +476,7 @@ RSpec.describe Admin::UsersController do
       Fabricate(:user_second_factor_totp, user: admin)
       Fabricate(:user_second_factor_backup, user: admin)
 
-      put "/admin/users/#{another_user.id}/grant_admin.json"
+      put "/admin/users/#{another_user.id}/grant_admin.json", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       expect(nonce).to be_present
       expect(another_user.reload.admin).to eq(false)

spec/requests/session_controller_spec.rb

@@ -1116,9 +1116,8 @@ describe SessionController do
 
   describe '#sso_provider' do
     let(:headers) { { host: Discourse.current_hostname } }
-
-    describe 'can act as an SSO provider' do
     let(:logo_fixture) { "http://#{Discourse.current_hostname}/uploads/logo.png" }
+    fab!(:user) { Fabricate(:user, password: "myfrogs123ADMIN", active: true, admin: true) }
 
     before do
       stub_request(:any, /#{Discourse.current_hostname}\/uploads/).to_return(
@@ -1140,7 +1139,7 @@ describe SessionController do
       @sso.nonce = "mynonce"
       @sso.return_sso_url = "http://somewhere.over.rainbow/sso"
 
-        @user = Fabricate(:user, password: "myfrogs123ADMIN", active: true, admin: true)
+      @user = user
       group = Fabricate(:group)
       group.add(@user)
 
@@ -1155,6 +1154,7 @@ describe SessionController do
       EmailToken.update_all(confirmed: true)
     end
 
+    describe 'can act as an SSO provider' do
       it "successfully logs in and redirects user to return_sso_url when the user is not logged in" do
         get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
 
@@ -1185,6 +1185,8 @@ describe SessionController do
         expect(sso2.avatar_url).to start_with(Discourse.base_url)
         expect(sso2.profile_background_url).to start_with(Discourse.base_url)
         expect(sso2.card_background_url).to start_with(Discourse.base_url)
+        expect(sso2.confirmed_2fa).to eq(nil)
+        expect(sso2.no_2fa_methods).to eq(nil)
       end
 
       it "it fails to log in if secret is wrong" do
@@ -1236,6 +1238,8 @@ describe SessionController do
         expect(sso2.avatar_url).to start_with(Discourse.base_url)
         expect(sso2.profile_background_url).to start_with(Discourse.base_url)
         expect(sso2.card_background_url).to start_with(Discourse.base_url)
+        expect(sso2.confirmed_2fa).to eq(nil)
+        expect(sso2.no_2fa_methods).to eq(nil)
       end
 
       it 'handles non local content correctly' do
@@ -1292,6 +1296,8 @@ describe SessionController do
         expect(sso2.avatar_url).to start_with("#{SiteSetting.s3_cdn_url}/original")
         expect(sso2.profile_background_url).to start_with(SiteSetting.s3_cdn_url)
         expect(sso2.card_background_url).to start_with(SiteSetting.s3_cdn_url)
+        expect(sso2.confirmed_2fa).to eq(nil)
+        expect(sso2.no_2fa_methods).to eq(nil)
       end
 
       it "successfully logs out and redirects user to return_sso_url when the user is logged in" do
@@ -1320,6 +1326,153 @@ describe SessionController do
         expect(response.cookies["_t"]).to be_blank
       end
     end
+
+    describe 'can act as a 2FA provider' do
+      fab!(:user_totp) { Fabricate(:user_second_factor_totp, user: user) }
+      before { @sso.require_2fa = true }
+
+      it 'requires the user to confirm 2FA before they are redirected to the SSO return URL' do
+        sign_in(user)
+        get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+        uri = URI(response.location)
+        expect(uri.hostname).to eq(Discourse.current_hostname)
+        expect(uri.path).to eq("/session/2fa")
+        nonce = uri.query.match(/\Anonce=([A-Za-z0-9]{32})\Z/)[1]
+        expect(nonce).to be_present
+
+        # attempt no. 1 to bypass 2fa
+        get "/session/sso_provider", params: {
+          second_factor_nonce: nonce
+        }
+        expect(response.status).to eq(401)
+        expect(response.parsed_body["error"]).to eq(
+          I18n.t("second_factor_auth.challenge_not_completed")
+        )
+
+        # attempt no. 2 to bypass 2fa
+        get "/session/sso_provider", params: {
+          second_factor_nonce: nonce
+        }.merge(Rack::Utils.parse_query(@sso.payload("secretForOverRainbow")))
+        expect(response.status).to eq(401)
+        expect(response.parsed_body["error"]).to eq(
+          I18n.t("second_factor_auth.challenge_not_completed")
+        )
+
+        # confirm 2fa
+        post "/session/2fa.json", params: {
+          nonce: nonce,
+          second_factor_token: ROTP::TOTP.new(user_totp.data).now,
+          second_factor_method: UserSecondFactor.methods[:totp]
+        }
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["ok"]).to eq(true)
+        expect(response.parsed_body["callback_method"]).to eq("GET")
+        expect(response.parsed_body["callback_path"]).to eq("/session/sso_provider")
+        expect(response.parsed_body["redirect_url"]).to be_blank
+
+        get "/session/sso_provider", params: {
+          second_factor_nonce: nonce
+        }
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["success"]).to eq("OK")
+        redirect_url = response.parsed_body["redirect_url"]
+        expect(redirect_url).to start_with("http://somewhere.over.rainbow/sso?sso=")
+        sso = DiscourseConnectProvider.parse(URI(redirect_url).query)
+        expect(sso.confirmed_2fa).to eq(true)
+        expect(sso.no_2fa_methods).to eq(nil)
+        expect(sso.username).to eq(user.username)
+        expect(sso.email).to eq(user.email)
+      end
+
+      it "doesn't accept backup codes" do
+        backup_codes = user.generate_backup_codes
+        sign_in(user)
+        get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+        uri = URI(response.location)
+        expect(uri.hostname).to eq(Discourse.current_hostname)
+        expect(uri.path).to eq("/session/2fa")
+        nonce = uri.query.match(/\Anonce=([A-Za-z0-9]{32})\Z/)[1]
+        expect(nonce).to be_present
+
+        post "/session/2fa.json", params: {
+          nonce: nonce,
+          second_factor_token: backup_codes.sample,
+          second_factor_method: UserSecondFactor.methods[:backup_codes]
+        }
+        expect(response.status).to eq(403)
+        get "/session/sso_provider", params: {
+          second_factor_nonce: nonce
+        }
+        expect(response.status).to eq(401)
+        expect(response.parsed_body["error"]).to eq(
+          I18n.t("second_factor_auth.challenge_not_completed")
+        )
+      end
+
+      context 'when the user has no 2fa methods' do
+        before { user_totp.destroy!; user.reload }
+
+        it 'redirects the user back to the SSO return url and indicates in the payload that they do not have 2fa methods' do
+          sign_in(user)
+          get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+
+          expect(response.status).to eq(302)
+          redirect_url = response.location
+          expect(redirect_url).to start_with("http://somewhere.over.rainbow/sso?sso=")
+          sso = DiscourseConnectProvider.parse(URI(redirect_url).query)
+          expect(sso.confirmed_2fa).to eq(nil)
+          expect(sso.no_2fa_methods).to eq(true)
+          expect(sso.username).to eq(user.username)
+          expect(sso.email).to eq(user.email)
+        end
+      end
+
+      context 'when there is no logged in user' do
+        it "redirects the user to login first" do
+          get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+          expect(response.status).to eq(302)
+          expect(response.location).to eq("http://#{Discourse.current_hostname}/login")
+        end
+
+        it "doesn't make the user confirm 2fa twice if they've just logged in and confirmed 2fa while doing so" do
+          get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+
+          post "/session.json", params: {
+            login: user.username,
+            password: "myfrogs123ADMIN",
+            second_factor_token: ROTP::TOTP.new(user_totp.data).now,
+            second_factor_method: UserSecondFactor.methods[:totp]
+          }, xhr: true, headers: headers
+          expect(response.status).to eq(204)
+          # the frontend will take care of actually redirecting the user
+          redirect_url = response.cookies["sso_destination_url"]
+          expect(redirect_url).to start_with("http://somewhere.over.rainbow/sso?sso=")
+          sso = DiscourseConnectProvider.parse(URI(redirect_url).query)
+          expect(sso.confirmed_2fa).to eq(true)
+          expect(sso.no_2fa_methods).to eq(nil)
+          expect(sso.username).to eq(user.username)
+          expect(sso.email).to eq(user.email)
+        end
+
+        it "doesn't indicate the user has confirmed 2fa after they've logged in if they have no 2fa methods" do
+          user_totp.destroy!
+          user.reload
+          get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
+
+          post "/session.json", params: {
+            login: user.username,
+            password: "myfrogs123ADMIN",
+          }, xhr: true, headers: headers
+          redirect_url = response.cookies["sso_destination_url"]
+          expect(redirect_url).to start_with("http://somewhere.over.rainbow/sso?sso=")
+          sso = DiscourseConnectProvider.parse(URI(redirect_url).query)
+          expect(sso.confirmed_2fa).to eq(nil)
+          expect(sso.no_2fa_methods).to eq(true)
+          expect(sso.username).to eq(user.username)
+          expect(sso.email).to eq(user.email)
+        end
+      end
+    end
   end
 
   describe '#create' do
@@ -2317,7 +2470,7 @@ describe SessionController do
     end
 
     it 'returns 401 if the challenge nonce has expired' do
-      post "/session/2fa/test-action"
+      post "/session/2fa/test-action", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       get "/session/2fa.json", params: { nonce: nonce }
       expect(response.status).to eq(200)
@@ -2330,7 +2483,7 @@ describe SessionController do
     end
 
     it 'responds with challenge data' do
-      post "/session/2fa/test-action"
+      post "/session/2fa/test-action", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       get "/session/2fa.json", params: { nonce: nonce }
       expect(response.status).to eq(200)
@@ -2352,7 +2505,7 @@ describe SessionController do
         enabled: true
       )
       Fabricate(:user_second_factor_backup, user: user)
-      post "/session/2fa/test-action", params: { allow_backup_codes: true }
+      post "/session/2fa/test-action", params: { allow_backup_codes: true }, xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       get "/session/2fa.json", params: { nonce: nonce }
       expect(response.status).to eq(200)
@@ -2379,7 +2532,7 @@ describe SessionController do
     end
 
     it 'returns 401 if the challenge nonce has expired' do
-      post "/session/2fa/test-action"
+      post "/session/2fa/test-action", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
 
       freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now
@@ -2395,7 +2548,7 @@ describe SessionController do
 
     it 'returns 403 if the 2FA method is not allowed' do
       Fabricate(:user_second_factor_backup, user: user)
-      post "/session/2fa/test-action"
+      post "/session/2fa/test-action", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       post "/session/2fa.json", params: {
         nonce: nonce,
@@ -2406,7 +2559,7 @@ describe SessionController do
     end
 
     it 'returns 403 if the user disables the 2FA method in the middle of the 2FA process' do
-      post "/session/2fa/test-action"
+      post "/session/2fa/test-action", xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
       token = ROTP::TOTP.new(user_second_factor.data).now
       user_second_factor.destroy!
@@ -2419,7 +2572,7 @@ describe SessionController do
     end
 
     it 'marks the challenge as successful if the 2fa succeeds' do
-      post "/session/2fa/test-action", params: { redirect_path: "/ggg" }
+      post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
 
       token = ROTP::TOTP.new(user_second_factor.data).now
@@ -2433,7 +2586,7 @@ describe SessionController do
       expect(response.parsed_body["ok"]).to eq(true)
       expect(response.parsed_body["callback_method"]).to eq("POST")
       expect(response.parsed_body["callback_path"]).to eq("/session/2fa/test-action")
-      expect(response.parsed_body["redirect_path"]).to eq("/ggg")
+      expect(response.parsed_body["redirect_url"]).to eq("/ggg")
 
       post "/session/2fa/test-action", params: { second_factor_nonce: nonce }
       expect(response.status).to eq(200)
@@ -2442,7 +2595,7 @@ describe SessionController do
     end
 
     it 'does not mark the challenge as successful if the 2fa fails' do
-      post "/session/2fa/test-action", params: { redirect_path: "/ggg" }
+      post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true
       nonce = response.parsed_body["second_factor_challenge_nonce"]
 
       token = ROTP::TOTP.new(user_second_factor.data).now.to_i

spec/support/test_second_factor_action.rb

@@ -6,7 +6,7 @@ class TestSecondFactorAction < SecondFactor::Actions::Base
 
   def second_factor_auth_required!(params)
     {
-      redirect_path: params[:redirect_path],
+      redirect_url: params[:redirect_url],
       callback_params: {
         saved_param_1: params[:saved_param_1],
         saved_param_2: params[:saved_param_2]