[LG-501] Only write to phone configuration table

app/controllers/users/two_factor_authentication_controller.rb

@@ -45,7 +45,6 @@ def phone_configuration
     end
 
     def validate_otp_delivery_preference_and_send_code
-      delivery_preference = phone_configuration.delivery_preference
       result = otp_delivery_selection_form.submit(otp_delivery_preference: delivery_preference)
       analytics.track_event(Analytics::OTP_DELIVERY_SELECTION, result.to_h)
 
@@ -57,6 +56,10 @@ def validate_otp_delivery_preference_and_send_code
       end
     end
 
+    def delivery_preference
+      phone_configuration&.delivery_preference || current_user.otp_delivery_preference
+    end
+
     def update_otp_delivery_preference_if_needed
       OtpDeliveryPreferenceUpdater.new(
         user: current_user,
@@ -67,8 +70,7 @@ def update_otp_delivery_preference_if_needed
 
     def handle_invalid_otp_delivery_preference(result)
       flash[:error] = result.errors[:phone].first
-      preference = current_user.phone_configuration.delivery_preference
-      redirect_to login_two_factor_url(otp_delivery_preference: preference)
+      redirect_to login_two_factor_url(otp_delivery_preference: delivery_preference)
     end
 
     def invalid_phone_number(exception, action:)
@@ -179,7 +181,7 @@ def delivery_params
     end
 
     def phone_to_deliver_to
-      return current_user.phone_configuration.phone if authentication_context?
+      return phone_configuration&.phone if authentication_context?
 
       user_session[:unconfirmed_phone]
     end

app/models/concerns/user_encrypted_attribute_overrides.rb

@@ -15,7 +15,8 @@ def find_with_email(email)
 
       email = email.downcase.strip
       email_fingerprint = create_fingerprint(email)
-      find_by(email_fingerprint: email_fingerprint)
+      resource = find_by(email_fingerprint: email_fingerprint)
+      resource if resource&.email == email
     end
 
     def create_fingerprint(email)

app/models/user.rb

@@ -3,6 +3,7 @@ class User < ApplicationRecord
   self.ignored_columns = %w[
     encrypted_password password_salt password_cost encryption_key
     recovery_code recovery_cost recovery_salt
+    encrypted_phone phone_confirmed_at
   ]
 
   include NonNullUuid
@@ -22,7 +23,6 @@ class User < ApplicationRecord
 
   include EncryptableAttribute
 
-  encrypted_attribute(name: :phone)
   encrypted_attribute(name: :otp_secret_key)
   encrypted_attribute_without_setter(name: :email)
 

app/services/pii/cacher.rb

@@ -38,6 +38,9 @@ def rotate_fingerprints(profile)
 
     def rotate_encrypted_attributes
       KeyRotator::AttributeEncryption.new(user).rotate
+      phone_configuration = user.phone_configuration
+      return if phone_configuration.blank?
+      KeyRotator::AttributeEncryption.new(phone_configuration).rotate
     end
 
     def stale_fingerprints?(profile)
@@ -49,7 +52,7 @@ def stale_email_fingerprint?
     end
 
     def stale_attributes?
-      user.stale_encrypted_phone? || user.stale_encrypted_email? ||
+      user.phone_configuration&.stale_encrypted_phone? || user.stale_encrypted_email? ||
         user.stale_encrypted_otp_secret_key?
     end
 

app/services/update_user.rb

@@ -5,7 +5,7 @@ def initialize(user:, attributes:)
   end
 
   def call
-    result = user.update!(attributes)
+    result = user.update!(attributes.except(:phone, :phone_confirmed_at))
     manage_phone_configuration
     result
   end
@@ -23,13 +23,7 @@ def manage_phone_configuration
   end
 
   def update_phone_configuration
-    configuration = user.phone_configuration
-    if phone_attributes[:phone].present?
-      configuration.update!(phone_attributes)
-    else
-      configuration.destroy
-      user.reload
-    end
+    user.phone_configuration.update!(phone_attributes)
   end
 
   def create_phone_configuration
@@ -39,10 +33,10 @@ def create_phone_configuration
 
   def phone_attributes
     @phone_attributes ||= {
-      phone: attribute(:phone),
-      confirmed_at: attribute(:phone_confirmed_at),
+      phone: attributes[:phone],
+      confirmed_at: attributes[:phone_confirmed_at],
       delivery_preference: attribute(:otp_delivery_preference),
-    }
+    }.delete_if { |_, value| value.nil? }
   end
 
   # This returns the named attribute if it's included in the changes, even if

