backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[SR] Enforce basic password constraints: no username, no email #4265

Closed herbdool closed 4 years ago

herbdool commented 4 years ago

Description of the need Currently Backdrop (and D7) allow anything to be entered in the password. I think that's okay for the most part (contrib modules like Password Policy (d7-only?) can provide more constraints) but I do feel that there are a couple obvious constraints that should be enforced: username and email address.

Proposed solution Don't allow users to enter username or email in their password.


~PR by @indigoxela: https://github.com/backdrop/backdrop/pull/3053~ ~PR by @jenlamptonhttps://github.com/backdrop/backdrop/pull/3281~ (separated UI for pw length) ~PR by @herbdool https://github.com/backdrop/backdrop/pull/3215~ PR by @jenlampton https://github.com/backdrop/backdrop/pull/3282 (removed UI for pw length)

indigoxela commented 4 years ago

There's a Backdrop module for that: https://github.com/backdrop-contrib/passphrase_policy (didn't use it yet)

But maybe it's really smarter to prevent users from setting their username or email as password (which is a horrible idea anyway).

What do people think?

herbdool commented 4 years ago

I agree that the passphrase_policy module is useful. Though I think that we can still provide some minimum safeguards in core: no usernames nor email address as password. (Or perhaps also a third: a minimum entropy).

I found this article quite helpful in what are some basics that we should enforce for all users: https://blog.codinghorror.com/password-rules-are-bullshit/

herbdool commented 4 years ago

Hm, just found out that Backdrop.evaluatePasswordStrength actually includes some of this checking: entropy, username as password, etc, based on https://github.com/dropbox/zxcvbn. It will just mark the password as poor but still allow people to save it.

indigoxela commented 4 years ago

The question is: why don't we already enforce at least a minimum length?

Currently even "0" is considered a password. Found that in the tests... , but I don't understand the reason for this.

The next question is: how much could get enforced without frustrating users?

Or is this already too much? From a security perspective, this isn't enough, though.

I usually don't use modules like passphrase_policy/password_policy because the risk of frustrating users is big. On the other hand zero validation doesn't seem right to me either.

alanmels commented 4 years ago

I thought to advocate the Passphrase Policy module, because it works great on couple of our test setups. However, after reading the last argument of @indigoxela that some users could get frustrated about the strictness of password policies, I also agree that some minimum measures must be enforced in core, leaving stricter policies to contrib.

stpaultim commented 4 years ago

I have very mixed feelings about this. I think it's critical that users have the ability to add a passphrase policy and the contrib modules seems to exist that does this.

During development, it's nice to just set username and password to the same thing. I don't do it myself, but I see tools like Lando do this and it's convenient during local development.

I could see the value of a config option in core that forces some minimal password requirements. I'd prefer the ability to turn that off for development, but as long as the requirements are not too complicated I guess I don't feel strongly about it.

As long as the module is available, I don't see this is a big problem either.

Graham-72 commented 4 years ago

I agree that we need some minimum measures in core like

Anything more should have an admin checkbox to apply it.

indigoxela commented 4 years ago

As @herbdool already mentioned in a previous comment, we already have smart password strength evaluation (in user.js)

We probably only need to add the same logic to a php validate function.

A quick idea is to add a section to /admin/config/people/settings like "Password policy". Probably some form fields:

The latter would enforce a calculated value above - let's say - 80. (But... some of these fields/settings seem conflicting to me. And maybe that's too technical for admins)

What do you think?

Graham-72 commented 4 years ago

:+1: I like this proposal.

indigoxela commented 4 years ago

I see consensus, that core should have a minimal password constraint feature (configurable).

I've created a PR for further discussion. This needs improvement. Some help text for the admin form, for instance.

A screenshot of the new section on /admin/config/people/settings:

screenshot-pass-constraints

Are there any opinions yet?

stpaultim commented 4 years ago

Good work.

olafgrabienski commented 4 years ago

Are there any opinions yet?

I like the option to have these basic password constraints in core.

During development, it's nice to just set username and password to the same thing. I don't do it myself, but I see tools like Lando do this and it's convenient during local development.

If Lando or other environments do this, they'd work only if the default for pass_not_username is FALSE. We could do so and put a message on the status report page with a link to change the setting.

