LG-449 - Cancelling account deletion should notify both factors

[stevegsa]

Why: Other than our event log and db there is no forensic evidence of a cancellation and an attacker controlling one of the factors can cancel the deletion.

How: Add an SMS and email to the cancel controller

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

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

[stevegsa]

Screenshots:

As before we get an email notification after a delete request:

New cancel email after cancellation from either factor:

New cancel SMS:

[jmhooper]

Should this be perform_later? Not that it matters while we're using the inline adapter, but thought I'd as since we're using deliver_later above.

[jmhooper]

Also, should we add a guard clause somewhere to protect against a nil phone number?

[stevegsa]

Great catch! I also need to fix another spot on nil phone. Rather than a guard clause I'll just check for nil.

[monfresh]

+1 to only calling the job if the user has a phone, since currently, it's possible for folks to not have a number.

[jmhooper]

The name for this makes me think it is resetting an SMS account. What do you think about AccountResetCancelSmsAlertJob

[stevegsa]

That name sounds like we are cancelling an sms alert. It appears the convention is SmsXXXJob.

[monfresh]

Or maybe one of these? Cancel makes it sound like we are canceling the Sms Alert.

• AccountResetCancellationSmsAlertJob
• SmsAlertForAccountResetCancellationJob

[stevegsa]

Originally I had SmsAccountResetCancelledJob. What do you think?

[monfresh]

The convention appears to also explain what the SMS is doing, such as sending an OTP or notifying about account reset, so then maybe SmsAccountResetCancellationNotifierJob?

[monfresh]

Can we please follow the convention where the Service/Form that the controller interacts with returns a FormResponse object? That way, you only need to make one call to analytics (as opposed to one for success, and one for failure), and it makes things consistent.

[monfresh]

In other words, the User UUID would be captured as extra_analytics_attributes inside the Service Object. Here's an example: https://github.com/18F/identity-idp/blob/master/app/services/email_confirmation_token_validator.rb

[monfresh]

As would the event name and token_valid attributes. The only parameter that would go in the analytics call is result.to_h, like we do everywhere else.

[stevegsa]

While I agree with that convention, the complexity of this method does not warrant an additional object.

[monfresh]

I don't understand. The complexity of which method? What I'm proposing is instead of a generic AccountResetService class that handles all aspects of this feature, it would be broken up into smaller classes that each take care of one aspect. Here, it would be the cancellation, so you would have a CancelAccountReset class with a submit or call method.

Also, services should not have Service in their name. That's redundant. The class should describe what it is doing. If you find yourself having to add Service to the end of a class name, that's usually a code smell indicating the class doesn't have a clearly defined purpose.

[monfresh]

Another test I like to do is to try to describe what the class does in one sentence. If the sentence contains the word and, that's usually a sign it's doing too much. For example, the current AccountResetService could be described as "it handles account reset requests, and account reset cancellations, and reports fraud, and sends notifications." That's a lot of stuff for one class!

[monfresh]

That's not to say that the one public method the class has can only do one thing. The class can make calls to other services, as long as those other calls are to private methods, and usually those methods would then call other classes to perform those actions. For example:

class CancelAccountReset
  def submit(params)
    success = valid?
    if success
      notify_user_via_email 
      notify_user_via_sms
    end
  end
  
  private
  
  def notify_user_via_email
    UserMailer.send_email
  end
  
  def notify_user_via_sms
    SmsAccountResetCancellationNotifierJob.perform_now
  end
end

[stevegsa]

You misunderstood. I love breaking up the service object. In fact I used that same methodology on controllers to breakup the rails monolith and everything funneling through routes. What I was referring to was an additional response object.

[monfresh]

The complexity of the method should not determine whether or not an additional response object is necessary. It's about consistency and expectations. We expect all analytics events to have at a minimum success and errors attributes so we can easily differentiate between successful and unsuccessful events. This makes queries consistent for people searching in Kibana, or for the analytics team to run queries and monitor events. If I wanted to see what percentage of cancellations had an invalid token, I would normally look for properties.event_properties.success, but that doesn't exist in this case. So then, I would have to dig into all the properties and figure out which key to use to determine success, which in this case is a custom key called token_valid. This results in a poor developer experience.

[monfresh]

Same comment here about using a Service Object whose only purpose here is to process the deletion and validate the token, and returns a FormResponse object, much like the EmailConfirmationTokenValidator I linked to above.

[monfresh]

The AccountReset service is currently doing too much. I would break it up into smaller services that each have a single responsibility.

[monfresh]

The general rule is that a Service Object should only have one public method.

[stevegsa]

I actually love that rule. Now all the rails community needs to do is apply it to controllers too. That was partly how in another life I was able to ditch the routes file and breakup the Rails monolith. Hence why you'll notice my controller names sometimes slip in a verb (which is not appropriate in this codebase). We can refactor that service object separately because I do think it's the right way to go.

[monfresh]

What are the url_helpers used for here?

[stevegsa]

Leftover from a copy and paste of the other job that needed it for account_reset_cancel_url. I'll remove it.

[monfresh]

Can we please add a spec for when the user doesn't have a phone to make sure it doesn't send an SMS?

[stevegsa]

I added a spec to the cancel_controller. This sms job will not be called if there is not a phone.

[monfresh]

I don't think these are needed.

[stevegsa]

reset_job_queues needed ActiveJobHelper. I'll remove the url_helpers

[monfresh]

For the sake of time, we can wait until later to refactor the AccountService class, as long as it's prioritized for the next sprint.

[monfresh]

I just remembered that both this and SmsAccountResetNotifierJob need to be using the same logic as SmsOtpSenderJob to determine whether to send the SMS via Programmable SMS or via Verify, and they both need to rescue Twilio errors and log them to analytics, similar to what we do in TwoFactorAuthenticationController.

[stevegsa]

Three sms jobs are still using the old format. What are the implications of them not using this new logic?

[monfresh]

The implication is that International users might not receive these messages. The reason we purchased the Verify service is to improve deliverability internationally.

Relatedly, would it be be worth it to mention in the email that they should also expect to receive an SMS on their phone (if they have one)?

[stevegsa]

That email idea might be useful. It would also be helpful for them to get an SMS telling them when they have the email that is used to delete their account. It seems some users are forgetting to check their email and then the link expires.

[monfresh]

Steve and I talked about this privately, so I'm documenting here for future reference. I had forgotten that Verify is only for sending OTP codes using Verify's own message. Its use case is not for sending custom messages, so we'll stick with Programmable SMS for this feature.

[konklone]

One wording nit - in the email version it says the request, and in the SMS version it says your request.

If I had to pick one, I'd say your request is better, as it would more quickly (and deservedly) raise a red flag with a user if it was not actually them that did it. Our system is behaving as if it was the recipient of this message who issued the cancellation, so if that assumption is wrong, the message should look in error and concerning.

[monfresh]

LGTM