From eb5a3cfded114ad1ef701e9e3f495c98d2f70ebd Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 13 Apr 2022 15:04:09 +0300 Subject: [PATCH] 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/controllers/second-factor-auth.js | 6 +- .../acceptance/second-factor-auth-test.js | 4 +- app/controllers/application_controller.rb | 23 +- app/controllers/session_controller.rb | 125 ++++----- app/models/concerns/second_factor_manager.rb | 30 ++- config/locales/server.en.yml | 2 + lib/discourse_connect_base.rb | 8 +- lib/discourse_connect_provider.rb | 1 + lib/second_factor/actions/base.rb | 14 +- .../actions/discourse_connect_provider.rb | 95 +++++++ lib/second_factor/actions/grant_admin.rb | 4 +- lib/second_factor/auth_manager.rb | 63 +++-- lib/second_factor/auth_manager_result.rb | 9 +- .../lib/concern/second_factor_manager_spec.rb | 38 +++ .../discourse_connect_provider_spec.rb | 247 ++++++++++++++++++ .../second_factor/actions/grant_admin_spec.rb | 16 +- spec/lib/second_factor/auth_manager_spec.rb | 196 ++++++++------ spec/requests/admin/users_controller_spec.rb | 14 +- spec/requests/session_controller_spec.rb | 245 +++++++++++++---- spec/support/test_second_factor_action.rb | 2 +- 20 files changed, 899 insertions(+), 243 deletions(-) create mode 100644 lib/second_factor/actions/discourse_connect_provider.rb create mode 100644 spec/lib/second_factor/actions/discourse_connect_provider_spec.rb diff --git a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js index 996e5f06ff9..4de8d672c31 100644 --- a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js +++ b/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) => { diff --git a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js index 68d122db3a5..417a31ecf26 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js +++ b/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"); }); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 36e4a4394f3..4e6b69a49cc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -251,9 +251,13 @@ class ApplicationController < ActionController::Base end rescue_from SecondFactor::AuthManager::SecondFactorRequired do |e| - render json: { - second_factor_challenge_nonce: e.nonce - }, status: 403 + 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) - action = action_class.new(guardian) + def run_second_factor!(action_class, action_data = nil) + 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 diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 585002b405e..418de7a48d6 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -39,77 +39,56 @@ class SessionController < ApplicationController end end - def sso_provider(payload = nil) - if SiteSetting.enable_discourse_connect_provider - begin - 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 + def sso_provider(payload = nil, confirmed_2fa_during_login = false) + if !SiteSetting.enable_discourse_connect_provider + render body: nil, status: 404 + return + 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 + result = run_second_factor!( + SecondFactor::Actions::DiscourseConnectProvider, + payload: payload, + confirmed_2fa_during_login: confirmed_2fa_during_login + ) - if sso.return_sso_url.blank? - render plain: "return_sso_url is blank, it must be provided", status: 400 - return - end - - if sso.logout - params[:return_url] = sso.return_sso_url + if result.second_factor_auth_skipped? + data = result.data + if data[:logout] + params[:return_url] = data[:return_sso_url] destroy return end - if current_user - 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(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) - else - redirect_to sso.to_url(sso.return_sso_url) - end - else - cookies[:sso_payload] = request.query_string + if data[:no_current_user] + cookies[:sso_payload] = payload || request.query_string redirect_to path('/login') + return end - else - render body: nil, status: 404 + + if request.xhr? + # for the login modal + cookies[:sso_destination_url] = data[:sso_redirect_url] + else + 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 + redirect_to result.data[:sso_redirect_url] + end + elsif result.second_factor_auth_completed? + 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 diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index d1974f94497..e21957e75ff 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5d86544ac9b..92b0f58b587 100644 --- a/config/locales/server.en.yml +++ b/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!" diff --git a/lib/discourse_connect_base.rb b/lib/discourse_connect_base.rb index 8264b7ede96..f2e7a7b8207 100644 --- a/lib/discourse_connect_base.rb +++ b/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 } diff --git a/lib/discourse_connect_provider.rb b/lib/discourse_connect_provider.rb index 20c10797e1b..8ddf51975c4 100644 --- a/lib/discourse_connect_provider.rb +++ b/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) diff --git a/lib/second_factor/actions/base.rb b/lib/second_factor/actions/base.rb index d55f2b560da..d3caa729828 100644 --- a/lib/second_factor/actions/base.rb +++ b/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) diff --git a/lib/second_factor/actions/discourse_connect_provider.rb b/lib/second_factor/actions/discourse_connect_provider.rb new file mode 100644 index 00000000000..b300f928554 --- /dev/null +++ b/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 diff --git a/lib/second_factor/actions/grant_admin.rb b/lib/second_factor/actions/grant_admin.rb index 563a3b0ace0..8b581da934c 100644 --- a/lib/second_factor/actions/grant_admin.rb +++ b/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 diff --git a/lib/second_factor/auth_manager.rb b/lib/second_factor/auth_manager.rb index 68e481f513f..163ae771259 100644 --- a/lib/second_factor/auth_manager.rb +++ b/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 - redirected to after the action is finished. When this key is omitted, the - redirect path is set to the homepage (/). + :redirect_url => where the user should be redirected after they confirm 2fa. + A relative path (must be subfolder-aware) is a valid value for this key. :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 - inherits `SecondFactor::Actions::Base` and implements the 3 methods mentioned - above. +* The `lib/second_factor/actions` directory where all existing actions live. * `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) } - @action.no_second_factors_enabled!(params) - create_result(:no_second_factor) - elsif nonce = params[:second_factor_nonce].presence - verify_second_factor_auth_completed(nonce, secure_session) - create_result(:second_factor_auth_completed) + if nonce = params[:second_factor_nonce].presence + data = verify_second_factor_auth_completed(nonce, secure_session) + create_result(:second_factor_auth_completed, data) + 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) } + 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_path: request.path, + callback_method: config[:callback_method] || request.request_method, + 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) - SecondFactor::AuthManagerResult.new(status) + def create_result(status, data = nil) + SecondFactor::AuthManagerResult.new(status, data) end end diff --git a/lib/second_factor/auth_manager_result.rb b/lib/second_factor/auth_manager_result.rb index 00e11653aa4..2b830b36c79 100644 --- a/lib/second_factor/auth_manager_result.rb +++ b/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 diff --git a/spec/lib/concern/second_factor_manager_spec.rb b/spec/lib/concern/second_factor_manager_spec.rb index 5d366424edc..1f0dd30cd8b 100644 --- a/spec/lib/concern/second_factor_manager_spec.rb +++ b/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 diff --git a/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb b/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb new file mode 100644 index 00000000000..2573737d11f --- /dev/null +++ b/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 diff --git a/spec/lib/second_factor/actions/grant_admin_spec.rb b/spec/lib/second_factor/actions/grant_admin_spec.rb index 9591f5941aa..21afcc2079a 100644 --- a/spec/lib/second_factor/actions/grant_admin_spec.rb +++ b/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) - SecondFactor::Actions::GrantAdmin.new(Guardian.new(user)) + def create_request(request_method: "GET", path: "/") + 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", diff --git a/spec/lib/second_factor/auth_manager_spec.rb b/spec/lib/second_factor/auth_manager_spec.rb index 0b33219ab8b..b483b6bbdc0 100644 --- a/spec/lib/second_factor/auth_manager_spec.rb +++ b/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 - TestSecondFactorAction.new(guardian) + def create_action(request = nil) + 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 - # expected + }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) do |ex| + expect(ex.nonce).to be_present end challenge = JSON .parse(secure_session["current_second_factor_auth_challenge"]) .deep_symbolize_keys - challenge[:successful] = successful - secure_session["current_second_factor_auth_challenge"] = challenge.to_json + 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 - 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) + it "prefers callback_method and callback_path from the output of the action's second_factor_auth_required! method if they're present" do request = create_request( request_method: "POST", path: "/abc/xyz" ) - secure_session = {} - 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 = create_action(request) action .expects(:second_factor_auth_required!) - .with({ expect_me: 131 }) - .returns({ callback_params: { call_me_back: 4314 } }) + .with({}) + .returns( + callback_params: { call_me_back: 4314 }, + callback_method: "PUT", + callback_path: "/test/443" + ) .once - action.expects(:second_factor_auth_completed!).never manager = create_manager(action) - request = create_request( - request_method: "POST", - path: "/abc/xyz" - ) 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 diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index bb6aac7e242..b0a4e8c4a22 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/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) diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index c1629a3a028..7c5a9ef2779 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1116,45 +1116,45 @@ describe SessionController do describe '#sso_provider' do let(:headers) { { host: Discourse.current_hostname } } + 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( + status: 200, + body: lambda { |request| file_from_fixtures("logo.png") } + ) + + SiteSetting.enable_discourse_connect_provider = true + SiteSetting.enable_discourse_connect = false + SiteSetting.enable_local_logins = true + SiteSetting.discourse_connect_provider_secrets = [ + "*|secret,forAll", + "*.rainbow|wrongSecretForOverRainbow", + "www.random.site|secretForRandomSite", + "somewhere.over.rainbow|secretForOverRainbow", + ].join("\n") + + @sso = DiscourseConnectProvider.new + @sso.nonce = "mynonce" + @sso.return_sso_url = "http://somewhere.over.rainbow/sso" + + @user = user + group = Fabricate(:group) + group.add(@user) + + @user.create_user_avatar! + UserAvatar.import_url_for_user(logo_fixture, @user) + UserProfile.import_url_for_user(logo_fixture, @user, is_card_background: false) + UserProfile.import_url_for_user(logo_fixture, @user, is_card_background: true) + + @user.reload + @user.user_avatar.reload + @user.user_profile.reload + EmailToken.update_all(confirmed: true) + end describe 'can act as an SSO provider' do - let(:logo_fixture) { "http://#{Discourse.current_hostname}/uploads/logo.png" } - - before do - stub_request(:any, /#{Discourse.current_hostname}\/uploads/).to_return( - status: 200, - body: lambda { |request| file_from_fixtures("logo.png") } - ) - - SiteSetting.enable_discourse_connect_provider = true - SiteSetting.enable_discourse_connect = false - SiteSetting.enable_local_logins = true - SiteSetting.discourse_connect_provider_secrets = [ - "*|secret,forAll", - "*.rainbow|wrongSecretForOverRainbow", - "www.random.site|secretForRandomSite", - "somewhere.over.rainbow|secretForOverRainbow", - ].join("\n") - - @sso = DiscourseConnectProvider.new - @sso.nonce = "mynonce" - @sso.return_sso_url = "http://somewhere.over.rainbow/sso" - - @user = Fabricate(:user, password: "myfrogs123ADMIN", active: true, admin: true) - group = Fabricate(:group) - group.add(@user) - - @user.create_user_avatar! - UserAvatar.import_url_for_user(logo_fixture, @user) - UserProfile.import_url_for_user(logo_fixture, @user, is_card_background: false) - UserProfile.import_url_for_user(logo_fixture, @user, is_card_background: true) - - @user.reload - @user.user_avatar.reload - @user.user_profile.reload - EmailToken.update_all(confirmed: true) - end - 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 diff --git a/spec/support/test_second_factor_action.rb b/spec/support/test_second_factor_action.rb index 58da03ab7ba..825d07a4bf6 100644 --- a/spec/support/test_second_factor_action.rb +++ b/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]