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
8 changes: 2 additions & 6 deletions app/controllers/account_reset/delete_account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class DeleteAccountController < ApplicationController
def show
render :show and return unless token

result = AccountReset::ValidateGrantedToken.new(token).call
result = AccountReset::ValidateGrantedToken.new(token, request, analytics).call
analytics.account_reset_granted_token_validation(**result.to_h)

if result.success?
Expand All @@ -15,13 +15,9 @@ def show

def delete
granted_token = session.delete(:granted_token)
result = AccountReset::DeleteAccount.new(granted_token).call
result = AccountReset::DeleteAccount.new(granted_token, request, analytics).call
analytics.account_reset_delete(**result.to_h.except(:email))

irs_attempts_api_tracker.account_reset_account_deleted(
success: result.success?,
failure_reason: irs_attempts_api_tracker.parse_failure_reason(result),
)
if result.success?
handle_successful_deletion(result)
else
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/account_reset/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def create
private

def create_account_reset_request
response = AccountReset::CreateRequest.new(current_user).call
response = AccountReset::CreateRequest.new(current_user, sp_session[:issuer]).call
irs_attempts_api_tracker.account_reset_request_submitted(
success: response.success?,
)
Expand Down
6 changes: 6 additions & 0 deletions app/models/account_reset_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ class AccountResetRequest < ApplicationRecord
self.ignored_columns = %w[reported_fraud_at]

belongs_to :user
# rubocop:disable Rails/InverseOf
belongs_to :requesting_service_provider,
class_name: 'ServiceProvider',
foreign_key: 'requesting_issuer',
primary_key: 'issuer'
# rubocop:enable Rails/InverseOf

def granted_token_valid?
granted_token.present? && !granted_token_expired?
Expand Down
6 changes: 4 additions & 2 deletions app/services/account_reset/create_request.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
module AccountReset
class CreateRequest
def initialize(user)
def initialize(user, requesting_issuer)
@user = user
@requesting_issuer = requesting_issuer
end

def call
Expand All @@ -17,7 +18,7 @@ def call

private

attr_reader :user
attr_reader :user, :requesting_issuer

def create_request
request = AccountResetRequest.create_or_find_by(user: user)
Expand All @@ -27,6 +28,7 @@ def create_request
cancelled_at: nil,
granted_at: nil,
granted_token: nil,
requesting_issuer: requesting_issuer,
)
request
end
Expand Down
8 changes: 6 additions & 2 deletions app/services/account_reset/delete_account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@ module AccountReset
class DeleteAccount
include ActiveModel::Model
include GrantedTokenValidator
include TrackIrsEvent

def initialize(token)
def initialize(token, request, analytics)
@token = token
@request = request
@analytics = analytics
end

def call
@success = valid?

track_account_age
track_mfa_method_counts
track_irs_event if sp

extra = extra_analytics_attributes

Expand All @@ -22,7 +26,7 @@ def call

private

attr_reader :success, :account_age, :mfa_method_counts
attr_reader :success, :account_age, :mfa_method_counts, :request, :analytics

# @return [Integer, nil] number of days since the account was confirmed (rounded) or nil if
# the account was not confirmed
Expand Down
36 changes: 36 additions & 0 deletions app/services/account_reset/track_irs_event.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Mixin for account reset event tracking
# Assumes these methods exist on the including class:
# - sp
# - success
# - errors
# - request
# - analytics
module AccountReset::TrackIrsEvent
def track_irs_event
irs_attempts_api_tracker.account_reset_account_deleted(
success: success,
failure_reason: event_failure_reason.presence,
)
end

def irs_attempts_api_tracker
@irs_attempts_api_tracker ||= IrsAttemptsApi::Tracker.new(
session_id: nil,
request: request,
user: user,
sp: sp,
cookie_device_uuid: cookies[:device],
sp_request_uri: nil,
enabled_for_session: sp.irs_attempts_api_enabled?,
Copy link
Contributor

Choose a reason for hiding this comment

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

@n1zyy this just reminder me of this line in your PR:

https://github.com/18F/identity-idp/pull/7951/files#diff-0414a45947b697e51d740c5ba32eb6c6445f4c795807250ce68106de5ff7e414R133

We have the enabled_for_session so the tracker will no-op if it's not enabled, we can do that instead of having a. nillable tracker

analytics: analytics,
)
end

