Skip to content

LG-13003: Make sure we are checking all relevant Threatmetrix flags where necessary #10391

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.

Sign up for GitHub

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

Merged
JackRyan1989 merged 9 commits into from
Apr 10, 2024
Merged

LG-13003: Make sure we are checking all relevant Threatmetrix flags where necessary #10391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
908bcdd
Make sure code before proofing results job checks that tmx is enabled
Apr 9, 2024
5608b82
undo unnecessary redirection for fraud rejection and make sure fraud …
Apr 9, 2024
d9751a1
changelog: Bug Fixes, In-Person Proofing, check all threatmetrix conf…
Apr 9, 2024
2e03c06
linting
Apr 9, 2024
29c5b62
realized that we need to redirect on fraud rejection and ipp failure …
Apr 9, 2024
7e11f90
Need to alter analytics now that we take into account the device prof…
Apr 9, 2024
bc9d350
updating profile maker spec
Apr 10, 2024
608f670
add explanatory comment
Apr 10, 2024
9b85139
Merge branch 'main' of github.com:18F/identity-idp into jryan/lg-1300…
Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/controllers/concerns/fraud_review_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,14 @@ def handle_fraud_rejection
redirect_to_fraud_rejection if fraud_rejection?
end

# Returns true if the user has not passed IPP at the post office and is
# flagged for fraud review, or has been rejected for fraud.
# Ultimately this is to allow users who fail at the post office to create another enrollment
# bypassing the typical flow of showing the Please Call or Fraud Rejection screens.
def in_person_prevent_fraud_redirection?
IdentityConfig.store.in_person_proofing_enforce_tmx &&
current_user.ipp_enrollment_status_not_passed?
current_user.ipp_enrollment_status_not_passed? &&
(fraud_review_pending? || fraud_rejection?)
Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 9, 2024

Choose a reason for hiding this comment

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

Undoing previous change around fraud rejection. See this slack thread for justification

Checking for fraud_review_pending and fraud_rejection because this is called before both fraud redirection cases.

Copy link
Copy Markdown
Contributor

@gina-yamada gina-yamada Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Jack- can you explain again why the additional check ((fraud_review_pending? || fraud_rejection?)) is needed? I am thinking about the method that is calling this method and I am not sure about this. 🤔

Update: After getting on a call with Jack and others on Joy- we talked through this logic - ipp_enrollment_status_not_passed (When a user fails IPP- we let them try again. I mixed up the logic so this is okay. Here is the table that says okay https://docs.google.com/document/d/1Xvhz5xXQgQK7oYDlHYBkG-8-v6mY9yS9g_s-70Kzr1g/edit)

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 10, 2024

Choose a reason for hiding this comment

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

I will add a comment above this method to help clarify what's going on here.

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 10, 2024

Choose a reason for hiding this comment

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

@gina-yamada @n1zyy @KeithNava Comment added here

end

def redirect_to_fraud_review
Expand Down
1 change: 1 addition & 0 deletions app/models/profile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ def activate(reason_deactivated: nil)

def tmx_status
return nil unless IdentityConfig.store.in_person_proofing_enforce_tmx
return nil unless FeatureManagement.proofing_device_profiling_decisioning_enabled?
Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 9, 2024

Choose a reason for hiding this comment

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

This has an impact on analytics, hence the change to the analytics spec.


fraud_pending_reason || :threatmetrix_pass
end
Expand Down
3 changes: 2 additions & 1 deletion app/services/idv/profile_maker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ def save_profile(

def set_idv_level(in_person_verification_needed:, selfie_check_performed:)
if in_person_verification_needed
if IdentityConfig.store.in_person_proofing_enforce_tmx
if IdentityConfig.store.in_person_proofing_enforce_tmx &&
FeatureManagement.proofing_device_profiling_decisioning_enabled?
Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 9, 2024

Choose a reason for hiding this comment

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

@matthinz I think this addresses your concerns here

Copy link
Copy Markdown
Contributor

@matthinz matthinz Apr 10, 2024

Choose a reason for hiding this comment

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

You might consider moving this into a new method in FeatureManagement--I believe the intention there is that when we have these kind of relationships between config flags we can encapsulate them in method in there. For example, you could do FeatureManagement.ipp_proofing_device_profiling_decisioning_enabled? (side note: I just filed a ticket to come up with a better name than "proofing_device_profiling" but for now that is what we have)

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Apr 10, 2024

Choose a reason for hiding this comment

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

@matthinz I think we have a bunch of these flags that could probably be moved in to that class. I think I'll open our own ticket for consolidating our features. Thanks!

:in_person
else
:legacy_in_person
Expand Down
10 changes: 6 additions & 4 deletions spec/features/idv/analytics_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
let(:fake_analytics) { FakeAnalytics.new }
let(:proofing_device_profiling) { :enabled }
let(:threatmetrix) { true }
let(:idv_level) { 'in_person' }
let(:threatmetrix_response) do
{ client: nil,
errors: {},
Expand Down Expand Up @@ -510,21 +511,21 @@
in_person_verification_pending: true,
address_verification_method: 'phone',
encrypted_profiles_missing: false,
active_profile_idv_level: nil, pending_profile_idv_level: 'in_person',
active_profile_idv_level: nil, pending_profile_idv_level: idv_level,
proofing_components: { document_check: 'usps', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }
},
'IdV: personal key acknowledgment toggled' => {
checked: true,
active_profile_idv_level: nil, pending_profile_idv_level: 'in_person',
active_profile_idv_level: nil, pending_profile_idv_level: idv_level,
proofing_components: { document_check: 'usps', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }
},
'IdV: personal key submitted' => {
address_verification_method: 'phone', fraud_review_pending: false, fraud_rejection: false, in_person_verification_pending: true, deactivation_reason: nil,
active_profile_idv_level: nil, pending_profile_idv_level: 'in_person',
active_profile_idv_level: nil, pending_profile_idv_level: idv_level,
proofing_components: { document_check: 'usps', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }
},
'IdV: in person ready to verify visited' => {
active_profile_idv_level: nil, pending_profile_idv_level: 'in_person',
active_profile_idv_level: nil, pending_profile_idv_level: idv_level,
proofing_components: { document_check: 'usps', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }
},
'IdV: user clicked what to bring link on ready to verify page' => {},
Expand Down Expand Up @@ -892,6 +893,7 @@

context 'proofing_device_profiling disabled' do
let(:proofing_device_profiling) { :disabled }
let(:idv_level) { 'legacy_in_person' }
let(:threatmetrix) { false }
let(:threatmetrix_response) do
{ client: 'tmx_disabled',
Expand Down
4 changes: 4 additions & 0 deletions spec/services/idv/profile_maker_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@
before do
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).
and_return(in_person_proofing_enforce_tmx_mock)
allow(IdentityConfig.store).to receive(:proofing_device_profiling).
and_return(:disabled)
end

let(:profile) do
Expand Down Expand Up @@ -177,6 +179,8 @@
before do
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).
and_return(in_person_proofing_enforce_tmx_mock)
allow(IdentityConfig.store).to receive(:proofing_device_profiling).
and_return(:enabled)
end

let(:profile) do
Expand Down