[SR][UX] Provide more guidance on password strength required

[herbdool]

Description of the need

Currently the description under the password field states that "Weak passwords will be rejected".

I've gotten feedback that it's not clear how strong the password needs to be in order to be accepted. What is "weak" in this context? Plus the actual threshold can be changed in config: user_password_strength_threshold defaults to "50" ("fair") but can be increased or decreased so that "weak", "fair", "good" or "excellent" passwords are required.

Proposed solution

Make the description dynamic so that it reflects the actual threshold set in config. For example:

Additional information

This is a follow-up to #4265

[indigoxela]

I've gotten feedback that it's not clear how strong the password needs to be in order to be accepted.

Fair enough, it's really a bit vague. Even more if the threshold has been adapted. I'm confident, that our "UX task force" will take care of the details.

Off topic: are there any plans yet for an admin interface for user_password_strength_threshold? Or is that feature hidden by concept? If we already have an issue for that - I either forgot or totally missed it.

[herbdool]

@indigoxela I do have a PR if you're interested in testing.

I didn't have plans for an admin interface for user_password_strength_threshold. It could be left to contrib. There's also a hook so that might mean contrib could make it even more dynamic: different for each role, for example.

[indigoxela]

I do have a PR if you're interested in testing.

It's not urgent, I'm short on time anyway.

There's also a hook so that might mean contrib could make it even more dynamic: different for each role, for example.

Do you mean hook_user_password_reject_weak_alter()? I think that only alters validation - after form submission. Or does that also have influence on the dynamic help text?

Update: wait, yes, it does have influence... I agree, in that case it's even more important to give a useful hint.

[klonos]

...If we already have an issue for that - I either forgot or totally missed it.

That setting could live under #3624

[klonos]

@herbdool I've left a couple of comments in the PR, but generally looks good. Thanks 👍🏼

[herbdool]

@indigoxela yes, that's the one (I was just going by memory before). I think user_password_reject_weak() and the alter hook just decide if the password should be checked or not for a particular user. And the help text only shows if it's true. So maybe that's good enough.

@klonos thanks. I kinda like that we're not exposing that setting in the UI, because most people won't need/want to change it.

[indigoxela]

I'll try to do some local testing with a custom hook implementation soon - out of curiosity and for better understanding.

I think the dynamic text is helpful for users. 👍

Re the admin interface for setting user_password_strength_threshold:
That could be part of another core issue, but I also get the point of @herbdool - "most people won't need/want to change it" - I agree, this might as well be a task for contrib.

[indigoxela]

Some quick local testing to see, how this plays with the hook and/or the threshold setting in config.

In a (minimal) module I implemented hook_user_password_reject_weak_alter() - I just copied the API example, so for members of the admin role weak passwords get rejected. Works like a charm, the help text shows up and weak password get rejected.
I also enabled user registration without email verification. No admin role, no strong password required (weakness gets indicated, but the password can still be used).

Then I (manually) switched setting user_password_strength_threshold to 70 - means: "fair" is not enough anymore, it needs to be "good". Yes, works exactly as it should - the text changes according to what value user_password_strength_threshold is set to.

So this all plays nicely even when using the hook. 👍 And, of course, it works fine without the hook.

A small recommendation, or actually a request: can we get a little more consistency in the terms?

We have:

• weak / fair / good / strong
• weak / fair / good / excellent

That's not something this PR introduces, but maybe we manage to fix it this time. 😏

Another thing regarding wording... I hope we don't end up with sentences like these:

Passwords must be strong or stronger
or...
Passwords must be excellent or stronger

Imagine you read that when registering on a site - I'm sure, you get the point. 😁

[indigoxela]

Works for me (except for some silly interface text, which isn't a blocker for me). Code review should be done by someone else. 😉

[klonos]

Code LGTM in general, but the logic could be improved. For instance, I see this bit being repeated twice:

    if ($reject_weak) {
      $description .= ' ' . t('Password must be <em>@threshold</em> or stronger.', array('@threshold' => $threshold_label));
      $description .= _user_password_policy_help();
    }

Any reason why we couldn't (conditionally) include the "Password must be..." text in _user_password_policy_help(), so that it's part of the <ul> that that function returns? If this entire conditional + also the $threshold_label logic get moved to _user_password_policy_help(), we wouldn't be calling t() twice on the same string, and that code duplication would be simplified to a single line: $description .= _user_password_policy_help();

Also, for consistency, the string should start with "The password must be...". And lastly, the threshold help text should always be shown in the "Account settings" form (for admins) - it doesn't as it is now.

[klonos]

@herbdool here's a PR against yours: herbdool/backdrop#14

It changes this:

...to this:

And also this:

...to this instead:

[herbdool]

I've incorporated @klonos suggested changes with some tweaks. I had to fix the CSS class since it was going to be a translated word which could easily break the CSS.

Also, I do not think we should confuse the indicator threshold for the criteria. The calculation takes into account: username, email, length and complexity. The output of that is a number and it must pass a threshold. That threshold cannot be the criteria itself.

[klonos]

Thanks @herbdool 🙏🏼 ...good call on the CSS class 👍🏼

Tested quickly and the only thing that I'd like us to fix is to hide the "The current password must be entered to set a new password." from the "New password" field when a user account is being edited by admins:

Since admins are allowed to update other users' passwords without entering their current password, this help text doesn't make sense in that case (there's no "Current password" field rendered in that form anyway).

Of course, that help text should remain in place when a user is editing their own password:

[herbdool]

Both @klonos and I worked on the code, so would be good to get someone else to review the code. @indigoxela are you up for it?

[klonos]

Thanks @herbdool ...I think that we should merge it into 1.19.x as well then.

[jenlampton]

Should this be pulled into 1.19.2?

Since 1.19.3 is coming out today with this fix, I'm going to mark this issue as fixed and close it.

[herbdool]

@jenlampton was this actually merged into 1.19.x as well? I only see it in 1.x.

[quicksketch]

Sorry folks. I seem to have only merged this into 1.x. I cherry-picked the commit back into 1.19.x as well today.