abersheeran / asgi-ratelimit

A ASGI Middleware to rate limit
Apache License 2.0
292 stars 11 forks source link

feature: Retry-After header #18

Closed euri10 closed 3 years ago

euri10 commented 3 years ago

this would be a good one to have: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After

abersheeran commented 3 years ago

I support this design. But how to pass the limited time to the 429 handler, I have no idea.

allerter commented 3 years ago

Hi. I'd thought about this too. Implementing this can be really easy in the Redis backend. All we need is the value of the TTL blocking:{user} command. If we add an extra call we could do this in RateLimitMiddleware.__call__:

if not has_rule or await self.backend.allow_request(url_path, user, rule):
    return await self.app(scope, receive, send)

# NEW
retry_after = await self.backend.block_ttl(user)
return await self.on_blocked(scope, receive, send, retry_after)

And this would be the implementation of Redis.blocked_time:

async def block_ttl(self, user: str) -> int:
    return await self._redis.ttl(f"blocking:{user}")

The downside of this solution is that it adds an extra call to the backed. There's another solution that wouldn't change the number of calls. We could change the return value of Redis.is_blocking and in turn Redis.allow_request to return an integer instead of a boolean. To do this Redis.is_blocking would call the TTL command above and return its value instead. -2 would mean the user is not blocked, -1 to mean the user is banned and any other integer would be the value of the retry-after header.

abersheeran commented 3 years ago

@allerter It is a good idea to let Backend return the time remaining to wait. When it returns 0, it means that there is no need to wait.

But just like other places, we can return to ASGI App through a parameterized function without destroying the ASGI interface.

# NEW
retry_after = await self.backend.block_ttl(user)
if retry_after == 0:
    handler = self.app
else:
    handler = self.on_blocked(retry_after)
return await handler(scope, receive, send)
allerter commented 3 years ago

Understandable. What about the extra request to the backend that backend.block_ttl makes? Is that okay? As I've explained above we can avoid it by changing the behavior of the methods mentioned above.

abersheeran commented 3 years ago

I think it should be refactored here to unify the interface. Backend only exposes one function. This function returns the time (integer) that needs to be waited before the next request can be requested. A value of 0 means that there is no need to wait.

euri10 commented 3 years ago

I think it should be refactored here to unify the interface. Backend only exposes one function. This function returns the time (integer) that needs to be waited before the next request can be requested. A value of 0 means that there is no need to wait.

I came to the same conclusion while working on https://github.com/abersheeran/asgi-ratelimit/pull/11 I implemented the feature for the SlidingRedisBackend in this draft PR but I'm lacking time at the moment to properly finish it , so sorry for that ! But maybe that can help discuss the interface required for the backend abc