Skip to content

Rate limit proofing by SSN (LG-4942)#5307

Merged
zachmargolis merged 4 commits intomainfrom
margolis-ssn-throttling-lg-4942
Aug 19, 2021
Merged

Rate limit proofing by SSN (LG-4942)#5307
zachmargolis merged 4 commits intomainfrom
margolis-ssn-throttling-lg-4942

Conversation

@zachmargolis
Copy link
Copy Markdown
Contributor

This is a kantara requirement, rate limit proofing by SSN (across users), builds on #5302 to not require a user ID

Comment on lines +18 to +23
throttle = Throttle.for(
target: Digest::SHA256.hexdigest(pii_from_doc[:ssn]),
throttle_type: :proof_ssn,
)

if throttle.throttled_else_increment?
Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to override idv_throttle_params from the base class:

def idv_throttle_params
{
user: current_user,
throttle_type: :idv_resolution,
}
end

My thinking is, that rate limiting helps to limit individual users doing IDV too much, so by making this a separate check, we still want to have both throttles

throttle_type: :proof_ssn,
step_name: self.class,
)
redirect_to idv_session_errors_failure_url
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the screens where we use the JavaScript spinner submission, yeah? I think it might handle these redirects gracefully, but worth double-checking, since normally it's dealing with happy-path redirects:

window.location.href = response.url;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed it worked as expected! The error message is a little funky, but that may be OK for now

Screen Shot 2021-08-19 at 9 21 44 AM

How important do you think it is to have a feature test that runs through this with JS enabled?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important do you think it is to have a feature test that runs through this with JS enabled?

Could be nice, but not a blocker. Do we at least have one for the happy path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachmargolis zachmargolis requested a review from aduth August 19, 2021 17:15
throttle_type: :proof_ssn,
step_name: self.class,
)
redirect_to idv_session_errors_failure_url
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important do you think it is to have a feature test that runs through this with JS enabled?

Could be nice, but not a blocker. Do we at least have one for the happy path?

Copy link
Copy Markdown
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@zachmargolis zachmargolis merged commit f8ff62a into main Aug 19, 2021
@zachmargolis zachmargolis deleted the margolis-ssn-throttling-lg-4942 branch August 19, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants