-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
packages/tupaia-web/src/features/Map/MapOverlaySelector/MapOverlayList.tsx
Outdated
Show resolved
Hide resolved
packages/database/src/migrations/20240822044835-addLoginAttemptsTable-modifies-schema.js
Outdated
Show resolved
Hide resolved
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.
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 👍
packages/central-server/src/apiV2/authenticate/TupaiaRateLimiter.js
Outdated
Show resolved
Hide resolved
packages/central-server/src/apiV2/authenticate/TupaiaRateLimiter.js
Outdated
Show resolved
Hide resolved
packages/central-server/src/apiV2/authenticate/TupaiaRateLimiter.js
Outdated
Show resolved
Hide resolved
packages/central-server/src/apiV2/authenticate/TupaiaRateLimiter.js
Outdated
Show resolved
Hide resolved
packages/central-server/src/apiV2/authenticate/TupaiaRateLimiter.js
Outdated
Show resolved
Hide resolved
packages/central-server/src/tests/apiV2/authenticate/authenticate.test.js
Show resolved
Hide resolved
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.
Looks good! One comment about needing to check that it's password auth when checking if rate limited, but pre-approving 👍
@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. |
merge: update branch with latest dev
* forward client ip to auth server when logging in * set trust proxy in central server * Update ApiBuilder.ts * Update createApp.js
merge: update branch with latest dev
merge: update branch with latest dev
merge: update branch with latest dev
Issue #: feat(security): RN-1108: Prevent too many login attempts
Changes:
Screenshots: