-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add slow api rate limiting #566
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! One question below. I added German translation. Hardcoding the rate limits is fine for now, but we might want to make that configurable in the future e.g. via env maybe?
# Ensure the response was rate limited | ||
assert response.status_code == 429, response.json() | ||
data = response.json() | ||
assert data['detail']['id'] == APIRateLimitExceeded.id_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the @limiter.limit("2/minute")
for this not work here? It returns 200 in tests instead of expected 429?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not right. I'll look into this next week.
This is due to the rate limiter's cache not being cleared between requests. But this also brings up another issue so this PR is on hold. |
* waiting-list/join - 2/minute * schedule/public/availability - 10/minute * schedule/public/availability/request - 10/minute
c0d3c43
to
ac32f81
Compare
Fixes #466
Rules:
I think we could reduce the request one to like 2 or 3, but it's not really a concern right now. Mostly just preemptively avoiding spam. I didn't hook it up with redis because I don't think we have redis setup correctly on prod yet. Shouldn't be a problem for us right now though.
It works off of remote ip address, I think that's probably okay.