LG-2246 Fix phone edit authorization issues

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -295,11 +295,7 @@ def decorated_user
 
   def reenter_phone_number_path
     locale = LinkLocaleResolver.locale
-    if MfaPolicy.new(current_user).multiple_factors_enabled?
-      manage_phone_path(locale: locale)
-    else
-      phone_setup_path(locale: locale)
-    end
+    phone_setup_path(locale: locale)
   end
 
   def confirmation_for_phone_change?

app/controllers/users/edit_phone_controller.rb

@@ -0,0 +1,65 @@
+module Users
+  class EditPhoneController < ReauthnRequiredController
+    include RememberDeviceConcern
+
+    before_action :confirm_two_factor_authenticated
+    before_action :confirm_user_can_edit_phone
+    before_action :confirm_user_can_remove_phone, only: %i[destroy]
+
+    def edit
+      analytics.track_event(Analytics::PHONE_CHANGE_VIEWED)
+      @edit_phone_form = EditPhoneForm.new(current_user, phone_configuration)
+    end
+
+    def update
+      @edit_phone_form = EditPhoneForm.new(current_user, phone_configuration)
+      result = @edit_phone_form.submit(edit_phone_params)
+      analytics.track_event(Analytics::PHONE_CHANGE_SUBMITTED, result.to_h)
+      if result.success?
+        redirect_to account_url
+      else
+        render :edit
+      end
+    end
+
+    def destroy
+      track_deletion_analytics_event
+      phone_configuration.destroy!
+      revoke_remember_device(current_user)
+      flash[:success] = t('two_factor_authentication.phone.delete.success')
+      redirect_to account_url
+    end
+
+    private
+
+    def confirm_user_can_edit_phone
+      render_not_found if phone_configuration.nil?
+      false
+    end
+
+    def confirm_user_can_remove_phone
+      return if MfaPolicy.new(current_user).more_than_two_factors_enabled?
+      flash[:error] = t('two_factor_authentication.phone.delete.failure')
+      redirect_to account_url
+      false
+    end
+
+    def track_deletion_analytics_event
+      analytics.track_event(
+        Analytics::PHONE_DELETION,
+        success: true,
+        phone_configuration_id: phone_configuration.id,
+      )
+      create_user_event(:phone_removed)
+    end
+
+    def phone_configuration
+      @phone_configuration ||= current_user.phone_configurations.find_by(id: params[:id])
+    end
+
+    def edit_phone_params
+      params.require(:edit_phone_form).permit(:delivery_preference,
+                                              :make_default_number)
+    end
+  end
+end

app/controllers/users/phones_controller.rb

@@ -19,98 +19,23 @@ def create
       end
     end
 
-    def edit
-      set_phone_id
-      # memoized for view
-      @edit_phone_form = EditPhoneForm.new(current_user, phone_configuration)
-    end
-
-    def update
-      @edit_phone_form = EditPhoneForm.new(current_user, phone_configuration)
-      if @edit_phone_form.submit(edit_params).success?
-        process_updates
-        bypass_sign_in current_user
-      else
-        render :edit
-      end
-    end
-
-    def delete
-      result = TwoFactorAuthentication::PhoneDeletionForm.new(
-        current_user, phone_configuration
-      ).submit
-      analytics.track_event(Analytics::PHONE_DELETION_REQUESTED, result.to_h)
-      if result.success?
-        handle_successful_delete
-      else
-        flash[:error] = t('two_factor_authentication.phone.delete.failure')
-      end
-
-      redirect_to account_url
-    end
-
     private
 
-    def render_edit
-      flash.now[:error] = t('errors.messages.phone_duplicate') if already_has_phone?
-      render :edit
-    end
-
-    # we only allow editing of the first configuration since we'll eventually be
-    # doing away with this controller. Once we move to multiple phones, we'll allow
-    # adding and deleting, but not editing.
-    def phone_configuration
-      MfaContext.new(current_user).phone_configuration(user_session[:phone_id])
-    end
-
     def user_params
       params.require(:new_phone_form).permit(:phone, :international_code,
                                              :otp_delivery_preference,
                                              :otp_make_default_number)
     end
 
-    def edit_params
-      params.require(:edit_phone_form).permit(:otp_delivery_preference,
-                                              :otp_make_default_number)
-    end
-
     def already_has_phone?
       @user_has_phone ||= @new_phone_form.already_has_phone?
     end
 
