Add option to disable password strength checking

I agree. For many sites the password check is annoying/confusing/unnecessary.

I spoke with adamfeldman on IRC about this issue. He has the code needed to make the change and is planning to make a module out of it. When this happens, I or someone else can roll a patch out of it.

adamfeldman, please link to your module whenever you get it online.

Cool, I'll work on it tomorrow.

Here's a first try at a patch -- adds a checkbox to the User Settings page to disable/enable password strength checking:

the default values, of the variable changes.

i suggest to enable it by default so here is a new patch.

i'm not really sure, whether the variable name is the best

Thanks for the default variable fix. Any suggestions on the variable name?

what about user_password_dynamic_validation ?
like the functionname.

or user_password_js_validation

I'm not in favor of putting the word "validation" into the variable name, just because its main purpose is to check the password's strength, not to validate it.

I think this is a great option to give the administrator, however the current patch changes the layout of the password fields. We should keep the password field layout the same regardless of the strength checker.

Actually, the field layout is done by the javascript, the layout change is simply a product of not adding the password checker js. Looks good to me.

I'm not sure we need this option. In 95% of the cases, it _is_ useful to have the password strength check.

However, I'm happy to have the discussion because I could be wrong. In what cases do you want to disable the checker?

Perhaps this is motivated by the scary password checker in d6. the one in d7 is much friendlier. I think the patch is committable still, though. If Dries disagrees, maybe use a variable with no UI?

In concept I think a password strength meter is a great feature, however in use I personally question its effectiveness, especially based on your target audience. For example a forum site on the topic of web security would have little use for a password strength checker, and a sysadmin would probably recieve emails about how inaccurate the standard password security meter was.

On the other side of things, if I were to setup a site for my extend family to share news/photos, I would get a few phone calls about what the password meter was. In this case, although the password meter might be a good service for teaching my family about password security, I don't think it would be a good use of my time. Furthermore, I doubt the content of the site is vitally important.

In both of these edge cases, I think the site administrator best knows their audience, and content. Although there are many other ways to hide this feature based on your target audience, I think a check box on the user settings page is by far the most usable.

+1 for mikejoconnor's comments. Some sites don't need it because the users are too advanced to need it, and some sites don't need it because the users aren't advanced enough to understand it. Either way, the admin should have the power to decide.

I'm personally don't see the need to have an option to disable the password strength checker in core and think it is placed in contrib module, but really can't think of a good argument why not to put it in core.

In response to timmillwood, I think it's confusing for it NOT to be in core. Password strength checking, while cool and sometimes useful, is such a superfluous thing that it doesn't make sense for it to be there without my permission, IMHO. I'm sort of surprised that it made it to core in the first place.

Also, D6 module is now at http://drupal.org/project/disablepwstrength.

Rerolled to match HEAD.

Marking "minor" since nobody seems crazy about it. I'd still like to see it in there, though.

Probably too late for 7.x

subscribing +1

I find it a pretty tough requirement to ask for a hard password which requires a combination of punctuation, capital, small letters and numbers! A user just made me aware that neither his bank nor his emails ask this of him. This has to be configurable if Drupal wants to continue to appeal to wide variety of different users.

I have users that like using comfortable, familiar, easy passwords. They feel that the warnings force them to change to a difficult password, then they forget what it was, then they get upset that this is "required". A warning might be nice, but gentler, so that they don't feel like it is mandatory (they usually don't read fine print) .
So yes it is needed and should be up to the site designer.

This should be an option to disable. Some users find this confusing, I've watched them be confused by it, and it should definitely be an option to disable.

Sad this didn't get rolled into 6x, let along 7x.

Why do we feel like we need to be making choices for people? If they want to have insecure or enable insecure passwords on a site, let them.

Of course, you can always disable with CSS.

#user-register .password-description {
display:none !important;
}

Out of curiosity, why isn't this entire feature a contrib module? It seems like something that could be easily handled in Contrib. The only reasonable argument I could think of is to ensure that the Admin user has a good password. I personally think that if someone can get a hosting account, and install Drupal, they know what a good password should be.

With that in mind, it seems like this feature is best kept in contrib.

Password checker should be optional and disabled.

- it's confusing to use
- it's not necessary. Even big websites don't have one (google? facebook? twitter?)
- it takes a split second to tick the appropriate tickbox to enable, while it can take an hour or two to figure out how to disable it

Speaking as unexperienced developer, it's very frustrating and incredibly time consuming to have to figure stuff out to REMOVE features. In my particular case I have tried for over one hour editing CSS and the damn thing, although invisible, is causing a blank space to appear all of a sudden under the password box, on drupal 7 with the included theme. (See image).

