From c5a075b6c2fc8c98ac58449b59ec7ac36fcf82ce Mon Sep 17 00:00:00 2001 From: Nicolas Temciuc Date: Fri, 15 Aug 2025 14:38:08 -0300 Subject: [PATCH] test: update specs to not require OTP for enabling WebAuthn as 2FA Co-authored-by: Santiago Rodriguez --- .../auth/sessions_controller_spec.rb | 4 +- .../webauthn_credentials_controller_spec.rb | 229 ++++++++---------- .../two_factor_authentication_methods_spec.rb | 19 -- .../users/two_factor_authentications_spec.rb | 7 +- 4 files changed, 100 insertions(+), 159 deletions(-) diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 949af2a4259..3b0031354e9 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -349,9 +349,9 @@ RSpec.describe Auth::SessionsController do end end - context 'with WebAuthn and OTP enabled as second factor' do + context 'with WebAuthn enabled as second factor' do let!(:user) do - Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret) + Fabricate(:user, email: 'x@y.com', password: 'abcdefgh') end let!(:webauthn_credential) do diff --git a/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb b/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb index cccf3c51d32..8d16fbb9921 100644 --- a/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb @@ -20,29 +20,10 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController 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 - 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 + expect(response).to have_http_status(200) end end end @@ -53,40 +34,21 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController sign_in user, scope: :user end - context 'when user has otp enabled' do + context 'when user has webauthn enabled' do before do - user.update(otp_required_for_login: true) + user.update(webauthn_id: WebAuthn.generate_user_id) + add_webauthn_credential(user) 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 - 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 + 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 + 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 @@ -110,50 +72,53 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController sign_in user, scope: :user end - context 'when user has otp enabled' do + context 'when user has webauthn enabled' do before do - user.update(otp_required_for_login: true) + user.update(webauthn_id: WebAuthn.generate_user_id) + add_webauthn_credential(user) 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 - it 'includes existing credentials in list of excluded credentials', :aggregate_failures do - expect { get :options }.to_not change(user, :webauthn_id) - - expect(response).to have_http_status(200) - - expect(controller.session[:webauthn_challenge]).to be_present - - excluded_credentials_ids = response.parsed_body['excludeCredentials'].pluck('id') - expect(excluded_credentials_ids).to match_array(user.webauthn_credentials.pluck(:external_id)) - end + expect(response).to have_http_status(200) end - context 'when user does not have webauthn enabled' do - it 'stores the challenge on the session and sets user webauthn_id', :aggregate_failures do - get :options + it 'stores the challenge on the session' do + get :options - expect(response).to have_http_status(200) - expect(controller.session[:webauthn_challenge]).to be_present - expect(user.reload.webauthn_id).to be_present - end + 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 = response.parsed_body['excludeCredentials'].pluck('id') + expect(excluded_credentials_ids).to match_array(user.webauthn_credentials.pluck(:external_id)) 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 + context 'when user does not have webauthn enabled' do + it 'returns http success' do get :options - expect(response).to redirect_to settings_two_factor_authentication_methods_path - expect(flash[:error]).to be_present + 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 @@ -183,29 +148,40 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController sign_in user, scope: :user end - context 'when user has enabled otp' do + context 'when user has enabled webauthn' do before do - user.update(otp_required_for_login: true) + user.update(webauthn_id: WebAuthn.generate_user_id) + add_webauthn_credential(user) end - context 'when user has enabled webauthn' do - before do - user.update(webauthn_id: WebAuthn.generate_user_id) - add_webauthn_credential(user) + 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 and does not change webauthn_id when creation succeeds', :aggregate_failures do + 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) - .and not_change(user, :webauthn_id) - - expect(response).to have_http_status(200) end - it 'fails when the nickname is already used' do + 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' } @@ -213,14 +189,19 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController expect(response).to have_http_status(422) expect(flash[:error]).to be_present end + end - it 'fails when the credential already exists' do + 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: Fabricate(:user).id, + 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 } @@ -230,29 +211,18 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController end end - context 'when user have not enabled webauthn and creation succeeds' do - it 'creates a webauthn credential' do - controller.session[:webauthn_challenge] = challenge + context 'when user have not enabled webauthn' do + context 'when 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) + 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 @@ -270,39 +240,30 @@ RSpec.describe Settings::TwoFactorAuthentication::WebauthnCredentialsController sign_in user, scope: :user end - context 'when user has otp enabled' do + context 'when user has webauthn enabled' do before do - user.update(otp_required_for_login: true) + user.update(webauthn_id: WebAuthn.generate_user_id) + add_webauthn_credential(user) end - context 'when user has webauthn enabled' do - before do - user.update(webauthn_id: WebAuthn.generate_user_id) - add_webauthn_credential(user) - end - - it 'redirects to 2FA methods list and shows flash success and deletes the credential when deletion succeeds', :aggregate_failures do - expect do - delete :destroy, params: { id: user.webauthn_credentials.take.id } - end.to change { user.webauthn_credentials.count }.by(-1) + 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 - 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 + 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 otp enabled' do - it 'requires otp enabled first' do + 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 diff --git a/spec/requests/settings/two_factor_authentication_methods_spec.rb b/spec/requests/settings/two_factor_authentication_methods_spec.rb index 2fda5ce9194..b6d37b65ab3 100644 --- a/spec/requests/settings/two_factor_authentication_methods_spec.rb +++ b/spec/requests/settings/two_factor_authentication_methods_spec.rb @@ -13,23 +13,4 @@ RSpec.describe 'Settings TwoFactorAuthenticationMethods' do end end end - - context 'when signed in' do - let(:user) { Fabricate(:user) } - - before { sign_in user } - - describe 'GET to /settings/two_factor_authentication_methods' do - describe 'when user has not enabled otp' do - before { user.update(otp_required_for_login: false) } - - it 'redirects to enable otp' do - get settings_two_factor_authentication_methods_path - - expect(response) - .to redirect_to(settings_otp_authentication_path) - end - end - end - end end diff --git a/spec/system/admin/users/two_factor_authentications_spec.rb b/spec/system/admin/users/two_factor_authentications_spec.rb index e09bc437b4b..25335ff43c6 100644 --- a/spec/system/admin/users/two_factor_authentications_spec.rb +++ b/spec/system/admin/users/two_factor_authentications_spec.rb @@ -26,15 +26,14 @@ RSpec.describe 'Admin Users TwoFactorAuthentications' do end end - context 'when user has OTP and WebAuthn enabled' do - before { user.update(otp_required_for_login: true, webauthn_id: WebAuthn.generate_user_id) } + context 'when user has WebAuthn enabled' do + before { user.update(webauthn_id: WebAuthn.generate_user_id) } it 'disables OTP and webauthn and redirects to admin account page' do visit admin_account_path(user.account.id) expect { disable_two_factor } - .to change { user.reload.otp_enabled? }.to(false) - .and(change { user.reload.webauthn_enabled? }.to(false)) + .to change { user.reload.webauthn_enabled? }.to(false) expect(page) .to have_title(user.account.pretty_acct) end