diff --git a/app/controllers/two_factor_authentication/recovery_code_verification_controller.rb b/app/controllers/two_factor_authentication/recovery_code_verification_controller.rb index ee44a39c745..0ee20bd7d91 100644 --- a/app/controllers/two_factor_authentication/recovery_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/recovery_code_verification_controller.rb @@ -5,8 +5,13 @@ class RecoveryCodeVerificationController < ApplicationController prepend_before_action :authenticate_user skip_before_action :handle_two_factor_authentication + def show + @recovery_code_form = RecoveryCodeForm.new(current_user) + end + def create - result = RecoveryCodeForm.new(current_user, personal_key).submit + @recovery_code_form = RecoveryCodeForm.new(current_user, personal_key) + result = @recovery_code_form.submit analytics.track_event(Analytics::MULTI_FACTOR_AUTH, result.merge(method: 'recovery code')) @@ -38,7 +43,7 @@ def pii end def personal_key - params[:code][:words].join(' ') + params[:recovery_code_form].require(:code).join(' ') end end end diff --git a/app/controllers/users/reactivate_profile_controller.rb b/app/controllers/users/reactivate_profile_controller.rb index e49a1b5c57d..ae4731b5c6d 100644 --- a/app/controllers/users/reactivate_profile_controller.rb +++ b/app/controllers/users/reactivate_profile_controller.rb @@ -6,7 +6,6 @@ def index def create @reactivate_profile_form = build_reactivate_profile_form - if @reactivate_profile_form.submit(flash) redirect_to profile_path else @@ -19,7 +18,7 @@ def create def build_reactivate_profile_form ReactivateProfileForm.new( current_user, - params[:reactivate_profile_form].permit(:recovery_code, :password) + params[:reactivate_profile_form].permit(:password, recovery_code: []) ) end end diff --git a/app/forms/reactivate_profile_form.rb b/app/forms/reactivate_profile_form.rb index f6c79f70640..0bac6abedfa 100644 --- a/app/forms/reactivate_profile_form.rb +++ b/app/forms/reactivate_profile_form.rb @@ -10,8 +10,11 @@ class ReactivateProfileForm attr_reader :user def initialize(user, attrs = {}) + attrs[:recovery_code] ||= [] @user = user super attrs + + @recovery_code = recovery_code.join(' ') end def submit(flash) diff --git a/app/forms/recovery_code_form.rb b/app/forms/recovery_code_form.rb index 4a1ae60c81d..97fa92cf9f3 100644 --- a/app/forms/recovery_code_form.rb +++ b/app/forms/recovery_code_form.rb @@ -1,5 +1,9 @@ class RecoveryCodeForm - def initialize(user, code) + include ActiveModel::Model + + attr_accessor :code + + def initialize(user, code = []) @user = user @code = code end @@ -14,7 +18,7 @@ def submit private - attr_reader :user, :code, :success + attr_reader :user, :success def valid_recovery_code? recovery_code_generator = RecoveryCodeGenerator.new(user) diff --git a/app/inputs/array_input.rb b/app/inputs/array_input.rb new file mode 100644 index 00000000000..e08b96709de --- /dev/null +++ b/app/inputs/array_input.rb @@ -0,0 +1,34 @@ +class ArrayInput < SimpleForm::Inputs::StringInput + # simple_form throws a deprecation warning noting that the + # method signature should be changed to add this parameter + def input(_wrapper_options) + input_html_options[:type] ||= input_type + + existing_values = Array(object.public_send(attribute_name)).map do + build + end + + existing_values.push(build) if existing_values.empty? + + safe_join(existing_values) + end + + def input_type + :text + end + + def build + options = { + value: nil, + class: 'block col-12 field', + autocomplete: 'off', + name: "#{object_name}[#{attribute_name}][]", + } + + builder.text_field(nil, input_html_options.merge(options)) + end + + def builder + @builder ||= :builder + end +end diff --git a/app/views/partials/personal_key/_entry_fields.html.slim b/app/views/partials/personal_key/_entry_fields.html.slim new file mode 100644 index 00000000000..1aeaf31d575 --- /dev/null +++ b/app/views/partials/personal_key/_entry_fields.html.slim @@ -0,0 +1,7 @@ +.mb3 + label.block.bold#personal-key-label + = t('simple_form.required.html') + t('forms.two_factor.recovery_code') + - Figaro.env.recovery_code_length.to_i.times do + = f.input attribute_name, as: :array, label: false, + input_html: { required: true, 'aria-labelledby': 'personal-key-label' } + end diff --git a/app/views/two_factor_authentication/recovery_code_verification/show.html.slim b/app/views/two_factor_authentication/recovery_code_verification/show.html.slim index 9a6ac2574eb..203415b645e 100644 --- a/app/views/two_factor_authentication/recovery_code_verification/show.html.slim +++ b/app/views/two_factor_authentication/recovery_code_verification/show.html.slim @@ -3,16 +3,10 @@ h1.h3.my0 = t('devise.two_factor_authentication.recovery_code_header_text') p.mt-tiny.mb0 = t('devise.two_factor_authentication.recovery_code_prompt') -= form_tag(:login_two_factor_recovery_code, method: :post, role: 'form', class: 'mt3 sm-mt4') do - label.block.bold#personal-key-label - = t('simple_form.required.html') + t('forms.two_factor.recovery_code') - - - Figaro.env.recovery_code_length.to_i.times do |_| - .mb2 - = block_text_field_tag 'code[words][]', '', required: true, autocomplete: 'off', - 'aria-labelledby': 'personal-key-label' - end - - = submit_tag t('forms.buttons.submit.default'), class: 'btn btn-primary mt2' += simple_form_for(@recovery_code_form, url: login_two_factor_recovery_code_path, + html: { autocomplete: 'off', method: :post, role: 'form' }) do |f| + = render 'partials/personal_key/entry_fields', f: f, attribute_name: :code + = f.button :submit, t('forms.buttons.submit.default'), class: 'btn btn-primary mt2' +end = render 'shared/cancel', link: destroy_user_session_path diff --git a/app/views/users/reactivate_profile/index.html.slim b/app/views/users/reactivate_profile/index.html.slim index 36f16247511..a0dc6f3b673 100644 --- a/app/views/users/reactivate_profile/index.html.slim +++ b/app/views/users/reactivate_profile/index.html.slim @@ -5,6 +5,6 @@ p.mt-tiny.mb0 = t('forms.reactivate_profile.instructions') = simple_form_for(@reactivate_profile_form, url: reactivate_profile_path, html: { autocomplete: 'off', method: :post, role: 'form' }) do |f| = f.error :base - = f.input :recovery_code, required: true + = render 'partials/personal_key/entry_fields', f: f, attribute_name: :recovery_code = f.input :password, required: true = f.button :submit, t('forms.reactivate_profile.submit'), class: 'mb1' diff --git a/spec/controllers/two_factor_authentication/recovery_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/recovery_code_verification_controller_spec.rb index d1485beb53d..3e6b331d8df 100644 --- a/spec/controllers/two_factor_authentication/recovery_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/recovery_code_verification_controller_spec.rb @@ -1,7 +1,9 @@ require 'rails_helper' describe TwoFactorAuthentication::RecoveryCodeVerificationController do - let(:code) { { words: ['foo'] } } + let(:code) { { code: ['foo'] } } + let(:payload) { { recovery_code_form: code } } + describe '#show' do context 'when there is no session (signed out or locked out), and the user reloads the page' do it 'redirects to the home page' do @@ -25,7 +27,7 @@ end it 'redirects to the profile' do - post :create, code: code + post :create, payload expect(response).to redirect_to profile_path end @@ -33,7 +35,7 @@ it 'calls handle_valid_otp' do expect(subject).to receive(:handle_valid_otp).and_call_original - post :create, code: code + post :create, payload end it 'tracks the valid authentication event' do @@ -43,7 +45,7 @@ expect(@analytics).to receive(:track_event). with(Analytics::MULTI_FACTOR_AUTH, analytics_hash) - post :create, code: code + post :create, payload end end @@ -59,11 +61,11 @@ it 'calls handle_invalid_otp' do expect(subject).to receive(:handle_invalid_otp).and_call_original - post :create, code: code + post :create, payload end it 're-renders the recovery code entry screen' do - post :create, code: code + post :create, payload expect(response).to render_template(:show) expect(flash[:error]).to eq t('devise.two_factor_authentication.invalid_recovery_code') @@ -81,7 +83,7 @@ with(Analytics::MULTI_FACTOR_AUTH, properties) expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) - 3.times { post :create, code: code } + 3.times { post :create, payload } end end end diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index df42bb50c8a..c62bfe62daf 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -183,14 +183,10 @@ def submit_prefilled_otp_code sign_in_before_2fa(user) code = RecoveryCodeGenerator.new(user).create - code_words = code.split(' ') - click_link t('devise.two_factor_authentication.recovery_code_fallback.link') - fields = page.all('input[type="text"]') + click_link t('devise.two_factor_authentication.recovery_code_fallback.link') - fields.size.times do |index| - fields[index].set(code_words[index]) - end + enter_recovery_code(code: code) click_submit_default click_acknowledge_recovery_code @@ -241,17 +237,4 @@ def submit_prefilled_otp_code expect(current_path).to eq root_path end end - - describe 'signing in and visiting endpoint that requires user to be signed out' do - it 'does not result in infinite redirect loop after submitting OTP code' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - sign_in_user(user) - - visit sign_up_email_path - click_submit_default - - expect(current_path).to eq profile_path - end - end end diff --git a/spec/features/visitors/password_recovery_spec.rb b/spec/features/visitors/password_recovery_spec.rb index 93b583a851a..ef472a58608 100644 --- a/spec/features/visitors/password_recovery_spec.rb +++ b/spec/features/visitors/password_recovery_spec.rb @@ -35,7 +35,7 @@ def scrape_recovery_code def reactivate_profile(password, recovery_code) fill_in 'Password', with: password - fill_in 'Recovery code', with: recovery_code + enter_recovery_code(code: recovery_code) click_button t('forms.reactivate_profile.submit') end @@ -292,7 +292,6 @@ def reactivate_profile(password, recovery_code) scenario 'resets password, uses recovery code as 2fa', email: true do recovery_code = recovery_code_from_pii(user, pii) - code_words = recovery_code.split(' ') trigger_reset_password_and_click_email_link(user.email) @@ -301,11 +300,7 @@ def reactivate_profile(password, recovery_code) click_link t('devise.two_factor_authentication.recovery_code_fallback.link') - fields = page.all('input[type="text"]') - - fields.size.times do |index| - fields[index].set(code_words[index]) - end + enter_recovery_code(code: recovery_code) click_submit_default diff --git a/spec/forms/reactivate_profile_form_spec.rb b/spec/forms/reactivate_profile_form_spec.rb index 51263f396f9..501adb672d2 100644 --- a/spec/forms/reactivate_profile_form_spec.rb +++ b/spec/forms/reactivate_profile_form_spec.rb @@ -3,7 +3,6 @@ describe ReactivateProfileForm do subject(:form) do ReactivateProfileForm.new(user, - recovery_code: recovery_code, password: password) end @@ -13,7 +12,7 @@ describe '#valid?' do let(:password) { 'asd' } - let(:recovery_code) { '123' } + let(:recovery_code) { %w(123 abc) } let(:valid_recovery_code?) { true } let(:valid_password?) { true } let(:recovery_code_decrypts?) { true } @@ -45,6 +44,12 @@ end context 'when recovery code does not match' do + subject(:form) do + ReactivateProfileForm.new(user, + recovery_code: recovery_code, + password: password) + end + let(:valid_recovery_code?) { false } it 'is invalid' do diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index c9f4397658d..2de28ccfff5 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -171,6 +171,15 @@ def click_acknowledge_recovery_code click_on t('forms.buttons.continue'), class: 'recovery-code-continue' end + def enter_recovery_code(code:, selector: 'input[type="text"]') + code_words = code.split(' ') + fields = page.all(selector) + + fields.zip(code_words) do |field, word| + field.set(word) + end + end + def loa3_sp_session Warden.on_next_request do |proxy| session = proxy.env['rack.session'] diff --git a/spec/views/two_factor_authentication/recovery_code_verification/show.html.slim_spec.rb b/spec/views/two_factor_authentication/recovery_code_verification/show.html.slim_spec.rb index 4d2ebecd8cb..09e9bd0145e 100644 --- a/spec/views/two_factor_authentication/recovery_code_verification/show.html.slim_spec.rb +++ b/spec/views/two_factor_authentication/recovery_code_verification/show.html.slim_spec.rb @@ -4,6 +4,7 @@ let(:user) { build_stubbed(:user, :signed_up) } before do + @recovery_code_form = RecoveryCodeForm.new(user) allow(view).to receive(:current_user).and_return(user) end