Problem/Motivation
For many sites, when a user changes their password, the password strength indicator can be both annoying and unnecessary. Currently it can only be disabled using a custom module and changes to the javascript function that checks the password.
It would be useful if a website administrator could disable the password strength indicator through the user interface.
Proposed resolution
Add a checkbox on the Account settings page that enables a website administrator to determine if the password strength indicator is displayed. The password strength indicator would default to 'on'.
Remaining tasks
(novice) Issue summary needs updating with issue summary template (how to: http://drupal.org/node/1427826)(novice) document steps to test/reproduce (how to: http://drupal.org/node/1468198)(novice) manual testing (how to: http://drupal.org/node/1489010)(novice) before and after screenshot of the config page(novice) before and after screenshot of the account edit page- (novice) check coding standards and documentation (how to: http://drupal.org/node/1487976)
Writing tests. (how to: http://drupal.org/node/1468170)
Steps to test
- Install Drupal 8.x using the standard profile.
- Apply patch in #72
- Login as admin
- Configuration menu > People > Account settings (
admin/config/people/accounts
) Registration and Cancellation fieldset - Confirm new checkbox "Enable password strength generator" (default off), positioned below existing checkbox "Require e-mail verification when a visitor creates an account."
- My Account > Edit tab (
user/1/edit
) - Confirm no password strength generator
- Return to Registration and Cancellation and check "Enable password strength generator"
- Click "Save configuration"
- Return to My Account > Edit tab
- Confirm password strength generator is now active
User interface changes
Addition of an 'Enable password strength indicator' on the Account settings page in the 'Registration and cancellation' section.
API changes
None
Original report by adamfeldman
There should be an option to disable the password strength check in the settings for user registration. Right now it can only be disabled by a custom module with a hack messing with the javascript function that checks the password.
Comment | File | Size | Author |
---|---|---|---|
#114 | password-strength-optional-432962-114.patch | 823 bytes | echoz |
#114 | password-strength-narrow-before.png | 59.7 KB | echoz |
#114 | password-strength-narrow-after.png | 59.71 KB | echoz |
#103 | drupal_password_strength_optional_with_test-432962-103.patch | 11.12 KB | shnark |
#103 | interdiff-98-103.txt | 905 bytes | shnark |
Comments
Comment #1
mcrittenden CreditAttribution: mcrittenden commentedI 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.
Comment #2
adamfeldman CreditAttribution: adamfeldman commentedhttp://media.fuselit.net/disablepwcheck.zip until i get cvs access
Comment #3
mcrittenden CreditAttribution: mcrittenden commentedCool, I'll work on it tomorrow.
Comment #4
mcrittenden CreditAttribution: mcrittenden commentedHere's a first try at a patch -- adds a checkbox to the User Settings page to disable/enable password strength checking:
Comment #5
dawehnerthe 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
Comment #6
mcrittenden CreditAttribution: mcrittenden commentedThanks for the default variable fix. Any suggestions on the variable name?
Comment #7
mcrittenden CreditAttribution: mcrittenden commentedComment #8
dawehnerwhat about user_password_dynamic_validation ?
like the functionname.
or user_password_js_validation
Comment #9
mcrittenden CreditAttribution: mcrittenden commentedI'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.
Comment #10
mikejoconnor CreditAttribution: mikejoconnor commentedI 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.
Comment #11
mikejoconnor CreditAttribution: mikejoconnor commentedActually, 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.
Comment #12
Dries CreditAttribution: Dries commentedI'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?
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps 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?
Comment #14
mikejoconnor CreditAttribution: mikejoconnor commentedIn 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.
Comment #15
mcrittenden CreditAttribution: mcrittenden commented+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.
Comment #16
timmillwoodI'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.
Comment #17
mcrittenden CreditAttribution: mcrittenden commentedIn 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.
Comment #19
mcrittenden CreditAttribution: mcrittenden commentedRerolled to match HEAD.
Comment #20
mcrittenden CreditAttribution: mcrittenden commentedComment #21
mcrittenden CreditAttribution: mcrittenden commentedMarking "minor" since nobody seems crazy about it. I'd still like to see it in there, though.
Comment #23
mcrittenden CreditAttribution: mcrittenden commentedProbably too late for 7.x
Comment #24
sobi3ch CreditAttribution: sobi3ch commentedsubscribing +1
Comment #25
mcrittenden CreditAttribution: mcrittenden commentedComment #26
mokko CreditAttribution: mokko commentedI 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.
Comment #27
MBroberg CreditAttribution: MBroberg commentedI 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.
Comment #28
glass.dimly CreditAttribution: glass.dimly commentedThis 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.
Comment #29
glass.dimly CreditAttribution: glass.dimly commentedOf course, you can always disable with CSS.
#user-register .password-description {
display:none !important;
}
Comment #30
mikejoconnor CreditAttribution: mikejoconnor commentedOut 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.
Comment #31
darioshanghai CreditAttribution: darioshanghai commentedPassword 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.
Comment #32
Rob C CreditAttribution: Rob C commentedComplete new patch.
This renders the check disabled by default and offers a checkbox on 'admin/config/people/accounts' to enable it.
Comment #33
Rob C CreditAttribution: Rob C commentedAdded this to #1837054: [META] Refactor account workflow (and config)
Will write a test this week if nobody beats me to it.
Comment #34
YesCT CreditAttribution: YesCT commentedtagging and updating issue summary.
Comment #35
dags CreditAttribution: dags commentedReroll
Comment #36
dags CreditAttribution: dags commentedThe 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.
Comment #37
dags CreditAttribution: dags commentedAnd a test. I tried to use XPath assertion here but couldn't get it to work - maybe because it's added by javascript?.
Comment #39
Bojhan CreditAttribution: Bojhan commented...
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedI picked this up yesterday to look at (http://core.drupalofficehours.org/task/1233) before @dags posted patches. I had a patch ready but it's changed now so I'll list what I found for the moment, including the work done by @dags:
user.admin.inc
Original code:
Suggested code:
user.settings.yml
Tests
user.js
Disabled as default
Comment #41
dags CreditAttribution: dags commentedThanks 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.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedHiya,
"Display password strength indicator during account creation and modification." sounds cool to me, although not sure how it affects referring to it as both password check/checker and indicator in text and code, should be one or the other rather than referring to both to cover all bases. I did look around for a Drupal glossary and found http://drupal.org/glossary but there doesn't seem to be different ones for different users so mostly technical, for example there's no mention of 'account' there but that seems to be the general term used, and your version explains it better.
So yes to your text, just thinking out loud ;)
Disable by default
Glad to hear you agree. May I suggest if those who disagree create a separate issue and link it here so we can discuss there and not hold this up further?
Cheers,
Steve
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedI have created a separate issue where the discussion can be continued with regards to disabling by default:
#1859298: Password strength check should be disabled by default
Comment #44
dags CreditAttribution: dags commentedRefactor 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.
Comment #45
dags CreditAttribution: dags commentedComment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedHi @dags
This still doesn't work - did you test before posting? I've attached patch with your code plus the text changes we talked about earlier (I standardised on 'indicator' instead of using both 'check' and 'indicator' in the UI) and the interface issue (fieldset/details type), but the js is still broken.
Here's what the config screen looked like previously:
Here's what it looks like now:
Here's the error on account edit screen:
Here's the errors when you start typing:
Here's the error during install if you disable:
Comment #47
dags CreditAttribution: dags commentedIt 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.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedNope, I tested in Firefox, Chrome & Safari, cleared & disabled caches. When I looked at the code a few days ago I had the feeling the code was perhaps in the wrong place so stuff was getting fired off first before your code is reached, but I don't know enough about js to know for sure.
Comment #49
dags CreditAttribution: dags commentedHmm it's working for me on Chrome 23.0.1271.95. I'll try to get in some more testing tomorrow.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedI've fixed it, but it's not great due to the fact that the strength checker and password match functionality are intertwined.
You had removed your previous if statement in the js:
So I put it back in before it adds any of the password meter html, and had to also put it in the passwordcheck function as the last thing that does is call the matching functionality, and the way they're attached to key-up and blur events I'm not really sure how to separate them out which it really needs doing, but, for the moment, it works ;) Would love a review ;)
Comment #51
YesCT CreditAttribution: YesCT commented@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.
Comment #52
YesCT CreditAttribution: YesCT commentedthere is some d.o tags bug... trying to add back tags.
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commented@YesCT thanks and yes just learning about interdiffs - have attached what we've missed out so far since I've been on this thread.
Comment #54
dags CreditAttribution: dags commented@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:
So I refactored it:
It now takes 1 parameter and is only called at the end of passwordCheck function like so:
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:
to this:
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.
Comment #55
dags CreditAttribution: dags commentedI 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.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedJust managed to grab some time to test, not looked at the code yet but the changes sound good!
First test I did was disable for install and that works fine, just the matching password check works (and better now when you change the first password as picks that up too as you said).
Second test was with it enabled and seems to work ok but the text is in the wrong position, it appears above the field. Not looked at the code to see why yet, thought I'd post a screenshot fyi while I do that.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #58
edrupal CreditAttribution: edrupal commentedTested following scenarious:
Tested on latest versions of following browsers (All on Windows 7), with Bartik theme:
All behaviour as expected.
With strength indicator enabled, indicator always showed as in attached screenshot
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedHi - this is during the install process not account editing. I just rebuilt a drupal 8 instance, applied the patch, made sure all browser caches cleared and here's the output:
Comment #59.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary with remaining tasks to help people who might be new
Comment #60
edrupal CreditAttribution: edrupal commentedAs a newbie introductory task I updated the issue summary to fit with the new template.
Merry Xmas
Comment #61
mrf CreditAttribution: mrf commentedIt 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.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commented@mrf it does - it's just we're having issues with the display on the installer and as automated tests don't work on the installer I am manually testing hence lots of emails re the installer - sorry for any confusion!
Comment #63
eddourd CreditAttribution: eddourd commentedadd this to ur template.php in theme folder
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedAfter looking at this again I see the display issue is not actually to do with @dags work on the js side but something else that has changed in D8 whilst we've been working on this issue.
I have created a new issue to deal with that problem:
#1882474: Password strength indicator display broken in install process
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedNow that #1882474: Password strength indicator display broken in install process has been fixed (yay!) I've re-tested and re-rolled this patch, all looking great now thanks for all the help especially from @dags and @YesCT!
Feel free to review patch ;)
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedSetting to 'needs review'.
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commented#65: drupal_password_strength_optional_with_test-432962-65.patch queued for re-testing.
Comment #69
YesCT CreditAttribution: YesCT commentedA 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.
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedI've re-rolled to the latest 8 as things had moved since previous patch posted.
There are tests in the patch, none for install process as not poss afaik.
Wasn't sure what you meant by 'bigger' screenshots so got the whole screen. Have sized them down though as on retina display so they come out huge! Will update summary next.
Here's the install process normally, with password strength indicator enabled:
Here's the install process with password strength indicator disabled (i.e. after patch enabled and 'password_strength' in user.settings.yml is set to '0'):
Here's the account settings config screen before the patch applied:
Here's the account settings config screen after the patch applied (see new option under registration and cancellation section):
Here's the account edit screen after the patch applied and with the indicator disabled:
Here's the account edit screen with indicator enabled:
Comment #70.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdate to issue summary to use standard template
Comment #71
Bojhan CreditAttribution: Bojhan commentedThis does not require a description.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedVersion attached without description line. Still not worked out how to easily do interdiffs yet (will do!) so just hazarding a guess that you can guess which one line I removed.
Screen shot of updated version without description:
Comment #73
YesCT CreditAttribution: YesCT commentedI 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.
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the links, will check them out. Just found out mum's in hospital after suspected stroke so not top on my mind at the moment but saw this was an easy patch to update and didn't want to hold things up. Still so much to learn!!!
Comment #75
YesCT CreditAttribution: YesCT commented@stevepurkiss no worries. no rush.
Comment #76
mgifford#72: drupal_password_strength_optional_with_test-432962-72.patch queued for re-testing.
Comment #77
chaloum CreditAttribution: chaloum commentedI'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.
Comment #78
YesCT CreditAttribution: YesCT commented@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.)
Comment #79
chaloum CreditAttribution: chaloum commented#78 YesCT I've changed my post #77. Hopefully the post is clearer.
Comment #80
simon.j.c CreditAttribution: simon.j.c commentedChecked 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.
Comment #81
YesCT CreditAttribution: YesCT commentedhmm, 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.
I might need to look deeper , but why is it showStrengthIndicator in one spot, and password_strength in another?
nit. it's is a contraction: it is. should be its markup
Comment #82
chaloum CreditAttribution: chaloum commentedRan the test locally and all use tests passed locally, 1589 passes, 0 fails, 0 exceptions, and 440 debug messages
Comment #82.0
chaloum CreditAttribution: chaloum commentedupdating status
Comment #82.1
inteja CreditAttribution: inteja commentedAdded steps to test
Comment #83
adammaloneCurrently 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.
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedWe did have an issue previously which was causing problems with the CSS but that was fixed: #1882474: Password strength indicator display broken in install process
I've just checked out the latest 8.x and it seems to work as expected for me when disabled:
I've re-rolled the patch as noticed a few lines were offset due to other changes.
Comment #85
mgiffordI 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.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedI've just checked it in a few different browsers (not that it should make any difference just being a form) on OSX and it's working fine - when you say you can't seem to enable it, could you elaborate? I haven't experienced any such issues.
It is enabled by default, although you can change that in
user.settings.yml
by changingpassword_strength: '1'
topassword_strength: '0'
We did previously have a description however that was removed after Bojhan's comment "This does not require a description." #432962-71: Add option to disable password strength checking. I do tend to agree with you though, some kind of warning should be there. Do you have any ideas on text? Are there any standard formats for warning messages such as this?
Comment #87
YesCT CreditAttribution: YesCT commentedwhen 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)
Comment #88
mgiffordOk, 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:
I was thinking of something like:
I don't think we have a security note or best practices pattern, but if we did:
Or maybe even something like:
How else does one let them know that this is any different than any of the other options?
Comment #89
mgifford@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.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedHow about this - before:
after:
So at least we have the word "Security" mentioned twice in case anyone disables it then complains, we can refer them back to that fact ;)
Updated patch & interdiff attached.
Comment #91
mgiffordWorks for me. @Bojhan?
Comment #92
mitron CreditAttribution: mitron commentedInstalled and manually tested. Works as expected.
Comment #93
chaloum CreditAttribution: chaloum commentedafter 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
Comment #94
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #95
Anonymous (not verified) CreditAttribution: Anonymous commentedYou have more faith in people than I must seem to have ;)
Personally I like it being there and don't find it offensive in any way and if it helps one person keep their site more secure then better in than out. A lot of people play with Drupal who don't know what they are doing so would prefer it there but not really bothered either way, just let me know if you need me to re-roll or do anything.
Comment #96
YesCT CreditAttribution: YesCT commentedI 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.
Comment #97
adammaloneAgree 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"
Comment #98
Anonymous (not verified) CreditAttribution: Anonymous commented@YesCT your wish is always my command ;) Here's #84 renamed and retested against latest 8.x.
@typhonius - @Bojhan said description not needed #432962-71: Add option to disable password strength checking
Comment #99
YesCT CreditAttribution: YesCT commentedwoo 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. :)
Comment #100
Anonymous (not verified) CreditAttribution: Anonymous commentedTrue, true. Interdiff attached. It'll be habit soon enough ;)
Welcome to comment #100!
Comment #101
adammaloneTests pass - manual testing works for me - RTBC!
Comment #102
mgiffordGreat. I updated the related patch here #1811240-19: Improve "password matches" and "password strength" accessibility to keep up.
Comment #103
shnark CreditAttribution: shnark commentedI did the change that YesCT recomended at the end of comment #81
Comment #104
mitron CreditAttribution: mitron commentedYes, it's great. Its pronoun is now correct. :)
Comment #105
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks @EllaTheHarpy I totally missed that!
Comment #106
catchDries 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.
Comment #107
Anonymous (not verified) CreditAttribution: Anonymous commented@catch Thanks for the update.
My 2p is the refactoring of code this patch performs decoupling the strength indicator from the password match is a Good Thing.
Comment #108
Dries CreditAttribution: Dries commentedChanged my mind about this feature. I'm no longer opposed to it. Committed this to 8.x. Thanks for the persistance everyone.
Comment #109
YesCT CreditAttribution: YesCT commentedremoving the separate task tags since this is fixed. :)
Comment #110
jibranI think we should have change notice for this. If not feel free to mark as fixed.
Comment #110.0
jibranFixed comment link
Comment #111
Gaelan CreditAttribution: Gaelan commentedAdded change notice.
Comment #112
jibranI think change notice looks good.
Comment #114
echoz CreditAttribution: echoz commentedIn #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.
Comment #115
tstoecklerI checked and the follow-up does indeed restore the (new but) previous and correct behavior. Thanks!
Comment #116
alexpottCommitted 87163be and pushed to 8.x. Thanks!
Comment #118
nod_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.
Comment #119
nod_tag
Comment #119.0
nod_correcting small spelling mistake