-
Notifications
You must be signed in to change notification settings - Fork 114
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
[HACKATHON] Initial crack at an IdV matcher #11624
[HACKATHON] Initial crack at an IdV matcher #11624
Conversation
Matcher is a state machine that collects IDV "attempts" as they happen and tries to suss out interesting things about them. [skip changelog]
attr_reader :current_idv_attempt | ||
attr_reader :idv_attempts |
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.
When we encounter a new "welcome submitted" event, a new IdvAttempt
is created and stored in current_idv_attempt
. Subsequent events then update the state of that attempt. If we see another "welcome submitted" event, the current_idv_attempt is put in idv_attempts
and a new one is started.
end | ||
|
||
when RATE_LIMIT_REACHED_EVENT | ||
handle_rate_limit_reached(event:) |
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 does not use for_current_idv_attempt because not all rate limits are idv-related. We need to figure that out first.
|
||
private | ||
|
||
def add_significant_event( |
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'm not super happy about introducing another overload of the term "event" here, but the idea is we capture only the most interesting things in a way we can display to the end user
bin/summarize-user-events
Outdated
find_cloudwatch_events do |event| | ||
Time.zone ||= 'America/New_York' | ||
|
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 easier to set the timezone once, instead of for each event?
find_cloudwatch_events do |event| | |
Time.zone ||= 'America/New_York' | |
Time.zone ||= 'America/New_York' | |
find_cloudwatch_events do |event| |
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.
Done in #11627
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 ran into an issue with this--I think that when the Cloudwatch client uses multiple threads each thread needs its time zone initialized.
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.
ah, I'll update it to use the configured zone.
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.
hmmm maybe we need to update the CW client to call that block back on the main thread only?
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 is a great start!
This PR adds a big, giant "IDV" matcher.
The matcher runs through event logs and observes when new attempts at identity verification start. It then tries to describe how they went.
Things it does well:
Things it does not so well:
Ultimately, identity verification is a "big" thing, and having a facility for sub-matchers rather than one monolithic matcher would probably be good. I don't think this PR represents the final form, but it does represent a form, and starts to capture some of the built-in knowledge you need to interpret IDV-related event logs.