-    def delivery_preference
-      phone_configuration&.delivery_preference || current_user.otp_delivery_preference
-    end
-
-    def process_updates
-      if @edit_phone_form.phone_config_changed?
-        analytics.track_event(Analytics::PHONE_CHANGE_REQUESTED)
-
-        OtpPreferenceUpdater.new(
-          user: current_user,
-          preference: @edit_phone_form.otp_delivery_preference,
-          default: @edit_phone_form.otp_make_default_number,
-          phone_id: user_session[:phone_id],
-        ).call
-      end
-      redirect_to account_url
-    end
-
     def confirm_phone
       flash[:notice] = t('devise.registrations.phone_update_needs_confirmation')
       prompt_to_confirm_phone(id: user_session[:phone_id], phone: @new_phone_form.phone,
                               selected_delivery_method: @new_phone_form.otp_delivery_preference,
                               selected_default_number: @new_phone_form.otp_make_default_number)
     end
-
-    def handle_successful_delete
-      flash[:success] = t('two_factor_authentication.phone.delete.success')
-      create_user_event(:phone_removed)
-    end
-
-    def set_phone_id
-      phone_id = params[:id]
-      user_session[:phone_id] = phone_id if phone_id.present?
-    end
   end
 end

app/forms/edit_phone_form.rb

@@ -1,26 +1,31 @@
 class EditPhoneForm
   include ActiveModel::Model
-  include RememberDeviceConcern
 
-  validates :otp_delivery_preference, inclusion: { in: %w[voice sms] }
+  validates :delivery_preference, inclusion: { in: %w[voice sms] }
 
-  attr_accessor :phone, :otp_delivery_preference, :otp_make_default_number, :phone_configuration
+  attr_reader :user, :phone_configuration, :delivery_preference, :make_default_number
 
   def initialize(user, phone_configuration)
-    self.user = user
-    self.phone_configuration = phone_configuration
-    self.otp_make_default_number = true if default_phone_configuration?
+    @user = user
+    @phone_configuration = phone_configuration
+    @delivery_preference = phone_configuration.delivery_preference
+    @make_default_number = default_phone_configuration?
   end
 
   def submit(params)
     ingest_submitted_params(params)
     success = valid?
-
-    self.phone = submitted_phone unless success
-    revoke_remember_device(user) if success
+    update_phone_configuration if success
     FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes)
   end
 
+  # :reek:FeatureEnvy
+  def masked_number
+    phone_number = phone_configuration.phone
+    return '' if !phone_number || phone_number.blank?
+    "***-***-#{phone_number[-4..-1]}"
+  end
+
   def delivery_preference_sms?
     phone_configuration&.delivery_preference == 'sms'
   end
@@ -29,38 +34,33 @@ def delivery_preference_voice?
     phone_configuration&.delivery_preference == 'voice'
   end
 
-  def phone_config_changed?
-    return true if phone_configuration&.delivery_preference != otp_delivery_preference
-    return true if otp_make_default_number && !default_phone_configuration?
-    false
-  end
-
-  # :reek:FeatureEnvy
-  def masked_number
-    phone_number = phone_configuration.phone
-    return '' if !phone_number || phone_number.blank?
-    "***-***-#{phone_number[-4..-1]}"
+  def default_phone_configuration?
+    phone_configuration == user.default_phone_configuration
   end
 
   private
 
-  attr_accessor :user, :submitted_phone
+  attr_writer :delivery_preference, :make_default_number
 
   def extra_analytics_attributes
     {
-      otp_delivery_preference: otp_delivery_preference,
+      delivery_preference: delivery_preference,
+      make_default_number: make_default_number,
+      phone_configuration_id: phone_configuration.id,
     }
   end
 
   def ingest_submitted_params(params)
-    delivery_prefs = params[:otp_delivery_preference]
-    default_prefs = params[:otp_make_default_number]
+    delivery_preference_submission = params[:delivery_preference]
+    make_default_number_submission = params[:make_default_number]
 
-    self.otp_delivery_preference = delivery_prefs if delivery_prefs
-    self.otp_make_default_number = true if default_prefs
+    self.delivery_preference = delivery_preference_submission if delivery_preference_submission
+    self.make_default_number = make_default_number_submission if make_default_number_submission
   end
 
-  def default_phone_configuration?
-    phone_configuration == user.default_phone_configuration
+  def update_phone_configuration
+    update_params = { delivery_preference: delivery_preference }
+    update_params[:made_default_at] = Time.zone.now if make_default_number
+    phone_configuration.update!(update_params)
   end
 end

app/services/analytics.rb

@@ -148,8 +148,9 @@ def browser_attributes
   PASSWORD_RESET_TOKEN = 'Password Reset: Token Submitted'.freeze
   PASSWORD_RESET_VISIT = 'Password Reset: Email Form Visited'.freeze
   PERSONAL_KEY_VIEWED = 'Personal Key Viewed'.freeze
