-
-
Notifications
You must be signed in to change notification settings - Fork 730
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: Disallow repeating last 5 passwords. #7552
Conversation
We'll store hashes for the last 5 passwords, fetch them all for the user wanting to change their password, and make sure the password does not verify against any of the 5 stored hashes.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
I'll update the frontend to handle the new PreviouslyUsedPassword Error. |
@@ -62,6 +63,7 @@ export interface ILoginUserRequest { | |||
} | |||
|
|||
const saltRounds = 10; | |||
const disallowNPreviousPasswords = 5; |
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 configurable instead of a constant?
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.
I think for now we do the same as we do with salt rounds, just leave it as a constant
async setPasswordHash( | ||
userId: number, | ||
passwordHash: string, | ||
disallowNPreviousPasswords: number, |
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 have a default so we don't need to pass it every time?
@@ -13,6 +13,7 @@ function mergeAll<T>(objects: Partial<T>[]): T { | |||
} | |||
|
|||
export function createTestConfig(config?: IUnleashOptions): IUnleashConfig { | |||
getLogger.setMuteError(true); |
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.
I wonder what's this, and whether it should be included in this PR.
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.
It's muting the error logger of our test outputs, so we don't end up with tons of error logging that isn't actually an error when running tests.
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.
LGTM. Left a few comments that we can optionally address.
I suggest manually going through the password screens just to confirm that the new behaviors make sense to you as well.
Co-authored-by: Nuno Góis <[email protected]>
We'll store hashes for the last 5 passwords, fetch them all for the user wanting to change their password, and make sure the password does not verify against any of the 5 stored hashes.
Includes some password-related UI/UX improvements and refactors. Also some fixes related to reset password rate limiting (instead of an unhandled exception), and token expiration on error.