Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .reek
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ UtilityFunction:
- LocaleHelper#locale_url_param
- IdvSession#timed_out_vendor_error
- JWT::Signature#sign
- SmsAccountResetCancellationNotifierJob#perform
'app/controllers':
InstanceVariableAssumption:
enabled: false
Expand Down
13 changes: 9 additions & 4 deletions app/controllers/account_reset/cancel_controller.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
module AccountReset
class CancelController < ApplicationController
def cancel
if AccountResetService.cancel_request(params[:token])
handle_success
account_reset = AccountResetService.cancel_request(params[:token])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if account_reset
handle_success(account_reset.user)
else
handle_failure
end
Expand All @@ -11,9 +12,13 @@ def cancel

private

def handle_success
analytics.track_event(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: true)
def handle_success(user)
analytics.track_event(Analytics::ACCOUNT_RESET,
event: :cancel, token_valid: true, user_id: user.uuid)
sign_out if current_user
UserMailer.account_reset_cancel(user.email).deliver_later
phone = user.phone
SmsAccountResetCancellationNotifierJob.perform_now(phone: phone) if phone.present?
flash[:success] = t('devise.two_factor_authentication.account_reset.successful_cancel')
end

Expand Down
9 changes: 5 additions & 4 deletions app/controllers/account_reset/delete_account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ class DeleteAccountController < ApplicationController
def show; end

def delete
analytics.track_event(Analytics::ACCOUNT_RESET, event: :delete, token_valid: true)
email = reset_session_and_set_email
user = @account_reset_request.user
analytics.track_event(Analytics::ACCOUNT_RESET,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

event: :delete, token_valid: true, user_id: user.uuid)
email = reset_session_and_set_email(user)
UserMailer.account_reset_complete(email).deliver_later
redirect_to account_reset_confirm_delete_account_url
end
Expand All @@ -19,8 +21,7 @@ def check_feature_enabled
redirect_to root_url unless FeatureManagement.account_reset_enabled?
end

def reset_session_and_set_email
user = @account_reset_request.user
def reset_session_and_set_email(user)
email = user.email
user.destroy!
sign_out
Expand Down
11 changes: 7 additions & 4 deletions app/controllers/account_reset/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,13 @@ def reset_session_with_email
end

def send_notifications
SmsAccountResetNotifierJob.perform_now(
phone: current_user.phone,
cancel_token: current_user.account_reset_request.request_token
)
phone = current_user.phone
if phone
SmsAccountResetNotifierJob.perform_now(
phone: phone,
cancel_token: current_user.account_reset_request.request_token
)
end
UserMailer.account_reset_request(current_user).deliver_later
end

Expand Down
13 changes: 13 additions & 0 deletions app/jobs/sms_account_reset_cancellation_notifier_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class SmsAccountResetCancellationNotifierJob < ApplicationJob
queue_as :sms

def perform(phone:)
TwilioService::Utils.new.send_sms(
to: phone,
body: I18n.t(
'jobs.sms_account_reset_cancel_job.message',
app: APP_NAME
)
)
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@stevegsa stevegsa Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 changes: 4 additions & 0 deletions app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,8 @@ def account_reset_granted(user, account_reset)
def account_reset_complete(email)
mail(to: email, subject: t('user_mailer.account_reset_complete.subject'))
end

def account_reset_cancel(email)
mail(to: email, subject: t('user_mailer.account_reset_cancel.subject'))
end
end
1 change: 1 addition & 0 deletions app/services/account_reset_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def self.cancel_request(token)
account_reset.update(cancelled_at: Time.zone.now,
request_token: nil,
granted_token: nil)
account_reset
end

def self.report_fraud(token)
Expand Down
16 changes: 16 additions & 0 deletions app/views/user_mailer/account_reset_cancel.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
p.lead == t('.intro', app: link_to(APP_NAME, Figaro.env.mailer_domain_name, class: 'gray'))

table.spacer
tbody
tr
td.s10 height="10px"
| &nbsp;
table.hr
tr
th
| &nbsp;

p == t('.help',
app: link_to(APP_NAME, Figaro.env.mailer_domain_name, class: 'gray'),
help_link: link_to(t('user_mailer.help_link_text'), MarketingSite.help_url),
contact_link: link_to(t('user_mailer.contact_link_text'), MarketingSite.contact_url))
2 changes: 1 addition & 1 deletion config/locales/devise/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ en:
code below.
please_try_again_html: Please try again in <strong id=%{id}>%{time_remaining}</strong>.
account_reset:
successful_cancel: Thank you. The request to delete your login.gov account
successful_cancel: Thank you. Your request to delete your login.gov account
has been cancelled.
link: deleting your account
text_html: If you can't use any of these security options above, you can reset
Expand Down
4 changes: 2 additions & 2 deletions config/locales/devise/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ es:
el código de seguridad a continuación.
please_try_again_html: Inténtelo de nuevo en <strong id=%{id}>%{time_remaining}</strong>.
account_reset:
successful_cancel: Gracias. La solicitud para eliminar su cuenta de login.gov
ha sido cancelado.
successful_cancel: Gracias. Su solicitud para eliminar su cuenta de login.gov
ha sido cancelada.
link: eliminando su cuenta
text_html: Si no puede usar ninguna de estas opciones de seguridad anteriores,
puede restablecer tus preferencias por %{link}.
Expand Down
4 changes: 2 additions & 2 deletions config/locales/devise/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ fr:
le code de sécurité ci-dessous.
please_try_again_html: Veuillez essayer de nouveau dans <strong id=%{id}>%{time_remaining}</strong>.
account_reset:
successful_cancel: Je vous remercie. La demande de suppression de votre compte
login.gov a été annulé.
successful_cancel: Je vous remercie. Votre demande de suppression de votre
compte login.gov a été annulée.
link: supprimer votre compte
text_html: Si vous ne pouvez pas utiliser l'une de ces options de sécurité
ci-dessus, vous pouvez réinitialiser vos préférences par %{link}.
Expand Down
2 changes: 2 additions & 0 deletions config/locales/jobs/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ en:
sms_otp_sender_job:
message: "%{code} is your %{app} one-time security code. This code will expire
in %{expiration} minutes."
sms_account_reset_cancel_job:
message: Your request to delete your login.gov account has been cancelled.
sms_account_reset_notifier_job:
message: 'You''ve requested to delete your login.gov account. Your request will
be processed in 24 hours. If you don’t want to delete your account, please
Expand Down
2 changes: 2 additions & 0 deletions config/locales/jobs/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ es:
sms_otp_sender_job:
message: "%{code} es su %{app} código de seguridad de sólo un uso. Este código
caducará en %{expiration} minutos."
sms_account_reset_cancel_job:
message: Su solicitud para eliminar su cuenta de login.gov ha sido cancelada.
sms_account_reset_notifier_job:
message: 'Has solicitado eliminar tu cuenta de login.gov. Su solicitud será
ser procesado en 24 horas. Si no desea eliminar su cuenta, por favor cancelar:
Expand Down
2 changes: 2 additions & 0 deletions config/locales/jobs/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ fr:
sms_otp_sender_job:
message: "%{code} est votre %{app} code de sécurité à utilisation unique. Ce
code expirera dans %{expiration} minutes."
sms_account_reset_cancel_job:
message: Votre demande de suppression de votre compte login.gov a été annulée.
sms_account_reset_notifier_job:
message: 'Vous avez demandé à supprimer votre compte login.gov. Votre demande
sera être traité en 24 heures. Si vous ne souhaitez pas supprimer votre compte,
Expand Down
5 changes: 5 additions & 0 deletions config/locales/user_mailer/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ en:
didn't request a password reset, you can ignore this message.
link_text: Create your account
subject: Your login.gov password reset request
account_reset_cancel:
intro: This email confirms you have cancelled the request to delete your login.gov
account.
subject: Delete your account
help: ''
account_reset_request:
help: ''
intro: You’ve requested to delete your login.gov account. <br><br><strong>Your
Expand Down
5 changes: 5 additions & 0 deletions config/locales/user_mailer/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ es:
intro: NOT TRANSLATED YET
link_text: NOT TRANSLATED YET
subject: NOT TRANSLATED YET
account_reset_cancel:
intro: Este correo electrónico confirma que ha cancelado la solicitud para eliminar
su cuenta de login.gov.
subject: Eliminar su cuenta
help: ''
account_reset_request:
help: ''
intro: Has solicitado eliminar tu cuenta de login.gov.<br><br><strong>Su la
Expand Down
5 changes: 5 additions & 0 deletions config/locales/user_mailer/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ fr:
intro: NOT TRANSLATED YET
link_text: NOT TRANSLATED YET
subject: NOT TRANSLATED YET
account_reset_cancel:
intro: Cet e-mail confirme que vous avez annulé la demande de suppression de
votre compte login.gov.
subject: Supprimer votre compte
help: ''
account_reset_request:
help: ''
intro: Vous avez demandé à supprimer votre compte login.gov.<br><br><strong>Votre
Expand Down
41 changes: 39 additions & 2 deletions spec/controllers/account_reset/cancel_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
require 'rails_helper'

