diff --git a/app/assets/javascripts/confirm-new-email/confirm-new-email.js.es6 b/app/assets/javascripts/confirm-new-email/confirm-new-email.js.es6
new file mode 100644
index 0000000000..dbcfdd25c4
--- /dev/null
+++ b/app/assets/javascripts/confirm-new-email/confirm-new-email.js.es6
@@ -0,0 +1,23 @@
+import { getWebauthnCredential } from "discourse/lib/webauthn";
+
+document.getElementById("submit-security-key").onclick = function(e) {
+ e.preventDefault();
+ getWebauthnCredential(
+ document.getElementById("security-key-challenge").value,
+ document
+ .getElementById("security-key-allowed-credential-ids")
+ .value.split(","),
+ credentialData => {
+ document.getElementById("security-key-credential").value = JSON.stringify(
+ credentialData
+ );
+
+ $(e.target)
+ .parents("form")
+ .submit();
+ },
+ errorMessage => {
+ document.getElementById("security-key-error").innerText = errorMessage;
+ }
+ );
+};
diff --git a/app/assets/javascripts/confirm-new-email/confirm-new-email.no-module.js.es6 b/app/assets/javascripts/confirm-new-email/confirm-new-email.no-module.js.es6
new file mode 100644
index 0000000000..0dd05dd8ca
--- /dev/null
+++ b/app/assets/javascripts/confirm-new-email/confirm-new-email.no-module.js.es6
@@ -0,0 +1 @@
+require("confirm-new-email/confirm-new-email").default();
diff --git a/app/assets/javascripts/discourse/controllers/email-login.js.es6 b/app/assets/javascripts/discourse/controllers/email-login.js.es6
index 01612c766f..4eab7fc1ba 100644
--- a/app/assets/javascripts/discourse/controllers/email-login.js.es6
+++ b/app/assets/javascripts/discourse/controllers/email-login.js.es6
@@ -23,15 +23,13 @@ export default Controller.extend({
actions: {
finishLogin() {
- let data = {};
+ let data = { second_factor_method: this.secondFactorMethod };
if (this.securityKeyCredential) {
- data = { security_key_credential: this.securityKeyCredential };
+ data.second_factor_token = this.securityKeyCredential;
} else {
- data = {
- second_factor_token: this.secondFactorToken,
- second_factor_method: this.secondFactorMethod
- };
+ data.second_factor_token = this.secondFactorToken;
}
+
ajax({
url: `/session/email-login/${this.model.token}`,
type: "POST",
diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6
index 5eabad52fb..c90a1b07f7 100644
--- a/app/assets/javascripts/discourse/controllers/login.js.es6
+++ b/app/assets/javascripts/discourse/controllers/login.js.es6
@@ -120,9 +120,8 @@ export default Controller.extend(ModalFunctionality, {
data: {
login: this.loginName,
password: this.loginPassword,
- second_factor_token: this.secondFactorToken,
+ second_factor_token: this.securityKeyCredential || this.secondFactorToken,
second_factor_method: this.secondFactorMethod,
- security_key_credential: this.securityKeyCredential,
timezone: moment.tz.guess()
}
}).then(
@@ -130,12 +129,9 @@ export default Controller.extend(ModalFunctionality, {
// Successful login
if (result && result.error) {
this.set("loggingIn", false);
- const invalidSecurityKey = result.reason === "invalid_security_key";
- const invalidSecondFactor =
- result.reason === "invalid_second_factor";
if (
- (invalidSecondFactor || invalidSecurityKey) &&
+ (result.security_key_enabled || result.totp_enabled) &&
!this.secondFactorRequired
) {
document.getElementById("modal-alert").style.display = "none";
@@ -145,9 +141,9 @@ export default Controller.extend(ModalFunctionality, {
secondFactorRequired: true,
showLoginButtons: false,
backupEnabled: result.backup_enabled,
- showSecondFactor: invalidSecondFactor,
- showSecurityKey: invalidSecurityKey,
- secondFactorMethod: invalidSecurityKey
+ showSecondFactor: result.totp_enabled,
+ showSecurityKey: result.security_key_enabled,
+ secondFactorMethod: result.security_key_enabled
? SECOND_FACTOR_METHODS.SECURITY_KEY
: SECOND_FACTOR_METHODS.TOTP,
securityKeyChallenge: result.challenge,
diff --git a/app/assets/javascripts/discourse/controllers/password-reset.js.es6 b/app/assets/javascripts/discourse/controllers/password-reset.js.es6
index 7f3718482e..f423f0bff9 100644
--- a/app/assets/javascripts/discourse/controllers/password-reset.js.es6
+++ b/app/assets/javascripts/discourse/controllers/password-reset.js.es6
@@ -1,4 +1,4 @@
-import { alias, or } from "@ember/object/computed";
+import { alias, or, readOnly } from "@ember/object/computed";
import Controller from "@ember/controller";
import { default as discourseComputed } from "discourse-common/utils/decorators";
import DiscourseURL from "discourse/lib/url";
@@ -18,6 +18,7 @@ export default Controller.extend(PasswordValidation, {
"model.second_factor_required",
"model.security_key_required"
),
+ otherMethodAllowed: readOnly("model.multiple_second_factor_methods"),
@discourseComputed("model.security_key_required")
secondFactorMethod(security_key_required) {
return security_key_required
@@ -51,9 +52,8 @@ export default Controller.extend(PasswordValidation, {
type: "PUT",
data: {
password: this.accountPassword,
- second_factor_token: this.secondFactorToken,
- second_factor_method: this.secondFactorMethod,
- security_key_credential: this.securityKeyCredential
+ second_factor_token: this.securityKeyCredential || this.secondFactorToken,
+ second_factor_method: this.secondFactorMethod
}
})
.then(result => {
diff --git a/app/assets/javascripts/discourse/templates/password-reset.hbs b/app/assets/javascripts/discourse/templates/password-reset.hbs
index c7c83156c5..d2126263c4 100644
--- a/app/assets/javascripts/discourse/templates/password-reset.hbs
+++ b/app/assets/javascripts/discourse/templates/password-reset.hbs
@@ -28,7 +28,7 @@
showSecurityKey=model.security_key_required
showSecondFactor=false
secondFactorMethod=secondFactorMethod
- otherMethodAllowed=secondFactorRequired
+ otherMethodAllowed=otherMethodAllowed
action=(action "authenticateSecurityKey")}}
{{/security-key-form}}
{{else}}
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 5dcd7ae2e1..46ff0a334e 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -745,23 +745,22 @@ class ApplicationController < ActionController::Base
return
end
- check_totp = current_user &&
- !request.format.json? &&
- !is_api? &&
- !current_user.anonymous? &&
- ((SiteSetting.enforce_second_factor == 'staff' && current_user.staff?) ||
- SiteSetting.enforce_second_factor == 'all') &&
- !current_user.totp_enabled?
+ return if !current_user
+ return if !should_enforce_2fa?
- if check_totp
- redirect_path = "#{GlobalSetting.relative_url_root}/u/#{current_user.username}/preferences/second-factor"
- if !request.fullpath.start_with?(redirect_path)
- redirect_to path(redirect_path)
- nil
- end
+ redirect_path = "#{GlobalSetting.relative_url_root}/u/#{current_user.username}/preferences/second-factor"
+ if !request.fullpath.start_with?(redirect_path)
+ redirect_to path(redirect_path)
+ nil
end
end
+ def should_enforce_2fa?
+ disqualified_from_2fa_enforcement = request.format.json? || is_api? || current_user.anonymous?
+ enforcing_2fa = ((SiteSetting.enforce_second_factor == 'staff' && current_user.staff?) || SiteSetting.enforce_second_factor == 'all')
+ !disqualified_from_2fa_enforcement && enforcing_2fa && !current_user.has_any_second_factor_methods_enabled?
+ end
+
def block_if_readonly_mode
return if request.fullpath.start_with?(path "/admin/backups")
raise Discourse::ReadOnly.new if !(request.get? || request.head?) && @readonly_mode
diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index 442ca7cb6d..0338bd85a4 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -8,6 +8,7 @@ class SessionController < ApplicationController
before_action :check_local_login_allowed, only: %i(create forgot_password email_login email_login_info)
before_action :rate_limit_login, only: %i(create email_login)
+ before_action :rate_limit_second_factor_totp, only: %i(create email_login)
skip_before_action :redirect_to_login_if_required
skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy one_time_password)
@@ -258,10 +259,6 @@ class SessionController < ApplicationController
end
def create
- unless params[:second_factor_token].blank?
- RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
- end
-
params.require(:login)
params.require(:password)
@@ -293,55 +290,14 @@ class SessionController < ApplicationController
end
if payload = login_error_check(user)
- render json: payload
- else
- if user.security_keys_enabled? && params[:second_factor_token].blank?
- security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new(
- user,
- params[:security_key_credential],
- challenge: Webauthn.challenge(user, secure_session),
- rp_id: Webauthn.rp_id(user, secure_session),
- origin: Discourse.base_url
- ).authenticate_security_key
- return invalid_security_key(user) if !security_key_valid
- return (user.active && user.email_confirmed?) ? login(user) : not_activated(user)
- end
-
- if user.totp_enabled?
- invalid_second_factor = !user.authenticate_second_factor(params[:second_factor_token], params[:second_factor_method].to_i)
- if (params[:security_key_credential].blank? || !user.security_keys_enabled?) && invalid_second_factor
- return render json: failed_json.merge(
- error: I18n.t("login.invalid_second_factor_code"),
- reason: "invalid_second_factor",
- backup_enabled: user.backup_codes_enabled?,
- multiple_second_factor_methods: user.has_multiple_second_factor_methods?
- )
- end
- elsif user.security_keys_enabled?
- # if we have gotten this far then the user has provided the totp
- # params for a security-key-only account
- return render json: failed_json.merge(
- error: I18n.t("login.invalid_second_factor_code"),
- reason: "invalid_second_factor",
- backup_enabled: user.backup_codes_enabled?,
- multiple_second_factor_methods: user.has_multiple_second_factor_methods?
- )
- end
-
- (user.active && user.email_confirmed?) ? login(user) : not_activated(user)
+ return render json: payload
end
- rescue ::Webauthn::SecurityKeyError => err
- invalid_security_key(user, err.message)
- end
- def invalid_security_key(user, err_message = nil)
- Webauthn.stage_challenge(user, secure_session) if !params[:security_key_credential]
- render json: failed_json.merge(
- error: err_message || I18n.t("login.invalid_security_key"),
- reason: "invalid_security_key",
- backup_enabled: user.backup_codes_enabled?,
- multiple_second_factor_methods: user.has_multiple_second_factor_methods?
- ).merge(Webauthn.allowed_credentials(user, secure_session))
+ if !authenticate_second_factor(user)
+ return render(json: @second_factor_failure_payload)
+ end
+
+ (user.active && user.email_confirmed?) ? login(user) : not_activated(user)
end
def email_login_info
@@ -385,41 +341,17 @@ class SessionController < ApplicationController
end
def email_login
- second_factor_token = params[:second_factor_token]
- second_factor_method = params[:second_factor_method].to_i
- security_key_credential = params[:security_key_credential]
token = params[:token]
matched_token = EmailToken.confirmable(token)
if !SiteSetting.enable_local_logins_via_email &&
- !matched_token&.user&.admin? # admin-login uses this route, so allow them even if disabled
+ !matched_token&.user&.admin? # admin-login uses this route, so allow them even if disabled
raise Discourse::NotFound
end
- if security_key_credential.present?
- if matched_token&.user&.security_keys_enabled?
- security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new(
- matched_token&.user,
- params[:security_key_credential],
- challenge: Webauthn.challenge(matched_token&.user, secure_session),
- rp_id: Webauthn.rp_id(matched_token&.user, secure_session),
- origin: Discourse.base_url
- ).authenticate_security_key
- return invalid_security_key(matched_token&.user) if !security_key_valid
- end
- else
- if matched_token&.user&.totp_enabled?
- if !second_factor_token.present?
- return render json: { error: I18n.t('login.invalid_second_factor_code') }
- elsif !matched_token.user.authenticate_second_factor(second_factor_token, second_factor_method)
- RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
- return render json: { error: I18n.t('login.invalid_second_factor_code') }
- end
- elsif matched_token&.user&.security_keys_enabled?
- # this means the user only has security key enabled
- # but has not provided credentials
- return render json: { error: I18n.t('login.invalid_second_factor_code') }
- end
+ user = matched_token&.user
+ if user.present? && !authenticate_second_factor(user)
+ return render(json: @second_factor_failure_payload)
end
if user = EmailToken.confirm(token)
@@ -434,8 +366,6 @@ class SessionController < ApplicationController
end
render json: { error: I18n.t('email_login.invalid_token') }
- rescue ::Webauthn::SecurityKeyError => err
- invalid_security_key(matched_token&.user, err.message)
end
def one_time_password
@@ -514,6 +444,21 @@ class SessionController < ApplicationController
private
+ def authenticate_second_factor(user)
+ second_factor_authentication_result = user.authenticate_second_factor(params, secure_session)
+ if !second_factor_authentication_result.ok
+ failure_payload = second_factor_authentication_result.to_h
+ if user.security_keys_enabled?
+ Webauthn.stage_challenge(user, secure_session)
+ failure_payload.merge!(Webauthn.allowed_credentials(user, secure_session))
+ end
+ @second_factor_failure_payload = failed_json.merge(failure_payload)
+ return false
+ end
+
+ true
+ end
+
def login_error_check(user)
return failed_to_login(user) if user.suspended?
@@ -596,6 +541,11 @@ class SessionController < ApplicationController
).performed!
end
+ def rate_limit_second_factor_totp
+ return if params[:second_factor_token].blank?
+ RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
+ end
+
def render_sso_error(status:, text:)
@sso_error = text
render status: status, layout: 'no_ember'
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index 80bb6346b8..36548585f3 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -105,7 +105,7 @@ class Users::OmniauthCallbacksController < ApplicationController
end
def user_found(user)
- if user.totp_enabled?
+ if user.has_any_second_factor_methods_enabled?
@auth_result.omniauth_disallow_totp = true
@auth_result.email = user.email
return
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index da4b6a19bc..774b7ed82c 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -14,7 +14,7 @@ class UsersController < ApplicationController
]
skip_before_action :check_xhr, only: [
- :show, :badges, :password_reset, :update, :account_created,
+ :show, :badges, :password_reset_show, :password_reset_update, :update, :account_created,
:activate_account, :perform_account_activation, :user_preferences_redirect, :avatar,
:my_redirect, :toggle_anon, :admin_login, :confirm_admin, :email_login, :summary,
:feature_topic, :clear_featured_topic
@@ -40,7 +40,8 @@ class UsersController < ApplicationController
:perform_account_activation,
:send_activation_email,
:update_activation_email,
- :password_reset,
+ :password_reset_show,
+ :password_reset_update,
:confirm_email_token,
:email_login,
:admin_login,
@@ -504,68 +505,79 @@ class UsersController < ApplicationController
}
end
- def password_reset
+ def password_reset_show
expires_now
-
token = params[:token]
+ password_reset_find_user(token, committing_change: false)
- if EmailToken.valid_token_format?(token)
- @user = if request.put?
- EmailToken.confirm(token)
- else
- EmailToken.confirmable(token)&.user
- end
-
- if @user
- secure_session["password-#{token}"] = @user.id
- else
- user_id = secure_session["password-#{token}"].to_i
- @user = User.find(user_id) if user_id > 0
- end
+ if !@error
+ security_params = {
+ is_developer: UsernameCheckerService.is_developer?(@user.email),
+ admin: @user.admin?,
+ second_factor_required: @user.totp_enabled?,
+ security_key_required: @user.security_keys_enabled?,
+ backup_enabled: @user.backup_codes_enabled?,
+ multiple_second_factor_methods: @user.has_multiple_second_factor_methods?
+ }
end
- second_factor_token = params[:second_factor_token]
- second_factor_method = params[:second_factor_method].to_i
- security_key_credential = params[:security_key_credential]
+ respond_to do |format|
+ format.html do
+ return render 'password_reset', layout: 'no_ember' if @error
- if second_factor_token.present? && UserSecondFactor.methods[second_factor_method]
+ Webauthn.stage_challenge(@user, secure_session)
+ store_preloaded(
+ "password_reset",
+ MultiJson.dump(security_params.merge(Webauthn.allowed_credentials(@user, secure_session)))
+ )
+
+ render 'password_reset'
+ end
+
+ format.json do
+ return render json: { message: @error } if @error
+
+ Webauthn.stage_challenge(@user, secure_session)
+ render json: security_params.merge(Webauthn.allowed_credentials(@user, secure_session))
+ end
+ end
+ end
+
+ def password_reset_update
+ expires_now
+ token = params[:token]
+ password_reset_find_user(token, committing_change: true)
+
+ if params[:second_factor_token].present?
RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
- second_factor_authenticated = @user&.authenticate_second_factor(second_factor_token, second_factor_method)
- elsif security_key_credential.present?
- security_key_authenticated = ::Webauthn::SecurityKeyAuthenticationService.new(
- @user,
- security_key_credential,
- challenge: Webauthn.challenge(@user, secure_session),
- rp_id: Webauthn.rp_id(@user, secure_session),
- origin: Discourse.base_url
- ).authenticate_security_key
end
- second_factor_totp_disabled = !@user&.totp_enabled?
- if second_factor_authenticated || second_factor_totp_disabled || security_key_authenticated
- secure_session["second-factor-#{token}"] = "true"
- end
+ # no point doing anything else if we can't even find
+ # a user from the token
+ if @user
- security_key_disabled = !@user&.security_keys_enabled?
- if security_key_authenticated || security_key_disabled
- secure_session["security-key-#{token}"] = "true"
- end
+ if !secure_session["second-factor-#{token}"]
+ second_factor_authentication_result = @user.authenticate_second_factor(params, secure_session)
+ if !second_factor_authentication_result.ok
+ user_error_key = second_factor_authentication_result.reason == "invalid_security_key" ? :user_second_factors : :security_keys
+ @user.errors.add(user_error_key, :invalid)
+ @error = second_factor_authentication_result.error
+ else
- valid_second_factor = secure_session["second-factor-#{token}"] == "true"
- valid_security_key = secure_session["security-key-#{token}"] == "true"
+ # this must be set because the first call we authenticate e.g. TOTP, and we do
+ # not want to re-authenticate on the second call to change the password as this
+ # will cause a TOTP error saying the code has already been used
+ secure_session["second-factor-#{token}"] = true
+ end
+ end
- if !@user
- @error = I18n.t('password_reset.no_token')
- elsif request.put?
- if !valid_second_factor
- @user.errors.add(:user_second_factors, :invalid)
- @error = I18n.t('login.invalid_second_factor_code')
- elsif !valid_security_key
- @user.errors.add(:security_keys, :invalid)
- @error = I18n.t('login.invalid_security_key')
- elsif @invalid_password = params[:password].blank? || params[:password].size > User.max_password_length
+ if @invalid_password = params[:password].blank? || params[:password].size > User.max_password_length
@user.errors.add(:password, :invalid)
- else
+ end
+
+ # if we have run into no errors then the user is a-ok to
+ # change the password
+ if @user.errors.empty?
@user.password = params[:password]
@user.password_required!
@user.user_auth_tokens.destroy_all
@@ -585,69 +597,45 @@ class UsersController < ApplicationController
respond_to do |format|
format.html do
- if @error
- render layout: 'no_ember'
- else
- Webauthn.stage_challenge(@user, secure_session)
- store_preloaded(
- "password_reset",
- MultiJson.dump(
- {
- is_developer: UsernameCheckerService.is_developer?(@user.email),
- admin: @user.admin?,
- second_factor_required: !valid_second_factor,
- security_key_required: !valid_security_key,
- backup_enabled: @user.backup_codes_enabled?
- }.merge(Webauthn.allowed_credentials(@user, secure_session))
- )
- )
- end
+ return render 'password_reset', layout: 'no_ember' if @error
- return redirect_to(wizard_path) if request.put? && Wizard.user_requires_completion?(@user)
+ Webauthn.stage_challenge(@user, secure_session)
+
+ security_params = {
+ is_developer: UsernameCheckerService.is_developer?(@user.email),
+ admin: @user.admin?,
+ second_factor_required: @user.totp_enabled?,
+ security_key_required: @user.security_keys_enabled?,
+ backup_enabled: @user.backup_codes_enabled?,
+ multiple_second_factor_methods: @user.has_multiple_second_factor_methods?
+ }.merge(Webauthn.allowed_credentials(@user, secure_session))
+
+ store_preloaded("password_reset", MultiJson.dump(security_params))
+
+ return redirect_to(wizard_path) if Wizard.user_requires_completion?(@user)
+
+ render 'password_reset'
end
format.json do
- if request.put?
- if @error || @user&.errors&.any?
- render json: {
- success: false,
- message: @error,
- errors: @user&.errors&.to_hash,
- is_developer: UsernameCheckerService.is_developer?(@user&.email),
- admin: @user&.admin?
- }
- else
- render json: {
- success: true,
- message: @success,
- requires_approval: !Guardian.new(@user).can_access_forum?,
- redirect_to: Wizard.user_requires_completion?(@user) ? wizard_path : nil
- }
- end
+ if @error || @user&.errors&.any?
+ render json: {
+ success: false,
+ message: @error,
+ errors: @user&.errors&.to_hash,
+ is_developer: UsernameCheckerService.is_developer?(@user&.email),
+ admin: @user&.admin?
+ }
else
- if @error || @user&.errors&.any?
- render json: {
- message: @error,
- errors: @user&.errors&.to_hash
- }
- else
- Webauthn.stage_challenge(@user, secure_session) if !valid_security_key && !security_key_credential.present?
- render json: {
- is_developer: UsernameCheckerService.is_developer?(@user.email),
- admin: @user.admin?,
- second_factor_required: !valid_second_factor,
- security_key_required: !valid_security_key,
- backup_enabled: @user.backup_codes_enabled?
- }.merge(Webauthn.allowed_credentials(@user, secure_session))
- end
+ render json: {
+ success: true,
+ message: @success,
+ requires_approval: !Guardian.new(@user).can_access_forum?,
+ redirect_to: Wizard.user_requires_completion?(@user) ? wizard_path : nil
+ }
end
end
end
- rescue ::Webauthn::SecurityKeyError => err
- render json: {
- message: err.message,
- errors: [err.message]
- }
end
def confirm_email_token
@@ -1358,6 +1346,20 @@ class UsersController < ApplicationController
private
+ def password_reset_find_user(token, committing_change:)
+ if EmailToken.valid_token_format?(token)
+ @user = committing_change ? EmailToken.confirm(token) : EmailToken.confirmable(token)&.user
+ if @user
+ secure_session["password-#{token}"] = @user.id
+ else
+ user_id = secure_session["password-#{token}"].to_i
+ @user = User.find(user_id) if user_id > 0
+ end
+ end
+
+ @error = I18n.t('password_reset.no_token') if !@user
+ end
+
def respond_to_suspicious_request
if suspicious?(params)
render json: {
diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb
index aaead3bf62..96f027880c 100644
--- a/app/controllers/users_email_controller.rb
+++ b/app/controllers/users_email_controller.rb
@@ -56,11 +56,26 @@ class UsersEmailController < ApplicationController
redirect_url = path("/u/confirm-new-email/#{params[:token]}")
- if !@error && @user.totp_enabled? && !@user.authenticate_second_factor(params[:second_factor_token], params[:second_factor_method].to_i)
- RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
- flash[:invalid_second_factor] = true
- redirect_to redirect_url
- return
+ RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! if params[:second_factor_token].present?
+
+ if !@error
+ # this is needed becase the form posts this field as JSON and it can be a
+ # hash when authenticatong security key.
+ if params[:second_factor_method].to_i == UserSecondFactor.methods[:security_key]
+ begin
+ params[:second_factor_token] = JSON.parse(params[:second_factor_token])
+ rescue JSON::ParserError
+ raise Discourse::InvalidParameters
+ end
+ end
+
+ second_factor_authentication_result = @user.authenticate_second_factor(params, secure_session)
+ if !second_factor_authentication_result.ok
+ flash[:invalid_second_factor] = true
+ flash[:invalid_second_factor_message] = second_factor_authentication_result.error
+ redirect_to redirect_url
+ return
+ end
end
if !@error
@@ -92,15 +107,22 @@ class UsersEmailController < ApplicationController
end
@show_invalid_second_factor_error = flash[:invalid_second_factor]
+ @invalid_second_factor_message = flash[:invalid_second_factor_message]
if !@error
- if @user.totp_enabled?
- @backup_codes_enabled = @user.backup_codes_enabled?
- if params[:show_backup].to_s == "true" && @backup_codes_enabled
- @show_backup_codes = true
- else
+ @backup_codes_enabled = @user.backup_codes_enabled?
+ if params[:show_backup].to_s == "true" && @backup_codes_enabled
+ @show_backup_codes = true
+ else
+ if @user.totp_enabled?
@show_second_factor = true
end
+ if @user.security_keys_enabled?
+ Webauthn.stage_challenge(@user, secure_session)
+ @show_security_key = params[:show_totp].to_s == "true" ? false : true
+ @security_key_challenge = Webauthn.challenge(@user, secure_session)
+ @security_key_allowed_credential_ids = Webauthn.allowed_credentials(@user, secure_session)[:allowed_credential_ids]
+ end
end
@to_email = @change_request.new_email
diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb
index 235242339f..b8362cae73 100644
--- a/app/models/concerns/second_factor_manager.rb
+++ b/app/models/concerns/second_factor_manager.rb
@@ -5,6 +5,10 @@ module SecondFactorManager
extend ActiveSupport::Concern
+ SecondFactorAuthenticationResult = Struct.new(
+ :ok, :error, :reason, :backup_enabled, :security_key_enabled, :totp_enabled, :multiple_second_factor_methods
+ )
+
def create_totp(opts = {})
require_rotp
UserSecondFactor.create!({
@@ -67,25 +71,121 @@ module SecondFactorManager
self&.security_keys.where(factor_type: UserSecurityKey.factor_types[:second_factor], enabled: true).exists?
end
+ def has_any_second_factor_methods_enabled?
+ totp_enabled? || security_keys_enabled?
+ end
+
def has_multiple_second_factor_methods?
- security_keys_enabled? && (totp_enabled? || backup_codes_enabled?)
+ security_keys_enabled? && totp_or_backup_codes_enabled?
+ end
+
+ def totp_or_backup_codes_enabled?
+ totp_enabled? || backup_codes_enabled?
+ end
+
+ def only_security_keys_enabled?
+ security_keys_enabled? && !totp_or_backup_codes_enabled?
+ end
+
+ def only_totp_or_backup_codes_enabled?
+ !security_keys_enabled? && totp_or_backup_codes_enabled?
end
def remaining_backup_codes
self&.user_second_factors&.backup_codes&.count
end
- def authenticate_second_factor(token, second_factor_method)
- if second_factor_method == UserSecondFactor.methods[:totp]
- authenticate_totp(token)
- elsif second_factor_method == UserSecondFactor.methods[:backup_codes]
- authenticate_backup_code(token)
- elsif second_factor_method == UserSecondFactor.methods[:security_key]
- # some craziness has happened if we have gotten here...like the user
- # switching around their second factor types then continuing an already
- # started login attempt
- false
+ def authenticate_second_factor(params, secure_session)
+ ok_result = SecondFactorAuthenticationResult.new(true)
+ return ok_result if !security_keys_enabled? && !totp_or_backup_codes_enabled?
+
+ second_factor_token = params[:second_factor_token]
+ second_factor_method = params[:second_factor_method]&.to_i
+
+ if second_factor_method.blank? || UserSecondFactor.methods[second_factor_method].blank?
+ return invalid_second_factor_method_result
end
+
+ if !valid_second_factor_method_for_user?(second_factor_method)
+ return not_enabled_second_factor_method_result
+ end
+
+ case second_factor_method
+ when UserSecondFactor.methods[:totp]
+ return authenticate_totp(second_factor_token) ? ok_result : invalid_totp_or_backup_code_result
+ when UserSecondFactor.methods[:backup_codes]
+ return authenticate_backup_code(second_factor_token) ? ok_result : invalid_totp_or_backup_code_result
+ when UserSecondFactor.methods[:security_key]
+ return authenticate_security_key(secure_session, second_factor_token) ? ok_result : invalid_security_key_result
+ end
+
+ # if we have gotten down to this point without being
+ # OK or invalid something has gone very weird.
+ invalid_second_factor_method_result
+ rescue ::Webauthn::SecurityKeyError => err
+ invalid_security_key_result(err.message)
+ end
+
+ def valid_second_factor_method_for_user?(method)
+ case method
+ when UserSecondFactor.methods[:totp]
+ return totp_enabled?
+ when UserSecondFactor.methods[:backup_codes]
+ return backup_codes_enabled?
+ when UserSecondFactor.methods[:security_key]
+ return security_keys_enabled?
+ end
+ false
+ end
+
+ def authenticate_security_key(secure_session, security_key_credential)
+ ::Webauthn::SecurityKeyAuthenticationService.new(
+ self,
+ security_key_credential,
+ challenge: Webauthn.challenge(self, secure_session),
+ rp_id: Webauthn.rp_id(self, secure_session),
+ origin: Discourse.base_url
+ ).authenticate_security_key
+ end
+
+ def invalid_totp_or_backup_code_result
+ invalid_second_factor_authentication_result(
+ I18n.t("login.invalid_second_factor_code"),
+ "invalid_second_factor"
+ )
+ end
+
+ def invalid_security_key_result(error_message = nil)
+ invalid_second_factor_authentication_result(
+ error_message || I18n.t("login.invalid_security_key"),
+ "invalid_security_key"
+ )
+ end
+
+ def invalid_second_factor_method_result
+ invalid_second_factor_authentication_result(
+ I18n.t("login.invalid_second_factor_method"),
+ "invalid_second_factor_method"
+ )
+ end
+
+ def not_enabled_second_factor_method_result
+ invalid_second_factor_authentication_result(
+ I18n.t("login.not_enabled_second_factor_method"),
+ "not_enabled_second_factor_method"
+ )
+ end
+
+ def invalid_second_factor_authentication_result(error_message, reason)
+ SecondFactorAuthenticationResult.new(
+ false,
+ error_message,
+ reason,
+ backup_codes_enabled?,
+ security_keys_enabled?,
+ totp_enabled?,
+ has_multiple_second_factor_methods?
+ )
end
def generate_backup_codes
@@ -127,8 +227,9 @@ module SecondFactorManager
codes = self&.user_second_factors&.backup_codes
codes.each do |code|
- stored_code = JSON.parse(code.data)["code_hash"]
- stored_salt = JSON.parse(code.data)["salt"]
+ parsed_data = JSON.parse(code.data)
+ stored_code = parsed_data["code_hash"]
+ stored_salt = parsed_data["salt"]
backup_hash = hash_backup_code(backup_code, stored_salt)
next unless backup_hash == stored_code
diff --git a/app/views/users_email/show_confirm_new_email.html.erb b/app/views/users_email/show_confirm_new_email.html.erb
index fe14cf53df..31108a7ac1 100644
--- a/app/views/users_email/show_confirm_new_email.html.erb
+++ b/app/views/users_email/show_confirm_new_email.html.erb
@@ -21,25 +21,49 @@
<%=form_tag(u_confirm_new_email_path, method: :put) do %>
<%= hidden_field_tag 'token', @token.token %>
+ <%= hidden_field_tag 'second_factor_token', nil, id: 'security-key-credential' %>
+
+
+ <% if @show_invalid_second_factor_error %>
+ <%= @invalid_second_factor_message %>
+ <% end %>
<% if @show_backup_codes %>
+
<%= link_to t("login.second_factor_toggle.totp"), show_backup: "false" %>
+
+ <% elsif @show_security_key %>
+ <%= hidden_field_tag 'security_key_challenge', @security_key_challenge, id: 'security-key-challenge' %>
+ <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:security_key] %>
+ <%= hidden_field_tag 'security_key_allowed_credential_ids', @security_key_allowed_credential_ids, id: 'security-key-allowed-credential-ids' %>
+
+
+ <% if @show_second_factor %>
+ <%= link_to t("login.security_key_alternative"), show_totp: "true" %>
+ <% end %>
+ <% if @backup_codes_enabled %>
+ <%= link_to t("login.second_factor_toggle.backup_code"), show_backup: "true" %>
+ <% end %>
<% elsif @show_second_factor %>
+
<% if @backup_codes_enabled %>
<%= link_to t("login.second_factor_toggle.backup_code"), show_backup: "true" %>
<% end %>
@@ -48,4 +72,11 @@
<% end %>
<%end%>
<% end%>
+
+ <%= preload_script "ember_jquery" %>
+ <%= preload_script "locales/#{I18n.locale}" %>
+ <%= preload_script "locales/i18n" %>
+ <%= preload_script "discourse/lib/webauthn" %>
+ <%= preload_script "confirm-new-email/confirm-new-email" %>
+ <%= preload_script "confirm-new-email/confirm-new-email.no-module" %>
diff --git a/config/application.rb b/config/application.rb
index 5f1246a7d5..e3bc620c5f 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -151,8 +151,8 @@ module Discourse
wizard-start.js
locales/i18n.js
discourse/lib/webauthn.js
- admin-login/admin-login.js
- admin-login/admin-login.no-module.js
+ confirm-new-email/confirm-new-email.js
+ confirm-new-email/confirm-new-email.no-module.js
onpopstate-handler.js
embed-application.js
}
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 75c51c64ac..8ad06adf43 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2319,6 +2319,8 @@ en:
auto_deleted_by_timer: "Automatically deleted by timer."
login:
+ invalid_second_factor_method: "The selected second factor method is invalid."
+ not_enabled_second_factor_method: "The selected second factor method is not enabled for your account."
security_key_description: "When you have your physical security key prepared press the Authenticate with Security Key button below."
security_key_alternative: "Try another way"
security_key_authenticate: "Authenticate with Security Key"
@@ -2364,7 +2366,7 @@ en:
invalid_second_factor_code: "Invalid authentication code. Each code can only be used once."
invalid_security_key: "Invalid security key."
second_factor_toggle:
- totp: "Use an authenticator app instead"
+ totp: "Use an authenticator app or security key instead"
backup_code: "Use a backup code instead"
admin:
diff --git a/config/routes.rb b/config/routes.rb
index 24a0542109..61fadd76d7 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -403,9 +403,9 @@ Discourse::Application.routes.draw do
get "#{root_path}/account-created/resent" => "users#account_created"
get "#{root_path}/account-created/edit-email" => "users#account_created"
- get({ "#{root_path}/password-reset/:token" => "users#password_reset" }.merge(index == 1 ? { as: :password_reset_token } : {}))
+ get({ "#{root_path}/password-reset/:token" => "users#password_reset_show" }.merge(index == 1 ? { as: :password_reset_token } : {}))
get "#{root_path}/confirm-email-token/:token" => "users#confirm_email_token", constraints: { format: 'json' }
- put "#{root_path}/password-reset/:token" => "users#password_reset"
+ put "#{root_path}/password-reset/:token" => "users#password_reset_update"
get "#{root_path}/activate-account/:token" => "users#activate_account"
put({ "#{root_path}/activate-account/:token" => "users#perform_account_activation" }.merge(index == 1 ? { as: 'perform_activate_account' } : {}))
diff --git a/lib/webauthn/security_key_authentication_service.rb b/lib/webauthn/security_key_authentication_service.rb
index 3256c0683f..e33c4d8f0b 100644
--- a/lib/webauthn/security_key_authentication_service.rb
+++ b/lib/webauthn/security_key_authentication_service.rb
@@ -9,7 +9,7 @@ module Webauthn
# the steps followed here. Memoized methods are called in their
# place in the step flow to make the process clearer.
def authenticate_security_key
- return false if @params.blank?
+ return false if @params.blank? || (!@params.is_a?(Hash) && !@params.is_a?(ActionController::Parameters))
# 3. Identify the user being authenticated and verify that this user is the
# owner of the public key credential source credentialSource identified by credential.id:
diff --git a/spec/components/concern/second_factor_manager_spec.rb b/spec/components/concern/second_factor_manager_spec.rb
index 9e6c845caf..36e4f066ce 100644
--- a/spec/components/concern/second_factor_manager_spec.rb
+++ b/spec/components/concern/second_factor_manager_spec.rb
@@ -3,8 +3,16 @@
require 'rails_helper'
RSpec.describe SecondFactorManager do
- fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp) }
- let(:user) { user_second_factor_totp.user }
+ fab!(:user) { Fabricate(:user) }
+ fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user: user) }
+ fab!(:user_security_key) do
+ Fabricate(
+ :user_security_key,
+ user: user,
+ public_key: valid_security_key_data[:public_key],
+ credential_id: valid_security_key_data[:credential_id]
+ )
+ end
fab!(:another_user) { Fabricate(:user) }
fab!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup) }
@@ -78,7 +86,7 @@ RSpec.describe SecondFactorManager do
describe "when user's second factor record is disabled" do
it 'should return false' do
- user.user_second_factors.totps.first.update!(enabled: false)
+ disable_totp
expect(user.totp_enabled?).to eq(false)
end
end
@@ -107,6 +115,252 @@ RSpec.describe SecondFactorManager do
end
end
+ describe "#has_multiple_second_factor_methods?" do
+ context "when security keys and totp are enabled" do
+ it "retrns true" do
+ expect(user.has_multiple_second_factor_methods?).to eq(true)
+ end
+ end
+
+ context "if the totp gets disabled" do
+ it "retrns false" do
+ disable_totp
+ expect(user.has_multiple_second_factor_methods?).to eq(false)
+ end
+ end
+
+ context "if the security key gets disabled" do
+ it "retrns false" do
+ disable_security_key
+ expect(user.has_multiple_second_factor_methods?).to eq(false)
+ end
+ end
+ end
+
+ describe "#only_security_keys_enabled?" do
+ it "returns true if totp disabled and security key enabled" do
+ disable_totp
+ expect(user.only_security_keys_enabled?).to eq(true)
+ end
+ end
+
+ describe "#only_totp_or_backup_codes_enabled?" do
+ it "returns true if totp enabled and security key disabled" do
+ disable_security_key
+ expect(user.only_totp_or_backup_codes_enabled?).to eq(true)
+ end
+ end
+
+ describe "#authenticate_second_factor" do
+ let(:params) { {} }
+ let(:secure_session) { {} }
+
+ context "when neither security keys nor totp/backup codes are enabled" do
+ before do
+ disable_security_key && disable_totp
+ end
+ it "returns OK, because it doesn't need to authenticate" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+
+ context "when only security key is enabled" do
+ before do
+ disable_totp
+ simulate_localhost_webauthn_challenge
+ Webauthn.stage_challenge(user, secure_session)
+ end
+
+ context "when security key params are valid" do
+ let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } }
+ it "returns OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+
+ context "when security key params are invalid" do
+ let(:params) do
+ {
+ second_factor_token: {
+ signature: 'bad',
+ clientData: 'bad',
+ authenticatorData: 'bad',
+ credentialId: 'bad'
+ },
+ second_factor_method: UserSecondFactor.methods[:security_key]
+ }
+ end
+ it "returns not OK" 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"))
+ end
+ end
+ end
+
+ context "when only totp is enabled" do
+ before do
+ disable_security_key
+ end
+
+ context "when totp is valid" do
+ let(:params) do
+ {
+ second_factor_token: user.user_second_factors.totps.first.totp_object.now,
+ second_factor_method: UserSecondFactor.methods[:totp]
+ }
+ end
+ it "returns OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+
+ context "when totp is invalid" do
+ let(:params) do
+ {
+ second_factor_token: "blah",
+ second_factor_method: UserSecondFactor.methods[:totp]
+ }
+ end
+ it "returns not OK" 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"))
+ end
+ end
+ end
+
+ context "when both security keys and totp are enabled" do
+ let(:invalid_method) { 4 }
+ let(:method) { invalid_method }
+
+ before do
+ simulate_localhost_webauthn_challenge
+ Webauthn.stage_challenge(user, secure_session)
+ end
+
+ context "when method selected is invalid" do
+ it "returns an error" 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"))
+ end
+ end
+
+ context "when method selected is TOTP" do
+ let(:method) { UserSecondFactor.methods[:totp] }
+ let(:token) { user.user_second_factors.totps.first.totp_object.now }
+
+ context "when totp params are provided" do
+ let(:params) do
+ {
+ second_factor_token: token,
+ second_factor_method: method
+ }
+ end
+
+ it "validates totp OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+
+ context "when the user does not have TOTP enabled" do
+ let(:token) { 'test' }
+ before do
+ user.totps.destroy_all
+ end
+
+ it "returns an error" 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"))
+ end
+ end
+ end
+ end
+
+ context "when method selected is Security Keys" do
+ let(:method) { UserSecondFactor.methods[:security_key] }
+
+ before do
+ simulate_localhost_webauthn_challenge
+ Webauthn.stage_challenge(user, secure_session)
+ end
+
+ context "when security key params are valid" do
+ let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: method } }
+ it "returns OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+
+ context "when the user does not have security keys enabled" do
+ before do
+ user.security_keys.destroy_all
+ end
+
+ it "returns an error" 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"))
+ end
+ end
+ end
+ end
+
+ context "when method selected is Backup Codes" do
+ let(:method) { UserSecondFactor.methods[:backup_codes] }
+ let!(:backup_code) { Fabricate(:user_second_factor_backup, user: user) }
+
+ context "when backup code params are provided" do
+ let(:params) do
+ {
+ second_factor_token: 'iAmValidBackupCode',
+ second_factor_method: method
+ }
+ end
+
+ context "when backup codes enabled" do
+ it "validates codes OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+
+ context "when backup codes disabled" do
+ before do
+ user.user_second_factors.backup_codes.destroy_all
+ end
+
+ it "returns an error" 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"))
+ end
+ end
+ end
+ end
+
+ context "when no totp params are provided" do
+ let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } }
+
+ it "validates the security key OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+
+ context "when totp params are provided" do
+ let(:params) do
+ {
+ second_factor_token: user.user_second_factors.totps.first.totp_object.now,
+ second_factor_method: UserSecondFactor.methods[:totp]
+ }
+ end
+
+ it "validates totp OK" do
+ expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+ end
+ end
+ end
+ end
+
context 'backup codes' do
describe '#generate_backup_codes' do
it 'should generate and store 10 backup codes' do
@@ -187,4 +441,12 @@ RSpec.describe SecondFactorManager do
end
end
end
+
+ def disable_totp
+ user.user_second_factors.totps.first.update!(enabled: false)
+ end
+
+ def disable_security_key
+ user.security_keys.first.destroy!
+ end
end
diff --git a/spec/lib/webauthn/security_key_authentication_service_spec.rb b/spec/lib/webauthn/security_key_authentication_service_spec.rb
index c8d0a88215..52e18c7988 100644
--- a/spec/lib/webauthn/security_key_authentication_service_spec.rb
+++ b/spec/lib/webauthn/security_key_authentication_service_spec.rb
@@ -62,6 +62,20 @@ describe Webauthn::SecurityKeyAuthenticationService do
expect(security_key.reload.last_used).not_to eq(nil)
end
+ context "when params is blank" do
+ let(:params) { nil }
+ it "returns false with no validation" do
+ expect(subject.authenticate_security_key).to eq(false)
+ end
+ end
+
+ context "when params is not blank and not a hash" do
+ let(:params) { 'test' }
+ it "returns false with no validation" do
+ expect(subject.authenticate_security_key).to eq(false)
+ end
+ end
+
context 'when the credential ID does not match any user security key in the database' do
let(:credential_id) { 'badid' }
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 5497386178..447a08d681 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -160,7 +160,7 @@ RSpec.configure do |config|
config.include MessageBus
config.include RSpecHtmlMatchers
config.include IntegrationHelpers, type: :request
- config.include WebauthnIntegrationHelpers, type: :request
+ config.include WebauthnIntegrationHelpers
config.include SiteSettingsHelpers
config.mock_framework = :mocha
config.order = 'random'
diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb
index 113bf25494..4aaf547e33 100644
--- a/spec/requests/application_controller_spec.rb
+++ b/spec/requests/application_controller_spec.rb
@@ -153,6 +153,42 @@ RSpec.describe ApplicationController do
get "/"
expect(response.status).to eq(200)
end
+
+ context "when enforcing second factor for staff" do
+ before do
+ SiteSetting.enforce_second_factor = "staff"
+ sign_in(admin)
+ end
+
+ context "when the staff member has not enabled TOTP or security keys" do
+ it "redirects the staff to the second factor preferences" do
+ get "/"
+ expect(response).to redirect_to("/u/#{admin.username}/preferences/second-factor")
+ end
+ end
+
+ context "when the staff member has enabled TOTP" do
+ before do
+ Fabricate(:user_second_factor_totp, user: admin)
+ end
+
+ it "does not redirects the staff to set up 2FA" do
+ get "/"
+ expect(response.status).to eq(200)
+ end
+ end
+
+ context "when the staff member has enabled security keys" do
+ before do
+ Fabricate(:user_security_key_with_random_credential, user: admin)
+ end
+
+ it "does not redirects the staff to set up 2FA" do
+ get "/"
+ expect(response.status).to eq(200)
+ end
+ end
+ end
end
describe 'invalid request params' do
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index bd932b16bf..43290fa7a4 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -323,7 +323,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(user.confirm_password?("securepassword")).to eq(false)
end
- context 'when user has second factor enabled' do
+ context 'when user has TOTP enabled' do
before do
user.create_totp(enabled: true)
end
@@ -346,6 +346,29 @@ RSpec.describe Users::OmniauthCallbacksController do
end
end
+ context 'when user has security key enabled' do
+ before do
+ Fabricate(:user_security_key_with_random_credential, user: user)
+ end
+
+ it 'should return the right response' do
+ get "/auth/google_oauth2/callback.json"
+
+ expect(response.status).to eq(302)
+
+ data = JSON.parse(cookies[:authentication_data])
+
+ expect(data["email"]).to eq(user.email)
+ expect(data["omniauth_disallow_totp"]).to eq(true)
+
+ user.update!(email: 'different@user.email')
+ get "/auth/google_oauth2/callback.json"
+
+ expect(response.status).to eq(302)
+ expect(JSON.parse(cookies[:authentication_data])["email"]).to eq(user.email)
+ end
+ end
+
context 'when sso_payload cookie exist' do
before do
SiteSetting.enable_sso_provider = true
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index e6a3556fe0..28afc72be6 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -312,6 +312,22 @@ RSpec.describe SessionController do
end
end
end
+
+ context "if the security_key_param is provided but only TOTP is enabled" do
+ it "does not log in the user" do
+ post "/session/email-login/#{email_token.token}.json", params: {
+ second_factor_token: 'foo',
+ second_factor_method: UserSecondFactor.methods[:totp]
+ }
+
+ expect(response.status).to eq(200)
+
+ expect(JSON.parse(response.body)["error"]).to eq(
+ I18n.t("login.invalid_second_factor_code")
+ )
+ expect(session[:current_user_id]).to eq(nil)
+ end
+ end
end
context "user has only security key enabled" do
@@ -343,7 +359,7 @@ RSpec.describe SessionController do
expect(session[:current_user_id]).to eq(nil)
response_body = JSON.parse(response.body)
expect(response_body['error']).to eq(I18n.t(
- 'login.invalid_second_factor_code'
+ 'login.not_enabled_second_factor_method'
))
end
end
@@ -351,7 +367,7 @@ RSpec.describe SessionController do
it" shows an error message and denies login" do
post "/session/email-login/#{email_token.token}.json", params: {
- security_key_credential: {
+ second_factor_token: {
signature: 'bad_sig',
clientData: 'bad_clientData',
credentialId: 'bad_credential_id',
@@ -375,7 +391,7 @@ RSpec.describe SessionController do
post "/session/email-login/#{email_token.token}.json", params: {
login: user.username,
password: 'myawesomepassword',
- security_key_credential: valid_security_key_auth_post_data,
+ second_factor_token: valid_security_key_auth_post_data,
second_factor_method: UserSecondFactor.methods[:security_key]
}
@@ -387,6 +403,46 @@ RSpec.describe SessionController do
end
end
end
+
+ context "user has security key and totp enabled" do
+ let!(:user_security_key) do
+ Fabricate(
+ :user_security_key,
+ user: user,
+ credential_id: valid_security_key_data[:credential_id],
+ public_key: valid_security_key_data[:public_key]
+ )
+ end
+ let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) }
+
+ it "doesnt allow logging in if the 2fa params are garbled" do
+ post "/session/email-login/#{email_token.token}.json", params: {
+ second_factor_method: UserSecondFactor.methods[:totp],
+ second_factor_token: "blah"
+ }
+
+ expect(response.status).to eq(200)
+ expect(session[:current_user_id]).to eq(nil)
+ response_body = JSON.parse(response.body)
+ expect(response_body['error']).to eq(I18n.t(
+ 'login.invalid_second_factor_code'
+ ))
+ end
+
+ it "doesnt allow login if both of the 2fa params are blank" do
+ post "/session/email-login/#{email_token.token}.json", params: {
+ second_factor_method: UserSecondFactor.methods[:totp],
+ second_factor_token: ""
+ }
+
+ expect(response.status).to eq(200)
+ expect(session[:current_user_id]).to eq(nil)
+ response_body = JSON.parse(response.body)
+ expect(response_body['error']).to eq(I18n.t(
+ 'login.invalid_second_factor_code'
+ ))
+ end
+ end
end
end
@@ -1190,9 +1246,8 @@ RSpec.describe SessionController do
post "/session.json", params: {
login: user.username,
password: 'myawesomepassword',
- security_key_credential: {},
second_factor_token: '99999999',
- second_factor_method: UserSecondFactor.methods[:totp]
+ second_factor_method: UserSecondFactor.methods[:security_key]
}
expect(response.status).to eq(200)
@@ -1200,7 +1255,7 @@ RSpec.describe SessionController do
response_body = JSON.parse(response.body)
expect(response_body["failed"]).to eq("FAILED")
expect(response_body['error']).to eq(I18n.t(
- 'login.invalid_second_factor_code'
+ 'login.invalid_security_key'
))
end
end
@@ -1210,7 +1265,7 @@ RSpec.describe SessionController do
post "/session.json", params: {
login: user.username,
password: 'myawesomepassword',
- security_key_credential: {
+ second_factor_token: {
signature: 'bad_sig',
clientData: 'bad_clientData',
credentialId: 'bad_credential_id',
@@ -1234,7 +1289,7 @@ RSpec.describe SessionController do
post "/session.json", params: {
login: user.username,
password: 'myawesomepassword',
- security_key_credential: valid_security_key_auth_post_data,
+ second_factor_token: valid_security_key_auth_post_data,
second_factor_method: UserSecondFactor.methods[:security_key]
}
@@ -1256,7 +1311,7 @@ RSpec.describe SessionController do
post "/session.json", params: {
login: user.username,
password: 'myawesomepassword',
- security_key_credential: valid_security_key_auth_post_data,
+ second_factor_token: valid_security_key_auth_post_data,
second_factor_method: UserSecondFactor.methods[:security_key]
}
@@ -1265,7 +1320,7 @@ RSpec.describe SessionController do
response_body = JSON.parse(response.body)
expect(response_body["failed"]).to eq("FAILED")
expect(JSON.parse(response.body)['error']).to eq(I18n.t(
- 'login.invalid_second_factor_code'
+ 'login.not_enabled_second_factor_method'
))
end
end
@@ -1279,12 +1334,12 @@ RSpec.describe SessionController do
it 'should return the right response' do
post "/session.json", params: {
login: user.username,
- password: 'myawesomepassword',
+ password: 'myawesomepassword'
}
expect(response.status).to eq(200)
expect(JSON.parse(response.body)['error']).to eq(I18n.t(
- 'login.invalid_second_factor_code'
+ 'login.invalid_second_factor_method'
))
end
end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index f7ba168a88..6fa59dc9d7 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -236,7 +236,7 @@ describe UsersController do
expect(response.status).to eq(200)
expect(response.body).to have_tag("div#data-preloaded") do |element|
json = JSON.parse(element.current_scope.attribute('data-preloaded').value)
- expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":false,"security_key_required":false,"backup_enabled":false}')
+ expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":false,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}')
end
expect(session["password-#{token}"]).to be_blank
@@ -349,7 +349,7 @@ describe UsersController do
expect(response.body).to have_tag("div#data-preloaded") do |element|
json = JSON.parse(element.current_scope.attribute('data-preloaded').value)
- expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":true,"security_key_required":false,"backup_enabled":false}')
+ expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":true,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}')
end
put "/u/password-reset/#{token}", params: {
@@ -420,21 +420,34 @@ describe UsersController do
it 'changes password with valid security key challenge and authentication' do
put "/u/password-reset/#{token}.json", params: {
password: 'hg9ow8yHG32O',
- security_key_credential: valid_security_key_auth_post_data,
+ second_factor_token: valid_security_key_auth_post_data,
second_factor_method: UserSecondFactor.methods[:security_key]
}
user.reload
expect(response.status).to eq(200)
+
expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true)
expect(user.user_auth_tokens.count).to eq(1)
end
+ it "does not change a password if a fake TOTP token is provided" do
+ put "/u/password-reset/#{token}.json", params: {
+ password: 'hg9ow8yHG32O',
+ second_factor_token: 'blah',
+ second_factor_method: UserSecondFactor.methods[:security_key]
+ }
+
+ user.reload
+ expect(response.status).to eq(200)
+ expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false)
+ end
+
context "when security key authentication fails" do
it 'shows an error message and does not change password' do
put "/u/password-reset/#{token}", params: {
password: 'hg9ow8yHG32O',
- security_key_credential: {
+ second_factor_token: {
signature: 'bad',
clientData: 'bad',
authenticatorData: 'bad',
@@ -446,7 +459,7 @@ describe UsersController do
user.reload
expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false)
expect(response.status).to eq(200)
- expect(JSON.parse(response.body)['errors']).to include(I18n.t("webauthn.validation.not_found_error"))
+ expect(response.body).to include(I18n.t("webauthn.validation.not_found_error"))
end
end
end
diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb
index 618a515552..2025ec2c75 100644
--- a/spec/requests/users_email_controller_spec.rb
+++ b/spec/requests/users_email_controller_spec.rb
@@ -115,6 +115,88 @@ describe UsersEmailController do
expect(user.email).to eq("new.n.cool@example.com")
end
end
+
+ context "security key required" do
+ fab!(:user_security_key) do
+ Fabricate(
+ :user_security_key,
+ user: user,
+ credential_id: valid_security_key_data[:credential_id],
+ public_key: valid_security_key_data[:public_key]
+ )
+ end
+
+ before do
+ simulate_localhost_webauthn_challenge
+ end
+
+ it 'requires a security key' do
+ get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+
+ expect(response.status).to eq(200)
+
+ response_body = response.body
+
+ expect(response_body).to include(I18n.t("login.security_key_authenticate"))
+ expect(response_body).to include(I18n.t("login.security_key_description"))
+ end
+
+ context "if the user has a TOTP enabled and wants to use that instead" do
+ before do
+ Fabricate(:user_second_factor_totp, user: user)
+ end
+
+ it 'allows entering the totp code instead' do
+ get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_totp=true"
+
+ expect(response.status).to eq(200)
+
+ response_body = response.body
+
+ expect(response_body).to include(I18n.t("login.second_factor_title"))
+ expect(response_body).not_to include(I18n.t("login.security_key_authenticate"))
+ end
+ end
+
+ it 'adds an error on a security key attempt' do
+ get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+ put "/u/confirm-new-email", params: {
+ token: user.email_tokens.last.token,
+ second_factor_token: "{}",
+ second_factor_method: UserSecondFactor.methods[:security_key]
+ }
+
+ expect(response.status).to eq(302)
+ expect(flash[:invalid_second_factor]).to eq(true)
+ end
+
+ it 'confirms with a correct security key token' do
+ get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+ put "/u/confirm-new-email", params: {
+ second_factor_token: valid_security_key_auth_post_data.to_json,
+ second_factor_method: UserSecondFactor.methods[:security_key],
+ token: user.email_tokens.last.token
+ }
+
+ expect(response.status).to eq(302)
+
+ user.reload
+ expect(user.email).to eq("new.n.cool@example.com")
+ end
+
+ context "if the security key data JSON is garbled" do
+ it "raises an invalid parameters error" do
+ get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+ put "/u/confirm-new-email", params: {
+ second_factor_token: "{someweird: 8notjson}",
+ second_factor_method: UserSecondFactor.methods[:security_key],
+ token: user.email_tokens.last.token
+ }
+
+ expect(response.status).to eq(400)
+ end
+ end
+ end
end
end