indigoxela commented 4 years ago

If Lando or other environments do this, they'd work only if the default for pass_not_username is FALSE

This is one of the things to evaluate. To my knowledge the install script uses different form items (not user_account_form), so the constraints don't apply there. And I think (to be evaluated!), drush uses user_save directly, which also means that the constraints don't apply. Of course, we don't want to break scripted installing.

indigoxela commented 4 years ago

Tests are passing now.

To move on, I'd like to get some help regarding text/wording of help texts. I already learned that many people here create much better help texts than I do :wink:

What information is helpful for admins when setting constraints? Should we explain, what these settings mean?

Another thought was: users nowadays almost expect that they have to input something cryptic with numbers/lowercase/uppercase/symbols...

But actually a long string (sentence) is better than a short complex password. What if we put something like "Long and simple is better than short and hard" at the bottom of the profile/reset form help text? (Just an idea...)

Another task: check that automated installs and user creation isn't blocked by this new feature. Should we create a todo-list for that?

Already checked:

klonos commented 4 years ago

Not sure about this (although I do not necessarily object). ...I mean, we do have contrib that allows enforcing password rules/policy, and I believe that communicating the password strength is enough for core.

What we need to fix is this:

Screen Shot 2020-01-30 at 6 11 13 am

^^ I have copy-pasted the email in the password field. I think that we should change this so that in that case the strength indicator is "poor".

If we decide that this is a core-worthy feature after all, then I would like to propose that we make it opt-in, instead of enforcing things. So let's keep whatever defaults we decide are best, but let's do something like this:

Screen Shot 2020-01-30 at 6 25 49 am

klonos commented 4 years ago

... @indigoxela while at it, I would be really grateful if you also removed the periods from the checkboxes and radio options in that form ...would be a shame to raise a separate issue and file a PR only for that 😉

klonos commented 4 years ago

...and for reference, here's how password setting works in WordPress:

  1. Password is automatically generated, and hidden from the admin (I absolutely love this feature btw):

    Screen Shot 2020-01-30 at 6 37 34 am

  2. Password can be shown, by clicking the respective button:

    Screen Shot 2020-01-30 at 6 38 09 am

  3. If you manually type a weak password, there is a confirmation checkbox:

    Screen Shot 2020-01-30 at 6 38 20 am

docwilmot commented 4 years ago

TBH I lean towards "there's a contrib solution" for this myself.

Graham-72 commented 4 years ago

I rather like this one (copied from above) and in core please. image

stpaultim commented 4 years ago

I was going to suggest making it possible to opt out and then I realized that if set the minimum length to "1" and unselect both boxes, you have essentially opted out. After all, it's impossible to have a password with less than one character (or am I missing something).

Having said that, I'm not against adding the "enforce constraints" radio button. I'm just noting that it MIGHT not be necessary (just convenient). Also, it does change the default, which I like.

IF we go with @klonos suggestion. I would recommend:

TITLE: Password Enforcement

O - No constraints - include password strength indicator O - Enforce minimum constraints on user passwords

(If second option is checked, then show the "constraint" options.)

NOTE: Thanks for work on this. I don't feel strongly about these changes, just offering them as ideas/suggestions.

klonos commented 4 years ago

(If box two is checked, then show the "constraint" options.)

These options should always be shown. They'd affect the "poor" password strength indicator - even if no enforcement.

indigoxela commented 4 years ago

we do have contrib that allows enforcing password rules/policy

Yes, and what we already discussed here in this issue is, that many of us find it wrong to have zero validation in core. Based on this discussion and consensus, I've created a PR.

Currently there's only a single Backdrop contrib module for password constraints, and that one is way too strict and has a weird UI. So Backdrop users only have the choice between no validation at all and frustratingly strict validation without ability to configure anything.

I would be really grateful if you also removed the periods from the checkboxes and radio options in that form

Sure... you mean the help text list, right?

I already asked for help re help text and admin form description. :wink:

herbdool commented 4 years ago

Yeah, I think it's wrong to leave all password validation to contrib. Only a few users will take advantage of it. The 80% will assume that Backdrop is going to keep their site secure. Having some obvious password validation (that match the JS suggestions) is an easy win and will help everyone.

indigoxela commented 4 years ago

