diff --git a/app/controllers/account_reset/delete_account_controller.rb b/app/controllers/account_reset/delete_account_controller.rb index a9d9d7625b8..94e02f119ec 100644 --- a/app/controllers/account_reset/delete_account_controller.rb +++ b/app/controllers/account_reset/delete_account_controller.rb @@ -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? @@ -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 diff --git a/app/controllers/account_reset/request_controller.rb b/app/controllers/account_reset/request_controller.rb index 03c07fb65cb..9c0f9dd3099 100644 --- a/app/controllers/account_reset/request_controller.rb +++ b/app/controllers/account_reset/request_controller.rb @@ -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?, ) diff --git a/app/models/account_reset_request.rb b/app/models/account_reset_request.rb index 673ea0e5d91..9080493eb66 100644 --- a/app/models/account_reset_request.rb +++ b/app/models/account_reset_request.rb @@ -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? diff --git a/app/services/account_reset/create_request.rb b/app/services/account_reset/create_request.rb index d0c9611d496..7d2dba04e4e 100644 --- a/app/services/account_reset/create_request.rb +++ b/app/services/account_reset/create_request.rb @@ -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 @@ -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) @@ -27,6 +28,7 @@ def create_request cancelled_at: nil, granted_at: nil, granted_token: nil, + requesting_issuer: requesting_issuer, ) request end diff --git a/app/services/account_reset/delete_account.rb b/app/services/account_reset/delete_account.rb index f0ceee8745e..998d5aaeac8 100644 --- a/app/services/account_reset/delete_account.rb +++ b/app/services/account_reset/delete_account.rb @@ -2,9 +2,12 @@ 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 @@ -12,6 +15,7 @@ def call track_account_age track_mfa_method_counts + track_irs_event if sp extra = extra_analytics_attributes @@ -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 diff --git a/app/services/account_reset/track_irs_event.rb b/app/services/account_reset/track_irs_event.rb new file mode 100644 index 00000000000..57a8349d419 --- /dev/null +++ b/app/services/account_reset/track_irs_event.rb @@ -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?, + 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 diff --git a/app/services/account_reset/validate_granted_token.rb b/app/services/account_reset/validate_granted_token.rb index 6558a3042ce..4550d0615d1 100644 --- a/app/services/account_reset/validate_granted_token.rb +++ b/app/services/account_reset/validate_granted_token.rb @@ -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 { diff --git a/app/validators/account_reset/granted_token_validator.rb b/app/validators/account_reset/granted_token_validator.rb index bd33ff73bfe..ac9d794d3ea 100644 --- a/app/validators/account_reset/granted_token_validator.rb +++ b/app/validators/account_reset/granted_token_validator.rb @@ -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 diff --git a/db/primary_migrate/20230309201053_add_sp_issuer_field_to_account_reset_request.rb b/db/primary_migrate/20230309201053_add_sp_issuer_field_to_account_reset_request.rb new file mode 100644 index 00000000000..44c34f3aed9 --- /dev/null +++ b/db/primary_migrate/20230309201053_add_sp_issuer_field_to_account_reset_request.rb @@ -0,0 +1,5 @@ +class AddSpIssuerFieldToAccountResetRequest < ActiveRecord::Migration[7.0] + def change + add_column :account_reset_requests, :requesting_issuer, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 7a161d48c19..2edc4ee8038 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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" @@ -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 diff --git a/spec/controllers/account_reset/delete_account_controller_spec.rb b/spec/controllers/account_reset/delete_account_controller_spec.rb index 8e0ca01ad78..1c22945b5c5 100644 --- a/spec/controllers/account_reset/delete_account_controller_spec.rb +++ b/spec/controllers/account_reset/delete_account_controller_spec.rb @@ -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 @@ -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 = { diff --git a/spec/features/account_reset/delete_account_spec.rb b/spec/features/account_reset/delete_account_spec.rb index 61d0d786310..cb0f3ef5e53 100644 --- a/spec/features/account_reset/delete_account_spec.rb +++ b/spec/features/account_reset/delete_account_spec.rb @@ -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) @@ -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 diff --git a/spec/services/account_reset/create_request_spec.rb b/spec/services/account_reset/create_request_spec.rb index b01872f278f..b8f1436c0b1 100644 --- a/spec/services/account_reset/create_request_spec.rb +++ b/spec/services/account_reset/create_request_spec.rb @@ -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 @@ -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 diff --git a/spec/services/account_reset/delete_account_spec.rb b/spec/services/account_reset/delete_account_spec.rb index f169e620689..75ed8e04f79 100644 --- a/spec/services/account_reset/delete_account_spec.rb +++ b/spec/services/account_reset/delete_account_spec.rb @@ -2,7 +2,25 @@ describe AccountReset::DeleteAccount do include AccountResetHelper + + let(:expired_token_message) do + t('errors.account_reset.granted_token_expired', app_name: APP_NAME) + end + let(:expired_token_error) { { token: [expired_token_message] } } let(:user) { create(:user) } + let(:request) { FakeRequest.new } + let(:analytics) { FakeAnalytics.new } + let(:fake_attempts_tracker) { IrsAttemptsApiTrackingHelper::FakeAttemptsTracker.new } + + let(:service_provider) do + create( + :service_provider, + active: true, + redirect_uris: ['http://localhost:7654/auth/result'], + ial: 2, + irs_attempts_api_enabled: true, + ) + end describe '#call' do it 'can be called even if DeletedUser exists' do @@ -10,7 +28,7 @@ grant_request(user) token = AccountResetRequest.where(user_id: user.id).first.granted_token DeletedUser.create_from_user(user) - AccountReset::DeleteAccount.new(token).call + AccountReset::DeleteAccount.new(token, request, analytics).call end context 'when user.confirmed_at is nil' do @@ -21,10 +39,47 @@ grant_request(user) token = AccountResetRequest.where(user_id: user.id).first.granted_token - expect { AccountReset::DeleteAccount.new(token).call }.to_not raise_error + expect do + AccountReset::DeleteAccount.new(token, request, analytics).call + end.to_not raise_error expect(User.find_by(id: user.id)).to be_nil end end + + context 'track irs event' do + before do + allow_any_instance_of(AccountReset::DeleteAccount).to receive( + :irs_attempts_api_tracker, + ).and_return(fake_attempts_tracker) + end + + it 'logs attempts api event with success true if the token is good' do + expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( + success: true, + failure_reason: nil, + ) + + create_account_reset_request_for(user, service_provider.issuer) + grant_request(user) + token = AccountResetRequest.where(user_id: user.id).first.granted_token + AccountReset::DeleteAccount.new(token, request, analytics).call + end + + it 'logs attempts api event with failure reason if the token is expired' do + expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( + success: false, + failure_reason: expired_token_error, + ) + + create_account_reset_request_for(user, service_provider.issuer) + grant_request(user) + + travel_to(Time.zone.now + 2.days) do + token = AccountResetRequest.first.granted_token + AccountReset::DeleteAccount.new(token, request, analytics).call + end + end + end end end diff --git a/spec/services/account_reset/validate_granted_token_spec.rb b/spec/services/account_reset/validate_granted_token_spec.rb new file mode 100644 index 00000000000..93b60bd773a --- /dev/null +++ b/spec/services/account_reset/validate_granted_token_spec.rb @@ -0,0 +1,49 @@ +require 'rails_helper' + +describe AccountReset::ValidateGrantedToken do + include AccountResetHelper + + let(:expired_token_message) do + t('errors.account_reset.granted_token_expired', app_name: APP_NAME) + end + let(:expired_token_error) { { token: [expired_token_message] } } + let(:user) { create(:user) } + let(:request) { FakeRequest.new } + let(:analytics) { FakeAnalytics.new } + let(:fake_attempts_tracker) { IrsAttemptsApiTrackingHelper::FakeAttemptsTracker.new } + + let(:service_provider) do + create( + :service_provider, + active: true, + redirect_uris: ['http://localhost:7654/auth/result'], + ial: 2, + irs_attempts_api_enabled: true, + ) + end + + describe '#call' do + context 'track irs event' do + before do + allow_any_instance_of(AccountReset::ValidateGrantedToken).to receive( + :irs_attempts_api_tracker, + ).and_return(fake_attempts_tracker) + end + + it 'logs attempts api event with failure reason if the token is expired' do + expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( + success: false, + failure_reason: expired_token_error, + ) + + create_account_reset_request_for(user, service_provider.issuer) + grant_request(user) + + travel_to(Time.zone.now + 2.days) do + token = AccountResetRequest.first.granted_token + AccountReset::ValidateGrantedToken.new(token, request, analytics).call + end + end + end + end +end diff --git a/spec/support/account_reset_helper.rb b/spec/support/account_reset_helper.rb index d82963627ce..54fb8e4364a 100644 --- a/spec/support/account_reset_helper.rb +++ b/spec/support/account_reset_helper.rb @@ -1,5 +1,5 @@ module AccountResetHelper - def create_account_reset_request_for(user) + def create_account_reset_request_for(user, requesting_issuer = nil) request = AccountResetRequest.create_or_find_by(user: user) request_token = SecureRandom.uuid request.update!( @@ -8,6 +8,7 @@ def create_account_reset_request_for(user) cancelled_at: nil, granted_at: nil, granted_token: nil, + requesting_issuer: requesting_issuer, ) request_token end