1159097 - RatelimitThrottle ratelimits on non http 2xx responses

Status: RESOLVED WONTFIX

(Input Graveyard :: Submission, defect)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

The RatelimitDecorator has an allow_request method where it both checks to see if the request should be ratelimited, but also does the bookkeeping which potentially causes future requests to be ratelimited.

This has the unfortunate problem of preventing people from sending an invalid email address, fixing it and then re-posting. The second post will get ratelimited.

What should probably happen is that the bookkeeping only gets done for HTTP 2xx responses and not for HTTP 4xx responses.

This bug covers double-checking all that, looking into it further, mulling over the whole thing while making a hot cup of tea and then looking deep into the fiery abyss of the universe without blinking for a bit. Then if it turns out to be a problem we want to fix, fixing it. If not, we should write up a lovely comment explaining why not.

This is related to one of the problems in bug #1155600.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

I think we should not do this until after we've upgraded django-rest-framework to something recent. Otherwise we're likely going to have to redo this.

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

Need to do use this:

https://github.com/jsocol/django-ratelimit/issues/72#issuecomment-128819537

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

I think this is 1 point of work. I'm pretty sure it's not hard to do or test.

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

There's some confusing things in this bug.

We have two different throttling systems and which one gets used depends on the context.

First, the feedback API uses fjord.base.utils.RatelimitThrottle which ties into django-rest-framework's throttling system which throttles API requests regardless of outcomes. The theory being that if you exceed your API request allotment, then you back off for a bit and then try again. This doesn't work well with client side validation like in bug #1155600 which uses the API.

Second, the generic feedback form handling views use the fjord.base.utils.ratelimit decorator which also limits the number of requests, but that form does client-side validation, so it should prevent submissions that aren't valid.

I'm changing this bug to cover the first issue where RatelimitThrottle shouldn't limit *all* API requests, but only successful ones.


Now that that's established, the django-rest-framework throttling system has no ability for a throttle to check before the request is handled and then do something after the request is handled based on whether it succeeded or not. Given that, I think we need to ditch the RatelimitThrottle and instead throttle the POST-handling method directly with a class mixin. That's a rewrite, so I think this is 2 points.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

New quarter!

Comment 6

[Osmose [:osmose, :mkelly]]

The Input service has been decommissioned (see bug 1315316) and has been replaced by a redirect to an external vendor (SurveyGizmo). I'm bulk WONTFIXing Input bugs that do not appear to be relevant anymore.