def cookies
request.cookie_jar
end

def event_failure_reason
errors.is_a?(ActiveModel::Errors) ? errors.messages.to_hash : errors
end
end
9 changes: 6 additions & 3 deletions app/services/account_reset/validate_granted_token.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@ module AccountReset
class ValidateGrantedToken
include ActiveModel::Model
include GrantedTokenValidator
include TrackIrsEvent

def initialize(token)
def initialize(token, request, analytics)
@token = token
@request = request
@analytics = analytics
end

def call
@success = valid?

track_irs_event if !success && sp
FormResponse.new(success: success, errors: errors, extra: extra_analytics_attributes)
end

private

attr_reader :success
attr_reader :success, :request, :analytics

def extra_analytics_attributes
{
Expand Down
5 changes: 5 additions & 0 deletions app/validators/account_reset/granted_token_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,10 @@ def account_reset_request
def user
account_reset_request&.user || AnonymousUser.new
end

def sp
return @sp if defined?(@sp)
@sp = account_reset_request&.requesting_service_provider
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddSpIssuerFieldToAccountResetRequest < ActiveRecord::Migration[7.0]
def change
add_column :account_reset_requests, :requesting_issuer, :string
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2023_03_07_203559) do
ActiveRecord::Schema[7.0].define(version: 2023_03_09_201053) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "pgcrypto"
Expand All @@ -26,6 +26,7 @@
t.string "granted_token"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.string "requesting_issuer"
t.index ["cancelled_at", "granted_at", "requested_at"], name: "index_account_reset_requests_on_timestamps"
t.index ["granted_token"], name: "index_account_reset_requests_on_granted_token", unique: true
t.index ["request_token"], name: "index_account_reset_requests_on_request_token", unique: true
Expand Down
32 changes: 0 additions & 32 deletions spec/controllers/account_reset/delete_account_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,6 @@
expect(response).to redirect_to account_reset_confirm_delete_account_url
end

it 'logs a good token to the attempts api' do
user = create(:user, :signed_up, :with_backup_code)
create(:phone_configuration, user: user, phone: Faker::PhoneNumber.cell_phone)
create_list(:webauthn_configuration, 2, user: user)
create_account_reset_request_for(user)
grant_request(user)

session[:granted_token] = AccountResetRequest.first.granted_token
stub_attempts_tracker

expect(@irs_attempts_api_tracker).to receive(:account_reset_account_deleted).with(
success: true,
failure_reason: nil,
)

delete :delete

expect(response).to redirect_to account_reset_confirm_delete_account_url
end

it 'redirects to root if the token does not match one in the DB' do
session[:granted_token] = 'foo'
stub_analytics
Expand All @@ -74,18 +54,6 @@
expect(flash[:error]).to eq(invalid_token_message)
end

it 'logs an error in irs attempts tracker' do
session[:granted_token] = 'foo'
stub_attempts_tracker

expect(@irs_attempts_api_tracker).to receive(:account_reset_account_deleted).with(
success: false,
failure_reason: invalid_token_error,
)

delete :delete
end

it 'displays a flash and redirects to root if the token is missing' do
stub_analytics
properties = {
Expand Down
100 changes: 100 additions & 0 deletions spec/features/account_reset/delete_account_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,23 @@

describe 'Account Reset Request: Delete Account', email: true do
include PushNotificationsHelper
include OidcAuthHelper
include IrsAttemptsApiTrackingHelper

let(:user) { create(:user, :signed_up) }
let(:user_email) { user.email_addresses.first.email }
let(:push_notification_url) { 'http://localhost/push_notifications' }

let(:service_provider) do
create(
:service_provider,
active: true,
redirect_uris: ['http://localhost:7654/auth/result'],
ial: 2,
irs_attempts_api_enabled: true,
)
end

context 'as an IAL1 user' do
it 'allows the user to delete their account after 24 hours' do
signin(user_email, user.password)
Expand Down Expand Up @@ -173,4 +185,92 @@
expect(page).to have_current_path(new_user_session_path)
end
end

context 'logs IRS attempts api events' do
before do
allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true)
mock_irs_attempts_api_encryption_key
end