-  PHONE_CHANGE_REQUESTED = 'Phone Number Change: requested'.freeze
-  PHONE_DELETION_REQUESTED = 'Phone Number Deletion: requested'.freeze
+  PHONE_CHANGE_SUBMITTED = 'Phone Number Change: Form submitted'.freeze
+  PHONE_CHANGE_VIEWED = 'Phone Number Change: Visited'.freeze
+  PHONE_DELETION = 'Phone Number Deletion: Submitted'.freeze
   PIV_CAC_LOGIN = 'PIV/CAC Login'.freeze
   PROFILE_ENCRYPTION_INVALID = 'Profile Encryption: Invalid'.freeze
   PROFILE_PERSONAL_KEY_CREATE = 'Profile: Created new personal key'.freeze

app/views/accounts/_phone.html.slim

@@ -14,5 +14,5 @@
           = PhoneConfigurationDecorator.new(phone_configuration).default_number_message
       .col.col-4.right-align
         = render @view_model.manage_action_partial,
-          path: manage_phone_url(id: phone_configuration.id),
+          path: manage_phone_path(id: phone_configuration.id),
           name: t('account.index.phone')

app/views/users/edit_phone/_delivery_preference_selection.html.erb

@@ -0,0 +1,34 @@
+<div class="mb3">
+  <fieldset class="m0 p0 border-none">
+    <legend class="mb1 h4 serif bold">
+      <%= t('two_factor_authentication.otp_delivery_preference.title') %>
+    </legend>
+    <p class="mt0 mb2" id="otp_delivery_preference_instruction">
+      <%= t('two_factor_authentication.otp_delivery_preference.instruction') %>
+    </p>
+    <label
+      class="btn-border col-12 sm-col-5 sm-mr2 mb2 sm-mb0"
+      for='edit_phone_form_delivery_preference_sms'
+    >
+      <div class="radio">
+        <%= radio_button_tag 'edit_phone_form[delivery_preference]',
+          :sms, @edit_phone_form.delivery_preference_sms?,
+          class: :otp_delivery_preference_sms %>
+        <span class="indicator"></span>
+        <%= t('two_factor_authentication.otp_delivery_preference.sms') %>
+      </div>
+    </label>
+    <label
+      class="btn-border col-12 sm-col-5 mb0"
+      for='edit_phone_form_delivery_preference_voice'
+    >
+      <div class="radio">
+        <%= radio_button_tag 'edit_phone_form[delivery_preference]',
+            :voice, @edit_phone_form.delivery_preference_voice?,
+            class: :otp_delivery_preference_voice %>
+        <span class="indicator"></span>
+        <%= t('two_factor_authentication.otp_delivery_preference.voice') %>
+      </div>
+    </label>
+  </fieldset>
+</div>

app/views/users/edit_phone/_make_default_number.html.erb

@@ -0,0 +1,18 @@
+<div class="mb3">
+  <fieldset class="m0 p0 border-none">
+    <legend class="mb1 h4 serif bold">
+      <%= t('two_factor_authentication.otp_make_default_number.title') %>
+    </legend>
+    <p class="mt0 mb2" id="otp_make_default_number_instruction">
+      <%= t('two_factor_authentication.otp_make_default_number.instruction') %>
+    </p>
+    <label class="btn-border col-8">
+      <div class="checkbox">
+        <%= check_box_tag 'edit_phone_form[make_default_number]',
+            :otp_make_default_number, @edit_phone_form.default_phone_configuration? %>
+        <span class="indicator"></span>
+        <%= t('two_factor_authentication.otp_make_default_number.label') %>
+      </div>
+    </label>
+  </fieldset>
+</div>

app/views/users/edit_phone/_remove_phone.html.erb

@@ -0,0 +1,8 @@
+<% if MfaPolicy.new(current_user).more_than_two_factors_enabled? %>
+  <br/>
+  <div class="sm-col-8 mb3">
+    <%= button_to t('forms.phone.buttons.delete'), manage_phone_path(id: @phone_configuration.id),
+                  class: 'btn btn-danger btn-wide',
+                  method: :delete %>
+  </div>
+<% end %>

app/views/users/edit_phone/edit.html.erb

@@ -0,0 +1,23 @@
+<% title t('titles.edit_info.phone') %>
+<h1 class="h3 my0">
+  <%= t('headings.edit_info.phone') %>
+</h1>
+
+<%= simple_form_for(@edit_phone_form,
+                    html: {autocomplete: 'off', method: :put, role: 'form'},
+                    url: manage_phone_path(id: @phone_configuration.id)) do |f| %>
+
+  <div class="mb1 h4">
+    <%= t('two_factor_authentication.phone_label') %>:&nbsp;
+    <strong><%= @edit_phone_form.masked_number %></strong>
+  </div><br/>
+
+  <%= render 'delivery_preference_selection' %>
+  <%= render 'make_default_number' %>
+
+  <%= f.button :submit, t('forms.buttons.submit.confirm_change'), class: 'no-auto-enable btn-wide' %>
+<% end %>
+
+<%= render 'remove_phone' %>
+
+<%= render 'shared/cancel', link: account_path %>