describe AccountReset::CancelController do
let(:user) { create(:user, :signed_up, phone: '+1 (703) 555-0000') }
before do
TwilioService::Utils.telephony_service = FakeSms
end

describe '#cancel' do
it 'logs a good token to the analytics' do
user = create(:user)
AccountResetService.new(user).create_request

stub_analytics
expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: true)
with(Analytics::ACCOUNT_RESET,
event: :cancel, token_valid: true, user_id: user.uuid)

post :cancel, params: { token: AccountResetRequest.all[0].request_token }
end
Expand All @@ -25,5 +30,37 @@
post :cancel
expect(response).to redirect_to root_url
end

it 'sends an SMS if there is a phone' do
AccountResetService.new(user).create_request
allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now)

post :cancel, params: { token: AccountResetRequest.all[0].request_token }

expect(SmsAccountResetCancellationNotifierJob).to have_received(:perform_now).with(
phone: user.phone
)
end

it 'does not send an SMS if there is no phone' do
AccountResetService.new(user).create_request
allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now)
user.phone = nil
user.save!

post :cancel, params: { token: AccountResetRequest.all[0].request_token }

expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now)
end

it 'sends an email' do
AccountResetService.new(user).create_request

@mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true)
allow(UserMailer).to receive(:account_reset_cancel).with(user.email).
and_return(@mailer)

post :cancel, params: { token: AccountResetRequest.all[0].request_token }
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
session[:granted_token] = AccountResetRequest.all[0].granted_token
stub_analytics
expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_RESET, event: :delete, token_valid: true)
with(Analytics::ACCOUNT_RESET, event: :delete, token_valid: true, user_id: user.uuid)

delete :delete
end
Expand Down
38 changes: 38 additions & 0 deletions spec/jobs/sms_account_reset_cancellation_notifier_job_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
require 'rails_helper'

describe SmsAccountResetCancellationNotifierJob do
include Features::ActiveJobHelper

describe '.perform' do
before do
reset_job_queues
TwilioService::Utils.telephony_service = FakeSms
FakeSms.messages = []
end

subject(:perform) do
SmsAccountResetCancellationNotifierJob.perform_now(
phone: '+1 (888) 555-5555'
)
end

it 'sends a message to the mobile number', twilio: true do
allow(Figaro.env).to receive(:twilio_messaging_service_sid).and_return('fake_sid')

TwilioService::Utils.telephony_service = FakeSms

perform

messages = FakeSms.messages

expect(messages.size).to eq(1)

msg = messages.first

expect(msg.messaging_service_sid).to eq('fake_sid')
expect(msg.to).to eq('+1 (888) 555-5555')
expect(msg.body).
to eq(I18n.t('jobs.sms_account_reset_cancel_job.message', app: APP_NAME))
end
end
end
3 changes: 2 additions & 1 deletion spec/services/account_reset_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@
describe '#cancel_request' do
it 'removes tokens from a account reset request' do
subject.create_request
AccountResetService.cancel_request(user.account_reset_request.request_token)
cancel = AccountResetService.cancel_request(user.account_reset_request.request_token)
arr = AccountResetRequest.find_by(user_id: user.id)
expect(arr.request_token).to_not be_present
expect(arr.granted_token).to_not be_present
expect(arr.requested_at).to be_present
expect(arr.cancelled_at).to be_present
expect(arr).to eq(cancel)
end

it 'does not raise an error for a cancel request with a blank token' do
Expand Down