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

feat(security): RN-1108: Prevent too many login attempts #5861

Open
wants to merge 42 commits into
base: dev
Choose a base branch
from

Conversation

tcaiger
Copy link
Contributor

@tcaiger tcaiger commented Aug 26, 2024

Issue #: feat(security): RN-1108: Prevent too many login attempts

Changes:

  • Install rate-limiter-flexible package
  • Add login_attempts table to store rate limiting state based on node-rate-limiter docs
  • Create a new TupaiaRateLimiter class that handles rate limiting logic
  • Update authenticate endpoint handler to apply rate limiting

Screenshots:

Screenshot 2024-08-27 at 11 14 14 AM

Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really clean @tcaiger 🙏 Had a few comments, but also realised we totally forgot about the other auth methods, so might wanna chat about how we wanna handle them 👍

Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One comment about needing to check that it's password auth when checking if rate limited, but pre-approving 👍

@tcaiger
Copy link
Contributor Author

tcaiger commented Sep 10, 2024

@rohan-bes This is ready for another review. I started working on adding the RateLimiting to the server-boilerplate middleware but then I decided that it is quite a big increase in scope since this ticket is called login rate limiting. Since the middleware change would involve adding rate limit handling to all micro-service requests, I think it should be a new ticket and be refined if it's something that we intend to do.

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.

3 participants