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
| 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.
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.
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
sherryhli
left a comment
There was a problem hiding this comment.
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