What's gonna happen is that I'll have to just live with it or accept a crappy theme until I learn how to hack the core. This is wrong.

Please consider simplicity a priority. Thank you.

Complete new patch.

This renders the check disabled by default and offers a checkbox on 'admin/config/people/accounts' to enable it.

Added this to #1837054: [META] Refactor account workflow (and config)
Will write a test this week if nobody beats me to it.

tagging and updating issue summary.

Reroll

The previous patch was also disabling the password match check, which we probably don't want to do since the user can't go anywhere without confirming their password. Attached is a patch to address that problem.

And a test. I tried to use XPath assertion here but couldn't get it to work - maybe because it's added by javascript?.

...

Thanks for the excellent review stevepurkiss.

user.admin.js
Agreed on all points but how about something like this for the description text: "Display password strength indicator during account creation and modification."

user.settings.yml
I'm investigating this issue.

Tests
Test can be added for the user edit page, but I don't believe there is any way to test the configuration form during installation.

user.js
Working on this as well.

Disabled as default
Totally agree with you. Enabled by default would also encourage the admin user to create a strong password during installation, which should be desired in most cases.

Refactor user.js to address issue from #40 and adds another test for the user edit page. Also adding the 'refactor account workflow' tag at YesCT's request.

It might be that your browser is caching an older version of the user.js file. If you're using Chrome, open up the Developer Tools and click the gear box in the bottom right. Check the 'Disable caching' box and retry the installation. I'm attaching another patch that also makes the text in the tests consistent.

Hmm it's working for me on Chrome 23.0.1271.95. I'll try to get in some more testing tomorrow.

@stevepurkiss good description of what you found and the change you made. please post an interdiff between 50 and 47 (interdiff-47-50.txt) that will help get review. :) And interdiffs are just a good habit.

there is some d.o tags bug... trying to add back tags.

@stevepurkiss Your patch looks good. I don't know how that if statement slipped through but good catch. I'm posting another patch that addresses some other issues I found while testing. Some of this stuff might be better suited for separate issues and/or separate patches... but I'm going to post it here until someone yells at me.

First thing I did was move the 'Enable password strength indicator' checkbox inside the 'Registration and Cancellation' fieldgroup because I felt like it was undeserving of it's own.

Then I noticed a Javascript exception popping up while switching between the password and confirm inputs:
Uncaught TypeError: Cannot read property 'confirmClass' of undefined user.js:72

This is an existing issue not caused by our patch. It happens when there is a value in the confirmInput and focus returns to the passwordInput. I tracked the problem to an improper use of "this" in the passwordCheckMatch function in user.js:

var passwordCheckMatch = function () {

        if (confirmInput.val()) {
          var success = passwordInput.val() === confirmInput.val();

          // Show the confirm result.
          confirmResult.css({ visibility: 'visible' });

          // Remove the previous styling if any exists.
          if (this.confirmClass) {
            confirmChild.removeClass(this.confirmClass);
          }     

          // Fill in the success message and set the class accordingly.
          var confirmClass = success ? 'ok' : 'error';
          confirmChild.html(translate['confirm' + (success ? 'Success' : 'Failure')]).addClass(confirmClass);
          this.confirmClass = confirmClass;
        }     
        else {
          confirmResult.css({ visibility: 'hidden' });
        }     
      };    

So I refactored it:

var passwordCheckMatch = function (confirmInputVal) {
        var success = passwordInput.val() === confirmInputVal;
        var confirmClass = success ? 'ok' : 'error';

        // Fill in the success message and set the class accordingly.
        confirmChild.html(translate['confirm' + (success ? 'Success' : 'Failure')])
          .removeClass('ok error').addClass(confirmClass);
      }; 

It now takes 1 parameter and is only called at the end of passwordCheck function like so:

if (confirmInput.val()) {
          passwordCheckMatch(confirmInput.val());
          confirmResult.css({ visibility: 'visible' }); 
        }   
        else {
          confirmResult.css({ visibility: 'hidden' }); 
        }   

Now passwordCheckMatch only handles checking if the passwords match and passwordCheck determines if it should show the confirmResult div.

Finally I changed these two lines:

// Monitor keyup and blur events.
      // Blur must be used because a mouse paste does not trigger keyup.
      passwordInput.keyup(passwordCheck).focus(passwordCheck).blur(passwordCheck);
      confirmInput.keyup(passwordCheckMatch).blur(passwordCheckMatch);

to this:

// Monitor input events.
      $.each([passwordInput, confirmInput], function () {
        this.bind('input', passwordCheck);
      }); 

