LG-283 Fix password reset links sent to unconfirmed email address

[mryenq]

Why:

Currently password and email address reset confirmation emails are sent
to a newly-added email address prior to validation. This results in a
potential loss of account access, in a scenario described in LG-283.

How:

Remove the use of unconfirmed_email for these messages, sending to the
existing account email address instead.

Specs are passing, and manually (and separately) tested scenarios
included:

• new account creation
• email confirmation, verify sent to the proper (existing) address
• change the email address, but do not confirm
• log out, request password reset
• verify confirmation message sent to the proper (old) address

This is a small change with a potentially broad impact, so extra scrutiny is recommended.

Question: Is there ever a scenario where an unconfirmed email should be sent a message, versus the currently-confirmed one (for any event)?

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

• For DB changes, check for missing indexes, check to see if the changes
  affect other apps (such as the dashboard), make sure the DB columns in the
  various environments are properly populated, coordinate with devops, plan
  migrations in separate steps.

• For route changes, make sure GET requests don't change state or result in
  destructive behavior. GET requests should only result in information being
  read, not written.

• For encryption changes, make sure it is compatible with data that was
  encrypted with the old code.

• For secrets changes, make sure to update the S3 secrets bucket with the
  new configs in all environments.

• Do not disable Rubocop or Reek offenses unless you are absolutely sure
  they are false positives. If you're not sure how to fix the offense, please
  ask a teammate.

• When reading data, write tests for nil values, empty strings,
  and invalid formats.

• When calling redirect_to in a controller, use _url, not _path.

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

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

[brodygov]

As written this causes email change confirmations to also be sent to the old address, which allows a user to confirm any email address, even ones they do not control.

The correct behavior here should involve different logic between password reset confirmation tokens (sent to old address) and email change confirmation tokens (sent to new address).

Could you add a test that we are not sending email change confirmation messages to the old address? That would be a big vulnerability if we ever rolled that out.

At some point we also might consider adding a feature to send notification to both the new and old email addresses. Some sites have a way to disavow an email change from the old address as a way to cancel mistakes or fix account takeovers.

[brodygov]

P.S. It would also be good to find a way to ensure that the message displayed to the user "We sent an email to {email}" accurately reflects the address we really sent the message to. For password resets today we incorrectly tell the user that we sent an email to the old address, and it would be good to ensure that those can't diverge.

https://github.com/18F/identity-private/issues/2606

[mryenq]

Among the changes with this last PR is the removal of config/initializers/devise_mailer_helpers.rb, added long ago to help keep email addresses from logs. That change was effectively superceded by e20dcb9, which customizes all job-specific log output to a standard format with specified parameters (verified in dev environment, and confirmed with DevOps in production logs). Thus all jobs—not just mailers—are filtered in this way, and the devise initializer can be removed, which permits notifications to function as expected.

This has been tested with emails generated during each step of our signup and account modification process, namely the following four flows: 1) Signup, 2) Email (account) change, 3) Password (account) change, and 4) Password reset ('forgot' path). To demonstrate when and which emails are generated, beginning from a fresh config:

1. Create a new account, myname@example.com, and email confirmation
2. Confirm via email link, complete 2FA and personal key steps, Account page is now viewable
3. Change ('Edit') password via Account page, 'Your password has been changed' email is received
4. Sign out. Follow 'Forgot your password?' link and submit form, 'Reset your password' email is received
5. Follow email link and complete process, and 'Your password has been changed' email is received
6. Finally, change ('Edit') email via Account page, 'Confirm your email' email is received at new account
7. Confirm via email link (signing back in if necessary), new email is reflected on Account, and 'Change your email address' confirmation email is received, indicating the change

Returning to a test of the scenario with steps from the original report, from scratch:

1. Create and confirm a new account
2. Change the associated email address on the Account page
3. Visually confirm change email received, but do not click confirmation link
4. Log out
5. Request a password reset from sign in page, response states that email is sent to original (not changed, unconfirmed) account
6. Verify that 'Reset your password' email is addressed to and correctly received by original (not changed, unconfirmed) account, and messaging is correct

Another change that falls out of this, is the use of a Reply-To header, consistent with From, and with both emails sent directly by our app via Rails, and those indirectly sent by Devise (which sends through Rails as well, but adjusts the headers as we've seen already) along the way. Research somewhat surprisingly didn't show much discussion with regard to deliverability and the use of this header, other than potential issues if the values are different (ours uses the same 'no-reply' address for both).

Your scrutiny is kindly requested...

[brodygov]

Looks good, but please add tests that we send email confirmation messages and password reset messages to the correct respective email addresses.

[brodygov]

AFAIK the Reply-To header isn't useful unless you want replies to be sent somewhere other than the From header.

[mryenq]

That was actually added for the sake of consistency. As mentioned, our small group of generated emails follows one of two paths: either via our own Rails mailers, or Devise-generated messages. (Both ultimately call mail() in Rails.)

Devise messages include the header by default, and since we're no longer customizing those, it made sense to add it to our own mailers as well, with no discernible cost (and possibly a small boost) to deliverability. Unless we don't like the semantics of a non-reply Reply-To, in which case we would need to again customize the headers. (I'd advise not.)

[mryenq]

@brodygov Updated, with specs for the two scenarios described. (We have a ton of existing coverage around much of this functionality, but perhaps none quite describing this particular issue.)

[monfresh]

This assumes that the user's email hasn't been changed yet, which could potentially change due to a bug. To be safe, what do you think about checking for an explicit email by defining a specific email when creating the user, then filling in the password reset field with that email (the actual string as opposed to user.email), and then verifiying that the email is sent to that string?

[monfresh]

LGTM

Added requested (and subsequent) coverage