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

Make password minimum requirements stronger

Open corrad82-4s opened this issue 3 years ago • 3 comments

References

Add references/links to any related issues or PRs. These may include:

Description

Short summary of changes (1-2 sentences). 422 error code is returned when password set by users, when they register for first time or reset password. This code means that password is invalid, according to minimum security requirements, checked by Backend.

Instructions for Reviewers

List of changes in this PR:

  • PasswordFormComponent has been extended to handle 422 response code from backend, showing a proper message to users
  • A Spec has been provided to test above behavior

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.

To test it, register a new user, and set a weak password as first one. Password won't be accepted. Set a more robust one, with a length included between 8 and 15 characters and at least an upper and lowercase chars and a number (according to default validation rule set in dspace.cfg). Password matching with these criteria will be accepted. Same test must be done in password recovery process.

Checklist

  • [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.
  • [ ] 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.

corrad82-4s avatar Aug 18 '22 08:08 corrad82-4s

Hi @tdonohue , thank you for your comments. We will take in charge requested changes. I agree, having both (different) validations in place lead to a not optimal User experience.

We can change it by removing the current minimum lenght validation, performed only on frontend side, or better just leave a check that password is not empty, unless ProfilePageSecurityFormComponent.passwordCanBeEmpty - https://github.com/4Science/dspace-angular/blob/8c40ed42da83388232e11ef876bde42b679f06b4/src/app/profile-page/profile-page-security-form/profile-page-security-form.component.ts#L56 property allows it.

On the other hand we would let in place just the new password robustness check performing the backend call. So, eventually, the user won't be "stopped" when trying to post an invalid password, but he will be informed after posting it by backend validation. I think it is more acceptable than having 2 distinct and not coherent messages. Do you agree?

corrad82-4s avatar Sep 01 '22 07:09 corrad82-4s

@corrad82-4s : I'm OK with leaving the check for a non-empty password & also the check that the passwords are identical between the two fields.

But, I think the minimum length check either needs to be removed or somehow updated to send a "validate only" request to the backend. For example: if the backend had a way to validate what the user has typed so far (without saving the password), then the UI could be updated to provide ongoing feedback to the user as they type their new password (i.e. on each character change). This would allow for the password validation error to be displayed until the user fully enters a new password that validates.

But, if it's not easily possible to add this sort of "validate only" option on each character change, then I think it's OK to simply require the user to submit a password before the validation errors are shown. If we go with this approach though, it'd be best to provide a note which describes what a valid password looks like, so that users don't need to submit an invalid password first before they understand what is valid.

tdonohue avatar Sep 01 '22 13:09 tdonohue

Hi @tdonohue, yes, I definitely agree in removing minimum lenght validation. Current backend logic, presented with related backend PR #8404 does not expose an endpoint to validate password, but invokes Regular Expression based validator during operation of password patch. We could isolate / replicate this check in a proper endpoint, to be called when users start entering their password, during character change, but I think this might affect a bit our original estimations.

In my opinion, a note about minimum password requirements could be useful either if we validate the password while it is entered or we do it after it has been submitted for patch operation. So I would add it.

Thank you

corrad82-4s avatar Sep 02 '22 09:09 corrad82-4s

Hello @tdonohue , thank you for your approval. Our developer pushed latest required changes.

corrad82-4s avatar Sep 21 '22 14:09 corrad82-4s

Merging as this is at +2. Thanks again @corrad82-4s !

tdonohue avatar Sep 22 '22 13:09 tdonohue