dspace-angular
dspace-angular copied to clipboard
Ask current password to user when setting a new password
References
- Fixes #1737
- Requires DSpace/DSpace#[pr-number] (REST API PR is coming)
Description
Added changes to pass current password as challenge value to BE to update password.
Instructions for Reviewers
Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.
List of changes in this PR:
- Added DynamicInputModel for current-password field to profile-page-security-form, if its profile security form.(profile-page-security-form.component.ts)
- Added @Output param to profile-page-security-form to emit current-password value to parent component.(profile-page-security-form.component.ts)
- Receive current password to profile-page which set in profile-page-security-form.(profile-page.component.ts)
- changed operation signature to call /password endpoint of BE with challenge value.(profile-page.component.ts)
- Added translation strings with key (en.json5)
- Added test case to handle currentPasswordValue emit.
- Added test case to handle when password is filled in, and is valid but return 403
Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.
Note: To test this feature use webui.user.assumelogin = true property in dspace/config/local.cfg file of Back End.
-
Go to Profile page, current password field should exist.
-
Type current password, enter new password and retype new password. Then try to save. 3. send the current password correctly.
-
send current password wrong:
-
send no current password:
-
create a new user from the sidebar menu.
-
Once created try to impersonate him.
-
In the profile section the “Security box should not be available anymore.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
- [x] My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
- [x] My PR passes TSLint validation using
yarn run lint
- [x] My PR doesn't introduce circular dependencies
- [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
- [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
- [x] If my PR includes new, third-party dependencies (in
package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
I am surprised that we are not using the method of emailing a link to a confirmation page, which DSpace has had for years. The change_password
email body is still present in config/emails/
and code to use it is still in org.dspace.eperson.AccountServiceImpl
. I would consider that at least as secure as requiring the old password, and it also works in the case of having forgotten the old password.
Noted in today's meeting to verify that an Admin can still add/reset another user's password
I'm going to be very picky here and point out that "challenge" is not just the wrong word; it's the wrong concept. The challenge is "current password:". The user has provided an invalid response. But that is a security term of art, and probably unclear to a lot of users. The least jargonic expression of the failure would be something like "that is not your current password."
While I'm picking nits: "Password:" would be unambiguous if it were "New password:" and "Retype to confirm:" would be unambiguous if it were "Retype new password to confirm:".
@tdonohue
your feedback should be addressed. Please mind for the third point you should update also the REST branch to properly check it
@tdonohue
fixed
@tdonohue merge conflicts resolved
Merging with 1 approval.