18F / identity-idp

Login.gov Core App: Identity Provider (IdP)
https://secure.login.gov/
Other
524 stars 112 forks source link

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

Closed matthinz closed 1 month ago

matthinz commented 1 month ago

🎫 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
lmgeorge commented 1 month ago

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.

matthinz commented 1 month ago

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

matthinz commented 1 month ago

@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!