LG-677 Add confirm screen on account reset cancel

[stevegsa]

Why: Some phones will visit the SMS link to retrieve a thumbnail and this triggers a cancel.

How: Add a confirm screen and have it call the old logic via a post request. Prevent token leakage by placing a valid token in session.

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.

[stevegsa]

Email (same as before):

New confirm screen:

[jgsmith-usds]

Style: return render :show unless token is shorter ⛳

I accidentally did this under a different ticket. I'm uncomfortable relying on the session so much. In my version, I just tacked the token onto the URL for a button that would send a POST. I'm not worried about a browser pre-fetching a POST. But if we want to make sure that someone can't stop an account deletion without going through these steps, then this might be the only way to force it.

[jgsmith-usds]

Would this make more sense under app/validators/ rather than as a service? Mainly keying off of the Validate part of the class name. But I can also see this as a general service or a form. Especially as a form since it returns a FormResponse.

[stevegsa]

Agreed. I'll need to refactor cancel now to use the validator

[monfresh]

If there isn't a token, we want to know about it, and display a helpful message to the user. This keeps it consistent with what AccountReset::Cancel does. Since the token validation now happens in two classes, it probably makes sense to extract it into a validator as Jim suggested, and include it in both classes.

[stevegsa]

Agreed. I'll need to refactor cancel now to use the validator

[stevegsa]

I just copied what DeleteAccountController was doing and it renders a missing token with no message. They will get an error message either way because the post won't work.

[monfresh]

Yeah, I realized after I made that comment that this is because of the whole remove the token from the URL thing.

[monfresh]

Was this reordering done manually or is there a bug in make normalize_yaml? It should be sorting keys alphabetically. Same question for the 2 files below.

[stevegsa]

make normalize_yaml

[monfresh]

Is Exit the right word to use? We don't use that word anywhere else in our app. Perhaps Cancel or Back to sign in page?

[stevegsa]

Eric had an issue with Cancel since the main purpose of the screen is to cancel something. Back to sign in page doesn't make sense either because they landed on the screen directly from a link in their email.

[monfresh]

Do we need a link at all? What is the user exiting? There isn't anything in progress that they would need to cancel, right?

[monfresh]

I suppose these are questions for the design/product team. I don't have any strong feelings.

[monfresh]

I know this was here before, but I wonder if we can take this opportunity to make the experience better for the user by displaying the flash error message if they happen to end up on the confirm screen without a token? For example, if somehow there is a bug and the link in the email doesn't include the token, they end up on the account_reset/cancel page, and when they click the button to confirm, they get redirected to the sign in page without any kind of message, which could confuse the user.

[stevegsa]

I believe the message they would get is Invalid token on the post. They do get the confirm screen with no token. The same thing would occur for truncated link/token. I believe this is how delete works now too.

[monfresh]

Agreed. I'm talking about an edge case where someone ended up visiting the account_reset/cancel path without any token in the params. In that case, no error would show up because of the first line render :show and return unless token. Then, if they submit the form, they will get redirected to the sign in page without an error message. So, instead of this:

def create
  result = AccountReset::Cancel.new(session[:cancel_token]).call
  track_event(result)
  handle_success if result.success?
  redirect_to root_url
end

we could do something like this:

def create
  result = AccountReset::Cancel.new(session[:cancel_token]).call

  track_event(result)
  sign_out if current_user

  if result.success?
    flash[:success] = t('two_factor_authentication.account_reset.successful_cancel')
  else
    flash[:error] = result.errors[:token].first
  end

  redirect_to root_url
end

Since this isn't directly related to this PR, I'll leave it up to you if you want to do it now or in another PR.

[stevegsa]

Yes. I confirmed there is no message on the post after no token. I thought there was. I'll update this.

[monfresh]

LGTM. Up to you if you want to update the controller now or later.

[stevegsa]

@monfresh thanks for your feedback. We can follow up later with a fix for blank tokens here and in the delete controller in a subsequent pr.