lib/tasks/create_test_accounts.rb

@@ -15,12 +15,10 @@ def create_account(email: 'joe.smith@email.com', password: 'salty pickles', mfa_
   user = User.create!(email: email)
   user.skip_confirmation!
   user.reset_password(password, password)
-  user.phone = mfa_phone || phone
-  user.phone_confirmed_at = Time.zone.now
   user.save!
   user.create_phone_configuration(
     phone: mfa_phone || phone,
-    confirmed_at: user.phone_confirmed_at,
+    confirmed_at: Time.zone.now,
     delivery_preference: user.otp_delivery_preference
   )
   Event.create(user_id: user.id, event_type: :account_created)

lib/tasks/dev.rake

@@ -88,9 +88,11 @@ namespace :dev do
     user.encrypted_email = args[:ee].encrypted
     user.skip_confirmation!
     user.reset_password(args[:pw], args[:pw])
-    user.phone = format('+1 (415) 555-%04d', args[:num])
-    user.phone_confirmed_at = Time.zone.now
-    create_phone_configuration_for(user)
+    user.create_phone_configuration(
+      delivery_preference: user.otp_delivery_preference,
+      phone: format('+1 (415) 555-%04d', args[:num]),
+      confirmed_at: Time.zone.now
+    )
     Event.create(user_id: user.id, event_type: :account_created)
   end
 
@@ -105,12 +107,4 @@ namespace :dev do
   def fingerprint(email)
     Pii::Fingerprinter.fingerprint(email)
   end
-
-  def create_phone_configuration_for(user)
-    user.create_phone_configuration(
-      phone: user.phone,
-      confirmed_at: user.phone_confirmed_at,
-      delivery_preference: user.otp_delivery_preference
-    )
-  end
 end

spec/controllers/account_recovery_setup_controller_spec.rb

@@ -24,7 +24,7 @@
 
   context 'user is piv_cac enabled but not phone enabled' do
     it 'redirects to account_url' do
-      user = build(:user, :signed_up, :with_piv_or_cac, phone: nil)
+      user = build(:user, :signed_up, :with_piv_or_cac, with: { mfa_enabled: false })
       stub_sign_in(user)
 
       get :index

spec/controllers/idv/phone_controller_spec.rb

@@ -20,7 +20,10 @@
   end
 
   describe '#new' do
-    let(:user) { build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now) }
+    let(:user) do
+      build(:user, :with_phone,
+            with: { phone: good_phone, confirmed_at: Time.zone.now })
+    end
 
     before do
       stub_verify_steps_one_and_two(user)
@@ -64,7 +67,7 @@
   describe '#create' do
     context 'when form is invalid' do
       before do
-        user = build(:user, phone: '+1 (415) 555-0130')
+        user = build(:user, :with_phone, with: { phone: '+1 (415) 555-0130' })
         stub_verify_steps_one_and_two(user)
         stub_analytics
         allow(@analytics).to receive(:track_event)
@@ -105,7 +108,7 @@
       end
 
       it 'tracks event with valid phone' do
-        user = build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now)
+        user = build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now })
         stub_verify_steps_one_and_two(user)
 
         put :create, params: { idv_phone_form: { phone: good_phone } }
@@ -124,7 +127,9 @@
 
       context 'when same as user phone' do
         it 'redirects to result page and sets phone_confirmed_at' do
-          user = build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now)
+          user = build(:user, :with_phone, with: {
+                         phone: good_phone, confirmed_at: Time.zone.now
+                       })
           stub_verify_steps_one_and_two(user)
 
           put :create, params: { idv_phone_form: { phone: good_phone } }
