abersheeran / asgi-ratelimit

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

Rate limit per request method #66

Closed pa-t closed 1 year ago

pa-t commented 1 year ago

I am working on implementing rate limiting with this library and ran into an issue when multiple endpoints share the same path but different methods. For example:

GET   /towns
POST  /towns

Using the pattern matching to set up limits would apply the same limit to both endpoints, but this is not desired.

...
config={
    r"^/towns": [Rule(second=10)],
}
...

A workaround I have found is in the auth function, getting the method and then using the method in the group name.

async def AUTH_FUNCTION(scope: Scope) -> Tuple[str, str]:
    ...
    method = scope['method'].lower()
    ...
    return user_unique_id, f"{method}-groupname"

Limits per method can then be set by

...
config={
    r"^/towns": [
        Rule(group="get-groupname", second=10),
        Rule(group="post-groupname", second=2),
    ],
}
...

Thoughts around improvements

In the rule.py, I see that the key in redis is

f"{path}:{user}:{name}": (limit, TTL[name])

Adding in the method to this key should take care of this on the backend

f"{path}:{method}:{user}:{name}": (limit, TTL[name])

And then in the Rule implementation, adding a field to specify method could look something like

Rule(group="groupname", method="get", second=10),
Rule(group="groupname", method="post", second=2),

Interested to hear some thoughts around this (and if i missed something obvious). Thanks

abersheeran commented 1 year ago
{
  'GET': {
    r"/path": [ Rule() ]
  },
  '*': {
    ......
  }
}

This seems to be fine too.

pa-t commented 1 year ago

Interesting, I didn't see that in the README / examples. So the config could look like

{
  'GET': {
    r"/path": [ Rule(second=10) ]
  },
  'POST': {
    r"/path": [ Rule(second=2) ]
  }
}

and the different rate limits would be applied per http method?

pa-t commented 1 year ago

I am testing this out right now and am not able to confirm this works

app.add_middleware(
    RateLimitMiddleware,
    authenticate=AUTH_FUNCTION,
    backend=RedisBackend(redis),
    config={
        'GET': {
            r"^/towns": [
                Rule(minute=1, block_time=10),
                Rule(group="authorized", minute=6),
                Rule(group="admin"),
            ]
        },
        'POST': {
            r"^/towns": [
                Rule(minute=1, block_time=100),
                Rule(group="authorized", minute=2),
                Rule(group="admin"),
            ]
        }
    }
)

I have logs in my auth function that are not showing up when i try this route. I am on version 0.9.0, is this feature on a different version?

abersheeran commented 1 year ago

Well, this is just another solution I thought of to this question you asked. It is not actually implemented in the code.

The question you have raised is good, and I think we should take the time to resolve it.

pa-t commented 1 year ago

Ahh i see, i thought you were saying it was already implemented. I took some time and made a quick implementation PR, let me know what you think. I was not able to get the tests to run, I'm not familiar with the httpx library, but I added a test that should cover it. This implementation works in my testing

pa-t commented 1 year ago

I saw the actions failed, I just pushed another commit with some more docs. Let me know what you think, thanks

abersheeran commented 1 year ago

I saw the actions failed, I just pushed another commit with some more docs. Let me know what you think, thanks

https://github.com/abersheeran/asgi-ratelimit/actions/runs/3269652151/jobs/5379899654

Looks like it's because of code formatting issues. Just run the following code to format.

black .
isort .
pa-t commented 1 year ago

fixed formatting and addressed your comments!

pa-t commented 1 year ago

Is there any way to re run a specific action job? Looks like only one failed and it was during the install dependencies Linux-Test (3.10, 4)

pa-t commented 1 year ago

@abersheeran I see the tests are passing now. What are your thoughts on getting this PR merged and the feature released? We would love to use this feature in our use case. Thanks!

abersheeran commented 1 year ago

Sorry for the lateness, the PR has been merged. Soon I will release a new version.

pa-t commented 1 year ago

@abersheeran just checking back in on the status of the new version release