-
Notifications
You must be signed in to change notification settings - Fork 1
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 default rate limiting #163
Conversation
Visit the preview URL for this PR (updated for commit c615443): https://uw-blueprint-starter-code--pr163-alex-add-rate-limit-ax9e87gy.web.app (expires Wed, 23 Feb 2022 03:31:41 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
backend/python/app/__init__.py
Outdated
@@ -47,6 +49,8 @@ def create_app(config_name): | |||
app.config["CORS_SUPPORTS_CREDENTIALS"] = True | |||
CORS(app) | |||
|
|||
Limiter(app, key_func=get_remote_address, default_limits=["15 per minute"]) |
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.
The rate limit seems to be on the lower side, though I'm not too sure what a reasonable value should be. How about we set a new env variable called BACKEND_API_DEFAULT_PER_MINUTE_RATE_LIMIT
to allow users to configure this value? We can leave 15 as the default.
Limiter(app, key_func=get_remote_address, default_limits=["15 per minute"]) | |
Limiter(app, key_func=get_remote_address, default_limits=[f"{os.getenv("BACKEND_API_DEFAULT_PER_MINUTE_RATE_LIMIT") or 15} per minute"]) |
Also, how easy would it be to override the rate limit for individual endpoints? It seems that we need to be a bit more strict with Firebase operations, see Firebase rate limits: https://firebase.google.com/docs/auth/limits
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.
Discussed during meeting, made the default rate limit configurable. Since it is per endpoint, leaving 15 as the default.
Overriding the rate limit looks simple. For TypeScript, they can create a new rate limiter and add it as a middleware for the route similar to other middlewares we have. For GraphQL, they can define all overridden rate limits in the shield function in graphql index.ts. For Python, they can annotate routes using the limiter object created by the Limiter call
e5302bf
to
c615443
Compare
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.
LGTM! 🚢
Regarding frontend error validation, one case that happens is that if the user cannot refresh token due to rate limit, success will be false and setAuthenticatedUser(null) will be called, logging out the user - what should the expected behaviour be in this case?
This behaviour seems fine to me (i.e. not totally unintuitive), we can leave the specifics of how to handle it to project teams
Fixes a CodeQL error in #150
Implementation description
app.use(limiter);
is commented for now since it applies to both REST requests and every GraphQL request, so you would only get 15 calls on /graphql overallSteps to test
TypeScript GraphQL
TypeScript REST
app.use(limiter);
in server.ts.Python REST
What should reviewers focus on?
Checklist