dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Ask current password to user when setting a new password

Open atarix83 opened this issue 2 years ago • 1 comments

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.

  1. Go to Profile page, current password field should exist.

  2. Type current password, enter new password and retype new password. Then try to save. 3. send the current password correctly.

  3. send current password wrong:

  4. send no current password:

  5. create a new user from the sidebar menu.

  6. Once created try to impersonate him.

  7. 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.

atarix83 avatar Sep 01 '22 15:09 atarix83

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.

mwoodiupui avatar Sep 01 '22 17:09 mwoodiupui

Noted in today's meeting to verify that an Admin can still add/reset another user's password

tdonohue avatar Sep 08 '22 14:09 tdonohue

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:".

mwoodiupui avatar Sep 15 '22 12:09 mwoodiupui

@tdonohue

your feedback should be addressed. Please mind for the third point you should update also the REST branch to properly check it

atarix83 avatar Sep 20 '22 09:09 atarix83

@tdonohue

fixed

atarix83 avatar Sep 21 '22 15:09 atarix83

@tdonohue merge conflicts resolved

atarix83 avatar Sep 23 '22 08:09 atarix83

Merging with 1 approval.

tdonohue avatar Sep 27 '22 18:09 tdonohue