-
Notifications
You must be signed in to change notification settings - Fork 115
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
LG-14282 | Pass phone number to Socure proofer #11272
Conversation
[changelog: Upcoming Features, Identity verification, send phone number to Socure]
changelog: Upcoming Features, Identity verification, send phone number to Socure
|
||
|
||
user_pii = pii | ||
user_pii[:best_effort_phone_number_for_socure] = best_effort_phone |
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.
Because we're doing something slightly unusual, we tack this onto the PII bundle so it's available for Socure, but omit it other places it might be used.
The naming is meant to convey that we accept that the phone number may not be present and shouldn't be relied on for other purposes.
@@ -91,6 +92,10 @@ def build_applicant( | |||
) | |||
|
|||
applicant_pii = decrypted_arguments[:applicant_pii] | |||
if applicant_pii[:phone].nil? && applicant_pii[:best_effort_phone_number_for_socure] |
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.
Note that, in actuality, nothing is passing :phone
into the Socure proofer today, so this conditional isn't strictly necessary. But it feels cleaner, and IMHO makes clearer what we're doing. (Plus it frees me from needing nil-safe operators on applicant_pii[:best_effort_phone_number_for_socure]
and keeps phone
and phone_source
together.)
@@ -32,7 +32,7 @@ def proof( | |||
ipp_enrollment_in_progress:, | |||
current_sp: | |||
) | |||
@applicant_pii = applicant_pii | |||
@applicant_pii = applicant_pii.except(:best_effort_phone_number_for_socure) |
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.
AAMVA and LexisNexis are not expecting this key.
@@ -33,7 +33,7 @@ def initialize(config) | |||
# @param [Hash] applicant | |||
# @return [Proofing::Resolution::Result] | |||
def proof(applicant) | |||
input = Input.new(applicant) | |||
input = Input.new(applicant.except(:phone_source)) |
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.
And phone_source
is just along for the ride into logs.
@@ -18,7 +18,7 @@ | |||
end | |||
|
|||
let(:applicant_pii) do | |||
Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE | |||
Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN |
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.
Maybe one more test in this file asserting that phone_source makes it into the logged event
@@ -48,6 +51,14 @@ def log_event_for_missing_threatmetrix_session_id | |||
analytics.idv_verify_info_missing_threatmetrix_session_id if idv_session.ssn_step_complete? | |||
end | |||
|
|||
def best_effort_phone |
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.
Probably want test coverage for this method. I don't think we have a spec for VerifyInfoConcern
, but you could either write one or put a test in the VerifyInfoController
spec.
@@ -491,4 +491,36 @@ | |||
end | |||
end | |||
end | |||
|
|||
describe '#best_effort_phone' 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.
@matthinz I did end up adding this in the controller spec based on your feedback. I set out to add a test for VerifyInfoConcern but it was turtles all the way down.
changelog: Upcoming Features, Identity verification, send phone number to Socure
changelog: Upcoming Features, Identity verification, send phone number to Socure
changelog: Upcoming Features, Identity verification, send phone number to Socure
🎫 Ticket
Link to the relevant ticket:
LG-14282
🛠 Summary of changes
When Socure is running in shadow mode (to allow us to evaluate Socure without gating results on it), this will send our best guess for the user's phone number to also be proofed.
It is very possible for there to not be a phone number available yet in the flow, and the story suggested variable naming to reflect that it should not be treated as a reliable indicator of phone number.
Privacy
Most privacy concerns are mooted by the fact that all of Socure is feature-flagged off in all environments (except my laptop), and all that work is undergoing detailed security+privacy review before it can be enabled. This Socure code is unreachable in production.
There is a trivial change in that we are sourcing the phone number from a different (less reliable) user input. Normally, on the page after this code executes, the user is prompted for a phone number to verify. Because the Socure call happens before that, we rely on the number provided for MFA or the hybrid-handoff flow. The user has to have consented to sharing data with a proofing provider to get this far, so this shouldn't have a privacy impact.
📜 Testing Plan
You can enable Socure locally in
application.yml(.default)
with creds from their sandbox dashboard to test this. (Be sure to not submit any actual PII!)When doing so, you should then receive a beautiful event in events.log like this one:
You are specifically looking for
socure_result
to include 'phone' inverified_attributes
indicating that it was passed successfully.Another option...
...is to litter `puts` statements all throughout the code. Just hypothetically. Why doesn't Markdown work in here?