0
0
Fork 0

Add WebAuthn as an alternative 2FA method (#14466)

* feat: add possibility of adding WebAuthn security keys to use as 2FA

This adds a basic UI for enabling WebAuthn 2FA. We did a little refactor
to the Settings page for editing the 2FA methods – now it will list the
methods that are available to the user (TOTP and WebAuthn) and from
there they'll be able to add or remove any of them.
Also, it's worth mentioning that for enabling WebAuthn it's required to
have TOTP enabled, so the first time that you go to the 2FA Settings
page, you'll be asked to set it up.
This work was inspired by the one donde by Github in their platform, and
despite it could be approached in different ways, we decided to go with
this one given that we feel that this gives a great UX.

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>

* feat: add request for WebAuthn as second factor at login if enabled

This commits adds the feature for using WebAuthn as a second factor for
login when enabled.
If users have WebAuthn enabled, now a page requesting for the use of a
WebAuthn credential for log in will appear, although a link redirecting
to the old page for logging in using a two-factor code will also be
present.

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>

* feat: add possibility of deleting WebAuthn Credentials

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>

* feat: disable WebAuthn when an Admin disables 2FA for a user

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>

* feat: remove ability to disable TOTP leaving only WebAuthn as 2FA

Following examples form other platforms like Github, we decided to make
Webauthn 2FA secondary to 2FA with TOTP, so that we removed the
possibility of removing TOTP authentication only, leaving users with
just WEbAuthn as 2FA. Instead, users will have to click on 'Disable 2FA'
in order to remove second factor auth.
The reason for WebAuthn being secondary to TOPT is that in that way,
users will still be able to log in using their code from their phone's
application if they don't have their security keys with them – or maybe
even lost them.

* We had to change a little the flow for setting up TOTP, given that now
  it's possible to setting up again if you already had TOTP, in order to
  let users modify their authenticator app – given that now it's not
  possible for them to disable TOTP and set it up again with another
  authenticator app.
  So, basically, now instead of storing the new `otp_secret` in the
  user, we store it in the session until the process of set up is
  finished.
  This was because, as it was before, when users clicked on 'Edit' in
  the new two-factor methods lists page, but then went back without
  finishing the flow, their `otp_secret` had been changed therefore
  invalidating their previous authenticator app, making them unable to
  log in again using TOTP.

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>

* refactor: fix eslint errors

The PR build was failing given that linting returning some errors.
This commit attempts to fix them.

* refactor: normalize i18n translations

The build was failing given that i18n translations files were not
normalized.
This commits fixes that.

* refactor: avoid having the webauthn gem locked to a specific version

* refactor: use symbols for routes without '/'

* refactor: avoid sending webauthn disabled email when 2FA is disabled

When an admins disable 2FA for users, we were sending two mails
to them, one notifying that 2FA was disabled and the other to notify
that WebAuthn was disabled.
As the second one is redundant since the first email includes it, we can
remove it and send just one email to users.

* refactor: avoid creating new env variable for webauthn_origin config

* refactor: improve flash error messages for webauthn pages

Co-authored-by: Facundo Padula <facundo.padula@cedarcode.com>
This commit is contained in:
santiagorodriguez96 2020-08-24 11:46:27 -03:00 committed by GitHub
parent a3ec9af9b0
commit e8d41bc2fe
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
53 changed files with 1851 additions and 395 deletions

View file

@ -5,8 +5,6 @@ require 'rails_helper'
describe Settings::TwoFactorAuthentication::ConfirmationsController do
render_views
let(:user) { Fabricate(:user, email: 'local-part@domain', otp_secret: 'thisisasecretforthespecofnewview') }
let(:user_without_otp_secret) { Fabricate(:user, email: 'local-part@domain') }
shared_examples 'renders :new' do
it 'renders the new view' do
@ -20,87 +18,101 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do
end
end
describe 'GET #new' do
context 'when signed in' do
subject do
sign_in user, scope: :user
get :new, session: { challenge_passed_at: Time.now.utc }
end
[true, false].each do |with_otp_secret|
let(:user) { Fabricate(:user, email: 'local-part@domain', otp_secret: with_otp_secret ? 'oldotpsecret' : nil) }
include_examples 'renders :new'
end
it 'redirects if not signed in' do
get :new
expect(response).to redirect_to('/auth/sign_in')
end
it 'redirects if user do not have otp_secret' do
sign_in user_without_otp_secret, scope: :user
get :new, session: { challenge_passed_at: Time.now.utc }
expect(response).to redirect_to('/settings/two_factor_authentication')
end
end
describe 'POST #create' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when form_two_factor_confirmation parameter is not provided' do
it 'raises ActionController::ParameterMissing' do
post :create, params: {}, session: { challenge_passed_at: Time.now.utc }
expect(response).to have_http_status(400)
end
end
describe 'when creation succeeds' do
it 'renders page with success' do
otp_backup_codes = user.generate_otp_backup_codes!
expect_any_instance_of(User).to receive(:generate_otp_backup_codes!) do |value|
expect(value).to eq user
otp_backup_codes
end
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, arg|
expect(value).to eq user
expect(arg).to eq '123456'
true
end
post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }, session: { challenge_passed_at: Time.now.utc }
expect(assigns(:recovery_codes)).to eq otp_backup_codes
expect(flash[:notice]).to eq 'Two-factor authentication successfully enabled'
expect(response).to have_http_status(200)
expect(response).to render_template('settings/two_factor_authentication/recovery_codes/index')
end
end
describe 'when creation fails' do
describe 'GET #new' do
context 'when signed in and a new otp secret has been setted in the session' do
subject do
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, arg|
expect(value).to eq user
expect(arg).to eq '123456'
false
end
post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }, session: { challenge_passed_at: Time.now.utc }
end
it 'renders the new view' do
subject
expect(response.body).to include 'The entered code was invalid! Are server time and device time correct?'
sign_in user, scope: :user
get :new, session: { challenge_passed_at: Time.now.utc, new_otp_secret: 'thisisasecretforthespecofnewview' }
end
include_examples 'renders :new'
end
it 'redirects if not signed in' do
get :new
expect(response).to redirect_to('/auth/sign_in')
end
it 'redirects if a new otp_secret has not been setted in the session' do
sign_in user, scope: :user
get :new, session: { challenge_passed_at: Time.now.utc }
expect(response).to redirect_to('/settings/otp_authentication')
end
end
context 'when not signed in' do
it 'redirects if not signed in' do
post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }
expect(response).to redirect_to('/auth/sign_in')
describe 'POST #create' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when form_two_factor_confirmation parameter is not provided' do
it 'raises ActionController::ParameterMissing' do
post :create, params: {}, session: { challenge_passed_at: Time.now.utc, new_otp_secret: 'thisisasecretforthespecofnewview' }
expect(response).to have_http_status(400)
end
end
describe 'when creation succeeds' do
it 'renders page with success' do
otp_backup_codes = user.generate_otp_backup_codes!
expect_any_instance_of(User).to receive(:generate_otp_backup_codes!) do |value|
expect(value).to eq user
otp_backup_codes
end
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, code, options|
expect(value).to eq user
expect(code).to eq '123456'
expect(options).to eq({ otp_secret: 'thisisasecretforthespecofnewview' })
true
end
expect do
post :create,
params: { form_two_factor_confirmation: { otp_attempt: '123456' } },
session: { challenge_passed_at: Time.now.utc, new_otp_secret: 'thisisasecretforthespecofnewview' }
end.to change { user.reload.otp_secret }.to 'thisisasecretforthespecofnewview'
expect(assigns(:recovery_codes)).to eq otp_backup_codes
expect(flash[:notice]).to eq 'Two-factor authentication successfully enabled'
expect(response).to have_http_status(200)
expect(response).to render_template('settings/two_factor_authentication/recovery_codes/index')
end
end
describe 'when creation fails' do
subject do
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, code, options|
expect(value).to eq user
expect(code).to eq '123456'
expect(options).to eq({ otp_secret: 'thisisasecretforthespecofnewview' })
false
end
expect do
post :create,
params: { form_two_factor_confirmation: { otp_attempt: '123456' } },
session: { challenge_passed_at: Time.now.utc, new_otp_secret: 'thisisasecretforthespecofnewview' }
end.to not_change { user.reload.otp_secret }
end
it 'renders the new view' do
subject
expect(response.body).to include 'The entered code was invalid! Are server time and device time correct?'
end
include_examples 'renders :new'
end
end
context 'when not signed in' do
it 'redirects if not signed in' do
post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }
expect(response).to redirect_to('/auth/sign_in')
end
end
end
end

View file

@ -0,0 +1,99 @@
# frozen_string_literal: true
require 'rails_helper'
describe Settings::TwoFactorAuthentication::OtpAuthenticationController do
render_views
let(:user) { Fabricate(:user) }
describe 'GET #show' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when user has OTP enabled' do
before do
user.update(otp_required_for_login: true)
end
it 'redirects to two factor authentciation methods list page' do
get :show
expect(response).to redirect_to settings_two_factor_authentication_methods_path
end
end
describe 'when user does not have OTP enabled' do
before do
user.update(otp_required_for_login: false)
end
it 'returns http success' do
get :show
expect(response).to have_http_status(200)
end
end
end
context 'when not signed in' do
it 'redirects' do
get :show
expect(response).to redirect_to new_user_session_path
end
end
end
describe 'POST #create' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when user has OTP enabled' do
before do
user.update(otp_required_for_login: true)
end
describe 'when creation succeeds' do
it 'redirects to code confirmation page without updating user secret and setting otp secret in the session' do
expect do
post :create, session: { challenge_passed_at: Time.now.utc }
end.to not_change { user.reload.otp_secret }
.and change { session[:new_otp_secret] }
expect(response).to redirect_to(new_settings_two_factor_authentication_confirmation_path)
end
end
end
describe 'when user does not have OTP enabled' do
before do
user.update(otp_required_for_login: false)
end
describe 'when creation succeeds' do
it 'redirects to code confirmation page without updating user secret and setting otp secret in the session' do
expect do
post :create, session: { challenge_passed_at: Time.now.utc }
end.to not_change { user.reload.otp_secret }
.and change { session[:new_otp_secret] }
expect(response).to redirect_to(new_settings_two_factor_authentication_confirmation_path)
end
end
end
end
context 'when not signed in' do
it 'redirects to login' do
get :show
expect(response).to redirect_to new_user_session_path
end
end
end
end