@@ -141,7 +146,9 @@
 
       context 'when different from user phone' do
         it 'redirects to otp page and does not set phone_confirmed_at' do
-          user = build(:user, phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now)
+          user = build(:user, :with_phone, with: {
+                         phone: '+1 (415) 555-0130', confirmed_at: Time.zone.now
+                       })
           stub_verify_steps_one_and_two(user)
 
           put :create, params: { idv_phone_form: { phone: good_phone } }
@@ -159,7 +166,9 @@
   end
 
   describe '#show' do
-    let(:user) { build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now) }
+    let(:user) do
+      build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now })
+    end
     let(:params) { { phone: good_phone } }
 
     before do
@@ -232,7 +241,9 @@
         end
 
         let(:params) { { phone: bad_phone } }
-        let(:user) { build(:user, phone: bad_phone, phone_confirmed_at: Time.zone.now) }
+        let(:user) do
+          build(:user, :with_phone, with: { phone: bad_phone, confirmed_at: Time.zone.now })
+        end
 
         it 'tracks event with invalid phone' do
           stub_analytics
@@ -257,7 +268,9 @@
 
       context 'attempt window has expired, previous attempts == max-1' do
         let(:two_days_ago) { Time.zone.now - 2.days }
-        let(:user) { build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now) }
+        let(:user) do
+          build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now })
+        end
 
         before do
           user.idv_attempts = max_attempts - 1
@@ -275,7 +288,8 @@
       end
 
       it 'passes the normalized phone to the background job' do
-        user = build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now)
+        user = build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now })
+
         stub_verify_steps_one_and_two(user)
 
         subject.params = { phone: normalized_phone }

spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb

@@ -33,7 +33,7 @@
     end
 
     it 'tracks the page visit and context' do
-      user = build_stubbed(:user, phone: '+1 (703) 555-0100')
+      user = build_stubbed(:user, :with_phone, with: { phone: '+1 (703) 555-0100' })
       stub_sign_in_before_2fa(user)
 
       stub_analytics
@@ -322,8 +322,6 @@
           end
 
           it 'does not update user phone or phone_confirmed_at attributes' do
-            expect(subject.current_user.phone).to eq('+1 202-555-1212')
-            expect(subject.current_user.phone_confirmed_at).to eq(@previous_phone_confirmed_at)
             expect(subject.current_user.phone_configuration.phone).to eq('+1 202-555-1212')
             expect(
               subject.current_user.phone_configuration.confirmed_at
@@ -355,8 +353,6 @@
 
       context 'when user does not have an existing phone number' do
         before do
-          subject.current_user.phone = nil
-          subject.current_user.phone_confirmed_at = nil
           subject.current_user.phone_configuration.destroy
           subject.current_user.phone_configuration = nil
           subject.current_user.create_direct_otp

spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb

@@ -80,7 +80,7 @@
       let(:payload) { { personal_key_form: personal_key } }
 
       before do
-        stub_sign_in_before_2fa(build(:user, phone: '+1 (703) 555-1212'))
+        stub_sign_in_before_2fa(build(:user, :with_phone, with: { phone: '+1 (703) 555-1212' }))
         form = instance_double(PersonalKeyForm)
         response = FormResponse.new(
           success: false, errors: {}, extra: { multi_factor_auth_method: 'personal key' }
@@ -100,7 +100,7 @@
 
     context 'when the user enters an invalid personal key' do
       before do
-        stub_sign_in_before_2fa(build(:user, phone: '+1 (703) 555-1212'))
+        stub_sign_in_before_2fa(build(:user, :with_phone, with: { phone: '+1 (703) 555-1212' }))
         form = instance_double(PersonalKeyForm)
         response = FormResponse.new(
           success: false, errors: {}, extra: { multi_factor_auth_method: 'personal key' }

spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb

@@ -3,7 +3,7 @@
 describe TwoFactorAuthentication::PivCacVerificationController do
   let(:user) do
     create(:user, :signed_up, :with_piv_or_cac,
-           phone: '+1 (703) 555-0000')
+           with: { phone: '+1 (703) 555-0000' })
   end
 
   let(:nonce) { 'once' }