diff --git a/.reek b/.reek index 54bff8852c2..76d5e66a6ff 100644 --- a/.reek +++ b/.reek @@ -47,7 +47,6 @@ FeatureEnvy: - Utf8Sanitizer#event_attributes - Utf8Sanitizer#remote_ip - Idv::Proofer#validate_vendors - - PersonalKeyGenerator#create_legacy_recovery_code - TwoFactorAuthenticationController#capture_analytics_for_exception InstanceVariableAssumption: exclude: diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index e8b7b909dfe..28f1d05e06f 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -224,7 +224,9 @@ def direct_otp_code end def personal_key_unavailable? - idv_or_confirmation_context? || profile_context? || current_user.personal_key.blank? + idv_or_confirmation_context? || + profile_context? || + current_user.encrypted_recovery_code_digest.blank? end def unconfirmed_phone? diff --git a/app/controllers/idv/sessions_controller.rb b/app/controllers/idv/sessions_controller.rb index d434f610094..3e05e51ff27 100644 --- a/app/controllers/idv/sessions_controller.rb +++ b/app/controllers/idv/sessions_controller.rb @@ -66,7 +66,7 @@ def step end def handle_idv_redirect - redirect_to account_url and return if current_user.personal_key.present? + redirect_to account_url and return if current_user.encrypted_recovery_code_digest.present? user_session[:personal_key] = create_new_code redirect_to manage_personal_key_url end diff --git a/app/controllers/sign_up/personal_keys_controller.rb b/app/controllers/sign_up/personal_keys_controller.rb index 1706c1da9a0..43349a675e2 100644 --- a/app/controllers/sign_up/personal_keys_controller.rb +++ b/app/controllers/sign_up/personal_keys_controller.rb @@ -20,11 +20,12 @@ def update def confirm_user_needs_initial_personal_key redirect_to(account_url) if user_session[:personal_key].nil? && - current_user.personal_key.present? + current_user.encrypted_recovery_code_digest.present? end def assign_initial_personal_key - user_session[:personal_key] = create_new_code if current_user.personal_key.nil? + return if current_user.encrypted_recovery_code_digest.present? + user_session[:personal_key] = create_new_code end def next_step diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 730dac32237..c0cac76ef95 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -112,7 +112,7 @@ def should_acknowledge_personal_key?(session) sp_session = session[:sp] - user.personal_key.blank? && (sp_session.blank? || sp_session[:loa3] == false) + user.encrypted_recovery_code_digest.blank? && (sp_session.blank? || sp_session[:loa3] == false) end def recent_events diff --git a/app/models/concerns/user_access_key_overrides.rb b/app/models/concerns/user_access_key_overrides.rb index a9ec587f14b..cb604ad68e6 100644 --- a/app/models/concerns/user_access_key_overrides.rb +++ b/app/models/concerns/user_access_key_overrides.rb @@ -17,7 +17,24 @@ def valid_password?(password) def password=(new_password) @password = new_password return if @password.blank? - self.encrypted_password_digest = Encryption::PasswordVerifier.digest(@password).to_s + self.encrypted_password_digest = Encryption::PasswordVerifier.digest(@password) + end + + def valid_personal_key?(normalized_personal_key) + Encryption::PasswordVerifier.verify( + password: normalized_personal_key, + digest: encrypted_recovery_code_digest + ) + end + + def personal_key + @personal_key + end + + def personal_key=(new_personal_key) + @personal_key = new_personal_key + return if @personal_key.blank? + self.encrypted_recovery_code_digest = Encryption::PasswordVerifier.digest(@personal_key) end # This is a devise method, which we are overriding. This should not be removed diff --git a/app/models/user.rb b/app/models/user.rb index 3208419e1d5..c55083cd1a1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -41,14 +41,6 @@ class User < ApplicationRecord attr_accessor :asserted_attributes - def personal_key - recovery_code - end - - def personal_key=(value) - self.recovery_code = value - end - def set_default_role self.role ||= :user end diff --git a/app/services/encryption/password_verifier.rb b/app/services/encryption/password_verifier.rb index cb8e5974605..9762bc7c2ea 100644 --- a/app/services/encryption/password_verifier.rb +++ b/app/services/encryption/password_verifier.rb @@ -37,7 +37,7 @@ def self.digest(password) uak.encryption_key, salt, uak.cost - ) + ).to_s end def self.verify(password:, digest:) diff --git a/app/services/personal_key_generator.rb b/app/services/personal_key_generator.rb index 1c613fc23ab..b5bf9710bc3 100644 --- a/app/services/personal_key_generator.rb +++ b/app/services/personal_key_generator.rb @@ -9,18 +9,13 @@ def initialize(user, length: 4) end def create - digest = create_encrypted_recovery_code_digest - # Until we drop the old columns, still write to them so that we can rollback - create_legacy_recovery_code(digest) + user.personal_key = raw_personal_key user.save! raw_personal_key.tr(' ', '-') end def verify(plaintext_code) - Encryption::PasswordVerifier.verify( - password: normalize(plaintext_code), - digest: user.encrypted_recovery_code_digest - ) + user.valid_personal_key?(normalize(plaintext_code)) end def normalize(plaintext_code) @@ -37,21 +32,6 @@ def normalize(plaintext_code) attr_reader :user - def create_legacy_recovery_code(digest) - user.personal_key = [ - digest.encryption_key, - digest.encrypted_password, - ].join(Encryption::Encryptors::AesEncryptor::DELIMITER) - user.recovery_salt = digest.password_salt - user.recovery_cost = digest.password_cost - end - - def create_encrypted_recovery_code_digest - digest = Encryption::PasswordVerifier.digest(raw_personal_key) - user.encrypted_recovery_code_digest = digest.to_s - digest - end - def encode_code(code:, length:, split:) decoded = Base32::Crockford.decode(code) Base32::Crockford.encode(decoded, length: length, split: split).tr('-', ' ') diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 6e640a3f68e..df3dd5ada23 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -17,7 +17,7 @@ trait :with_personal_key do after :build do |user| - user.personal_key = PersonalKeyGenerator.new(user).create + PersonalKeyGenerator.new(user).create 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 11ec4f488a9..d4192dd3915 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -665,14 +665,14 @@ def submit_prefilled_otp_code # For example, when migrating users from another DB it 'displays personal key and redirects to profile' do user = create(:user, :signed_up) - UpdateUser.new(user: user, attributes: { personal_key: nil }).call + UpdateUser.new(user: user, attributes: { encrypted_recovery_code_digest: nil }).call sign_in_user(user) click_button t('forms.buttons.submit.default') fill_in 'code', with: user.reload.direct_otp click_button t('forms.buttons.submit.default') - expect(user.reload.personal_key).not_to be_nil + expect(user.reload.encrypted_recovery_code_digest).not_to be_nil click_acknowledge_personal_key diff --git a/spec/features/users/regenerate_personal_key_spec.rb b/spec/features/users/regenerate_personal_key_spec.rb index cc66086bda4..0e44a04223d 100644 --- a/spec/features/users/regenerate_personal_key_spec.rb +++ b/spec/features/users/regenerate_personal_key_spec.rb @@ -26,11 +26,11 @@ context 'regenerating personal key' do scenario 'displays new code' do user = sign_in_and_2fa_user - old_code = user.personal_key + old_digest = user.encrypted_recovery_code_digest click_button t('account.links.regenerate_personal_key') - expect(user.reload.personal_key).to_not eq old_code + expect(user.reload.encrypted_recovery_code_digest).to_not eq old_digest end end @@ -40,13 +40,13 @@ user = sign_in_and_2fa_user - old_code = user.personal_key + old_digest = user.encrypted_recovery_code_digest first(:link, t('forms.buttons.edit')).click click_on(t('links.cancel')) click_on(t('account.links.regenerate_personal_key')) - expect(user.reload.personal_key).to_not eq old_code + expect(user.reload.encrypted_recovery_code_digest).to_not eq old_digest end end @@ -62,13 +62,13 @@ context 'visitting the personal key path' do scenario 'does not regenerate the personal and redirects to account' do user = sign_in_and_2fa_user - old_code = user.personal_key + old_digest = user.encrypted_recovery_code_digest visit sign_up_personal_key_path user.reload - expect(user.personal_key).to eq(old_code) + expect(user.encrypted_recovery_code_digest).to eq(old_digest) expect(current_path).to eq(account_path) end end diff --git a/spec/forms/password_reset_email_form_spec.rb b/spec/forms/password_reset_email_form_spec.rb index 53165a481bc..d58a11fc4bf 100644 --- a/spec/forms/password_reset_email_form_spec.rb +++ b/spec/forms/password_reset_email_form_spec.rb @@ -9,7 +9,7 @@ describe '#submit' do context 'when email is valid and user exists' do it 'returns hash with properties about the event and the user' do - user = build(:user, :signed_up, email: 'test1@test.com') + user = create(:user, :signed_up, email: 'test1@test.com') subject = PasswordResetEmailForm.new('Test1@test.com') extra = { @@ -65,7 +65,7 @@ context 'when recaptcha is valid' do it 'returns hash with properties about the event and the user' do - user = build(:user, :signed_up, email: 'test1@test.com') + user = create(:user, :signed_up, email: 'test1@test.com') captcha_results = mock_captcha(enabled: true, present: true, valid: true) subject = PasswordResetEmailForm.new('Test1@test.com', captcha_results) @@ -90,7 +90,7 @@ context 'when recaptcha is invalid' do it 'returns hash with properties about the event and the user' do - user = build(:user, :signed_up, email: 'test1@test.com') + user = create(:user, :signed_up, email: 'test1@test.com') captcha_results = mock_captcha(enabled: true, present: true, valid: false) subject = PasswordResetEmailForm.new('Test1@test.com', captcha_results) diff --git a/spec/services/encryption/password_verifier_spec.rb b/spec/services/encryption/password_verifier_spec.rb index 5f045ad2b8b..36c09a39c33 100644 --- a/spec/services/encryption/password_verifier_spec.rb +++ b/spec/services/encryption/password_verifier_spec.rb @@ -9,12 +9,13 @@ digest = described_class.digest('saltypickles') uak = Encryption::UserAccessKey.new(password: 'saltypickles', salt: salt) - uak.unlock(digest.encryption_key) + parsed_digest = JSON.parse(digest, symbolize_names: true) + uak.unlock(parsed_digest[:encryption_key]) - expect(digest.encrypted_password).to eq(uak.encrypted_password) - expect(digest.encryption_key).to eq(uak.encryption_key) - expect(digest.password_salt).to eq(salt) - expect(digest.password_cost).to eq(uak.cost) + expect(parsed_digest[:encrypted_password]).to eq(uak.encrypted_password) + expect(parsed_digest[:encryption_key]).to eq(uak.encryption_key) + expect(parsed_digest[:password_salt]).to eq(salt) + expect(parsed_digest[:password_cost]).to eq(uak.cost) end end @@ -22,14 +23,14 @@ it 'returns true if the password matches' do password = 'saltypickles' - digest = described_class.digest(password).to_s + digest = described_class.digest(password) result = described_class.verify(password: password, digest: digest) expect(result).to eq(true) end it 'returns false if the password does not match' do - digest = described_class.digest('saltypickles').to_s + digest = described_class.digest('saltypickles') result = described_class.verify(password: 'pepperpickles', digest: digest) expect(result).to eq(false) diff --git a/spec/services/personal_key_generator_spec.rb b/spec/services/personal_key_generator_spec.rb index 0740657cb84..18308f2a27e 100644 --- a/spec/services/personal_key_generator_spec.rb +++ b/spec/services/personal_key_generator_spec.rb @@ -27,7 +27,7 @@ def stub_random_phrase generator.create - expect(user.personal_key).to_not eq personal_key + expect(user.encrypted_recovery_code_digest).to_not eq personal_key end it 'generates a phrase of 4 words by default' do @@ -44,20 +44,10 @@ def stub_random_phrase it 'sets the encrypted recovery code digest' do user = create(:user) generator = PersonalKeyGenerator.new(user) - generator.create + key = generator.create - encrypted_recovery_code_data = JSON.parse( - user.encrypted_recovery_code_digest, symbolize_names: true - ) - - expect( - encrypted_recovery_code_data[:encryption_key] - ).to eq(user.personal_key.split('.').first) - expect( - encrypted_recovery_code_data[:encrypted_password] - ).to eq(user.personal_key.split('.').second) - expect(encrypted_recovery_code_data[:password_cost]).to eq(user.recovery_cost) - expect(encrypted_recovery_code_data[:password_salt]).to eq(user.recovery_salt) + expect(user.encrypted_recovery_code_digest).to_not be_empty + expect(generator.verify(key)).to eq(true) end end diff --git a/spec/support/shared_examples_for_personal_keys.rb b/spec/support/shared_examples_for_personal_keys.rb index 3be6aa7c51e..f9e76283884 100644 --- a/spec/support/shared_examples_for_personal_keys.rb +++ b/spec/support/shared_examples_for_personal_keys.rb @@ -3,11 +3,11 @@ context 'regenerating personal key with `Get another code` button' do scenario 'displays a flash message and a new code' do - old_code = @user.reload.personal_key + old_digest = @user.reload.encrypted_recovery_code_digest click_button t('users.personal_key.get_another') - expect(@user.reload.personal_key).to_not eq old_code + expect(@user.reload.encrypted_recovery_code_digest).to_not eq old_digest expect(page).to have_content t('notices.send_code.personal_key') end end