From 15fc0e22a0e69cf679ed1e5ab140963fa757fa52 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 21 Jul 2023 15:01:53 -0500 Subject: [PATCH 1/5] Allow setting remembered device preference when logging in with backup codes changelog: Improvements, Remembered Device, Allow setting remembered device preference when logging in with backup codes Co-authored-by: Andrew Duthie --- .../concerns/remember_device_concern.rb | 8 +++---- .../two_factor_authenticatable_methods.rb | 6 +++--- .../backup_code_verification_controller.rb | 3 ++- .../otp_verification_controller.rb | 2 +- .../totp_verification_controller.rb | 2 +- .../webauthn_verification_controller.rb | 2 +- .../users/totp_setup_controller.rb | 2 +- .../users/webauthn_setup_controller.rb | 2 +- .../backup_code_verification/show.html.erb | 10 +++++++++ ...ackup_code_verification_controller_spec.rb | 21 +++++++++++++++++++ 10 files changed, 45 insertions(+), 13 deletions(-) diff --git a/app/controllers/concerns/remember_device_concern.rb b/app/controllers/concerns/remember_device_concern.rb index 3eee6b1cc47..62c19f2c053 100644 --- a/app/controllers/concerns/remember_device_concern.rb +++ b/app/controllers/concerns/remember_device_concern.rb @@ -1,12 +1,12 @@ module RememberDeviceConcern extend ActiveSupport::Concern - def save_user_opted_remember_device_pref - cookies.encrypted[:user_opted_remember_device_preference] = params[:remember_device] + def save_user_opted_remember_device_pref(remember_device_preference) + cookies.encrypted[:user_opted_remember_device_preference] = remember_device_preference end - def save_remember_device_preference - return if params[:remember_device] != '1' && params[:remember_device] != 'true' + def save_remember_device_preference(remember_device_preference) + return if remember_device_preference != '1' && remember_device_preference != 'true' cookies.encrypted[:remember_device] = { value: RememberDeviceCookie.new(user_id: current_user.id, created_at: Time.zone.now).to_json, expires: remember_device_cookie_expiration, diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index d23e5aec323..d2f88563ab8 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -85,9 +85,9 @@ def reset_attempt_count_if_user_no_longer_locked_out ).call end - def handle_remember_device - save_user_opted_remember_device_pref - save_remember_device_preference + def handle_remember_device_preference(remember_device_preference) + save_user_opted_remember_device_pref(remember_device_preference) + save_remember_device_preference(remember_device_preference) end # Method will be renamed in the next refactor. diff --git a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb index ac95d916b83..231ca2b780d 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -61,6 +61,7 @@ def handle_invalid_backup_code def handle_result(result) if result.success? + handle_remember_device_preference(backup_code_params[:remember_device]) handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE, ) @@ -72,7 +73,7 @@ def handle_result(result) end def backup_code_params - params.require(:backup_code_verification_form).permit :backup_code + params.require(:backup_code_verification_form).permit(:backup_code, :remember_device) end def handle_valid_backup_code diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 9804a65166f..6a3697aa3bf 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,7 +21,7 @@ def create result = otp_verification_form.submit post_analytics(result) if result.success? - handle_remember_device + handle_remember_device_preference(params[:remember_device]) if UserSessionContext.confirmation_context?(context) handle_valid_confirmation_otp diff --git a/app/controllers/two_factor_authentication/totp_verification_controller.rb b/app/controllers/two_factor_authentication/totp_verification_controller.rb index 9190fa183ed..d55520cfdec 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -26,7 +26,7 @@ def create handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP, ) - handle_remember_device + handle_remember_device_preference(params[:remember_device]) redirect_to after_sign_in_path_for(current_user) else handle_invalid_otp(context: context, type: 'totp') diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index f567c10b06c..71d86ab312d 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -47,7 +47,7 @@ def handle_valid_webauthn auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, ) end - handle_remember_device + handle_remember_device_preference(params[:remember_device]) redirect_to after_sign_in_path_for(current_user) end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index f9325094da2..a1bbbfd6bc8 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -82,7 +82,7 @@ def process_valid_code handle_valid_verification_for_confirmation_context( auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP, ) - handle_remember_device + handle_remember_device_preference(params[:remember_device]) flash[:success] = t('notices.totp_configured') user_session.delete(:new_totp_secret) redirect_to next_setup_path || after_mfa_setup_path diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 83b35aeeb5a..7960ec0219b 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -155,7 +155,7 @@ def process_valid_webauthn(form) platform_authenticator: form.platform_authenticator?, enabled_mfa_methods_count: mfa_user.enabled_mfa_methods_count, ) - handle_remember_device + handle_remember_device_preference(params[:remember_device]) if form.platform_authenticator? handle_valid_verification_for_confirmation_context( auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, diff --git a/app/views/two_factor_authentication/backup_code_verification/show.html.erb b/app/views/two_factor_authentication/backup_code_verification/show.html.erb index 7516e477e25..9626d5e4345 100644 --- a/app/views/two_factor_authentication/backup_code_verification/show.html.erb +++ b/app/views/two_factor_authentication/backup_code_verification/show.html.erb @@ -12,6 +12,16 @@ html: { autocomplete: 'off', method: :post }, ) do |f| %> <%= render 'partials/backup_code/entry_fields', f: f, attribute_name: :backup_code %> + <%= f.input( + :remember_device, + as: :boolean, + label: t('forms.messages.remember_device'), + wrapper_html: { class: 'margin-top-2' }, + input_html: { + class: 'usa-checkbox__input--bordered', + checked: @presenter.remember_device_box_checked?, + }, + ) %> <%= f.submit t('forms.buttons.submit.default'), class: 'display-block margin-y-5' %> <% end %> diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 42ea35f41fa..3f70e0a1401 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -50,6 +50,27 @@ expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end + context 'with remember_device in the params' do + it 'saves an encrypted cookie' do + remember_device_cookie = instance_double(RememberDeviceCookie) + allow(remember_device_cookie).to receive(:to_json).and_return('asdf1234') + allow(RememberDeviceCookie).to receive(:new).and_return(remember_device_cookie) + + stub_sign_in_before_2fa(user) + post( + :create, + params: { + backup_code_verification_form: { + backup_code: backup_codes.first, + remember_device: '1', + }, + }, + ) + + expect(cookies.encrypted[:remember_device]).to eq('asdf1234') + end + end + it 'tracks the valid authentication event when there are exisitng codes' do freeze_time do stub_sign_in_before_2fa(user) From b8250fb4508f16e550f93ba8ab8cad6d359b0b9c Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 21 Jul 2023 15:32:33 -0500 Subject: [PATCH 2/5] fix spacing Co-authored-by: Andrew Duthie --- app/views/partials/backup_code/_entry_fields.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/partials/backup_code/_entry_fields.html.erb b/app/views/partials/backup_code/_entry_fields.html.erb index b3919abaed8..4fa5131e844 100644 --- a/app/views/partials/backup_code/_entry_fields.html.erb +++ b/app/views/partials/backup_code/_entry_fields.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.input attribute_name, label: t('forms.two_factor.backup_code'), input_html: { autocapitalize: 'none', From 07c64e6557a9bb517d5cb5f936393ae988375c37 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 21 Jul 2023 16:08:49 -0500 Subject: [PATCH 3/5] Update spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb Co-authored-by: Zach Margolis --- .../backup_code_verification_controller_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 3f70e0a1401..8c5c90ff505 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -52,8 +52,7 @@ context 'with remember_device in the params' do it 'saves an encrypted cookie' do - remember_device_cookie = instance_double(RememberDeviceCookie) - allow(remember_device_cookie).to receive(:to_json).and_return('asdf1234') + remember_device_cookie = instance_double(RememberDeviceCookie, to_json: 'asdf1234') allow(RememberDeviceCookie).to receive(:new).and_return(remember_device_cookie) stub_sign_in_before_2fa(user) From 7f3758355a3e48edfb097ee9b9efc4b30a2f8114 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 21 Jul 2023 16:19:59 -0500 Subject: [PATCH 4/5] remove stubbing of remember device cookie --- ...ackup_code_verification_controller_spec.rb | 36 ++++++---- .../otp_verification_controller_spec.rb | 68 ++++++++++++------- 2 files changed, 65 insertions(+), 39 deletions(-) diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 8c5c90ff505..4fb4a09ddd1 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -52,21 +52,31 @@ context 'with remember_device in the params' do it 'saves an encrypted cookie' do - remember_device_cookie = instance_double(RememberDeviceCookie, to_json: 'asdf1234') - allow(RememberDeviceCookie).to receive(:new).and_return(remember_device_cookie) - stub_sign_in_before_2fa(user) - post( - :create, - params: { - backup_code_verification_form: { - backup_code: backup_codes.first, - remember_device: '1', - }, - }, - ) - expect(cookies.encrypted[:remember_device]).to eq('asdf1234') + freeze_time do + expect(cookies.encrypted[:remember_device]).to eq nil + post( + :create, + params: { + backup_code_verification_form: { + backup_code: backup_codes.first, + remember_device: '1', + }, + }, + ) + + remember_device_cookie = RememberDeviceCookie.from_json( + cookies.encrypted[:remember_device], + ) + expiration_interval = IdentityConfig.store.remember_device_expiration_hours_aal_1.hours + expect( + remember_device_cookie.valid_for_user?( + user: user, + expiration_interval: expiration_interval, + ), + ).to eq true + end end end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index c92f8d16db2..fe373f8ffac 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -321,20 +321,28 @@ context 'with remember_device in the params' do it 'saves an encrypted cookie' do - remember_device_cookie = instance_double(RememberDeviceCookie) - allow(remember_device_cookie).to receive(:to_json).and_return('asdf1234') - allow(RememberDeviceCookie).to receive(:new).and_return(remember_device_cookie) - - post( - :create, - params: { - code: subject.current_user.direct_otp, - otp_delivery_preference: 'sms', - remember_device: '1', - }, - ) + freeze_time do + expect(cookies.encrypted[:remember_device]).to eq nil + post( + :create, + params: { + code: subject.current_user.direct_otp, + otp_delivery_preference: 'sms', + remember_device: '1', + }, + ) - expect(cookies.encrypted[:remember_device]).to eq('asdf1234') + remember_device_cookie = RememberDeviceCookie.from_json( + cookies.encrypted[:remember_device], + ) + expiration_interval = IdentityConfig.store.remember_device_expiration_hours_aal_1.hours + expect( + remember_device_cookie.valid_for_user?( + user: subject.current_user, + expiration_interval: expiration_interval, + ), + ).to eq true + end end end @@ -674,20 +682,28 @@ context 'with remember_device in the params' do it 'saves an encrypted cookie' do - remember_device_cookie = instance_double(RememberDeviceCookie) - allow(remember_device_cookie).to receive(:to_json).and_return('asdf1234') - allow(RememberDeviceCookie).to receive(:new).and_return(remember_device_cookie) - - post( - :create, - params: { - code: subject.current_user.direct_otp, - otp_delivery_preference: 'sms', - remember_device: '1', - }, - ) + freeze_time do + expect(cookies.encrypted[:remember_device]).to eq nil + post( + :create, + params: { + code: subject.current_user.direct_otp, + otp_delivery_preference: 'sms', + remember_device: '1', + }, + ) - expect(cookies.encrypted[:remember_device]).to eq('asdf1234') + remember_device_cookie = RememberDeviceCookie.from_json( + cookies.encrypted[:remember_device], + ) + expiration_interval = IdentityConfig.store.remember_device_expiration_hours_aal_1.hours + expect( + remember_device_cookie.valid_for_user?( + user: subject.current_user, + expiration_interval: expiration_interval, + ), + ).to eq true + end end end From a2277ab2e72203b1b8c97d052ddb8342fd205ab4 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 24 Jul 2023 09:16:29 -0500 Subject: [PATCH 5/5] fix spec --- .../two_factor_authentication/backup_code_sign_up_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/two_factor_authentication/backup_code_sign_up_spec.rb b/spec/features/two_factor_authentication/backup_code_sign_up_spec.rb index caa558dae38..0071e8826f6 100644 --- a/spec/features/two_factor_authentication/backup_code_sign_up_spec.rb +++ b/spec/features/two_factor_authentication/backup_code_sign_up_spec.rb @@ -39,6 +39,7 @@ expect(page).to \ have_content t('two_factor_authentication.login_options.backup_code_info') visit login_two_factor_backup_code_path + uncheck(t('forms.messages.remember_device')) fill_in :backup_code_verification_form_backup_code, with: codes[index] click_on 'Submit' if index == BackupCodeGenerator::NUMBER_OF_CODES - 1