View file

@ -0,0 +1,374 @@
# frozen_string_literal: true
require 'rails_helper'
require 'webauthn/fake_client'
describe Settings::TwoFactorAuthentication::WebauthnCredentialsController do
render_views
let(:user) { Fabricate(:user) }
let(:domain) { "#{Rails.configuration.x.use_https ? 'https' : 'http' }://#{Rails.configuration.x.web_domain}" }
let(:fake_client) { WebAuthn::FakeClient.new(domain) }
def add_webauthn_credential(user)
Fabricate(:webauthn_credential, user_id: user.id, nickname: 'USB Key')
end
describe 'GET #new' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
context 'when user has otp enabled' do
before do
user.update(otp_required_for_login: true)
end
it 'returns http success' do
get :new
expect(response).to have_http_status(200)
end
end
context 'when user does not have otp enabled' do
before do
user.update(otp_required_for_login: false)
end
it 'requires otp enabled first' do
get :new
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
end
describe 'GET #index' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
context 'when user has otp enabled' do
before do
user.update(otp_required_for_login: true)
end
context 'when user has webauthn enabled' do
before do
user.update(webauthn_id: WebAuthn.generate_user_id)
add_webauthn_credential(user)
end
it 'returns http success' do
get :index
expect(response).to have_http_status(200)
end
end
context 'when user does not has webauthn enabled' do
it 'redirects to 2FA methods list page' do
get :index
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when user does not have otp enabled' do
before do
user.update(otp_required_for_login: false)
end
it 'requires otp enabled first' do
get :index
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when not signed in' do
it 'redirects to login' do
delete :index
expect(response).to redirect_to new_user_session_path
end
end
end
describe 'GET /options #options' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
context 'when user has otp enabled' do
before do
user.update(otp_required_for_login: true)
end
context 'when user has webauthn enabled' do
before do
user.update(webauthn_id: WebAuthn.generate_user_id)
add_webauthn_credential(user)
end
it 'returns http success' do
get :options
expect(response).to have_http_status(200)
end
it 'stores the challenge on the session' do
get :options
expect(@controller.session[:webauthn_challenge]).to be_present
end
it 'does not change webauthn_id' do
expect { get :options }.to_not change { user.webauthn_id }
end
it "includes existing credentials in list of excluded credentials" do
get :options
excluded_credentials_ids = JSON.parse(response.body)['excludeCredentials'].map { |credential| credential['id'] }
expect(excluded_credentials_ids).to match_array(user.webauthn_credentials.pluck(:external_id))
end
end
context 'when user does not have webauthn enabled' do
it 'returns http success' do
get :options
expect(response).to have_http_status(200)
end
it 'stores the challenge on the session' do
get :options
expect(@controller.session[:webauthn_challenge]).to be_present
end
it 'sets user webauthn_id' do
get :options
expect(user.reload.webauthn_id).to be_present
end
end
end
context 'when user has not enabled otp' do
before do
user.update(otp_required_for_login: false)
end
it 'requires otp enabled first' do
get :options
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when not signed in' do
it 'redirects to login' do
get :options
expect(response).to redirect_to new_user_session_path
end
end
end
describe 'POST #create' do
let(:nickname) { 'SecurityKeyNickname' }
let(:challenge) do
WebAuthn::Credential.options_for_create(
user: { id: user.id, name: user.account.username, display_name: user.account.display_name }
).challenge
end
let(:new_webauthn_credential) { fake_client.create(challenge: challenge) }
context 'when signed in' do
before do
sign_in user, scope: :user
end
context 'when user has enabled otp' do
before do
user.update(otp_required_for_login: true)
end
context 'when user has enabled webauthn' do
before do
user.update(webauthn_id: WebAuthn.generate_user_id)
add_webauthn_credential(user)
end
context 'when creation succeeds' do
it 'returns http success' do
@controller.session[:webauthn_challenge] = challenge
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
expect(response).to have_http_status(200)
end
it 'adds a new credential to user credentials' do
@controller.session[:webauthn_challenge] = challenge
expect do
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
end.to change { user.webauthn_credentials.count }.by(1)
end
it 'does not change webauthn_id' do
@controller.session[:webauthn_challenge] = challenge
expect do
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
end.to_not change { user.webauthn_id }
end
end
context 'when the nickname is already used' do
it 'fails' do
@controller.session[:webauthn_challenge] = challenge
post :create, params: { credential: new_webauthn_credential, nickname: 'USB Key' }
expect(response).to have_http_status(500)
expect(flash[:error]).to be_present
end
end
context 'when the credential already exists' do
before do
user2 = Fabricate(:user)
public_key_credential = WebAuthn::Credential.from_create(new_webauthn_credential)
Fabricate(:webauthn_credential,
user_id: user2.id,
external_id: public_key_credential.id,
public_key: public_key_credential.public_key)
end
it 'fails' do
@controller.session[:webauthn_challenge] = challenge
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
expect(response).to have_http_status(500)
expect(flash[:error]).to be_present
end
end
end
context 'when user have not enabled webauthn' do
context 'creation succeeds' do
it 'creates a webauthn credential' do
@controller.session[:webauthn_challenge] = challenge
expect do
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
end.to change { user.webauthn_credentials.count }.by(1)
end
end
end
end
context 'when user has not enabled otp' do
before do
user.update(otp_required_for_login: false)
end
it 'requires otp enabled first' do
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when not signed in' do
it 'redirects to login' do
post :create, params: { credential: new_webauthn_credential, nickname: nickname }
expect(response).to redirect_to new_user_session_path
end
end
end
describe 'DELETE #destroy' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
context 'when user has otp enabled' do
before do
user.update(otp_required_for_login: true)
end
context 'when user has webauthn enabled' do
before do
user.update(webauthn_id: WebAuthn.generate_user_id)
add_webauthn_credential(user)
end
context 'when deletion succeeds' do
it 'redirects to 2FA methods list and shows flash success' do
delete :destroy, params: { id: user.webauthn_credentials.take.id }
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:success]).to be_present
end
it 'deletes the credential' do
expect do
delete :destroy, params: { id: user.webauthn_credentials.take.id }
end.to change { user.webauthn_credentials.count }.by(-1)
end
end
end
context 'when user does not have webauthn enabled' do
it 'redirects to 2FA methods list and shows flash error' do
delete :destroy, params: { id: '1' }
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when user does not have otp enabled' do
it 'requires otp enabled first' do
delete :destroy, params: { id: '1' }
expect(response).to redirect_to settings_two_factor_authentication_methods_path
expect(flash[:error]).to be_present
end
end
end
context 'when not signed in' do
it 'redirects to login' do
delete :destroy, params: { id: '1' }
expect(response).to redirect_to new_user_session_path
end
end
end
end

