-
Notifications
You must be signed in to change notification settings - Fork 166
LG-11997 | Conditionally marks users as fraudulent #9928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
60b47fc
ac041b0
d1020d9
52b39fc
515d6b6
ba9c58b
2300254
81bfd78
929f845
ca8e665
ae0ef1f
29b06a3
039bd7c
46ce9d6
fa914e6
7eb3800
952b935
30ba485
75e97fc
a8007a9
0afc7cd
89b2d6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,7 +99,6 @@ def check_enrollment(enrollment) | |
|
|
||
| status_check_attempted_at = Time.zone.now | ||
| enrollment_outcomes[:enrollments_checked] += 1 | ||
| response = nil | ||
|
|
||
| response = proofer.request_proofing_results( | ||
| enrollment.unique_id, enrollment.enrollment_code | ||
|
|
@@ -305,6 +304,15 @@ def handle_expired_status_update(enrollment, response, response_message) | |
| end | ||
| end | ||
|
|
||
| def handle_fraud_review_pending(enrollment) | ||
|
n1zyy marked this conversation as resolved.
|
||
| enrollment.profile.deactivate_for_fraud_review | ||
|
|
||
| analytics(user: enrollment.user). | ||
| idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review( | ||
| **enrollment_analytics_attributes(enrollment, complete: true), | ||
| ) | ||
|
n1zyy marked this conversation as resolved.
|
||
| end | ||
|
|
||
| def handle_unexpected_response(enrollment, response_message, reason:, cancel: true) | ||
| enrollment.cancelled! if cancel | ||
|
|
||
|
|
@@ -363,21 +371,24 @@ def handle_successful_status_update(enrollment, response) | |
| reason: 'Successful status update', | ||
| job_name: self.class.name, | ||
| ) | ||
| enrollment.profile.activate_after_passing_in_person | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This, and 373-380, are now conditional on the user not being |
||
| enrollment.update( | ||
| status: :passed, | ||
| proofed_at: proofed_at, | ||
| status_check_completed_at: Time.zone.now, | ||
| ) | ||
|
|
||
| # send SMS and email | ||
| send_enrollment_status_sms_notification(enrollment: enrollment) | ||
| send_verified_email(enrollment.user, enrollment) | ||
| analytics(user: enrollment.user).idv_in_person_usps_proofing_results_job_email_initiated( | ||
| **email_analytics_attributes(enrollment), | ||
| email_type: 'Success', | ||
| job_name: self.class.name, | ||
| ) | ||
| unless fraud_result_pending?(enrollment) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to check the config setting here too? Based on your specs I think no, but I'm not totally clear on when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! I think we should. We're not the first ones to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better yet, your suggestion in https://github.com/18F/identity-idp/pull/9928/files#r1467975629 gets us this for free. |
||
| enrollment.profile&.activate_after_passing_in_person | ||
|
|
||
| # send SMS and email | ||
| send_enrollment_status_sms_notification(enrollment: enrollment) | ||
|
n1zyy marked this conversation as resolved.
|
||
| send_verified_email(enrollment.user, enrollment) | ||
| analytics(user: enrollment.user).idv_in_person_usps_proofing_results_job_email_initiated( | ||
| **email_analytics_attributes(enrollment), | ||
| email_type: 'Success', | ||
| job_name: self.class.name, | ||
| ) | ||
| end | ||
| end | ||
|
|
||
| def handle_unsupported_secondary_id(enrollment, response) | ||
|
|
@@ -405,12 +416,23 @@ def handle_unsupported_secondary_id(enrollment, response) | |
| ) | ||
| end | ||
|
|
||
| def fraud_result_pending?(enrollment) | ||
| IdentityConfig.store.in_person_proofing_enforce_tmx && | ||
| enrollment.profile&.fraud_pending_reason.present? | ||
| end | ||
|
n1zyy marked this conversation as resolved.
|
||
|
|
||
| def process_enrollment_response(enrollment, response) | ||
| unless response.is_a?(Hash) | ||
| handle_response_is_not_a_hash(enrollment) | ||
| return | ||
| end | ||
|
|
||
| # We want to deactivate them regardless of status, but then allow the | ||
| # case statement below to pick up the correct flow. | ||
| if fraud_result_pending?(enrollment) | ||
| handle_fraud_review_pending(enrollment) | ||
| end | ||
|
|
||
| case response['status'] | ||
|
n1zyy marked this conversation as resolved.
|
||
| when IPP_STATUS_PASSED | ||
| if passed_with_unsupported_secondary_id_type?(response) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,8 @@ def save_profile( | |
|
|
||
| profile.save! | ||
| profile.deactivate_for_gpo_verification if gpo_verification_needed | ||
| if fraud_pending_reason.present? && !gpo_verification_needed | ||
|
|
||
| if fraud_pending_reason.present? && !gpo_verification_needed && !in_person_verification_needed | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @svalexander This is the fix you helped catch yesterday! 🥳 I also updated the ProfileMakerSpec to cover this. I think it would have taken way longer for me to find this if you hadn't caught it; thanks! |
||
| profile.deactivate_for_fraud_review | ||
| end | ||
| profile | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,7 @@ in_person_email_reminder_early_benchmark_in_days: 11 | |
| in_person_email_reminder_final_benchmark_in_days: 1 | ||
| in_person_email_reminder_late_benchmark_in_days: 4 | ||
| in_person_proofing_enabled: false | ||
| in_person_proofing_enforce_tmx: false | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Global default is false |
||
| in_person_proofing_opt_in_enabled: false | ||
| in_person_enrollment_validity_in_days: 30 | ||
| in_person_enrollments_ready_job_email_body_pattern: '\A\s*(?<enrollment_code>\d{16})\s*\Z' | ||
|
|
@@ -392,6 +393,7 @@ development: | |
| hmac_fingerprinter_key_queue: '["11111111111111111111111111111111", "22222222222222222222222222222222"]' | ||
| identity_pki_local_dev: true | ||
| in_person_proofing_enabled: true | ||
| in_person_proofing_enforce_tmx: true | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it's true in dev |
||
| in_person_send_proofing_notifications_enabled: true | ||
| logins_per_ip_limit: 5 | ||
| logo_upload_enabled: true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1148,6 +1148,94 @@ | |
| error_type: Faraday::NilStatusError, | ||
| ) | ||
| end | ||
|
|
||
| context 'when a user was flagged with fraud_pending_reason' do | ||
| before(:each) do | ||
| pending_enrollment.profile.update!(fraud_pending_reason: 'threatmetrix_review') | ||
| stub_request_passed_proofing_results | ||
| end | ||
|
|
||
| it 'updates the enrollment' do | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one example happens whether the flag is true or false, because it happens before we look at fraud statuses. It feels a little lonely out here though. |
||
| freeze_time do | ||
| expect(pending_enrollment.status).to eq 'pending' | ||
| job.perform(Time.zone.now) | ||
| expect(pending_enrollment.reload.status).to eq 'passed' | ||
| end | ||
| end | ||
|
|
||
| context 'when the in_person_proofing_enforce_tmx flag is false' do | ||
| before do | ||
| allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx). | ||
| and_return(false) | ||
| end | ||
|
|
||
| it 'activates the profile' do | ||
| job.perform(Time.zone.now) | ||
| profile = pending_enrollment.reload.profile | ||
| expect(profile).to be_active | ||
| end | ||
|
|
||
| it 'does not mark the user as fraud_review_pending_at' do | ||
| job.perform(Time.zone.now) | ||
|
|
||
| profile = pending_enrollment.reload.profile | ||
| expect(profile).not_to be_fraud_review_pending | ||
| end | ||
|
|
||
| it 'does not log a user_sent_to_fraud_review analytics event' do | ||
| job.perform(Time.zone.now) | ||
|
|
||
| expect(job_analytics).not_to have_logged_event( | ||
| :idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review, | ||
| ) | ||
| end | ||
| end | ||
|
|
||
| context 'when the in_person_proofing_enforce_tmx flag is true' do | ||
| before do | ||
| allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx). | ||
| and_return(true) | ||
| end | ||
|
|
||
| it 'preserves the fraud_pending_reason attribute' do | ||
| job.perform(Time.zone.now) | ||
|
|
||
| expect(pending_enrollment.profile.fraud_pending_reason).to eq 'threatmetrix_review' | ||
| end | ||
|
|
||
| it 'logs a user_sent_to_fraud_review analytics event' do | ||
| job.perform(Time.zone.now) | ||
|
|
||
| expect(job_analytics).to have_logged_event( | ||
| :idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review, | ||
| hash_including( | ||
| enrollment_code: pending_enrollment.enrollment_code, | ||
| ), | ||
| ) | ||
| end | ||
|
|
||
| it 'does not send a success email' do | ||
| user = pending_enrollment.user | ||
|
|
||
| freeze_time do | ||
| expect do | ||
| job.perform(Time.zone.now) | ||
| end.not_to have_enqueued_mail(UserMailer, :in_person_verified).with( | ||
| params: { user: user, email_address: user.email_addresses.first }, | ||
| args: [{ enrollment: pending_enrollment }], | ||
| ) | ||
| end | ||
| end | ||
|
|
||
| it 'marks the user as fraud_review_pending_at' do | ||
| job.perform(Time.zone.now) | ||
|
|
||
| profile = pending_enrollment.reload.profile | ||
| expect(profile.fraud_review_pending_at).not_to be_nil | ||
| expect(profile).not_to be_active | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The AC may have changed. You and I talked about exiting behavior vs what was in the issue. I would have expected fraud_review_pending to be null and fraud_review_pending_at to have a timestamp on in. I may have missed the ask here and/or the requirements/AC have changed upon diving into the code and discovering existing processes. If they have changed- we can update the AC in the ticket.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for flagging this. It did change; there is now a link in the Jira ticket to the Slack thread where it came up. It turns out that the existing pattern is to leave I tried to be clever with def fraud_review_pending?
fraud_review_pending_at.present?
endBut in hindsight I think it might be clearer if I just did |
||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'Proofed with secondary id' do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,15 +65,18 @@ | |
| end | ||
|
|
||
| context 'with fraud review needed' do | ||
| let(:gpo_verification_needed) { false } | ||
| let(:in_person_verification_needed) { false } | ||
| let(:profile) do | ||
| subject.save_profile( | ||
| fraud_pending_reason: 'threatmetrix_review', | ||
| gpo_verification_needed: false, | ||
| gpo_verification_needed: gpo_verification_needed, | ||
| deactivation_reason: nil, | ||
| in_person_verification_needed: false, | ||
| in_person_verification_needed: in_person_verification_needed, | ||
| selfie_check_performed: false, | ||
| ) | ||
| end | ||
|
|
||
| it 'creates a pending profile for fraud review' do | ||
| expect(profile.activated_at).to be_nil | ||
| expect(profile.active).to eq(false) | ||
|
|
@@ -84,9 +87,26 @@ | |
| expect(profile.initiating_service_provider).to eq(nil) | ||
| expect(profile.verified_at).to be_nil | ||
| end | ||
|
|
||
| it 'marks the profile as legacy_unsupervised' do | ||
| expect(profile.idv_level).to eql('legacy_unsupervised') | ||
| end | ||
|
|
||
| context 'when GPO verification is needed' do | ||
| let(:gpo_verification_needed) { true } | ||
|
|
||
| it 'is not fraud_review_pending?' do | ||
| expect(profile.fraud_review_pending?).to eq(false) | ||
| end | ||
| end | ||
|
|
||
| context 'when IPP is needed' do | ||
| let(:in_person_verification_needed) { true } | ||
|
|
||
| it 'is not fraud_review_pending?' do | ||
| expect(profile.fraud_review_pending?).to eq(false) | ||
| end | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test my change to the ProfileMaker. Without the change, but with this test, the GPO verification case passes, but the IPP one did not. |
||
| end | ||
|
|
||
| context 'with gpo_verification_needed' do | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment on line 102 was useless since we unconditionally assign it here. My editor happened to highlight it while I was working my way through here.