-
Notifications
You must be signed in to change notification settings - Fork 161
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
Changes from all commits
206a468
dc29a66
8bf0ec4
8ebc93a
05697ad
5755551
46d1c97
fab11b8
dbaf5a8
181ca80
c4fbd40
cb276e6
91b1736
3130112
4ebb7cd
fd4dcab
87a34bf
ca499e4
e829c13
6af08a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,16 @@ def perform( | |
device_profiling_success: callback_log_data&.device_profiling_success, | ||
timing: timer.results, | ||
) | ||
|
||
if IdentityConfig.store.idv_socure_shadow_mode_enabled | ||
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, | ||
) | ||
end | ||
end | ||
|
||
private | ||
|
@@ -89,7 +99,7 @@ def make_vendor_proofing_requests( | |
) | ||
result = progressive_proofer.proof( | ||
applicant_pii: applicant_pii, | ||
user_email: user.confirmed_email_addresses.first.email, | ||
user_email: user_email_for_proofing(user), | ||
threatmetrix_session_id: threatmetrix_session_id, | ||
request_ip: request_ip, | ||
ipp_enrollment_in_progress: ipp_enrollment_in_progress, | ||
|
@@ -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 commentThe 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., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
end | ||
|
||
def log_threatmetrix_info(threatmetrix_result, user) | ||
logger_info_hash( | ||
name: 'ThreatMetrix', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
# frozen_string_literal: true | ||
|
||
class SocureShadowModeProofingJob < ApplicationJob | ||
include JobHelpers::StaleJobHelper | ||
|
||
queue_as :low | ||
|
||
discard_on JobHelpers::StaleJobHelper::StaleJobError | ||
|
||
# @param [String] document_capture_session_result_id | ||
# @param [String] encrypted_arguments | ||
# @param [String,nil] service_provider_issuer | ||
# @param [String] user_email | ||
# @param [String] user_uuid | ||
def perform( | ||
document_capture_session_result_id:, | ||
encrypted_arguments:, | ||
service_provider_issuer:, | ||
user_email:, | ||
user_uuid: | ||
) | ||
raise_stale_job! if stale_job?(enqueued_at) | ||
|
||
user = User.find_by(uuid: user_uuid) | ||
raise "User not found: #{user_uuid}" if !user | ||
|
||
analytics = create_analytics( | ||
user:, | ||
service_provider_issuer:, | ||
) | ||
|
||
proofing_result = load_proofing_result(document_capture_session_result_id:) | ||
if !proofing_result | ||
analytics.idv_socure_shadow_mode_proofing_result_missing | ||
return | ||
end | ||
|
||
applicant = build_applicant(encrypted_arguments:, user_email:) | ||
|
||
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 commentThe 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? |
||
socure_result: socure_result.to_h, | ||
user_id: user.uuid, | ||
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], | ||
], | ||
Comment on lines
+46
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are copied from where we log the |
||
) | ||
end | ||
|
||
def create_analytics( | ||
user:, | ||
service_provider_issuer: | ||
) | ||
Analytics.new( | ||
user:, | ||
request: nil, | ||
sp: service_provider_issuer, | ||
session: {}, | ||
) | ||
end | ||
|
||
def format_proofing_result_for_logs(proofing_result) | ||
proofing_result.to_h.tap do |hash| | ||
hash.dig(:context, :stages, :threatmetrix)&.delete(:response_body) | ||
end | ||
end | ||
|
||
def load_proofing_result(document_capture_session_result_id:) | ||
DocumentCaptureSession.new( | ||
result_id: document_capture_session_result_id, | ||
).load_proofing_result&.result | ||
end | ||
Comment on lines
+74
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
def build_applicant( | ||
encrypted_arguments:, | ||
user_email: | ||
) | ||
decrypted_arguments = JSON.parse( | ||
Encryption::Encryptors::BackgroundProofingArgEncryptor.new.decrypt(encrypted_arguments), | ||
symbolize_names: true, | ||
) | ||
|
||
applicant_pii = decrypted_arguments[:applicant_pii] | ||
|
||
{ | ||
**applicant_pii.slice( | ||
:first_name, | ||
:last_name, | ||
:address1, | ||
:address2, | ||
:city, | ||
:state, | ||
:zipcode, | ||
:phone, | ||
:dob, | ||
:ssn, | ||
), | ||
email: user_email, | ||
} | ||
end | ||
|
||
def proofer | ||
@proofer ||= Proofing::Socure::IdPlus::Proofer.new( | ||
Proofing::Socure::IdPlus::Config.new( | ||
api_key: IdentityConfig.store.socure_idplus_api_key, | ||
base_url: IdentityConfig.store.socure_idplus_base_url, | ||
timeout: IdentityConfig.store.socure_idplus_timeout_in_seconds, | ||
), | ||
) | ||
end | ||
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.
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 intoResolutionProofingJob
, but then theIdV: 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 toverify 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 toResolutionProofingJob
.(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