FEATURE: Centralized 2FA page (#15377)

2FA support in Discourse was added and grown gradually over the years: we first
added support for TOTP for logins, then we implemented backup codes, and last
but not least, security keys. 2FA usage was initially limited to logging in,
but it has been expanded and we now require 2FA for risky actions such as
adding a new admin to the site.

As a result of this gradual growth of the 2FA system, technical debt has
accumulated to the point where it has become difficult to require 2FA for more
actions. We now have 5 different 2FA UI implementations and each one has to
support all 3 2FA methods (TOTP, backup codes, and security keys) which makes
it difficult to maintain a consistent UX for these different implementations.
Moreover, there is a lot of repeated logic in the server-side code behind these
5 UI implementations which hinders maintainability even more.

This commit is the first step towards repaying the technical debt: it builds a
system that centralizes as much as possible of the 2FA server-side logic and
UI. The 2 main components of this system are:

1. A dedicated page for 2FA with support for all 3 methods.
2. A reusable server-side class that centralizes the 2FA logic (the
`SecondFactor::AuthManager` class).

From a top-level view, the 2FA flow in this new system looks like this:

1. User initiates an action that requires 2FA;

2. Server is aware that 2FA is required for this action, so it redirects the
user to the 2FA page if the user has a 2FA method, otherwise the action is
performed.

3. User submits the 2FA form on the page;

4. Server validates the 2FA and if it's successful, the action is performed and
the user is redirected to the previous page.

A more technically-detailed explanation/documentation of the new system is
available as a comment at the top of the `lib/second_factor/auth_manager.rb`
file. Please note that the details are not set in stone and will likely change
in the future, so please don't use the system in your plugins yet.

Since this is a new system that needs to be tested, we've decided to migrate
only the 2FA for adding a new admin to the new system at this time (in this
commit). Our plan is to gradually migrate the remaining 2FA implementations to
the new system.

For screenshots of the 2FA page, see PR #15377 on GitHub.
This commit is contained in:
Osama Sayegh
2022-02-17 12:12:59 +03:00
committed by GitHub
parent c71c107649
commit dd6ec65061
27 changed files with 1673 additions and 29 deletions

View File

@ -6,6 +6,10 @@ class SessionController < ApplicationController
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)
skip_before_action :check_xhr, only: %i(second_factor_auth_show)
requires_login only: [:second_factor_auth_show, :second_factor_auth_perform]
ACTIVATE_USER_KEY = "activate_user"
def csrf
@ -424,6 +428,113 @@ class SessionController < ApplicationController
render layout: 'no_ember', locals: { hide_auth_buttons: true }
end
def second_factor_auth_show
user = current_user
nonce = params.require(:nonce)
challenge = nil
error_key = nil
status_code = 200
begin
challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session)
rescue SecondFactor::BadChallenge => exception
error_key = exception.error_translation_key
status_code = exception.status_code
end
json = {}
if challenge
json.merge!(
totp_enabled: user.totp_enabled?,
backup_enabled: user.backup_codes_enabled?,
allowed_methods: challenge[:allowed_methods]
)
if user.security_keys_enabled?
Webauthn.stage_challenge(user, secure_session)
json.merge!(Webauthn.allowed_credentials(user, secure_session))
json[:security_keys_enabled] = true
else
json[:security_keys_enabled] = false
end
else
json[:error] = I18n.t(error_key)
end
respond_to do |format|
format.html do
store_preloaded("2fa_challenge_data", MultiJson.dump(json))
raise ApplicationController::RenderEmpty.new
end
format.json do
render json: json, status: status_code
end
end
end
def second_factor_auth_perform
nonce = params.require(:nonce)
challenge = nil
error_key = nil
status_code = 200
begin
challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session)
rescue SecondFactor::BadChallenge => exception
error_key = exception.error_translation_key
status_code = exception.status_code
end
if error_key
json = failed_json.merge(
ok: false,
error: I18n.t(error_key),
reason: "challenge_not_found_or_expired"
)
render json: failed_json.merge(json), status: status_code
return
end
# no proper error messages for these cases because the only way they can
# happen is if someone is messing with us.
# the first one can only happen if someone disables a 2FA method after
# they're redirected to the 2fa page and then uses the same method they've
# disabled.
second_factor_method = params[:second_factor_method].to_i
if !current_user.valid_second_factor_method_for_user?(second_factor_method)
raise Discourse::InvalidAccess.new
end
# and this happens if someone tries to use a 2FA method that's not accepted
# for the action they're trying to perform. e.g. using backup codes to
# grant someone admin status.
if !challenge[:allowed_methods].include?(second_factor_method)
raise Discourse::InvalidAccess.new
end
if !challenge[:successful]
rate_limit_second_factor!(current_user)
second_factor_auth_result = current_user.authenticate_second_factor(params, secure_session)
if second_factor_auth_result.ok
challenge[:successful] = true
challenge[:generated_at] += 1.minute.to_i
secure_session["current_second_factor_auth_challenge"] = challenge.to_json
else
error_json = second_factor_auth_result
.to_h
.deep_symbolize_keys
.slice(:ok, :error, :reason)
.merge(failed_json)
render json: error_json, status: 400
return
end
end
render json: {
ok: true,
callback_method: challenge[:callback_method],
callback_path: challenge[:callback_path],
redirect_path: challenge[:redirect_path]
}, status: 200
end
def forgot_password
params.require(:login)