Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion app/forms/reactivate_profile_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def validate_password
end

def validate_recovery_code
return if recovery_code.blank? || valid_recovery_code?
return if recovery_code.blank? || (valid_recovery_code? && recovery_code_decrypts?)
errors.add :recovery_code, :recovery_code_incorrect
end

Expand All @@ -68,6 +68,12 @@ def valid_password?
user.valid_password?(password)
end

def recovery_code_decrypts?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just add the rescue to valid_recovery_code??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the rescue as close to where the exception would be thrown as possible, so it was clear to whomever read it 6 mos from now why there's a rescue at all.

The code can be valid and still not be able to decrypt the PII. In fact, that's exactly the use case here. The code supplied is valid as a recovery code, but not as a decryption key.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, for some reason I thought that RecoveryCodeGenerator#verify actually tried decrypting, but it just compares the recovery code to its hash on the profile. This makes more sense now

decrypted_pii.present?
rescue Pii::EncryptionError => _err
false
end

def valid_recovery_code?
RecoveryCodeGenerator.new(user).verify(recovery_code)
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/partials/personal_key/_key.slim
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
= t('users.recovery_code.header')
.navy.monospace
- code.split(' ').each do |word|
p.bold.mb0.h3
p.bold.mb0.h3 data-recovery='word'
= word

.left.h5.mt2
Expand Down
1 change: 1 addition & 0 deletions app/views/profile/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ h2.h3.my0
= link_to authenticator_start_url, class: btn_cls do
span.hide = t('.authentication_app')
= t('forms.buttons.enable')
- unless @has_password_reset_profile
.py-12p.border-top.border-bottom
.clearfix.mxn1
.sm-col.sm-col-10.px1 = t('profile.items.recovery_code')
Expand Down
75 changes: 57 additions & 18 deletions spec/features/visitors/password_recovery_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -225,30 +225,69 @@ def reset_password_and_sign_back_in(user, password = 'a really long password')
end
end

scenario 'LOA3 user resets password and reactivates profile with recovery code', email: true do
user = create(:user, :signed_up)
context 'LOA3 user' do
let(:user) { create(:user, :signed_up) }
let(:new_password) { 'some really awesome new password' }
let(:pii) { { ssn: '666-66-1234', dob: '1920-01-01' } }

recovery_code = recovery_code_from_pii(user, ssn: '1234', dob: '1920-01-01')
password = 'some really awesome new password'
scenario 'resets password and reactivates profile with recovery code', email: true do
recovery_code = recovery_code_from_pii(user, pii)

visit new_user_password_path
fill_in 'Email', with: user.email
click_button t('forms.buttons.continue')
open_last_email
click_email_link_matching(/reset_password_token/)
visit new_user_password_path
fill_in 'Email', with: user.email
click_button t('forms.buttons.continue')
open_last_email
click_email_link_matching(/reset_password_token/)

reset_password_and_sign_back_in(user, new_password)
click_submit_default
enter_correct_otp_code_for_user(user)

expect(page).to have_content t('profile.index.reactivation.instructions')
click_link t('profile.index.reactivation.reactivate_button')

fill_in 'Password', with: new_password
fill_in 'Recovery code', with: recovery_code
click_button t('forms.reactivate_profile.submit')

expect(page).to have_content t('idv.messages.recovery_code')
end

reset_password_and_sign_back_in(user, password)
click_submit_default
enter_correct_otp_code_for_user(user)
scenario 'resets password, makes recovery code, attempts reactivate profile', email: true do
_recovery_code = recovery_code_from_pii(user, pii)

expect(page).to have_content t('profile.index.reactivation.instructions')
click_link t('profile.index.reactivation.reactivate_button')
visit new_user_password_path
fill_in 'Email', with: user.email
click_button t('forms.buttons.continue')
open_last_email
click_email_link_matching(/reset_password_token/)

reset_password_and_sign_back_in(user, new_password)
click_submit_default
enter_correct_otp_code_for_user(user)

expect(page).to have_content t('profile.index.reactivation.instructions')

fill_in 'Password', with: password
fill_in 'Recovery code', with: recovery_code
click_button t('forms.reactivate_profile.submit')
visit manage_recovery_code_path

expect(page).to have_content t('idv.messages.recovery_code')
new_recovery_code_words = []
page.all(:css, 'p[data-recovery="word"]').each do |node|
new_recovery_code_words << node.text
end
new_recovery_code = new_recovery_code_words.join(' ')

click_on(t('forms.buttons.continue'))

expect(page).to have_content t('profile.index.reactivation.instructions')

click_link t('profile.index.reactivation.reactivate_button')

fill_in 'Password', with: new_password
fill_in 'Recovery code', with: new_recovery_code
click_button t('forms.reactivate_profile.submit')

expect(page).to have_content t('errors.messages.recovery_code_incorrect')
end
end

scenario 'user takes too long to click the reset password link' do
Expand Down
2 changes: 2 additions & 0 deletions spec/forms/reactivate_profile_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
let(:recovery_code) { '123' }
let(:valid_recovery_code?) { true }
let(:valid_password?) { true }
let(:recovery_code_decrypts?) { true }

before do
allow(form).to receive(:valid_password?).and_return(valid_password?)
allow(form).to receive(:valid_recovery_code?).and_return(valid_recovery_code?)
allow(form).to receive(:recovery_code_decrypts?).and_return(recovery_code_decrypts?)
end

context 'when required attributes are not present' do
Expand Down
38 changes: 31 additions & 7 deletions spec/views/profile/index.html.slim_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,39 @@
end
end

it 'contains a recovery code section' do
user = User.new
allow(view).to receive(:current_user).and_return(user)
context 'when has_password_reset_profile is false' do
before do
assign(:has_password_reset_profile, false)
end

render
it 'contains a recovery code section' do
user = User.new
allow(view).to receive(:current_user).and_return(user)

render

expect(rendered).to have_content t('profile.items.recovery_code')
expect(rendered).
to have_link(t('profile.links.regenerate_recovery_code'), href: manage_recovery_code_path)
end
end

context 'when has_password_reset_profile is true' do
before do
assign(:has_password_reset_profile, true)
end

expect(rendered).to have_content t('profile.items.recovery_code')
expect(rendered).
to have_link(t('profile.links.regenerate_recovery_code'), href: manage_recovery_code_path)
it 'lacks a recovery code section' do
user = User.new
allow(view).to receive(:current_user).and_return(user)

render

expect(rendered).to_not have_content t('profile.items.recovery_code')
expect(rendered).to_not have_link(
t('profile.links.regenerate_recovery_code'), href: manage_recovery_code_path
)
end
end

it 'contains account history' do
Expand Down