Blur doesn't actually catch paste events (at least on Chrome) as it was intended. A better option is to listen on 'input' and that will catch pastes and keyup - so no need to listen on blur or focus either. Most browsers should support the 'input' event but, I'm not sure if all the browsers that Drupal supports, support it.

I also made some minor text changes which you can see in the interdiff. This patch should fix a couple bugs and hopefully make the code more (and not less) readable.

I forgot to mention that I added a new key to the $password_settings array called 'showStrengthIndicator' which gets converted to a true boolean value in Javascript so we don't have to check "typeof someSetting != 'undefined'". I thought this was cleaner and made the code more readable. There was a bug related to this in my last patch so here's a new one.

Tested following scenarious:

1. Strength indicator enabled
2. Strength indicator disabled

Tested on latest versions of following browsers (All on Windows 7), with Bartik theme:

• Firefox
• Internet Explorer
• Safari
• Chrome

All behaviour as expected.

With strength indicator enabled, indicator always showed as in attached screenshot

As a newbie introductory task I updated the issue summary to fit with the new template.

Merry Xmas

It seems a bit strange to not have this apply to the user edit screen as well.

If this is intentional and not an oversight, I would suggest re-naming the config to something more like 'registration_password_strength' to make that clear.

add this to ur template.php in theme folder

function themename_element_info_alter(&$types) {
  if (isset($types['password_confirm']['#process']) && (($position = array_search('user_form_process_password_confirm', $types['password_confirm']['#process'])) !== FALSE)) {
    unset($types['password_confirm']['#process'][$position]);
  }
}

A new (and maybe bigger) before and after screenshot might help since the password strength checker is fixed.
Also, feel free to update the issue summary. :)

For example: Does this still need tests? I know it was mentioned earlier tests do not work for during the installer.

This does not require a description.

I do interdiffs by making a branch. then applying previous patch. then make a new branch, then make my new changes. then I can diff against the previous branch for an interdiff, and diff against 8.x for the patch.

like... http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

I think for bigger screenshots, I meant to make the window smaller, so that when the screenshot is posted the text is more readable. Shoot for 600px wide or so on the window.

@stevepurkiss no worries. no rush.

#72: drupal_password_strength_optional_with_test-432962-72.patch queued for re-testing.

I've checked the patch #72 and it installs but does not work as expected
The images are as follows:

This is the before image of the config for the password strength settings

And now the password strength is displaying as per normal and in accordance with the settings

Now the password strength setting is set for disable the password strength visual

But after setting the passwords strength to disabled the password strength visual still displays

I'm expecting the visual password strength not to display after it was disabled in the the settings.

@chaloum, Thanks for doing those screenshots. Having before and after is helpful for comparison.

In what way did it not work as expected?