View file

@ -0,0 +1,49 @@
# frozen_string_literal: true
require 'rails_helper'
describe Settings::TwoFactorAuthenticationMethodsController do
render_views
let(:user) { Fabricate(:user) }
describe 'GET #index' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when user has enabled otp' do
before do
user.update(otp_required_for_login: true)
end
it 'returns http success' do
get :index
expect(response).to have_http_status(200)
end
end
describe 'when user has not enabled otp' do
before do
user.update(otp_required_for_login: false)
end
it 'redirects to enable otp' do
get :index
expect(response).to redirect_to(settings_otp_authentication_path)
end
end
end
context 'when not signed in' do
it 'redirects' do
get :index
expect(response).to redirect_to '/auth/sign_in'
end
end
end
end

View file

@ -1,125 +0,0 @@
# frozen_string_literal: true
require 'rails_helper'
describe Settings::TwoFactorAuthenticationsController do
render_views
let(:user) { Fabricate(:user) }
describe 'GET #show' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when user requires otp for login already' do
it 'returns http success' do
user.update(otp_required_for_login: true)
get :show
expect(response).to have_http_status(200)
end
end
describe 'when user does not require otp for login' do
it 'returns http success' do
user.update(otp_required_for_login: false)
get :show
expect(response).to have_http_status(200)
end
end
end
context 'when not signed in' do
it 'redirects' do
get :show
expect(response).to redirect_to '/auth/sign_in'
end
end
end
describe 'POST #create' do
context 'when signed in' do
before do
sign_in user, scope: :user
end
describe 'when user requires otp for login already' do
it 'redirects to show page' do
user.update(otp_required_for_login: true)
post :create
expect(response).to redirect_to(settings_two_factor_authentication_path)
end
end
describe 'when creation succeeds' do
it 'updates user secret' do
before = user.otp_secret
post :create, session: { challenge_passed_at: Time.now.utc }
expect(user.reload.otp_secret).not_to eq(before)
expect(response).to redirect_to(new_settings_two_factor_authentication_confirmation_path)
end
end
end
context 'when not signed in' do
it 'redirects' do
get :show
expect(response).to redirect_to '/auth/sign_in'
end
end
end
describe 'POST #destroy' do
before do
user.update(otp_required_for_login: true)
end
context 'when signed in' do
before do
sign_in user, scope: :user
end
it 'turns off otp requirement with correct code' do
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, arg|
expect(value).to eq user
expect(arg).to eq '123456'
true
end
post :destroy, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }
expect(response).to redirect_to(settings_two_factor_authentication_path)
user.reload
expect(user.otp_required_for_login).to eq(false)
end
it 'does not turn off otp if code is incorrect' do
expect_any_instance_of(User).to receive(:validate_and_consume_otp!) do |value, arg|
expect(value).to eq user
expect(arg).to eq '057772'
false
end
post :destroy, params: { form_two_factor_confirmation: { otp_attempt: '057772' } }
user.reload
expect(user.otp_required_for_login).to eq(true)
end
it 'raises ActionController::ParameterMissing if code is missing' do
post :destroy
expect(response).to have_http_status(400)
end
end
it 'redirects if not signed in' do
get :show
expect(response).to redirect_to '/auth/sign_in'
end
end
end