-
Notifications
You must be signed in to change notification settings - Fork 438
Change password #1558
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
base: development
Are you sure you want to change the base?
Change password #1558
Conversation
…ranslations for "change.password" to data.ts
…o allow for the showing of more than one kind of modal. Adjusted handleClose() and handleShow() accordingly. Added a modalType to login modal and created a copy with modalType 'changePassword' as placeholder.
…alType to allow for the showing of more than one kind of modal. Adjusted handleClose() and handleShow() accordingly. Added a modalType to login modal and created a copy with modalType 'changePassword' as placeholder." This reverts commit c20bec1.
….ts, correct modal now shows when clicking change password.
Co-authored-by: GoKartMan89 <[email protected]> Co-authored-by: caleb-stark <[email protected]> Co-authored-by: Andrew-Ryer <[email protected]>
…into change password modal.
huss
left a comment
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.
Thanks to @Andrew-Ryer, @natetowsley, @GoKartMan89 & @caleb-stark for this contribution to OED. I'm sorry it took about a week to get back to you. Overall, it works well. I have made some comments to consider. Please let me know if anything is not clear or I can help.
src/client/app/translations/data.ts
Outdated
| "calibration.save.database": "Save changes to database", | ||
| "calibration.submit.button": "Submit", | ||
| "cancel": "Cancel", | ||
| "change.password": "Change password", |
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'm wondering if these should start with password as do a number of related ones such as password.new.enter.
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 changing change.password. I'm thinking that current.password and current.password.enter might also be best if they start with password so in that section and consistent.
src/server/routes/users.js
Outdated
| } | ||
| }; | ||
| if (!validate(req.body, validParams).valid) { | ||
| res.status(400).json({ message: 'Invalid params' }); |
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 realize this is not done elsewhere in this file but the newer (and I think better) way is as in src/server/routes/conversions.js where it has:
const validatorResult = validate(req.body, validConversion);
if (!validatorResult.valid) {
log.warn(`Got request to edit conversions with invalid conversion data, errors: ${validatorResult.errors}`);
failure(res, 400, `Got request to edit conversions with invalid conversion data, errors: ${validatorResult.errors}`);
} else {
This uses the standard way of returning the message and logs it which is a good idea for failed login attempts. What do you think?
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 worked with the newer method except no sub-message would show on client side underneath "Failed to change password" on the error notification when using failure(). I also tried to edit all routes to use this new method for consistency (still have the copy if that's worth looking into) but ultimately couldn't get it to work so replaced failure() line with res.status(400).
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 was curious why it did not work so I tried to do it. I should soon push a commit to make the change. It also address some other items involving security that were outside the work you were asked to do. I welcome any thoughts you have about my changes.
| return ( | ||
| <> | ||
| {/* Unsaved Warning Component */} | ||
| {showUnsavedWarning && ( |
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'm not seeing the warning even if I made changes. I have not looked into this.
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'm still seeing this issue. For example, on the edit user page for admins if you type any characters in either password field and then click off the modal it asks what to do in a popup. On this page it does not give the popup. Let me know if you don't see this or need any help.
…into change_password
messages in the data.ts translation file
…ssword. Formatting and indentation fixes.
…into change_password
|
@huss Changes addressed. Please let us know if there's anything else we should do. |
- The messages are also displayed to the user. - Changes information returned to mask info for security reasons. Also logs more info for security. - Minor changes in message wording for translation. - Minor formatting changes.
huss
left a comment
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.
Thanks to @Andrew-Ryer, @natetowsley, @GoKartMan89 & @caleb-stark for the updated code to address my previous comments. There are a couple of old comments that I thought should still be addressed and I added to the comment chain. I also pushed a commit to use success/failure since you indicated issues in getting it to work. It also includes other changes to address ongoing security work that was not part of what you were doing. I would welcome you looking at the code to see if you think it is correct/appropriate and verifying it works as desired. Please let me know if anything is not clear or I can help.
| return ( | ||
| <> | ||
| {/* Unsaved Warning Component */} | ||
| {showUnsavedWarning && ( |
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'm still seeing this issue. For example, on the edit user page for admins if you type any characters in either password field and then click off the modal it asks what to do in a popup. On this page it does not give the popup. Let me know if you don't see this or need any help.
src/client/app/translations/data.ts
Outdated
| "calibration.save.database": "Save changes to database", | ||
| "calibration.submit.button": "Submit", | ||
| "cancel": "Cancel", | ||
| "change.password": "Change password", |
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 changing change.password. I'm thinking that current.password and current.password.enter might also be best if they start with password so in that section and consistent.
src/server/routes/users.js
Outdated
| } | ||
| }; | ||
| if (!validate(req.body, validParams).valid) { | ||
| res.status(400).json({ message: 'Invalid params' }); |
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 was curious why it did not work so I tried to do it. I should soon push a commit to make the change. It also address some other items involving security that were outside the work you were asked to do. I welcome any thoughts you have about my changes.
|
For unknown reasons my recent comments are showing up twice. I'm just going to leave them to avoid any more issues. Sorry. |
Description
This PR implements the full User Password Change feature in OED, enabling any authenticated user to securely change their own password via the Options → Change Password menu, without admin intervention. The work spans backend API, frontend RTK Query integration, a new modal UI component, and integration into the main application with translations.
On the backend, it adds a new POST /api/users/changePassword endpoint protected by authentication middleware that verifies the user’s JWT, validates input (including length constraints), confirms the current password with bcrypt.compare, securely hashes the new password with bcrypt.hash, and updates the database via User.updateUserPassword. On the frontend, it introduces a changePassword RTK Query mutation and a ChangePasswordModal React component that displays the current username, collects current/new/confirm password, performs real-time validation, and shows success/error notifications. The feature is fully integrated into HeaderButtonsComponent via a “Change Password” entry in the Options dropdown and wired into the translations system so all UI text is available in English, French, and Spanish.
Fixes #1526 - Allow each user to change password
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
Contributors:
Andrew Ryer — @Andrew-Ryer — [email protected]
Nathan Towsley — @natetowsley — [email protected]
Tyler Herndon — @GoKartMan89 — [email protected]
Caleb Stark — @caleb-stark — [email protected]