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
24 changes: 22 additions & 2 deletions app/controllers/users/delete_controller.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,39 @@
module Users
class DeleteController < ReauthnRequiredController
class DeleteController < ApplicationController
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 noticed reviewing this that this controller doesn't track any analytics events. How'd you feel about adding those here?

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.

Happy to add them! Good idea, thanks

before_action :confirm_two_factor_authenticated
before_action :confirm_current_password, only: [:delete]

def show; end
def show
analytics.track_event(Analytics::ACCOUNT_DELETE_VISITED)
end

def delete
send_push_notifications
current_user.destroy!
sign_out
flash[:success] = t('devise.registrations.destroyed')
analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: true)
redirect_to root_url
end

private

def confirm_current_password
return if valid_password?

flash[:error] = t('idv.errors.incorrect_password')
analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: false)
render :show
end

def valid_password?
current_user.valid_password?(password)
end

def password
params.fetch(:user, {})[:password].presence
end

def send_push_notifications
return if Figaro.env.push_notifications_enabled != 'true'
PushNotification::AccountDelete.new.call(current_user.id)
Expand Down
2 changes: 2 additions & 0 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ def browser_attributes

# rubocop:disable Metrics/LineLength
ACCOUNT_RESET = 'Account Reset'.freeze
ACCOUNT_DELETE_SUBMITTED = 'Account Delete submitted'.freeze
ACCOUNT_DELETE_VISITED = 'Account Delete visited'.freeze
ACCOUNT_DELETION = 'Account Deletion Requested'.freeze
ACCOUNT_RESET_VISIT = 'Account deletion and reset visited'.freeze
ACCOUNT_VISIT = 'Account Page Visited'.freeze
Expand Down
30 changes: 30 additions & 0 deletions app/views/users/delete/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<div class="p0 cntnr-xxskinny border-box bg-white rounded-xxl modal-warning">
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 slim file bites the dust 💯

<h2 class="my2 fs-20p sans-serif regular center">
<%= t('users.delete.heading') %>
</h2>
<hr class="mb3 bw4 rounded" />
<div class="mb1 bold">
<%= t('users.delete.subheading') %>
</div>
<ul class="px2">
<li class="mb1"><%= t('users.delete.bullet_1', app: APP_NAME) %></li>
<li class="mb1"><%= current_user.decorate.delete_account_bullet_key %></li>
<li class="mb1"><%= t('users.delete.bullet_3', app: APP_NAME) %></li>
</ul>
<div>
<%= simple_form_for(current_user, url: account_delete_path,
html: { autocomplete: 'off', method: :post, role: 'form' }) do |f| %>
<div class="mb3">
<%= t('users.delete.instructions') %>
</div>
<%= f.input :password, label: t('idv.form.password'), required: true %>
<%= f.button :submit,
t('users.delete.actions.delete'),
class: 'btn btn-primary col-12 mb2 p2 rounded-lg' %>
<% end %>

<%= link_to t('users.delete.actions.cancel'), account_path,
role: 'button',
class: 'center btn col-12 p2 rounded-lg border border-blue blue border-box' %>
</div>
</div>
18 changes: 0 additions & 18 deletions app/views/users/delete/show.html.slim

This file was deleted.

2 changes: 2 additions & 0 deletions config/locales/doc_auth/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ en:
start_over: Start over
take_picture: Take photo
take_picture_retry: Retake photo
upload_picture: Upload photo
upload_picture_retry: Re-upload photo
use_phone: Use your phone
forms:
address1: Address
Expand Down
2 changes: 2 additions & 0 deletions config/locales/doc_auth/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ es:
start_over: Comenzar de nuevo
take_picture: Tomar la foto
take_picture_retry: Retomar foto
upload_picture: Subir foto
upload_picture_retry: Volver a subir la foto
use_phone: Usa tu telefono
forms:
address1: Dirección
Expand Down
2 changes: 2 additions & 0 deletions config/locales/doc_auth/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ fr:
start_over: Recommencer
take_picture: Prendre une photo
take_picture_retry: Reprendre la photo
upload_picture: Envoyer la photo
upload_picture_retry: Re-télécharger la photo
use_phone: Utilisez votre téléphone
forms:
address1: Adresse
Expand Down
1 change: 1 addition & 0 deletions config/locales/users/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ en:
name, address, date of birth and Social Security number from our system."
bullet_3: You won't be able to securely access your information using %{app}.
heading: Are you sure you want to delete your account?
instructions: Enter your password to confirm that you want to delete your account.
subheading: If you delete your account
personal_key:
close: Close
Expand Down
1 change: 1 addition & 0 deletions config/locales/users/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ es:
dirección, fecha de nacimiento y número de Seguro Social de nuestro sistema."
bullet_3: Usted no podrá tener acceso seguro a su información usando %{app}
heading: "¿Está seguro que desea eliminar su cuenta?"
instructions: Ingrese su contraseña para confirmar que desea eliminar su cuenta.
subheading: Si elimina su cuenta
personal_key:
close: Cerrar
Expand Down
2 changes: 2 additions & 0 deletions config/locales/users/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ fr:
bullet_3: Vous ne pourrez pas accéder en toute sécurité à vos informations en
utilisant %{app}.
heading: Êtes-vous sûr de vouloir supprimer votre compte?
instructions: Saisissez votre mot de passe pour confirmer que vous souhaitez
supprimer votre compte.
subheading: Si vous supprimez votre compte
personal_key:
close: Fermer
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@
get '/account' => 'accounts#show'
get '/account/devices/:id/events' => 'events#show', as: :account_events
get '/account/delete' => 'users/delete#show', as: :account_delete
delete '/account/delete' => 'users/delete#delete'
post '/account/delete' => 'users/delete#delete'
get '/account/reactivate/start' => 'reactivate_account#index', as: :reactivate_account
put '/account/reactivate/start' => 'reactivate_account#update'
get '/account/reactivate/verify_password' => 'users/verify_password#new', as: :verify_password
Expand Down
63 changes: 55 additions & 8 deletions spec/controllers/users/delete_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,50 +4,97 @@
include Features::MailerHelper

