Stop reading old recovery code columns

.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:

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?

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

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

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

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

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

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:)

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('-', ' ')

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
 

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
 

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

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)

spec/services/encryption/password_verifier_spec.rb

@@ -9,27 +9,28 @@
       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
 
   describe '.verify' do
     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)

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
 

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