Skip to content

LG-14393: Require threatmetrix_session_id for verify info step#11254

Merged
matthinz merged 3 commits intomainfrom
matthinz/14393-no-nil-session-ids
Sep 19, 2024
Merged

LG-14393: Require threatmetrix_session_id for verify info step#11254
matthinz merged 3 commits intomainfrom
matthinz/14393-no-nil-session-ids

Conversation

@matthinz
Copy link
Contributor

@matthinz matthinz commented Sep 17, 2024

🎫 Ticket

Link to the relevant ticket:
LG-14393

🛠 Summary of changes

This PR updates the preconditions of VerifyInfoController (remote unsupervised only--IPP is being tracked in LG-14552) to require the presence of a threatmetrix_session_id. This is to work around a rare case where a user might end up on the verify info screen with no session id present. This is not an ideal fix, but rather an attempt to prevent a condition where a user will have no chance of passing identity verification.

📜 Testing Plan

  1. Apply the patch below (hit copy, then pbpaste | git apply) to ensure no threatmetrix_session_id is written to your session
  2. Go through IdV up to the SSN screen
  3. When you click the Continue button, you should not see the Verify info screen, but rather the "Update your SSN" screen
  4. Verify that clicking the Update button does not advance you to the verify info screen.
  5. Revert the change made in step 1, click Update, and you should advance to the Verify Info screen.

The patch

diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb
index a17d45813..f748ad328 100644
--- a/app/services/idv/steps/threat_metrix_step_helper.rb
+++ b/app/services/idv/steps/threat_metrix_step_helper.rb
@@ -15,7 +15,7 @@ module Idv

       def generate_threatmetrix_session_id(updating_ssn)
         if should_generate_new_threatmetrix_session_id?(updating_ssn)
-          idv_session.threatmetrix_session_id = SecureRandom.uuid
+          return SecureRandom.uuid
         end

         idv_session.threatmetrix_session_id

@matthinz matthinz requested review from a team September 17, 2024 23:41
Copy link
Contributor

@gina-yamada gina-yamada Sep 18, 2024

Choose a reason for hiding this comment

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

Thanks for including this test. Nice to see if you revisit the ssn page you will not get a new tmx session id.
Code looks good. I don't have time to manually test at the moment but will try to get back to this before end of day

@lmgeorge
Copy link
Contributor

Suggestion (non-blocking): Could ypu add an event that logs when a nil threatmetrix_session_id happens in this scenario? It would be useful (at minimum) to know how often this happens, especially if we see more people than expected failing or abandoning verification at this step after this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is a method we could add to Idv::Session to cover this? That way we could do something like this:

idv_session.ssn_step_complete? && idv_session.ipp_document_capture_complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I like that suggestion. I'm currently tweaking this code based on @lmgeorge's feedback above, but I will probably do this too

Copy link
Contributor

@jennyverdeyen jennyverdeyen left a comment

Choose a reason for hiding this comment

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

When I run the testing steps with the IPP flow, I get this broken redirect behavior upon hitting "Continue" on the SSN page. It looks like this only happens with the patch that makes it so there's no threatmetrix_session_id.

I haven't dug into why this is happening but I wanted to raise this since I saw it.

Screen.Recording.2024-09-18.at.4.07.31.PM.mov

@matthinz
Copy link
Contributor Author

@jennyverdeyen Ooh, thanks for the video. I will look into this

@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 16a6b6e to aa76e7b Compare September 18, 2024 20:47
@matthinz matthinz changed the base branch from main to matthinz/precondition-analytics September 18, 2024 20:47
@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from aa76e7b to 6c92b5c Compare September 18, 2024 21:08
@matthinz matthinz marked this pull request as draft September 18, 2024 21:34
@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 6c92b5c to 504a0d4 Compare September 18, 2024 22:14
@matthinz matthinz changed the base branch from matthinz/precondition-analytics to main September 18, 2024 22:14
@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 504a0d4 to 39663ef Compare September 18, 2024 23:30
@matthinz
Copy link
Contributor Author

@jennyverdeyen Ok, new plan: I'm going to do the remote verify info controller in this pr and tackle the IPP one separately. Thank you for your help!

@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 39663ef to 8ff985c Compare September 18, 2024 23:35
…Info screen

If the user does not have a session ID, redirect them back to the SSN step so that they get one.

changelog: Internal, Identity verification, Prevent errors during verify info step due to missing session id.
- If the user is updating their SSN but their is no session id present, generate a new one
@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 8ff985c to 4ddbad5 Compare September 18, 2024 23:37
@matthinz matthinz force-pushed the matthinz/14393-no-nil-session-ids branch from 4ddbad5 to 927e791 Compare September 19, 2024 16:10
@matthinz matthinz marked this pull request as ready for review September 19, 2024 16:51
Copy link
Contributor

@jennyverdeyen jennyverdeyen left a comment

Choose a reason for hiding this comment

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

Re-tested with the updates to not include the IPP portion. No issues now, looks great!

@matthinz matthinz merged commit f32384a into main Sep 19, 2024
@matthinz matthinz deleted the matthinz/14393-no-nil-session-ids branch September 19, 2024 20:33
AShukla-GSA pushed a commit that referenced this pull request Sep 30, 2024
* Require threatmetrix_session_id be present in idv_session for Verify Info screen

If the user does not have a session ID, redirect them back to the SSN step so that they get one.

changelog: Internal, Identity verification, Prevent errors during verify info step due to missing session id.

* Update ThreatMetrix session ID generation logic

- If the user is updating their SSN but their is no session id present, generate a new one

* Add an analytics event when missing tmx session id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants