Create servlet to change password#54
Conversation
gauravmishra
left a comment
gauravmishra
left a comment
There was a problem hiding this comment.
Few minor comments. The major change still needed is to persist the password changes to PersistentStore. Otherwise, the password will be set back to the original password once the session ends.
| if (userStore.isUserRegistered(username)) { | ||
| User user = userStore.getUser(username); | ||
| if (BCrypt.checkpw(password, user.getPassword())) { | ||
| user.setPassword(oldpassword); |
There was a problem hiding this comment.
Also need to update datastore for persistence.
|
|
||
| if (userStore.isUserRegistered(username)) { | ||
| User user = userStore.getUser(username); | ||
| if (BCrypt.checkpw(password, user.getPassword())) { |
There was a problem hiding this comment.
you need to check oldpassword here rather than password; then setPassword to password rather than oldpassword in the next line.
| request.getRequestDispatcher("/WEB-INF/view/login.jsp").forward(request, response); | ||
| } | ||
| } else { | ||
| request.setAttribute("error", "That username was not found."); |
There was a problem hiding this comment.
Would "No user logged in" be a more appropriate error message here?
gauravmishra
left a comment
gauravmishra
left a comment
There was a problem hiding this comment.
Left some comments on changes required...some other suggestions that would be good to have:
- Add a change password button in profile that takes user to the /changepassword page
- Invalid old password should redirect to profile instead of login as the user is logged in
- Check and raise error if old password and new password are the same.
- You don't need messageStore in ChangePasswordServlet; can remove it.
| if (userStore.isUserRegistered(username)) { | ||
| User user = userStore.getUser(username); | ||
| if (BCrypt.checkpw(oldpassword, user.getPassword())) { | ||
| if (request.getParameter("update") != null) { |
There was a problem hiding this comment.
this should be "newpassword" instead "update"
| if (BCrypt.checkpw(oldpassword, user.getPassword())) { | ||
| if (request.getParameter("update") != null) { | ||
| String newPassword = request.getParameter("newpassword"); | ||
| user.setPassword(newPassword); |
There was a problem hiding this comment.
you've to hash the password before setting it:
String passwordHash = BCrypt.hashpw(newPassword, BCrypt.gensalt());
user.setPassword(passwordHash);
2 participants
I didn't create the test file just because I'm behind but hopefully I updated the User entity in the datastore correctly -- let me know if y'all find anything I should fix! Thanks!