feat: change own password confirmation #3894
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
kwasniew
requested review from
andreas-unleash,
sjaanus,
nunogois and
chriswk
and removed request for
andreas-unleash
| const { password, confirmPassword } = req.body; | ||
| if (password === confirmPassword) { | ||
| const { password, confirmPassword, oldPassword } = req.body; | ||
| if (password === confirmPassword && oldPassword != null) { |
There was a problem hiding this comment.
this should be part of the service but didn't want to change too many things in this PR
| @@ -362,6 +362,22 @@ class UserService { | |||
| await this.resetTokenService.expireExistingTokensForUser(userId); | |||
| } | |||
|
|
|||
| async changePasswordWithVerification( | |||
There was a problem hiding this comment.
delegates to changePassword but performs extra check of old password hash
| const currentPasswordHash = await this.store.getPasswordHash(userId); | ||
| const match = await bcrypt.compare(oldPassword, currentPasswordHash); | ||
| if (!match) { | ||
| throw new PasswordMismatch( |
There was a problem hiding this comment.
this results in 401 in the error translation layer
| user.id, | ||
| password, | ||
| oldPassword, | ||
| ); | ||
| res.status(200).end(); | ||
| } else { | ||
| res.status(400).end(); |
There was a problem hiding this comment.
not sure if we shouldn't make it 401 to be more precise
frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Nuno Góis <github@nunogois.com>
| const { password, confirmPassword } = req.body; | ||
| if (password === confirmPassword) { | ||
| const { password, confirmPassword, oldPassword } = req.body; | ||
| if (password === confirmPassword && oldPassword != null) { |
There was a problem hiding this comment.
Maybe it doesn't hurt to check for truthy here instead of != null so we can cover undefined and empty strings as well. But I'm guessing undefined is converted to null anyways.
| if (password === confirmPassword && oldPassword != null) { | |
| if (password === confirmPassword && oldPassword) { |
There was a problem hiding this comment.
This part was test-driven and missing oldPassword is turned into null as you mentioned so I'd rather stick to the original version since I can't even trigger a test scenario for undefined to test it.
| async changePasswordWithVerification( | ||
| userId: number, | ||
| newPassword: string, | ||
| oldPassword: string, |
There was a problem hiding this comment.
I expected also a check of newPassword !== oldPassword, but since we don't have any 'password expiration'/'force password change' this should be OK.
About the changes
When changing your own password you should specify the old password. I decided to place old password field above the new one.
Show new password error:
Show old password error:
Important files
Discussion points