Skip to content
This repository has been archived by the owner on Jun 5, 2019. It is now read-only.

rate-limit initial implementation #69

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

podviaznikov
Copy link
Contributor

Added/Changed:

  1. rate-limits for secure API endpoints
  2. added redis dependency. Updated project to work on Heroku but didn't tested it there.
  3. rate limits can be customized though the config file

Open questions:

  1. Maybe we shouldn't have rate-limits for admins?
  2. Login endpoints are not rate-limited. Should discuss strategy in more details

Todo:

  1. Cover rate-limits with tests

@hulbert
Copy link
Contributor

hulbert commented Sep 25, 2014

Couldn't this cause issues since the web interface uses the API? Isn't this just rate limiting logged in users' requests? Perhaps I am misunderstanding the goal

@podviaznikov
Copy link
Contributor Author

@hulbert I had the same question as you. That is why I had first open question. I thought that we should at least disable rate-limits for admins because it will prevent from UI use.

@davidkaneda
Copy link
Contributor

@podviaznikov @hulbert Hey guys, great stuff. Here are some thoughts on the questions:

  • I wouldn’t worry too much about the admin app usage... This work is actually more for when we allow admins to generate API tokens (or use OAuth), but essentially: Once we start gating "external" API requests (currently we just use session-based, which won't be enough for app devs). For now, perhaps just bump apiHourlyLimit to ~10,000?
  • The Redis2go choice is great, and thanks for adding into Travis as well :)
  • What are your thoughts/questions on the login aspect? Does it not make sense to use the rate-limiter here (and easier to save a lock on the User model?). Totally up to you... would love to see an implementation here, though :)

All make sense? Just let me know if you have extra questions/ideas!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants