From 31f833693dd948fe4515b2f58774a07d300bc589 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 14 Mar 2023 14:18:50 -0400 Subject: [PATCH 01/16] squashing everything down --- .../concerns/idv/verify_info_concern.rb | 2 +- app/models/profile.rb | 24 ++++++ app/services/cloud_front_header_parser.rb | 1 + .../idv/steps/threat_metrix_step_helper.rb | 3 +- app/services/irs_attempts_api/tracker.rb | 5 +- .../irs_attempts_api/tracker_events.rb | 14 +++- .../idv/verify_info_controller_spec.rb | 6 ++ spec/models/profile_spec.rb | 78 +++++++++++++++++++ .../cloud_front_header_parser_spec.rb | 18 ++++- 9 files changed, 145 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index 72a3768cef1..5260c7d63f4 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -231,7 +231,7 @@ def add_proofing_costs(results) elsif stage == :threatmetrix # transaction_id comes from request_id tmx_id = hash[:transaction_id] - log_irs_tmx_fraud_check_event(hash) if tmx_id + log_irs_tmx_fraud_check_event(hash, current_user) if tmx_id add_cost(:threatmetrix, transaction_id: tmx_id) if tmx_id end end diff --git a/app/models/profile.rb b/app/models/profile.rb index 09c5b62a085..a356e6c64f1 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -49,6 +49,10 @@ def activate def activate_after_passing_review update!(fraud_review_pending: false, fraud_rejection: false) + irs_attempts_api_tracker&.fraud_review_adjudicated( + decision: 'pass', + fraud_fingerprint: Digest::SHA1.hexdigest(user.uuid), + ) activate end @@ -62,6 +66,10 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) + irs_attempts_api_tracker&.fraud_review_adjudicated( + decision: 'reject', + fraud_fingerprint: Digest::SHA1.hexdigest(user.uuid), + ) UserAlerts::AlertUserAboutAccountRejected.call(user) if notify_user end @@ -120,6 +128,22 @@ def has_proofed_before? Profile.where(user_id: user_id).where.not(activated_at: nil).where.not(id: self.id).exists? end + def irs_attempts_api_tracker + return @irs_attempts_api_tracker if defined?(@irs_attempts_api_tracker) + @irs_attempts_api_tracker = if initiating_service_provider&.irs_attempts_api_enabled? + IrsAttemptsApi::Tracker.new( + session_id: nil, + request: nil, + user: user, + sp: initiating_service_provider, + cookie_device_uuid: nil, + sp_request_uri: nil, + enabled_for_session: true, + analytics: nil, # FIXME, set this + ) + end + end + private def personal_key_generator diff --git a/app/services/cloud_front_header_parser.rb b/app/services/cloud_front_header_parser.rb index 11392f4dd13..fbc06d52c0e 100644 --- a/app/services/cloud_front_header_parser.rb +++ b/app/services/cloud_front_header_parser.rb @@ -10,6 +10,7 @@ def client_port # Source IP and port for client connection to CloudFront def viewer_address + return nil unless @request&.headers # guard against request being nil @request.headers['CloudFront-Viewer-Address'] end end diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 62eae9de24a..271ba3886b5 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -47,7 +47,7 @@ def threatmetrix_iframe_url(session_id) ) end - def log_irs_tmx_fraud_check_event(result) + def log_irs_tmx_fraud_check_event(result, user) return unless IdentityConfig.store.irs_attempt_api_track_tmx_fraud_check_event return unless FeatureManagement.proofing_device_profiling_collecting_enabled? success = result[:review_status] == 'pass' @@ -63,6 +63,7 @@ def log_irs_tmx_fraud_check_event(result) irs_attempts_api_tracker.idv_tmx_fraud_check( success: success, + fraud_fingerprint: Digest::SHA1.hexdigest(user&.uuid), failure_reason: failure_reason, ) end diff --git a/app/services/irs_attempts_api/tracker.rb b/app/services/irs_attempts_api/tracker.rb index a4d64d4a07c..8adfa8aed24 100644 --- a/app/services/irs_attempts_api/tracker.rb +++ b/app/services/irs_attempts_api/tracker.rb @@ -16,7 +16,7 @@ def initialize(session_id:, request:, user:, sp:, cookie_device_uuid:, end def track_event(event_type, metadata = {}) - return if !enabled? + return unless enabled? if metadata.has_key?(:failure_reason) && (metadata[:failure_reason].blank? || @@ -43,7 +43,8 @@ def track_event(event_type, metadata = {}) if enabled? if IdentityConfig.store.irs_attempt_api_payload_size_logging_enabled - analytics.irs_attempts_api_event_metadata( + # analytics may be nil when called from outside ApplicationController + analytics&.irs_attempts_api_event_metadata( event_type: event_type, unencrypted_payload_num_bytes: event.payload_json.bytesize, recorded: true, diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index 4338a16b002..ea7f95d0e3b 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -67,6 +67,17 @@ def forgot_password_new_password_submitted(success:, failure_reason: nil) ) end + # @param [String] decision One of 'reject' or 'pass' + # @param [String] fraud_fingerprint An opaque identifier to correlate with a TMX check + # A profile offlined for review has been approved or rejected. + def fraud_review_adjudicated(decision:, fraud_fingerprint:) + track_event( + :fraud_review_adjudicated, + decision: decision, + fraud_fingerprint: fraud_fingerprint, + ) + end + # @param ["mobile", "desktop"] upload_method method chosen for uploading id verification # A user has selected id document upload method def idv_document_upload_method_selected(upload_method:) @@ -285,10 +296,11 @@ def idv_ssn_submitted(ssn:) # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification - def idv_tmx_fraud_check(success:, failure_reason: nil) + def idv_tmx_fraud_check(success:, fraud_fingerprint:, failure_reason: nil) track_event( :idv_tmx_fraud_check, success: success, + fraud_fingerprint: fraud_fingerprint, failure_reason: failure_reason, ) end diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index ef51bd651bd..63d4b7ca637 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -201,6 +201,8 @@ document_capture_session end + let(:user_hash_uuid) { Digest::SHA1.hexdigest(subject.current_user.uuid) } + let(:expected_failure_reason) { DocAuthHelper::SAMPLE_TMX_SUMMARY_REASON_CODE } before do @@ -217,6 +219,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: true, + fraud_fingerprint: user_hash_uuid, failure_reason: nil, ) get :show @@ -229,6 +232,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, + fraud_fingerprint: user_hash_uuid, failure_reason: expected_failure_reason, ) get :show @@ -241,6 +245,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, + fraud_fingerprint: user_hash_uuid, failure_reason: expected_failure_reason, ) get :show @@ -253,6 +258,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, + fraud_fingerprint: user_hash_uuid, failure_reason: expected_failure_reason, ) get :show diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index b79ceaf2990..f8b82a5fe70 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -2,6 +2,7 @@ describe Profile do let(:user) { create(:user, :signed_up, password: 'a really long sekrit') } + let(:hashed_uuid) { Digest::SHA1.hexdigest(user.uuid) } let(:another_user) { create(:user, :signed_up) } let(:profile) { create(:profile, user: user) } @@ -299,6 +300,42 @@ expect(profile).to be_active end + + context 'when the initiating_sp is the IRS' do + it 'logs an attempt event' do + sp = create(:service_provider, :irs) + profile = create( + :profile, + user: user, + active: false, + fraud_review_pending: true, + initiating_service_provider: sp, + ) + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + + expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). + with(decision: 'pass', fraud_fingerprint: hashed_uuid) + profile.activate_after_passing_review + end + end + + context 'when the initiating_sp is not the IRS' do + it 'does not log an attempt event' do + sp = create(:service_provider) + profile = create( + :profile, + user: user, + active: false, + fraud_review_pending: true, + initiating_service_provider: sp, + ) + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_falsey + + expect(profile.irs_attempts_api_tracker).to be nil + profile.activate_after_passing_review + end + end end describe '#deactivate_for_fraud_review' do @@ -353,6 +390,47 @@ expect { profile }.to change(ActionMailer::Base.deliveries, :count).by(0) end end + + context 'when the SP is the IRS' do + it 'logs an event' do + sp = create(:service_provider, :irs) + profile = create( + :profile, + user: user, + active: false, + fraud_review_pending: true, + initiating_service_provider: sp, + ) + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + + expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). + with(decision: 'reject', fraud_fingerprint: hashed_uuid) + + profile.reject_for_fraud(notify_user: true) + end + end + + context 'when the SP is not the IRS' do + it 'does not log an event' do + sp = create(:service_provider) + profile = create( + :profile, + user: user, + active: false, + fraud_review_pending: true, + initiating_service_provider: sp, + ) + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_falsey + + expect(profile.irs_attempts_api_tracker).to be nil + + profile.reject_for_fraud(notify_user: true) + end + end end describe 'scopes' do diff --git a/spec/services/cloud_front_header_parser_spec.rb b/spec/services/cloud_front_header_parser_spec.rb index 159bd2df848..927c084fee7 100644 --- a/spec/services/cloud_front_header_parser_spec.rb +++ b/spec/services/cloud_front_header_parser_spec.rb @@ -39,7 +39,23 @@ describe '#client_port' do it 'returns nil' do - expect(subject.client_port).to eq nil + expect(subject.client_port).to be nil + end + end + end + + context 'with no request included' do + let(:req) { nil } + + describe '#viewer_address' do + it 'returns nil' do + expect(subject.viewer_address).to be nil + end + end + + describe '#client_port' do + it 'returns nil' do + expect(subject.client_port).to be nil end end end From a40288f50481a06e6e404e2c89c173cd0f8a03ad Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Tue, 14 Mar 2023 12:39:47 -0400 Subject: [PATCH 02/16] Add analytics; work around silly rubocop gripe --- app/models/profile.rb | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index a356e6c64f1..809a8bb1217 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -130,17 +130,25 @@ def has_proofed_before? def irs_attempts_api_tracker return @irs_attempts_api_tracker if defined?(@irs_attempts_api_tracker) - @irs_attempts_api_tracker = if initiating_service_provider&.irs_attempts_api_enabled? - IrsAttemptsApi::Tracker.new( - session_id: nil, - request: nil, - user: user, - sp: initiating_service_provider, - cookie_device_uuid: nil, - sp_request_uri: nil, - enabled_for_session: true, - analytics: nil, # FIXME, set this - ) + analytics = Analytics.new( + user: user, + request: nil, + sp: initiating_service_provider&.issuer, + session: {}, + ahoy: nil + ) + if initiating_service_provider&.irs_attempts_api_enabled? + @irs_attempts_api_tracker + IrsAttemptsApi::Tracker.new( + session_id: nil, + request: nil, + user: user, + sp: initiating_service_provider, + cookie_device_uuid: nil, + sp_request_uri: nil, + enabled_for_session: true, + analytics: analytics, + ) end end From 221109cd2621ea7a85106016f1ccefcb16ec8f26 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Tue, 14 Mar 2023 17:59:16 -0400 Subject: [PATCH 03/16] Now with valid syntax! changlog: Internal, Attempts API, Log events for fraud rejection --- app/models/profile.rb | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 809a8bb1217..5fffee7b6ad 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -138,17 +138,16 @@ def irs_attempts_api_tracker ahoy: nil ) if initiating_service_provider&.irs_attempts_api_enabled? - @irs_attempts_api_tracker - IrsAttemptsApi::Tracker.new( - session_id: nil, - request: nil, - user: user, - sp: initiating_service_provider, - cookie_device_uuid: nil, - sp_request_uri: nil, - enabled_for_session: true, - analytics: analytics, - ) + @irs_attempts_api_tracker = IrsAttemptsApi::Tracker.new( + session_id: nil, + request: nil, + user: user, + sp: initiating_service_provider, + cookie_device_uuid: nil, + sp_request_uri: nil, + enabled_for_session: true, + analytics: analytics, + ) end end From 57d15bce35aa0953e1e4a4153cf41f1c86cd9074 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Tue, 14 Mar 2023 18:12:42 -0400 Subject: [PATCH 04/16] Indicate whether this is an automatic or manual rejection --- app/models/profile.rb | 2 +- spec/models/profile_spec.rb | 34 ++++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 5fffee7b6ad..bddbd5938e6 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -67,7 +67,7 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) irs_attempts_api_tracker&.fraud_review_adjudicated( - decision: 'reject', + decision: notify_user ? 'manual_reject' : 'automatic_reject', fraud_fingerprint: Digest::SHA1.hexdigest(user.uuid), ) UserAlerts::AlertUserAboutAccountRejected.call(user) if notify_user diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index f8b82a5fe70..e0aeff49d11 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -392,23 +392,41 @@ end context 'when the SP is the IRS' do - it 'logs an event' do - sp = create(:service_provider, :irs) - profile = create( + let(:sp) { create(:service_provider, :irs) } + let(:profile) do + create( :profile, user: user, active: false, fraud_review_pending: true, initiating_service_provider: sp, ) - allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + end - expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + context 'and notify_user is true' do + it 'logs an event with manual_reject' do + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) - expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). - with(decision: 'reject', fraud_fingerprint: hashed_uuid) + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy - profile.reject_for_fraud(notify_user: true) + expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). + with(decision: 'manual_reject', fraud_fingerprint: hashed_uuid) + + profile.reject_for_fraud(notify_user: true) + end + end + + context 'and notify_user is false' do + it 'logs an event with automatic_reject' do + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + + expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). + with(decision: 'automatic_reject', fraud_fingerprint: hashed_uuid) + + profile.reject_for_fraud(notify_user: false) + end end end From aa5912db9e9bd655658f285b1ea987674e0e599b Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 15 Mar 2023 10:33:00 -0400 Subject: [PATCH 05/16] Remove extraneous comment changelog: Internal, Attempts API, Log events for fraud rejection (LG-8664) --- app/services/cloud_front_header_parser.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/cloud_front_header_parser.rb b/app/services/cloud_front_header_parser.rb index fbc06d52c0e..f350ae2d38a 100644 --- a/app/services/cloud_front_header_parser.rb +++ b/app/services/cloud_front_header_parser.rb @@ -10,7 +10,7 @@ def client_port # Source IP and port for client connection to CloudFront def viewer_address - return nil unless @request&.headers # guard against request being nil + return nil unless @request&.headers @request.headers['CloudFront-Viewer-Address'] end end From 58dc792306d1c3d89960f4bbfcad4bfd4c4c0226 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 15 Mar 2023 16:31:23 -0400 Subject: [PATCH 06/16] Trivial rubocop fix --- app/models/profile.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index bddbd5938e6..c43c9ee0132 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -135,7 +135,7 @@ def irs_attempts_api_tracker request: nil, sp: initiating_service_provider&.issuer, session: {}, - ahoy: nil + ahoy: nil, ) if initiating_service_provider&.irs_attempts_api_enabled? @irs_attempts_api_tracker = IrsAttemptsApi::Tracker.new( From 379362af64b8dda320bcd56b09fa6c59b8cfd2ce Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 14:04:05 -0400 Subject: [PATCH 07/16] Adds FraudEvent and supporting classes Needs text cleanup and feature flag still --- app/models/fraud_event.rb | 10 ++++++++++ app/models/profile.rb | 10 ++++++++-- app/models/user.rb | 1 + app/services/idv/steps/threat_metrix_step_helper.rb | 12 +++++++++++- app/services/irs_attempts_api/tracker_events.rb | 11 +++++++---- .../20230322000756_create_fraud_events.rb | 11 +++++++++++ db/schema.rb | 10 +++++++++- 7 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 app/models/fraud_event.rb create mode 100644 db/primary_migrate/20230322000756_create_fraud_events.rb diff --git a/app/models/fraud_event.rb b/app/models/fraud_event.rb new file mode 100644 index 00000000000..e17da71ea5d --- /dev/null +++ b/app/models/fraud_event.rb @@ -0,0 +1,10 @@ +# Feel free to propose a better name. +# We need to stash the user's irs_session_id when a user is flagged for fraud. +# The IRS asked us to store our session ID, too. +# We give this back to them when a user is manually approved/rejected. +# It COULD make sense to put other stuff here and make this the primary way of tracking this, +# but the immediate need is to just have a way to link this data back to a user before there +# is a ServiceProviderIdentity. +class FraudEvent < ApplicationRecord + belongs_to :user +end diff --git a/app/models/profile.rb b/app/models/profile.rb index c43c9ee0132..d941874e9d1 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -49,9 +49,12 @@ def activate def activate_after_passing_review update!(fraud_review_pending: false, fraud_rejection: false) + fraud_event = user.fraud_events&.last irs_attempts_api_tracker&.fraud_review_adjudicated( decision: 'pass', - fraud_fingerprint: Digest::SHA1.hexdigest(user.uuid), + fraud_event_id: fraud_event&.id, + cached_irs_session_id: fraud_event&.irs_session_id, + cached_login_session_id: fraud_event&.login_session_id, ) activate end @@ -66,9 +69,12 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) + fraud_event = user.fraud_events&.last # TBD - what if this isn't found? irs_attempts_api_tracker&.fraud_review_adjudicated( decision: notify_user ? 'manual_reject' : 'automatic_reject', - fraud_fingerprint: Digest::SHA1.hexdigest(user.uuid), + fraud_event__id: fraud_event&.id, + cached_irs_session_id: fraud_event&.irs_session_id, + cached_login_session_id: fraud_event&.login_session_id, ) UserAlerts::AlertUserAboutAccountRejected.call(user) if notify_user end diff --git a/app/models/user.rb b/app/models/user.rb index fb50ed5bd1e..76f57673791 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,6 +44,7 @@ class User < ApplicationRecord source: :service_provider_record has_many :sign_in_restrictions, dependent: :destroy has_many :in_person_enrollments, dependent: :destroy + has_many :fraud_events # TBD, do we want to support multiple? has_one :pending_in_person_enrollment, -> { where(status: :pending).order(created_at: :desc) }, diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 271ba3886b5..287af27ad3b 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -50,6 +50,16 @@ def threatmetrix_iframe_url(session_id) def log_irs_tmx_fraud_check_event(result, user) return unless IdentityConfig.store.irs_attempt_api_track_tmx_fraud_check_event return unless FeatureManagement.proofing_device_profiling_collecting_enabled? + + # This doesn't really feel like the right place, but there's not a more obvious one. + # We currently only need this for Attempts API stuff. + fraud_event = FraudEvent.create( + user: user, + irs_session_id: irs_attempts_api_session_id, + # I think the to_s here is only needed in badly-constructed tests? + login_session_id: Digest::SHA1.hexdigest(user&.unique_session_id.to_s), + ) + success = result[:review_status] == 'pass' if !success && (tmx_summary_reason_code = result.dig( @@ -63,7 +73,7 @@ def log_irs_tmx_fraud_check_event(result, user) irs_attempts_api_tracker.idv_tmx_fraud_check( success: success, - fraud_fingerprint: Digest::SHA1.hexdigest(user&.uuid), + fraud_event_id: fraud_event.id, failure_reason: failure_reason, ) end diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index ea7f95d0e3b..453244934f0 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -70,11 +70,14 @@ def forgot_password_new_password_submitted(success:, failure_reason: nil) # @param [String] decision One of 'reject' or 'pass' # @param [String] fraud_fingerprint An opaque identifier to correlate with a TMX check # A profile offlined for review has been approved or rejected. - def fraud_review_adjudicated(decision:, fraud_fingerprint:) + def fraud_review_adjudicated(decision:, fraud_event_id:, cached_irs_session_id:, + cached_login_session_id:) track_event( :fraud_review_adjudicated, decision: decision, - fraud_fingerprint: fraud_fingerprint, + fraud_event_id: fraud_event_id, + cached_irs_session_id: cached_irs_session_id, + cached_login_session_id: cached_login_session_id, ) end @@ -296,11 +299,11 @@ def idv_ssn_submitted(ssn:) # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification - def idv_tmx_fraud_check(success:, fraud_fingerprint:, failure_reason: nil) + def idv_tmx_fraud_check(success:, fraud_event_id:, failure_reason: nil) track_event( :idv_tmx_fraud_check, success: success, - fraud_fingerprint: fraud_fingerprint, + fraud_event_id: fraud_event_id, failure_reason: failure_reason, ) end diff --git a/db/primary_migrate/20230322000756_create_fraud_events.rb b/db/primary_migrate/20230322000756_create_fraud_events.rb new file mode 100644 index 00000000000..eabe2fdca2a --- /dev/null +++ b/db/primary_migrate/20230322000756_create_fraud_events.rb @@ -0,0 +1,11 @@ +class CreateFraudEvents < ActiveRecord::Migration[7.0] + def change + create_table :fraud_events do |t| + t.integer :user_id + t.string :irs_session_id + t.string :login_session_id + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 2edc4ee8038..1e393ec9870 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_09_201053) do +ActiveRecord::Schema[7.0].define(version: 2023_03_22_000756) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "pgcrypto" @@ -226,6 +226,14 @@ t.index ["user_id", "created_at"], name: "index_events_on_user_id_and_created_at" end + create_table "fraud_events", force: :cascade do |t| + t.integer "user_id" + t.string "irs_session_id" + t.string "login_session_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "iaa_gtcs", force: :cascade do |t| t.string "gtc_number", null: false t.integer "mod_number", default: 0, null: false From 86ca981f3568597aee82503ad29d02d6aa0c7a08 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 16:21:43 -0400 Subject: [PATCH 08/16] Some cleanup --- app/models/profile.rb | 2 +- .../idv/steps/threat_metrix_step_helper.rb | 31 +++++++++---------- .../idv/verify_info_controller_spec.rb | 13 ++++---- spec/models/profile_spec.rb | 24 ++++++++++++-- 4 files changed, 43 insertions(+), 27 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index d941874e9d1..97f0b41cad1 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -72,7 +72,7 @@ def reject_for_fraud(notify_user:) fraud_event = user.fraud_events&.last # TBD - what if this isn't found? irs_attempts_api_tracker&.fraud_review_adjudicated( decision: notify_user ? 'manual_reject' : 'automatic_reject', - fraud_event__id: fraud_event&.id, + fraud_event_id: fraud_event&.id, cached_irs_session_id: fraud_event&.irs_session_id, cached_login_session_id: fraud_event&.login_session_id, ) diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 287af27ad3b..c6335f6f6ff 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -51,30 +51,27 @@ def log_irs_tmx_fraud_check_event(result, user) return unless IdentityConfig.store.irs_attempt_api_track_tmx_fraud_check_event return unless FeatureManagement.proofing_device_profiling_collecting_enabled? - # This doesn't really feel like the right place, but there's not a more obvious one. - # We currently only need this for Attempts API stuff. - fraud_event = FraudEvent.create( - user: user, - irs_session_id: irs_attempts_api_session_id, - # I think the to_s here is only needed in badly-constructed tests? - login_session_id: Digest::SHA1.hexdigest(user&.unique_session_id.to_s), - ) - success = result[:review_status] == 'pass' - if !success && (tmx_summary_reason_code = result.dig( - :response_body, - :tmx_summary_reason_code, - )) - failure_reason = { - tmx_summary_reason_code: tmx_summary_reason_code, - } + unless success + fraud_event = FraudEvent.create( + user: user, + irs_session_id: irs_attempts_api_session_id, + # I think the to_s here is only needed in badly-constructed tests? + login_session_id: Digest::SHA1.hexdigest(user&.unique_session_id.to_s), + ) + + if (tmx_summary_reason_code = result.dig(:response_body,:tmx_summary_reason_code)) + failure_reason = { + tmx_summary_reason_code: tmx_summary_reason_code, + } + end end irs_attempts_api_tracker.idv_tmx_fraud_check( success: success, - fraud_event_id: fraud_event.id, failure_reason: failure_reason, + fraud_event_id: fraud_event&.id, ) end end diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index 07e98c1fc48..4c5c37098ba 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -201,8 +201,6 @@ document_capture_session end - let(:user_hash_uuid) { Digest::SHA1.hexdigest(subject.current_user.uuid) } - let(:expected_failure_reason) { DocAuthHelper::SAMPLE_TMX_SUMMARY_REASON_CODE } before do @@ -211,6 +209,9 @@ allow(IdentityConfig.store).to receive(:proofing_device_profiling).and_return(:enabled) allow(IdentityConfig.store).to receive(:irs_attempt_api_track_tmx_fraud_check_event). and_return(true) + # maw: This is rather inelegant, but gets tests passing for the moment. + # Should really stub this out better. + allow_any_instance_of(FraudEvent).to receive(:id).and_return(123) end context 'when threatmetrix response is Pass' do @@ -219,7 +220,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: true, - fraud_fingerprint: user_hash_uuid, + fraud_event_id: nil, failure_reason: nil, ) get :show @@ -232,7 +233,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_fingerprint: user_hash_uuid, + fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show @@ -245,7 +246,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_fingerprint: user_hash_uuid, + fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show @@ -258,7 +259,7 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_fingerprint: user_hash_uuid, + fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index e0aeff49d11..2d24b8baad0 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -315,7 +315,13 @@ expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). - with(decision: 'pass', fraud_fingerprint: hashed_uuid) + with( + decision: 'pass', + # FIXME: stub these out somewhere, or should we move this test? + fraud_event_id: nil, + cached_irs_session_id: nil, + cached_login_session_id: nil, + ) profile.activate_after_passing_review end end @@ -410,7 +416,13 @@ expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). - with(decision: 'manual_reject', fraud_fingerprint: hashed_uuid) + with( + decision: 'manual_reject', + # FIXME: These shouldn't be nil. + fraud_event_id: nil, + cached_irs_session_id: nil, + cached_login_session_id: nil, + ) profile.reject_for_fraud(notify_user: true) end @@ -423,7 +435,13 @@ expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). - with(decision: 'automatic_reject', fraud_fingerprint: hashed_uuid) + with( + decision: 'automatic_reject', + # FIXME: + fraud_event_id: nil, + cached_irs_session_id: nil, + cached_login_session_id: nil, + ) profile.reject_for_fraud(notify_user: false) end From 9abe73c4196b3386d73371e30c6490e5a8801afc Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 16:57:42 -0400 Subject: [PATCH 09/16] User has_one, not _many, fraud_event(s) --- app/models/profile.rb | 4 ++-- app/models/user.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 97f0b41cad1..a0639ccbe23 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -49,7 +49,7 @@ def activate def activate_after_passing_review update!(fraud_review_pending: false, fraud_rejection: false) - fraud_event = user.fraud_events&.last + fraud_event = user.fraud_event irs_attempts_api_tracker&.fraud_review_adjudicated( decision: 'pass', fraud_event_id: fraud_event&.id, @@ -69,7 +69,7 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) - fraud_event = user.fraud_events&.last # TBD - what if this isn't found? + fraud_event = user.fraud_event irs_attempts_api_tracker&.fraud_review_adjudicated( decision: notify_user ? 'manual_reject' : 'automatic_reject', fraud_event_id: fraud_event&.id, diff --git a/app/models/user.rb b/app/models/user.rb index 76f57673791..4e87f9c2791 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,7 +44,7 @@ class User < ApplicationRecord source: :service_provider_record has_many :sign_in_restrictions, dependent: :destroy has_many :in_person_enrollments, dependent: :destroy - has_many :fraud_events # TBD, do we want to support multiple? + has_one :fraud_event, dependent: :destroy has_one :pending_in_person_enrollment, -> { where(status: :pending).order(created_at: :desc) }, From 73e54d920f648c3308c896ce13797c9fc7e029ea Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 17:12:26 -0400 Subject: [PATCH 10/16] Clean up yard docs, remove unnecessary nil-safe operator --- app/services/irs_attempts_api/tracker.rb | 3 +-- app/services/irs_attempts_api/tracker_events.rb | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/services/irs_attempts_api/tracker.rb b/app/services/irs_attempts_api/tracker.rb index 8adfa8aed24..2067cf4cbda 100644 --- a/app/services/irs_attempts_api/tracker.rb +++ b/app/services/irs_attempts_api/tracker.rb @@ -43,8 +43,7 @@ def track_event(event_type, metadata = {}) if enabled? if IdentityConfig.store.irs_attempt_api_payload_size_logging_enabled - # analytics may be nil when called from outside ApplicationController - analytics&.irs_attempts_api_event_metadata( + analytics.irs_attempts_api_event_metadata( event_type: event_type, unencrypted_payload_num_bytes: event.payload_json.bytesize, recorded: true, diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index 453244934f0..9f1f9cc9a5f 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -67,8 +67,10 @@ def forgot_password_new_password_submitted(success:, failure_reason: nil) ) end - # @param [String] decision One of 'reject' or 'pass' - # @param [String] fraud_fingerprint An opaque identifier to correlate with a TMX check + # @param [String] decision One of 'pass', 'manual_reject', or 'automated_reject' + # @param [Integer] fraud_event_id Can be used to correlate with an idv_tmx_fraud_check event + # @param [String] cached_irs_session_id The IRS session id ('tid') the user had when flagged + # @param [String] cached_login_session_id The Login.gov session id the user had when flagged # A profile offlined for review has been approved or rejected. def fraud_review_adjudicated(decision:, fraud_event_id:, cached_irs_session_id:, cached_login_session_id:) @@ -296,6 +298,7 @@ def idv_ssn_submitted(ssn:) end # @param [Boolean] success + # @param [Integer] fraud_event_id - Can be used to correlate events with fraud_review_adjudicated events # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification From 718b76b992b0b270a7124582acf8dd493d29b731 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 18:19:59 -0400 Subject: [PATCH 11/16] lint fixes --- app/services/idv/steps/threat_metrix_step_helper.rb | 2 +- app/services/irs_attempts_api/tracker_events.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index c6335f6f6ff..77b66c6021d 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -61,7 +61,7 @@ def log_irs_tmx_fraud_check_event(result, user) login_session_id: Digest::SHA1.hexdigest(user&.unique_session_id.to_s), ) - if (tmx_summary_reason_code = result.dig(:response_body,:tmx_summary_reason_code)) + if (tmx_summary_reason_code = result.dig(:response_body, :tmx_summary_reason_code)) failure_reason = { tmx_summary_reason_code: tmx_summary_reason_code, } diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index 9f1f9cc9a5f..b5ec7773a3c 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -298,7 +298,7 @@ def idv_ssn_submitted(ssn:) end # @param [Boolean] success - # @param [Integer] fraud_event_id - Can be used to correlate events with fraud_review_adjudicated events + # @param [Integer] fraud_event_id - Maps to a fraud_review_adjudicated event # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification From 9c6adccceaa58930a3064f69f6b6654cf63b18a1 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 23 Mar 2023 19:33:28 -0400 Subject: [PATCH 12/16] PR feedback --- app/models/fraud_event.rb | 10 ---------- app/models/fraud_review_request.rb | 5 +++++ app/models/phone_number_opt_out.rb | 2 +- app/models/profile.rb | 14 ++++++-------- app/models/user.rb | 2 +- .../idv/steps/threat_metrix_step_helper.rb | 6 ++---- app/services/irs_attempts_api/tracker_events.rb | 9 ++------- .../20230322000756_create_fraud_events.rb | 11 ----------- .../20230322000756_create_fraud_review_requests.rb | 14 ++++++++++++++ db/schema.rb | 4 +++- .../controllers/idv/verify_info_controller_spec.rb | 7 ------- spec/models/profile_spec.rb | 5 ----- 12 files changed, 34 insertions(+), 55 deletions(-) delete mode 100644 app/models/fraud_event.rb create mode 100644 app/models/fraud_review_request.rb delete mode 100644 db/primary_migrate/20230322000756_create_fraud_events.rb create mode 100644 db/primary_migrate/20230322000756_create_fraud_review_requests.rb diff --git a/app/models/fraud_event.rb b/app/models/fraud_event.rb deleted file mode 100644 index e17da71ea5d..00000000000 --- a/app/models/fraud_event.rb +++ /dev/null @@ -1,10 +0,0 @@ -# Feel free to propose a better name. -# We need to stash the user's irs_session_id when a user is flagged for fraud. -# The IRS asked us to store our session ID, too. -# We give this back to them when a user is manually approved/rejected. -# It COULD make sense to put other stuff here and make this the primary way of tracking this, -# but the immediate need is to just have a way to link this data back to a user before there -# is a ServiceProviderIdentity. -class FraudEvent < ApplicationRecord - belongs_to :user -end diff --git a/app/models/fraud_review_request.rb b/app/models/fraud_review_request.rb new file mode 100644 index 00000000000..d4f8a39325a --- /dev/null +++ b/app/models/fraud_review_request.rb @@ -0,0 +1,5 @@ +class FraudReviewRequest < ApplicationRecord + include NonNullUuid + + belongs_to :user +end diff --git a/app/models/phone_number_opt_out.rb b/app/models/phone_number_opt_out.rb index 5f85cd2bff8..868e26c5781 100644 --- a/app/models/phone_number_opt_out.rb +++ b/app/models/phone_number_opt_out.rb @@ -1,4 +1,4 @@ -# Represents a record of a phone number that has beed opted out of SMS in AWS Pinpoint +# Represents a record of a phone number that has been opted out of SMS in AWS Pinpoint # AWS maintains separate opt-out lists per region, so this helps us keep track across regions class PhoneNumberOptOut < ApplicationRecord include NonNullUuid diff --git a/app/models/profile.rb b/app/models/profile.rb index a0639ccbe23..27b69180ffe 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -49,12 +49,11 @@ def activate def activate_after_passing_review update!(fraud_review_pending: false, fraud_rejection: false) - fraud_event = user.fraud_event + fraud_review_request = user.fraud_review_requests.last irs_attempts_api_tracker&.fraud_review_adjudicated( decision: 'pass', - fraud_event_id: fraud_event&.id, - cached_irs_session_id: fraud_event&.irs_session_id, - cached_login_session_id: fraud_event&.login_session_id, + cached_irs_session_id: fraud_review_request&.irs_session_id, + cached_login_session_id: fraud_review_request&.login_session_id, ) activate end @@ -69,12 +68,11 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) - fraud_event = user.fraud_event + fraud_review_request = user.fraud_review_requests.last irs_attempts_api_tracker&.fraud_review_adjudicated( decision: notify_user ? 'manual_reject' : 'automatic_reject', - fraud_event_id: fraud_event&.id, - cached_irs_session_id: fraud_event&.irs_session_id, - cached_login_session_id: fraud_event&.login_session_id, + cached_irs_session_id: fraud_review_request&.irs_session_id, + cached_login_session_id: fraud_review_request&.login_session_id, ) UserAlerts::AlertUserAboutAccountRejected.call(user) if notify_user end diff --git a/app/models/user.rb b/app/models/user.rb index 4e87f9c2791..740db97ce74 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,7 +44,7 @@ class User < ApplicationRecord source: :service_provider_record has_many :sign_in_restrictions, dependent: :destroy has_many :in_person_enrollments, dependent: :destroy - has_one :fraud_event, dependent: :destroy + has_many :fraud_review_requests, dependent: :destroy has_one :pending_in_person_enrollment, -> { where(status: :pending).order(created_at: :desc) }, diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 77b66c6021d..2bc215a011e 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -54,11 +54,10 @@ def log_irs_tmx_fraud_check_event(result, user) success = result[:review_status] == 'pass' unless success - fraud_event = FraudEvent.create( + fraud_event = FraudReviewRequest.create( user: user, irs_session_id: irs_attempts_api_session_id, - # I think the to_s here is only needed in badly-constructed tests? - login_session_id: Digest::SHA1.hexdigest(user&.unique_session_id.to_s), + login_session_id: Digest::SHA1.hexdigest(user.unique_session_id.to_s), ) if (tmx_summary_reason_code = result.dig(:response_body, :tmx_summary_reason_code)) @@ -71,7 +70,6 @@ def log_irs_tmx_fraud_check_event(result, user) irs_attempts_api_tracker.idv_tmx_fraud_check( success: success, failure_reason: failure_reason, - fraud_event_id: fraud_event&.id, ) end end diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index b5ec7773a3c..1f4eb829fa6 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -68,16 +68,13 @@ def forgot_password_new_password_submitted(success:, failure_reason: nil) end # @param [String] decision One of 'pass', 'manual_reject', or 'automated_reject' - # @param [Integer] fraud_event_id Can be used to correlate with an idv_tmx_fraud_check event # @param [String] cached_irs_session_id The IRS session id ('tid') the user had when flagged # @param [String] cached_login_session_id The Login.gov session id the user had when flagged # A profile offlined for review has been approved or rejected. - def fraud_review_adjudicated(decision:, fraud_event_id:, cached_irs_session_id:, - cached_login_session_id:) + def fraud_review_adjudicated(decision:, cached_irs_session_id:, cached_login_session_id:) track_event( :fraud_review_adjudicated, decision: decision, - fraud_event_id: fraud_event_id, cached_irs_session_id: cached_irs_session_id, cached_login_session_id: cached_login_session_id, ) @@ -298,15 +295,13 @@ def idv_ssn_submitted(ssn:) end # @param [Boolean] success - # @param [Integer] fraud_event_id - Maps to a fraud_review_adjudicated event # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification - def idv_tmx_fraud_check(success:, fraud_event_id:, failure_reason: nil) + def idv_tmx_fraud_check(success:, failure_reason: nil) track_event( :idv_tmx_fraud_check, success: success, - fraud_event_id: fraud_event_id, failure_reason: failure_reason, ) end diff --git a/db/primary_migrate/20230322000756_create_fraud_events.rb b/db/primary_migrate/20230322000756_create_fraud_events.rb deleted file mode 100644 index eabe2fdca2a..00000000000 --- a/db/primary_migrate/20230322000756_create_fraud_events.rb +++ /dev/null @@ -1,11 +0,0 @@ -class CreateFraudEvents < ActiveRecord::Migration[7.0] - def change - create_table :fraud_events do |t| - t.integer :user_id - t.string :irs_session_id - t.string :login_session_id - - t.timestamps - end - end -end diff --git a/db/primary_migrate/20230322000756_create_fraud_review_requests.rb b/db/primary_migrate/20230322000756_create_fraud_review_requests.rb new file mode 100644 index 00000000000..24116a3e74b --- /dev/null +++ b/db/primary_migrate/20230322000756_create_fraud_review_requests.rb @@ -0,0 +1,14 @@ +class CreateFraudReviewRequests < ActiveRecord::Migration[7.0] + def change + create_table :fraud_review_requests do |t| + t.integer :user_id + t.string :uuid + t.string :irs_session_id + t.string :login_session_id + + t.timestamps + + t.index :user_id, unique: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c2545752507..5dce1006baa 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -226,12 +226,14 @@ t.index ["user_id", "created_at"], name: "index_events_on_user_id_and_created_at" end - create_table "fraud_events", force: :cascade do |t| + create_table "fraud_review_requests", force: :cascade do |t| t.integer "user_id" + t.string "uuid" t.string "irs_session_id" t.string "login_session_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index ["user_id"], name: "index_fraud_review_requests_on_user_id" end create_table "iaa_gtcs", force: :cascade do |t| diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index 4c5c37098ba..32718d46f86 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -209,9 +209,6 @@ allow(IdentityConfig.store).to receive(:proofing_device_profiling).and_return(:enabled) allow(IdentityConfig.store).to receive(:irs_attempt_api_track_tmx_fraud_check_event). and_return(true) - # maw: This is rather inelegant, but gets tests passing for the moment. - # Should really stub this out better. - allow_any_instance_of(FraudEvent).to receive(:id).and_return(123) end context 'when threatmetrix response is Pass' do @@ -220,7 +217,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: true, - fraud_event_id: nil, failure_reason: nil, ) get :show @@ -233,7 +229,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show @@ -246,7 +241,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show @@ -259,7 +253,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - fraud_event_id: 123, failure_reason: expected_failure_reason, ) get :show diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 2d24b8baad0..dae140ca6ad 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -2,7 +2,6 @@ describe Profile do let(:user) { create(:user, :signed_up, password: 'a really long sekrit') } - let(:hashed_uuid) { Digest::SHA1.hexdigest(user.uuid) } let(:another_user) { create(:user, :signed_up) } let(:profile) { create(:profile, user: user) } @@ -317,8 +316,6 @@ expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). with( decision: 'pass', - # FIXME: stub these out somewhere, or should we move this test? - fraud_event_id: nil, cached_irs_session_id: nil, cached_login_session_id: nil, ) @@ -419,7 +416,6 @@ with( decision: 'manual_reject', # FIXME: These shouldn't be nil. - fraud_event_id: nil, cached_irs_session_id: nil, cached_login_session_id: nil, ) @@ -438,7 +434,6 @@ with( decision: 'automatic_reject', # FIXME: - fraud_event_id: nil, cached_irs_session_id: nil, cached_login_session_id: nil, ) From 7a528d26124a363a1580155eec56b1c7071d2f10 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Fri, 24 Mar 2023 11:22:49 -0400 Subject: [PATCH 13/16] Adds feature flag; cleans up along the way --- app/models/profile.rb | 23 +++++++------ config/application.yml.default | 1 + lib/identity_config.rb | 1 + spec/models/profile_spec.rb | 63 ++++++++++++++++++++++------------ 4 files changed, 57 insertions(+), 31 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 27b69180ffe..9ec87bbb34d 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -49,12 +49,7 @@ def activate def activate_after_passing_review update!(fraud_review_pending: false, fraud_rejection: false) - fraud_review_request = user.fraud_review_requests.last - irs_attempts_api_tracker&.fraud_review_adjudicated( - decision: 'pass', - cached_irs_session_id: fraud_review_request&.irs_session_id, - cached_login_session_id: fraud_review_request&.login_session_id, - ) + track_fraud_review_adjudication(decision: 'pass') activate end @@ -68,11 +63,8 @@ def deactivate_for_fraud_review def reject_for_fraud(notify_user:) update!(active: false, fraud_review_pending: false, fraud_rejection: true) - fraud_review_request = user.fraud_review_requests.last - irs_attempts_api_tracker&.fraud_review_adjudicated( + track_fraud_review_adjudication( decision: notify_user ? 'manual_reject' : 'automatic_reject', - cached_irs_session_id: fraud_review_request&.irs_session_id, - cached_login_session_id: fraud_review_request&.login_session_id, ) UserAlerts::AlertUserAboutAccountRejected.call(user) if notify_user end @@ -157,6 +149,17 @@ def irs_attempts_api_tracker private + def track_fraud_review_adjudication(decision:) + if IdentityConfig.store.irs_attempt_api_track_idv_fraud_review + fraud_review_request = user.fraud_review_requests.last + irs_attempts_api_tracker&.fraud_review_adjudicated( + decision: decision, + cached_irs_session_id: fraud_review_request&.irs_session_id, + cached_login_session_id: fraud_review_request&.login_session_id, + ) + end + end + def personal_key_generator @personal_key_generator ||= PersonalKeyGenerator.new(user) end diff --git a/config/application.yml.default b/config/application.yml.default index f7c4873d6da..732602c3a9d 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -152,6 +152,7 @@ irs_attempt_api_event_ttl_seconds: 86400 irs_attempt_api_event_count_default: 1000 irs_attempt_api_event_count_max: 10000 irs_attempt_api_payload_size_logging_enabled: true +irs_attempt_api_track_idv_fraud_review: false irs_attempt_api_track_tmx_fraud_check_event: false key_pair_generation_percent: 0 logins_per_ip_track_only_mode: false diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 64855830f63..7d9c2d628ac 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -232,6 +232,7 @@ def self.build_store(config_map) config.add(:irs_attempt_api_event_count_default, type: :integer) config.add(:irs_attempt_api_event_count_max, type: :integer) config.add(:irs_attempt_api_payload_size_logging_enabled, type: :boolean) + config.add(:irs_attempt_api_track_idv_fraud_review, type: :boolean) config.add(:irs_attempt_api_track_tmx_fraud_check_event, type: :boolean) config.add(:irs_attempt_api_public_key) config.add(:irs_attempt_api_public_key_id) diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index dae140ca6ad..533e2d9a211 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -301,25 +301,48 @@ end context 'when the initiating_sp is the IRS' do - it 'logs an attempt event' do - sp = create(:service_provider, :irs) - profile = create( + let(:sp) { create(:service_provider, :irs) } + let(:profile) do + create( :profile, user: user, active: false, fraud_review_pending: true, initiating_service_provider: sp, ) - allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) - expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy - - expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). - with( - decision: 'pass', - cached_irs_session_id: nil, - cached_login_session_id: nil, - ) - profile.activate_after_passing_review + end + + context 'when the feature flag is enabled' do + before do + allow(IdentityConfig.store).to receive(:irs_attempt_api_track_idv_fraud_review). + and_return(true) + end + + it 'logs an attempt event' do + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + + expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). + with( + hash_including(decision: 'pass'), + ) + profile.activate_after_passing_review + end + end + + context 'when the feature flag is disabled' do + before do + allow(IdentityConfig.store).to receive(:irs_attempt_api_track_idv_fraud_review). + and_return(false) + end + + it 'does not log an attempt event' do + allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy + + expect(profile.irs_attempts_api_tracker).not_to receive(:fraud_review_adjudicated) + profile.activate_after_passing_review + end end end @@ -409,15 +432,14 @@ context 'and notify_user is true' do it 'logs an event with manual_reject' do allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + allow(IdentityConfig.store).to receive(:irs_attempt_api_track_idv_fraud_review). + and_return(true) expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). with( - decision: 'manual_reject', - # FIXME: These shouldn't be nil. - cached_irs_session_id: nil, - cached_login_session_id: nil, + hash_including(decision: 'manual_reject'), ) profile.reject_for_fraud(notify_user: true) @@ -427,15 +449,14 @@ context 'and notify_user is false' do it 'logs an event with automatic_reject' do allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return(true) + allow(IdentityConfig.store).to receive(:irs_attempt_api_track_idv_fraud_review). + and_return(true) expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_truthy expect(profile.irs_attempts_api_tracker).to receive(:fraud_review_adjudicated). with( - decision: 'automatic_reject', - # FIXME: - cached_irs_session_id: nil, - cached_login_session_id: nil, + hash_including(decision: 'automatic_reject'), ) profile.reject_for_fraud(notify_user: false) From deac2e3735ef4b22c2fda6891a183c373c69230d Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Fri, 24 Mar 2023 12:21:14 -0400 Subject: [PATCH 14/16] Removes useless variable assignment --- app/services/idv/steps/threat_metrix_step_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 2bc215a011e..143067ed286 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -54,7 +54,7 @@ def log_irs_tmx_fraud_check_event(result, user) success = result[:review_status] == 'pass' unless success - fraud_event = FraudReviewRequest.create( + FraudReviewRequest.create( user: user, irs_session_id: irs_attempts_api_session_id, login_session_id: Digest::SHA1.hexdigest(user.unique_session_id.to_s), From e3861f61358a650a14c4505aba44dc3f0b74d613 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Mon, 27 Mar 2023 15:32:47 -0400 Subject: [PATCH 15/16] Apply suggestions from code review Co-authored-by: Zach Margolis --- app/models/profile.rb | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 9ec87bbb34d..11eb45ac471 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -125,26 +125,22 @@ def has_proofed_before? end def irs_attempts_api_tracker - return @irs_attempts_api_tracker if defined?(@irs_attempts_api_tracker) - analytics = Analytics.new( - user: user, + @irs_attempts_api_tracker ||= IrsAttemptsApi::Tracker.new( + session_id: nil, request: nil, - sp: initiating_service_provider&.issuer, - session: {}, - ahoy: nil, - ) - if initiating_service_provider&.irs_attempts_api_enabled? - @irs_attempts_api_tracker = IrsAttemptsApi::Tracker.new( - session_id: nil, - request: nil, + user: user, + sp: initiating_service_provider, + cookie_device_uuid: nil, + sp_request_uri: nil, + enabled_for_session: initiating_service_provider&.irs_attempts_api_enabled?, + analytics: Analytics.new( user: user, - sp: initiating_service_provider, - cookie_device_uuid: nil, - sp_request_uri: nil, - enabled_for_session: true, - analytics: analytics, - ) - end + request: nil, + sp: initiating_service_provider&.issuer, + session: {}, + ahoy: nil, + ), + ) end private @@ -152,7 +148,7 @@ def irs_attempts_api_tracker def track_fraud_review_adjudication(decision:) if IdentityConfig.store.irs_attempt_api_track_idv_fraud_review fraud_review_request = user.fraud_review_requests.last - irs_attempts_api_tracker&.fraud_review_adjudicated( + irs_attempts_api_tracker.fraud_review_adjudicated( decision: decision, cached_irs_session_id: fraud_review_request&.irs_session_id, cached_login_session_id: fraud_review_request&.login_session_id, From 92adfd9c72aac1c87f07c437493c2d43a3375822 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Mon, 27 Mar 2023 17:10:29 -0400 Subject: [PATCH 16/16] Fix rubocop + tests --- app/models/profile.rb | 2 +- spec/models/profile_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 11eb45ac471..66ac064e4ad 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -133,7 +133,7 @@ def irs_attempts_api_tracker cookie_device_uuid: nil, sp_request_uri: nil, enabled_for_session: initiating_service_provider&.irs_attempts_api_enabled?, - analytics: Analytics.new( + analytics: Analytics.new( user: user, request: nil, sp: initiating_service_provider&.issuer, diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 533e2d9a211..f701a0c7b99 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -358,7 +358,7 @@ ) expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_falsey - expect(profile.irs_attempts_api_tracker).to be nil + expect(profile.irs_attempts_api_tracker).not_to receive(:fraud_review_adjudicated) profile.activate_after_passing_review end end @@ -478,7 +478,7 @@ expect(profile.initiating_service_provider.irs_attempts_api_enabled?).to be_falsey - expect(profile.irs_attempts_api_tracker).to be nil + expect(profile.irs_attempts_api_tracker).not_to receive(:fraud_review_adjudicated) profile.reject_for_fraud(notify_user: true) end