-
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-13963: 👤 Socure shadow mode background job #11139
Conversation
SocureShadowModeProofingJob.perform_later( | ||
document_capture_session_result_id: document_capture_session.result_id, | ||
encrypted_arguments:, | ||
service_provider_issuer:, | ||
user_email: user_email_for_proofing(user), | ||
user_uuid: user.uuid, | ||
) |
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.
More to do here around including phone number (which we don't currently pass to ResolutionProofingJob). Captured as LG-14282
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.
did we consider enqueuing this job in parallel from inside IDV agent?
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.
This job depends on the resolution proofing job completing before it can run (since it reads results from the proofing doc capture session). So parallel wouldn't work here
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.
Is that so we can compare the results easily in Cloudwatch? I feel like we would be able to accomplish that with a coalesce() | stats ... by properties.user_id
or maybe we come up with a job id to join them by that's shared?
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.
Ooh, yeah, I like the job id idea, lemme noodle on that
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.
There is maybe an argument for using trace_id
as a value that can connect the two jobs... It gets passed into ResolutionProofingJob
, but then the IdV: doc auth verify proofing results
event gets logged by the verify info concern as part of a different request, and so will have a different trace id.
So then I thought maybe create a new socure_shadow_mode_job_id
, but that gets a little awkward because we have to store it on the DocumentCaptureSession so that the verify info concern can include it in the payload sent to verify proofing results
so that we can then link those events with the socure events.
I don't think it's good enough to use user_id as a key here, since a single user can have multiple submissions.
Which brings me back to the solution I'm using here: schedule the job after the resolution proofing job has run, log payloads for existing + socure in a new event.
The intention here is to run this for a short period of time to gather data, so one advantage to this approach is that the change is mostly contained to the SocureShadowModeProofingJob
class, with a little tweak to ResolutionProofingJob
.
(I did just create LG-14328 to try addressing some of the awkwardness caused by logging our event from a controller rather than a background job)
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.
ok! I was thinking this, but for all the reasons you stated this seems fine as-is too
job_id = SecureRandom.uuid
ResolutionProofingJob.perform_later(job_id:, ...)
SocureShadowModeProofingJob.perform_later(job_id:, ...)
pii_like_keypaths: [ | ||
[:errors, :ssn], | ||
[:resolution_result, :context, :stages, :resolution, :errors, :ssn], | ||
[:resolution_result, :context, :stages, :residential_address, :errors, :ssn], | ||
[:resolution_result, :context, :stages, :threatmetrix, :response_body, :first_name], | ||
[:resolution_result, :context, :stages, :state_id, :state_id_jurisdiction], | ||
], |
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.
These are copied from where we log the Verify proofing results
event. Socure results don't include PII so we should be good.
|
||
it 'does not log an idv_socure_shadow_mode_proofing_result_missing event' do | ||
perform | ||
expect(analytics).not_to have_logged_event(:idv_socure_shadow_mode_proofing_result_missing) |
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.
We did not previously support not_to have_logged_event
-- that's what the update to HaveLoggedEventMatcher
(below) is about.
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.
Thanks for adding it!
def load_proofing_result(document_capture_session_result_id:) | ||
DocumentCaptureSession.new( | ||
result_id: document_capture_session_result_id, | ||
).load_proofing_result&.result | ||
end |
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.
I realized that the proofing result should still be available assuming the SocureShadowModeProofingJob runs soon after the ResolutionProofingJob, so it makes more sense to load the result from there rather than pass it in (which I'd been thinking when I wrote the ticket).
If jobs get delayed and this result is occasionally not available that's not a big deal.
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.
I have a few nits and questions, and see that Zach added some as well, but overall this is looking good. Only commenting for the moment as I haven't tested this yet, but no real issues.
@@ -109,6 +119,10 @@ def make_vendor_proofing_requests( | |||
) | |||
end | |||
|
|||
def user_email_for_proofing(user) | |||
user.confirmed_email_addresses.first.email |
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.
I got distracted wondering if we needed to worry about getting a nil in here somewhere. It looks like there's a lot of prior art for not needing it, though.
That, in turn, led to me wondering if this should actually be implemented on User as, e.g., user.first_email_address
or something.
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.
yeah user.email
is a deprecated method but having user.first_email_address
would be very clear
Add job to make requests to Socure's KYC API and log the results alongside the original resolution proofing result. changelog: Upcoming Features, Identity verification, Add background job for Socure KYC proofing
- When flag is enabled, invoke Socure KYC as well
These are real big and mess with Cloudwatch's ability to parse fields out oflogs.
Co-authored-by: Zach Margolis <[email protected]>
5274082
to
181ca80
Compare
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.
Thanks for giving Joy a heads-up about this PR. Looks great. 👏🏻
socure_result = proofer.proof(applicant) | ||
|
||
analytics.idv_socure_shadow_mode_proofing_result( | ||
resolution_result: format_proofing_result_for_logs(proofing_result), |
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.
Non-blocking nit: My read is that this method removes the threatmetrix response body. Would it be worth indicating that in the method name?
|
||
it 'does not log an idv_socure_shadow_mode_proofing_result_missing event' do | ||
perform | ||
expect(analytics).not_to have_logged_event(:idv_socure_shadow_mode_proofing_result_missing) |
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.
Thanks for adding it!
(Previously: #11093)
🎫 Ticket
Link to the relevant ticket:
LG-13963
🛠 Summary of changes
Adds a new disabled background job,
SocureShadowModeProofingJob
, to do proofing with Socure KYC and log the result.To enable the job, you need to set
idv_socure_shadow_mode: true
in your config. Then it will be scheduled automatically by the ResolutionProofingJob.📜 Testing Plan
To test this locally, you will need a Socure KYC API key. To get one:
Then update your
application.yml
:Then, complete identity verification. DO NOT USE REAL PII. You should see a new event logged in your events.log called
idv_socure_shadow_mode_proofing_result
:Here is an example
You should also verify that this job does not run when
idv_socure_shadow_mode_enabled
is not set or is disabled.