LG-13136: Fix rate limiting logic when user is on last try by night-jellyfish · Pull Request #10538 · 18F/identity-idp

night-jellyfish · 2024-05-01T18:46:54Z

🎫 Ticket

🛠 Summary of changes

When we used limited? we found the user was prevented from completing their last chance at a request, even if the request came back from TrueID as a success. Due to the use of >= in limited?, in this situation the attempts and max_attempts were equal. While polling for the results, the user would be short-circuited to the too_many_requests logic.

Instead we needed a way to only prevent that if the user had exceeded their max amount of tries.

📜 Testing Plan

Set doc_auth_max_attempts to a low number locally in your application.yml
Fail an attempt due to doc auth errors up to minus one the amount of times you allowed in the first step (so if you changed to 5, fail 4 times with doc auth errors)
For your last attempt, use a successful image / yml and make sure you're able to get through to the SSN page.

app/services/rate_limiter.rb

dawei-nava · 2024-05-01T18:55:38Z

app/controllers/idv/capture_doc_status_controller.rb

I am thinking whether we should completely remove this check, due to the way rate_limiter implemented.

And technically this controller wont be calle, since when doc_auth is submitted then the rate_limit should not be exceeded.

I am trying out removing this call locally. Everything seems to be passing so far.

My main concern is that - if we shouldn't ever call this controller, is there some sort of earlier check that's misfiring? Because if it shouldn't have ever been called, then this user should never have encountered this issue...

@night-jellyfish , i meant this wont be called technically if rate limited, unless something goes wrong or called intentionally.

Can you point me to the earlier check that would filter out rate limited requests? I am having a hard time finding it.

@night-jellyfish here, it's a controller before_action hook

identity-idp/app/controllers/idv/document_capture_controller.rb

Line 11 in add15b5

before_action :confirm_not_rate_limited, except: [:update]

also

identity-idp/app/controllers/idv/link_sent_controller.rb

Line 10 in add15b5

before_action :confirm_not_rate_limited

Thank you!! I also found this relevant conversation that further supports your point.

With that in mind, I updated this PR to instead delete the check for rate limiting, as you suggested. I still need to test this manually.

I think I need to also add a feature test for this, perhaps.

When we used `limited?` we found the user was prevented from completing their last chance at a request, due to the `>=`, since the attempts and max_attempts were equal. Instead we needed a way to only prevent that if the user had exceeded their max amount of tries.