describe '#show' do
it 'shows' do
it 'shows and logs a visit' do
stub_analytics
stub_signed_in_user

expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_DELETE_VISITED)

get :show

expect(response).to render_template(:show)
end
end

describe '#delete' do
let(:password) { ControllerHelper::VALID_PASSWORD }
subject(:delete) { post :delete, params: { user: { password: password } } }

context 'with an incorrect password' do
let(:password) { 'wrong' }

it 'flashes a banner and renders the form' do
stub_signed_in_user
delete
expect(response).to render_template(:show)
expect(flash[:error]).to eq(t('idv.errors.incorrect_password'))
end

it 'does not delete the user' do
stub_signed_in_user
expect { delete }.to_not change(User, :count)
end

it 'logs a failed submit' do
stub_analytics
stub_signed_in_user

expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_DELETE_SUBMITTED, success: false)

delete
end
end

it 'redirects to the root path' do
stub_signed_in_user
post :delete
delete
expect(response).to redirect_to root_url
end

it 'deletes user' do
user = stub_signed_in_user
expect(User.where(id: user.id).length).to eq(1)
post :delete
delete
expect(User.where(id: user.id).length).to eq(0)
end

it 'logs a succesful submit' do
stub_analytics
stub_signed_in_user

expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_DELETE_SUBMITTED, success: true)

delete
end

it 'does not delete identities to prevent uuid reuse' do
user = stub_signed_in_user
user.identities << Identity.create(
service_provider: 'foo',
last_authenticated_at: Time.zone.now,
)
expect(Identity.where(user_id: user.id).length).to eq(1)
post :delete
delete
expect(Identity.where(user_id: user.id).length).to eq(1)
end

it 'deletes profile information for ial2' do
profile = create(:profile, :active, :verified, pii: { ssn: '1234', dob: '1920-01-01' })
stub_sign_in(profile.user)
user = stub_sign_in
create(:profile, :active, :verified, user: user, pii: { ssn: '1234', dob: '1920-01-01' })
expect(Profile.count).to eq(1)
post :delete
delete
expect(Profile.count).to eq(0)
end
end

def stub_signed_in_user
user = create(:user, :signed_up, email: 'old_email@example.com')
user = create(:user,
:signed_up,
email: 'old_email@example.com',
password: ControllerHelper::VALID_PASSWORD)
stub_sign_in(user)
end
end
9 changes: 0 additions & 9 deletions spec/features/two_factor_authentication/change_factor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,6 @@
end
end
end

scenario 'deleting account' do
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.

cc @jmhooper this spec checked that we were prompted for 2fa before deleting account, basically testing that the controller inherited from the Reauth controller. I figured that by prompting for the password we'd be OK without, but I think that controller also checks another factor. Do you think it's still OK to remove this?

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.

Yep, I think this is obsolete now

visit account_delete_path

expect(page).to have_content t('help_text.no_factor.delete_account')
complete_2fa_confirmation

expect(current_path).to eq account_delete_path
end
end

def complete_2fa_confirmation
Expand Down
4 changes: 4 additions & 0 deletions spec/features/users/user_profile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
expect(User.count).to eq 1
expect(AgencyIdentity.count).to eq 1

fill_in(t('idv.form.password'), with: Features::SessionHelper::VALID_PASSWORD)
click_button t('users.delete.actions.delete')
expect(page).to have_content t('devise.registrations.destroyed')
expect(current_path).to eq root_path
Expand Down Expand Up @@ -59,6 +60,7 @@
},
)

fill_in(t('idv.form.password'), with: Features::SessionHelper::VALID_PASSWORD)
click_button t('users.delete.actions.delete')
expect(request).to have_been_requested
end
Expand All @@ -74,6 +76,7 @@
expect(User.count).to eq 1
expect(Profile.count).to eq 1

fill_in(t('idv.form.password'), with: profile.user.password)
click_button t('users.delete.actions.delete')
expect(page).to have_content t('devise.registrations.destroyed')
expect(current_path).to eq root_path
Expand All @@ -89,6 +92,7 @@
sign_in_live_with_2fa(profile.user)
visit account_path
click_link(t('account.links.delete_account'), href: account_delete_path)
fill_in(t('idv.form.password'), with: profile.user.password)
click_button t('users.delete.actions.delete')

expect(User.count).to eq 0
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

describe 'users/delete/show.html.slim' do
describe 'users/delete/show.html.erb' do
let(:user) { build_stubbed(:user, :signed_up) }
let(:decorated_user) { user.decorate }

Expand Down