search

fablabbcn / smartcitizen-api

The Smart Citizen Engine
https://developer.smartcitizen.me
GNU Affero General Public License v3.0
10 stars 4 forks source link

Enhancement/190 skip throttling for researchers and admins #236

Closed timcowlishaw closed 1 year ago

timcowlishaw commented 1 year ago

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!

oscgonfer commented 1 year ago

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 commented 1 year ago

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

oscgonfer commented 1 year ago

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 commented 1 year ago

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