Skip to content
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

Merged
merged 2 commits into from
Feb 21, 2022
Merged

Add default rate limiting #163

merged 2 commits into from
Feb 21, 2022

Conversation

alexguo8
Copy link
Contributor

@alexguo8 alexguo8 commented Feb 10, 2022

Fixes a CodeQL error in #150

Implementation description

  • Added default rate limit of 15 calls per minute for TypeScript REST, TypeScript GraphQL, and Python REST
  • This should apply to each endpoint separately
  • 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 overall
  • Added some missing error validation to API calls on frontend

Steps to test

TypeScript GraphQL

  1. Go to localhost:3000 and login.
  2. Click Display Entities and then Go Back 15 times (you can change the rate limit for testing if this is too high). On the 16th try, it should rate limit you and stop displaying the results. We currently do not handle errors on this page so you may need to add an onError to the useQuery hook to see the error.

TypeScript REST

  1. Set up TypeScript REST on the frontend (change DisplayTableContainer to use REST queries). Also, uncomment app.use(limiter); in server.ts.
  2. Go to localhost:3000 and login.
  3. Repeat step 2 in TypeScript GraphQL above.

Python REST

  1. Set up Python REST - remember to change MG_DATABASE_URL in .env to the python db
  2. Go to localhost:3000 and login.
  3. Repeat step 2 in TypeScript GraphQL above.

What should reviewers focus on?

  • What should the default rate limit be?
  • Are there any other places in the frontend that I missed with error validation? (with rate limiting, some places where we did not catch errors will now crash more easily)
  • 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?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@github-actions
Copy link

github-actions bot commented Feb 10, 2022

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 🌎

@@ -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"])
Copy link
Member

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.

Suggested change
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

Copy link
Contributor Author

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

@alexguo8 alexguo8 force-pushed the alex/add-rate-limit branch from e5302bf to c615443 Compare February 16, 2022 03:28
Copy link
Member

@sherryhli sherryhli left a 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

@alexguo8 alexguo8 merged commit 254132d into main Feb 21, 2022
@alexguo8 alexguo8 deleted the alex/add-rate-limit branch February 21, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants