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]