I also like @klonos' suggestion to make it fully opt-in (with radios). Then there's no change in core behavior until an admin decides to use this setting.

I find @stpaultim's text suggestion for the second radio slightly better.

And I think it's better to toggle the constraint options visibility state based on these radios. Then it's clearer, that these settings only apply, if constraints are enabled.

herbdool commented 4 years ago

@indigoxela perhaps existing sites don't have it enabled by default but new sites have it enabled.

klonos commented 4 years ago

Sure... you mean the help text list, right?

Nope, I mean these:

Screen Shot 2020-01-31 at 2 18 35 am

Screen Shot 2020-01-31 at 2 18 51 am

AFAIK, radio and checkbox options do not have periods (even when they are phrases).

klonos commented 4 years ago

And I think it's better to toggle the constraint options visibility state based on these radios. Then it's clearer, that these settings only apply, if constraints are enabled.

The way I see it, even when the admin selects to only indicate poor password strength, they still need to control which aspects would affect this. So the various options should be always visible to allow affecting the "threshold" of poor. In other words:

indigoxela commented 4 years ago

Nope, I mean these:

@klonos I see... I'm afraid, that's out of the scope of this issue. At least, I'd hesitate (and wouldn't recommend) to mix it with the password policy feature.

klonos commented 4 years ago

I'm afraid, that's out of the scope ...

I know 😞 ...it'd just seem pointless to raise a separate issue and file a PR only for those. When I work on UX/UI issues, I alway fix small things like these if they are in the same form. ...it's like removing extraneous spaces/tabs in code, and fixing comments that go beyond the 80 character limit.

indigoxela commented 4 years ago

even when the admin selects to only indicate poor password strength, they still need to control which aspects would affect this

Sorry, I don't get it yet...

The strength indicator is pure javascript and (at least currently) not in any way connected to these settings (which are for server side validation). The logic behind that indicator is slightly different and more complex.

The three options that are currently used for server side validation are the ones that we were able to agree on as being "basic constraints". (Note the title of this issue) :wink:

klonos commented 4 years ago

Yeah, this is how I thought this feature would work:

indigoxela commented 4 years ago

@klonos I think you're over-complicating things a bit. I always saw the indicator and server side validation as completely independent from each other.

Besides the fact that Backdrop.evaluatePasswordStrength (user.js) also needs to take email address fields into account when calculating strength, I wouldn't change anything there.

klonos commented 4 years ago

Yeah, but see the disconnect between visual indication and validation error in this for example:

Screen Shot 2020-01-30 at 6 11 13 am

^^ this tells the user that the password they entered is "green", but if they try to save the form, they'll get a validation error (because the password same as email constraint is set). I would expect the indicator to reflect the fact that entering the email as password is considered "poor".

indigoxela commented 4 years ago

@klonos ...please... read my previous comment. I already explained that.

indigoxela commented 4 years ago

As some of you were really baffled by the mismatch of js indicator and server side validation, I've fixed that now. Email is also considered now for indication.

I also added the radios to opt-in and opt-out.

According to @herbdool's idea to turn it on by default for fresh installs, but keep it off for existing ones, I tried that now. This might need further discussion, though. I don't know if Backdrop does things like that anyway (different settings for existing and new).

docwilmot commented 4 years ago

@indigoxela good stuff. Would it be possible to have a different password strength method for passwords that would fail validation? Instead of just saying "poor", say "not allowed" or such? I do agree that it seems contradictory to say the password is just poor when we know well that it wont even be allowed. Not absolutely necessary, I suppose, but seems would be more complete, and the code doesnt look like it would be much.

indigoxela commented 4 years ago

Would it be possible to have a different password strength method for passwords that would fail validation?

That's an interesting idea... Do I get this right, you mean to enhance the indicator to show "not allowed" if:

This might be easy, or maybe it's not - I'll have a look ASAP.

docwilmot commented 4 years ago

I actually meant to type "message", not "method" but yes you got the exact idea.

klonos commented 4 years ago

Brilliant!! Thanks @indigoxela and @docwilmot 👍

klonos commented 4 years ago

I've done a quick test in the PR sandbox, and everything seems to be working as expected 👍 ...I have made some UI text suggestions in the PR (mainly avoiding using the word "please").