In the previous commit, we were looking to prevent the `capture_doc_status_controller` from setting the rate limit too early. In this commit, we are instead removing the logic to even look at rate limiting here, since [as Dawei pointed out](#10538 (comment)), and as I found [a past discussion for](#9370 (comment)), this code should never be reached if rate limited.

…c when user is on last try

… test.

amirbey

👀

amirbey

When I get rate limited during they hybrid flow, the desktop is stuck on the polling page. It'd help to add a feature test for hybrid rate limiting

eileen-nava

I was able to successfully complete all the steps in the testing plan. I can approve this once you address Amir's feedback.

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

eileen-nava · 2024-05-07T18:59:38Z

When I get rate limited during the hybrid flow, the desktop is stuck on the polling page. It'd help to add a feature test for hybrid rate limiting

@amirbey When you were testing the hybrid flow and got rate limited, did the desktop still display the We sent a message to your phone screen? I think we might have a ticket/figma related to this, but I wanted to be sure I understood what you were referring to. Thanks!

amirbey · 2024-05-07T19:30:15Z

When I get rate limited during the hybrid flow, the desktop is stuck on the polling page. It'd help to add a feature test for hybrid rate limiting

@amirbey When you were testing the hybrid flow and got rate limited, did the desktop still display the We sent a message to your phone screen? I think we might have a ticket/figma related to this, but I wanted to be sure I understood what you were referring to. Thanks!

hi @eileen-nava ... yes, that screen remains unchanged after the user gets rate limited

eileen-nava · 2024-05-07T20:54:57Z

@amirbey @night-jellyfish I followed up with Kelli and realized that we had a Figma and ticket for a different issue. Kelli documented the past behavior in this figma and Team Ada fixed it in LG-9813.

night-jellyfish · 2024-05-08T00:15:02Z

I pushed up a commit to restore the original test that looked for that page.

Unfortunately I'm struggling to get both tests to pass. @amirbey or @dawei-nava perhaps we could look at it tomorrow?

dawei-nava · 2024-05-10T14:34:25Z

app/forms/idv/api_image_upload_form.rb

persist it immediately in case we get racing condition in poller

This timestamp is used at another place

identity-idp/app/controllers/concerns/idv/verify_info_concern.rb

Line 23 in 9a22735

document_capture_session.requested_at = Time.zone.now

In this case, guess there is no racing concern, so no need to persist immediately.

Also, should we reset it after doc auth since it use used at this place also, maybe not.

dawei-nava · 2024-05-10T14:34:59Z

app/forms/idv/api_image_upload_form.rb

Need explicitly save failed result and result timestamp when finger print check disabled.

dawei-nava · 2024-05-10T14:35:44Z

app/services/doc_auth/mock/doc_auth_mock_client.rb

Adding delays in mock client to facilitate tests that reflect reality, useful for hybrid flow poller testing.

This is a good idea. 👍🏻

dawei-nava · 2024-05-10T14:36:17Z

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

disable check so we can resubmit same images, easier to test.

dawei-nava · 2024-05-10T14:36:48Z

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

Add delays longer than a polling cycle which is 5 sec.

dawei-nava · 2024-05-10T15:18:09Z

Future works:

network exception should not be counted for rate limiting, this includes LN service return 200 but indicate service not available in response message sometimes.
examine behavior of exceptions on hybrid flow? like image upload controller exception, capture status controller exception/availability when polling

dawei-nava · 2024-05-10T15:45:01Z

app/controllers/idv/capture_doc_status_controller.rb


    def redo_document_capture_pending?
+      return true if !session_result && document_capture_session&.requested_at.present?
      return unless session_result&.dig(:captured_at)


we may take a detailed look, is it necessary? what's our assumption when this method is invoked? what are the edge cases?

eileen-nava · 2024-05-16T15:47:40Z

Team Timnit paused work on this bug in anticipation of the upcoming team transition. The comments provide guidance for whoever picks it up in the future. Thanks, @night-jellyfish and @dawei-nava, for all your work on this bug!

dawei-nava reviewed May 1, 2024

View reviewed changes

app/services/rate_limiter.rb Outdated Show resolved Hide resolved

dawei-nava reviewed May 1, 2024

View reviewed changes

Brittany Greaner added 2 commits May 1, 2024 16:05

night-jellyfish force-pushed the brittany/lg-1316-fix-rate-limit-bug branch from 7753cb3 to 67569af Compare May 1, 2024 23:14

Brittany Greaner and others added 5 commits May 1, 2024 16:15

changelog: User-Facing Improvements, Doc Auth, Fix rate limiting logi…

bc1c63f

…c when user is on last try

Rm added method and tests

c3ee24a

WIP

a9891c5

LG-13136: complete remove rate limit check in polling controller. Fix…

043a3b3

… test.

Use wait instead of sleep

3bb3c34

amirbey approved these changes May 7, 2024

View reviewed changes

amirbey self-requested a review May 7, 2024 16:05

amirbey requested changes May 7, 2024

View reviewed changes

Merge branch 'main' into brittany/lg-1316-fix-rate-limit-bug

cdc8919

eileen-nava reviewed May 7, 2024

View reviewed changes

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb Outdated Show resolved Hide resolved

Brittany Greaner added 3 commits May 7, 2024 14:52

Rm unnecessary user variable from specs

d147bee

Add back in test to make sure rate limiting happens on last page hybrid

bfc3b31

Rm unused index on loop

33e333b

LG-13136: deal with last rate limited failure.

208a5b8

eileen-nava self-requested a review May 9, 2024 18:25

LG-13136: dealing with multiple issues.

53dc047

dawei-nava reviewed May 10, 2024

View reviewed changes

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb Outdated

Copy link

Contributor

dawei-nava May 10, 2024

Choose a reason for hiding this comment

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

disable check so we can resubmit same images, easier to test.

dawei-nava reviewed May 10, 2024

View reviewed changes

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb Outdated

Copy link

Contributor

dawei-nava May 10, 2024

Choose a reason for hiding this comment

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

Add delays longer than a polling cycle which is 5 sec.

dawei-nava force-pushed the brittany/lg-1316-fix-rate-limit-bug branch from 547f055 to 7ff8083 Compare May 10, 2024 14:46

LG-13136: linter.

9a22735

dawei-nava force-pushed the brittany/lg-1316-fix-rate-limit-bug branch from 7ff8083 to 9a22735 Compare May 10, 2024 14:48

dawei-nava reviewed May 10, 2024

View reviewed changes

eileen-nava closed this May 16, 2024

Conversation

night-jellyfish commented May 1, 2024 • edited by eileen-nava Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirbey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirbey left a comment

Choose a reason for hiding this comment

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eileen-nava commented May 7, 2024

Uh oh!

amirbey commented May 7, 2024

Uh oh!

eileen-nava commented May 7, 2024

Uh oh!

night-jellyfish commented May 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava commented May 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

night-jellyfish commented May 1, 2024 •

edited by eileen-nava

Loading

dawei-nava May 1, 2024 •

edited

Loading

amirbey left a comment •

edited

Loading

dawei-nava May 10, 2024 •

edited

Loading

dawei-nava commented May 10, 2024 •

edited

Loading