-
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
#2783 limit number of failed login and reset password requests #2791
#2783 limit number of failed login and reset password requests #2791
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2791 +/- ##
==========================================
- Coverage 63.01% 58.87% -4.15%
==========================================
Files 18 114 +96
Lines 1214 4532 +3318
Branches 187 1227 +1040
==========================================
+ Hits 765 2668 +1903
- Misses 449 1864 +1415
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Everything works fine, the messages telling about going over the requests limit are showing fine on the frontend. I left some comments about code
…rength bar, test for login and registration
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.
Great work! 💯
verification/curator-service/ui/src/components/landing-page/ChangePasswordForm.tsx
Outdated
Show resolved
Hide resolved
verification/curator-service/ui/src/components/landing-page/SignUpForm.tsx
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 good, not sure about some of the enums -- and the time limit windows vary
|
||
updateFailedAttempts( | ||
user._id, | ||
AttemptName.ResetPassword, |
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.
should be ForgotPassword
// Check if token exists | ||
const passwordResetToken = await tokens().findOne({ | ||
userId, | ||
}); | ||
if (!passwordResetToken) { | ||
updateFailedAttempts( | ||
userId, | ||
AttemptName.ForgotPassword, |
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.
ResetPassword? not clear what's the difference
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.
Thank you for your input! Of course, I made a mistake and assigned the wrong AttemptNames in sections of code that you highlighted but to answer your question I named them (AttemptName) accordingly:
- Login - counter for login
- ResetPassword - counter for resetting a password in a user's profile (already logged in)
- ForgotPassword - counter for requesting a reset password link that is located on the landing page of the curator portal
- ResetPasswordWithToken - counter for resetting the password through a form to which the user has access by an email link
const numberTimeLimiters = { | ||
loginAttempt: { | ||
maxNumberOfFailedLogins: 8, | ||
timeWindowForFailedLogins: 60, //min |
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.
Better to put the unit in the property name like timeWindowForFailedLoginsMinutes
import rateLimit from 'express-rate-limit'; | ||
|
||
export const loginLimiter = rateLimit({ | ||
windowMs: 20 * 60 * 1000, // 20 minutes |
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.
Should this be 60 minutes? It's different from numberTimeLimiters
}); | ||
|
||
export const resetPasswordLimiter = rateLimit({ | ||
windowMs: 30 * 60 * 1000, // 30 minutes |
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.
same here, different from numberTimeLimiters
Changes: