Skip to content
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

Enhancement/190 skip throttling for researchers and admins #236

Merged

Conversation

timcowlishaw
Copy link
Contributor

Solution to #190 - This proved to be a little tricky, as the throttling happens in Rack middleware,
before we've authenticated the user, and attempting to move authentication earlier
in the request cycle threw up a load of very weird problems, as well as being less
than ideal - as even throttled requests would have to hit the database.
See this other branch for the details of this abandoned approach.

The solution here is, on authenticated requests, to set a flag in the cache for
the requesting IP (with a timeout) in order to suspend throttling for that IP.
therefore, on the next request, throttling will be disabled.

One drawback of this is that unauthorized users on the same IP as an admin or researcher
will not be throttled - however, this is hopefully a corner case, and I can't think of
any other solution that doesn't involve authenticating users before throttling
(with the problems stated above), or making requests stateful using a session cookie
(which both immediately makes our API less RESTful, and makes the throttling trivial
to bypass by simply not sending the cookie.

Let me know if this is an acceptable solution!

@timcowlishaw timcowlishaw requested a review from oscgonfer May 3, 2023 07:05
@oscgonfer
Copy link
Contributor

Hi!

One question: as I understand it, the cached IP is only set by an authorized request, which will constantly be updated if (and only if) there are more authorized requests, right?

Also, there is no effect to un-authenticated requests (no matter where they are), correct?

@timcowlishaw timcowlishaw force-pushed the enhancement/190-skip-throttling-for-researchers-and-admins branch from 716c881 to fb238ae Compare May 25, 2023 06:14
@timcowlishaw
Copy link
Contributor Author

@oscgonfer I think this is ready for you now! All tested on staging and working with cloudflare!

This proved to be a little tricky, as the throttling happens in Rack middleware,
*before* we've authenticated the user, and attempting to move authentication earlier
in the request cycle threw up a load of very weird problems, as well as being less
than ideal - as even throttled requests would have to hit the database.

The solution here is, on authenticated requests, to set a flag in the cache for
the requesting IP (with a timeout) in order to suspend throttling for that IP.
therefore, on the next request, throttling will be disabled.

One drawback of this is that unauthorized users on the same IP as an admin or researcher
will not be throttled - however, this is hopefully a corner case, and I can't think of
any other solution that doesn't involve authenticating users before throttling
(with the problems stated above), or making requests stateful using a session cookie
(which both immediately makes our API less RESTful, and makes the throttling trivial
to bypass by simply not sending the cookie.

Let me know if this is an acceptable solution!
We realised that given the production and staging environments were behind cloudflare, `request.ip` refers to the cloudflare endpoint making the request, leading to inconsistent behaviour.
@timcowlishaw timcowlishaw force-pushed the enhancement/190-skip-throttling-for-researchers-and-admins branch from fb238ae to 5d67bfa Compare May 25, 2023 06:46
@oscgonfer
Copy link
Contributor

oscgonfer commented May 30, 2023

Hi @timcowlishaw, now unauthenticated requests do throttle correctly after the remote_ip changes, but I have just tested with a user that has role_mask = 5 (admin) in the request headers and still got a throttling in my test. Could it be?

The request is:

curl 'https://staging-api.smartcitizen.me/v0/devices/1' -X GET -H 'Content-Type: application/json;charset=UTF-8' -H 'Authorization: Bearer <admin-token-bearer>'

@timcowlishaw
Copy link
Contributor Author

Hmm, I will investigate - i didn't see this behaviour when i tested, but it's entirely possible I overlooked some particular case. apologies!

@oscgonfer oscgonfer merged commit 00b2b5c into master May 31, 2023
@timcowlishaw timcowlishaw deleted the enhancement/190-skip-throttling-for-researchers-and-admins branch July 4, 2024 08:20
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.

2 participants