[LG-424] disable phone by deleting

[jgsmith-usds]

Why:
We want users able to remove a phone from their account
if their threat model changes.

How:
We let users remove a configured phone if they have at
least one other MFA configured that isn't personal key.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the checklists below. These represent the more critical elements
of our code quality guidelines. The rest of the list can be found in
CONTRIBUTING.md

Controllers

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated
  as the first callback.

Database

• Unsafe migrations are implemented over several PRs and over several
  deploys to avoid production errors. The strong_migrations gem
  will warn you about unsafe migrations and has great step-by-step instructions
  for various scenarios.

• Indexes were added if necessary. This article provides a good overview
  of indexes in Rails.

• Verified that the changes don't affect other apps (such as the dashboard)

• When relevant, a rake task is created to populate the necessary DB columns
  in the various environments right before deploying, taking into account the users
  who might not have interacted with this column yet (such as users who have not
  set a password yet)

• Migrations against existing tables have been tested against a copy of the
  production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
  issues. In that case, all the migration did was add a new column and an index to
  the Users table, which might seem innocuous.

Encryption

• The changes are compatible with data that was encrypted with the old code.

Routes

• GET requests are not vulnerable to CSRF attacks (i.e. they don't change
  state or result in destructive behavior).

Session

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

Testing

• Tests added for this feature/bug
• Prefer feature/integration specs over controller specs
• When adding code that reads data, write tests for nil values, empty strings,
  and invalid inputs.

[jgsmith-usds]

The option only shows up if the phone isn't the only MFA configured.

[jmhooper]

nice

[monfresh]

I found an even better way to get the final array of presenters with less code and without sacrificing readability. I was gonna submit it as a separate PR, but you're free to include it in here if you like it:

def all_sms_and_voice_options_plus_one_of_each_remaining_option(configurations)
  presenters = configurations.flat_map(&:selection_presenters)
  phone_presenters, other_presenters = presenters.partition do |presenter| 
    phone_presenter_class?(presenter.class)
  end
  phone_presenters + other_presenters.uniq(&:class)
end

This would allow us to remove these methods:

def presenters_for_options_that_are_not_phone(presenters)
  presenters.select { |presenter| !phone_presenter_class?(presenter.class) }.uniq(&:class)
end

def presenters_for_phone_options(presenters)
  presenters.select { |presenter| phone_presenter_class?(presenter.class) }
end

[jgsmith-usds]

The change was an autofix from Rubocop (make lintfix). I'm going to do a follow-on PR to capture the translation fixes you mentioned elsewhere. I can revisit this in that PR as well.

[jgsmith-usds]

Actually, I'll make the fixes in this PR. I thought I had merged this one, but obviously I didn't.

[jmhooper]

Tested this out and it seems to work 👍

[monfresh]

Supprimer le téléphone

[monfresh]

Or maybe Supprimer le numéro de téléphone is better

[monfresh]

I think we want numéro de before each occurrence of téléphone.

[monfresh]

Did you mean to put this line after assigning the result so that errors can be captured in analytics?

[jgsmith-usds]

yeah. I forgot to go back and update this. Thanks for pointing it out.

[monfresh]

What do you think about making these validations for consistency with other forms? Also, should we localize the error messages?

[jgsmith-usds]

I thought about that, but I was hoping to get it in for this week's deploy. Obviously, I missed that, so I can go back and make validators for them. I'll make them so they're reusable since we might want to do a similar treatment for some other MFA methods.

[monfresh]

This isn't necessary. It's tracked automatically for authenticated users.

[jgsmith-usds]

Ahh. Okay. I had seen this elsewhere, so cargo-culted it.

[monfresh]

It's only needed when the user is not yet authenticated, but we can find out who they are based on a token, for example, when confirming email, resetting password, etc.

[monfresh]

What is the attack vector here? The user and configuration that are passed in to the form are based on the current user. In what scenario would the configuration not be owned by the user?

[monfresh]

I wanted to document this before I forget, and because I probably won't have time to verify this today. When I was testing this last week, I think I came across this bug:

1. If you already have a phone in your account, delete it
2. Add a new phone
3. When prompted for the OTP, click on the link that says something about "enter a different number"

Expected Result: You see the same phone setup page as the one when you clicked "Add a new phone"
Actual Result: You see the phone setup page that appears when you are creating your account for the first time

[jgsmith-usds]

Good call. That was something at the back of my mind, but hadn't really done much to chase down situations.

[jgsmith-usds]

Looks like that behavior is the current behavior if someone created an account without a phone number and later added it. Deleting the phone number makes the account the same as if it had been created without a phone at all. I'm digging in to see if I can make the experience better.

[jgsmith-usds]

I think I have the behavior tweaked to be less confusing.