indigoxela commented 4 years ago

@klonos many thanks for your help regarding wording.

The js strength indicator now considers constraint settings as suggested.

Hm... to be honest, I'm not convinced yet. This is somewhat strange (for users), especially regarding minlength. But try it yourself.

indigoxela commented 4 years ago

The PR is currently conflicting - that was expected, I'll fix that later.

To describe why I think that we shouldn't show "not allowed" when the password is too short:

When not copy/pasting the password, but actually typing, the "not allowed" appears when starting to type.

That's a WTF moment for users, they might think "Hey, what did I do wrong? I only started typing!".

Instead of "not allowed" maybe we should show a very short red bar. Or are there better suggestions?

indigoxela commented 4 years ago

Based on my findings (see my previous comment), I've updated the PR.

Please test and review, feedback is welcome.

stpaultim commented 4 years ago

I think it's pretty good. I wasn't convinced it was necessary, but I understand the reason. My personal preference would be to have the enforcement off by default - but that is probably the developer speaking, not the site admin.

One last idea (although this could be a follow-up issue or it could be rubbish). Would it be worth having something on the site status page for password enforcement. Just a reminder to a site admin as their checking their site status to let them know if password enforcement is on or not?

I recognize that real estate is precious on that page, but given the security implications would this be worth it? (I do not feel strongly about this, just throwing it out there)

indigoxela commented 4 years ago

My personal preference would be to have the enforcement off by default

That's one of the things that are pending a final decision. Currently in my PR it's off for existing sites (when updating), but on for new sites. That's a core team decision, though.

Would it be worth having something on the site status page for password enforcement...

That's a good point. What should we describe there? Would that be a hint or a warning? (Rather not a warning, as probably the password constraints should be fully optional.)

klonos commented 4 years ago

My personal preference would be to have the enforcement off by default

I'm neither hot nor cold about this ATM.

Would it be worth having something on the site status page for password enforcement...

I think that this is a better candidate for the dedicated "Security" section (#3624). I would then expect the status page to only hold a "Security summary" indication, so things do not become cluttered. What do you think @stpaultim ?

@indigoxela I would expect the "not allowed" indicator text color to be red (same as poor), but here's what it currently looks like:

Screen Shot 2020-02-25 at 5 26 01 am

...here's an idea:

Screen Shot 2020-02-25 at 6 05 08 am

Denoting the "offending" restriction in the password field description with a red color + all "passing" restrictions with green (and ✓/✗ instead of bullets), the user will have a better idea of what they may be doing wrong.

stpaultim commented 4 years ago

I think that this is a better candidate for the dedicated "Security" section (#3624). I would then expect the status page to only hold a "Security summary" indication, so things do not become cluttered. What do you think @stpaultim ?

I would agree. But, I could see adding it to the status page until there is a security page to put it on. Unless, we're confident that the security page is coming soon. ;-)

That's a good point. What should we describe there? Would that be a hint or a warning? (Rather not a warning, as probably the password constraints should be fully optional.)

Yeah, I don't think it should be a warning, because it's optional. I think it's just an easy status reminder to draw attention to what choice is active at any moment.

klonos commented 4 years ago

@indigoxela one more thing: I think that Password strength: not allowed does not make sense; how about something like Password strength: weak (not allowed) ...so the (not allowed) thing gets appended only if the restrictions are enforced.

indigoxela commented 4 years ago

I think that this is a better candidate for the dedicated "Security" section (#3624)

I agree, a security section seems more suitable for information like that. Is there any time schedule yet for that new feature?

Regarding the colors: currently there are no tags/wrappers for styling these text parts yet.

Regarding replacing the list bullets with utf-8 chars in the description text (item list, translatable): is there any example for an existing help text, that does it that way?

Password strength: weak (not allowed) ehm... @klonos are you sure? I'm not a native speaker, but hat seems a little weird to me. Especially the combination of "weak" and "not allowed". I'd suppose, something is either weak or not allowed (not usable at all), but to my understanding it can't be both.

indigoxela commented 4 years ago

Regarding the wording we currently struggle with, I had an idea: What about "too weak" instead of "not allowed"?

That fits the label "Password strength" and still denotes the password as being not strong enough yet. Or are there better ideas?