(also, it's totally ok, and helpful to inline the images. a img tag can be added by hand, or dreditor has a an "embed" link that makes it easy.)

#78 YesCT I've changed my post #77. Hopefully the post is clearer.

Checked patch at #72

Working as expected:
After applying the patch the Config at People's Accounts adds a check box "Enable password strength indicator".

Not working as expected:
In the User accounts area, the indicator still exists.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateTest.phpundefined
@@ -36,6 +36,17 @@ protected function testUserAdd() {
+    // Test that the password strength indicator displays.
+    $config = config('user.settings');
+
+    $config->set('password_strength', TRUE)->save();
+    $this->drupalGet('admin/people/create');
+    $this->assertRaw(t('Password strength:'), 'The password strength indicator is displayed.');
+
+    $config->set('password_strength', FALSE)->save();
+    $this->drupalGet('admin/people/create');
+    $this->assertNoRaw(t('Password strength:'), 'The password strength indicator is not displayed.');
+

hmm, the test passed last time it went to the testbot, and it's suppose to make sure the indicator is not shown. I wonder what is going on.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserEditTest.phpundefined
@@ -76,6 +76,18 @@ function testUserEdit() {
+    $config->set('password_strength', FALSE)->save();

+++ b/core/modules/user/user.moduleundefined
@@ -2433,10 +2433,18 @@ function _user_mail_notify($op, $account, $langcode = NULL) {
+    'showStrengthIndicator' => FALSE,
...
+    $password_settings['showStrengthIndicator'] = TRUE;

I might need to look deeper , but why is it showStrengthIndicator in one spot, and password_strength in another?

+++ b/core/modules/user/user.jsundefined
@@ -23,70 +23,61 @@ Drupal.behaviors.password = {
+      // If the password strength indicator is enabled, add it's markup.

nit. it's is a contraction: it is. should be its markup

Ran the test locally and all use tests passed locally, 1589 passes, 0 fails, 0 exceptions, and 440 debug messages

updating status

Added steps to test

Currently the patch above causes the following to happen:

The reason the tests pass is because they look for the "Password Strength" text - however since it is 'undefined', tests think it's ok.

Perhaps it would be a lot easier to put the password strength checked into a form item and simply not output it if the box isn't ticked.

I can't seem to enable the "Enable password strength indicator" checkbox here admin/config/people/accounts

That should be enabled by default. I'd also like to see some security warning on this so that people are reminded that they should only disable this security reminder if they know what they are doing. For most sites this is a bad idea.

when I run into problems not being able to reproduce, I make sure I'm all clean, maybe by doing:

git stash
git checkout 8.x
sudo rm -r sites
git checkout sites
git pull --rebase

if things are really strange:
sudo rm -r drupal
git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git
(copy and pasted from http://drupal.org/project/drupal/git-instructions)

and then if I'm lucky and these work, then something like:
drush am 432962
(and if I'm not testing an install screen)
drush -y si --account-pass=admin --db-url="mysql://root:root@localhost/drupal" --site-name="disable checker 432962"
[edit added following 2 lines]
drush si drops the db tables, so if testing install, or just want to install by hand this is helpful to drop the tables before doing the install
drush -y sql-drop --db-url="mysql://root:root@localhost/drupal"

then, if things are still strange, I clear my browser cache (or do a shift refresh)

Ok, I tried again and applied the patch in http://simplytest.me/project/drupal/8.x

So neat that you can do that. Anyways, it must have been because I was re-using an old DB with different variables.

Ok, so the code works!

The description that Bojhan objected to was:

Encourage users to create more secure passwords by displaying a password strength indicator during account creation and modification.

I was thinking of something like:

The security of your site and users could be put at risk if you disable this. Please ensure you know what you are doing before disabling this.

I don't think we have a security note or best practices pattern, but if we did:

Security Note: This feature gives some basic protection to see that your users have reasonable passwords. Only disable it if you are confident you've addressed these.

Or maybe even something like:

Advanced Options: You can disable this but doing so could make your site less secure.

How else does one let them know that this is any different than any of the other options?

@YesCT - I like your list. That should go in a handbook note somewhere as I know I'd like to refer to it again and expect others could make use of that list too.

Works for me. @Bojhan?

Installed and manually tested. Works as expected.

after following YesCT's comments #87 I can confirm the patch works as expected. When the "Enable password strength indicator" is checked the the visual indicator no longer displays when changing a users password

I really dont see a warning like this as being necessary. It tends to be really annoying when Drupal is points a warning finger at you, when you want to do something like this. I am sure most people who disable this will consciously decide to do this, its not like this is a super easy to find setting.

I think we should do what Bojhan says.
displaying the strength indicator or not goes not actually make people pick more secure passwords. So I understand the intent, but I think we should reserve skip the description here.

Agree with #94 & #96. A security warning like that makes it seem dangerous to disable. Similar messages are found on some of the Drupal core permissions which could actually present security risks to the site if applied incorrectly.

Perhaps just a descriptive message:
"When registering or changing password, users will see a guide indicating the password strength"

woo hoo!

now, this is the perfect situation for an interdiff, since the most recent change was a small one, and people have previously reviewed the patch. with an interdiff they can do a quick review and mark it rtbc. without the interdiff, they have to read through the whole patch looking for anything that might have accidentally changed. :)

Tests pass - manual testing works for me - RTBC!

Great. I updated the related patch here #1811240-19: Improve "password matches" and "password strength" accessibility to keep up.

I did the change that YesCT recomended at the end of comment #81

Yes, it's great. Its pronoun is now correct. :)

Dries didn't like this first time around and I'm not completely sure myself. So giving him another chance to look at this before it goes in.

Changed my mind about this feature. I'm no longer opposed to it. Committed this to 8.x. Thanks for the persistance everyone.

removing the separate task tags since this is fixed. :)

I think we should have change notice for this. If not feel free to mark as fixed.

Fixed comment link

Added change notice.

I think change notice looks good.

In #1864466: Password strength checker hidden on small screens in user.js we changed prepend to append to have the password strength indicator move below the password field on narrow viewports. The patch from this issue changed this instance of append back to prepend, thereby placing the password strength indicator above the password field (on narrow screens). This patch only changes it back, and in my testing, I can't see that it changes anything in this issue's feature, and it returns the other as it was intended.

I checked and the follow-up does indeed restore the (new but) previous and correct behavior. Thanks!

Committed 87163be and pushed to 8.x. Thanks!

Please tag your issues with "JavaScript" if a patch touches a JS file in any way. This patch reintroduced a bad use of $.each() that was fixed a year ago #2057371: Re-Replace all $.each() with filtered for loop.

Thank you.

tag

correcting small spelling mistake