it 'allows the user to delete their account after 24 hours and log irs event' do
visit_idp_from_ial1_oidc_sp(
client_id: service_provider.issuer,
)

signin(user_email, user.password)
click_link t('two_factor_authentication.login_options_link_text')
click_link t('two_factor_authentication.account_reset.link')
expect(page).
to have_content strip_tags(
t('account_reset.recovery_options.try_method_again'),
)
click_link t('account_reset.request.yes_continue')
expect(page).
to have_content strip_tags(
t('account_reset.request.delete_account'),
)
click_button t('account_reset.request.yes_continue')

expect(page).
to have_content strip_tags(
t('account_reset.confirm_request.instructions_start'),
)
expect(page).
to have_content user_email
expect(page).
to have_content strip_tags(
t('account_reset.confirm_request.instructions_end'),
)
expect(page).to have_content t('account_reset.confirm_request.security_note')
expect(page).to have_content t('account_reset.confirm_request.close_window')

reset_email
set_new_browser_session
events = irs_attempts_api_tracked_events(timestamp: Time.zone.now)
expected_event_types = %w[mfa-login-phone-otp-sent login-email-and-password-auth
account-reset-request-submitted]
received_event_types = events.map(&:event_type)

expect(events.count).to eq received_event_types.count
expect(received_event_types).to match_array(expected_event_types)

travel_to(Time.zone.now + 2.days + 1.second) do
AccountReset::GrantRequestsAndSendEmails.new.perform(Time.zone.today)
open_last_email
click_email_link_matching(/delete_account\?token/)

expect(page).to have_content(t('account_reset.delete_account.title'))
expect(page).to have_current_path(account_reset_delete_account_path)

click_button t('account_reset.request.yes_continue')

expect(page).to have_content(
strip_tags(
t(
'account_reset.confirm_delete_account.info_html',
email: user_email,
link: t('account_reset.confirm_delete_account.link_text'),
),
),
)
expect(page).to have_current_path(account_reset_confirm_delete_account_path)
expect(User.where(id: user.id)).to be_empty
deleted_user = DeletedUser.find_by(user_id: user.id)
expect(deleted_user.user_id).to eq(user.id)
expect(deleted_user.uuid).to eq(user.uuid)
expect(last_email.subject).to eq t('user_mailer.account_reset_complete.subject')

click_link t('account_reset.confirm_delete_account.link_text')

expect(page).to have_current_path(sign_up_email_path)

events = irs_attempts_api_tracked_events(timestamp: Time.zone.now)
expected_event_types = %w[account-reset-account-deleted]
received_event_types = events.map(&:event_type)

expect(events.count).to eq received_event_types.count
expect(received_event_types).to match_array(expected_event_types)
end
end
end
end
14 changes: 13 additions & 1 deletion spec/services/account_reset/create_request_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
require 'rails_helper'

RSpec.describe AccountReset::CreateRequest do
subject(:create_request) { described_class.new(user) }
subject(:requesting_issuer) { 'example-issuer' }
subject(:create_request) { described_class.new(user, requesting_issuer) }

describe '#call' do
context 'when the user does not have a phone' do
Expand All @@ -23,5 +24,16 @@
expect(response.to_h[:message_id]).to be_present
end
end

context 'when requesting_issuer is passed' do
let(:user) { build(:user) }

it 'it stores requesting_issuer' do
create_request.call
reset_request = AccountResetRequest.find_by(user_id: user.id)

expect(reset_request.requesting_issuer).to eq requesting_issuer
end
end
end
end
Loading