The minimum password length feature in Logintoboggan allows admins to set any minimum length, and adds a message advising the user of what the minimum password length is to the set/change password fieldset in the registration or profile edit form.

However, core (in D7 at least) enforces a minimum 6 character length, and displays messages to this effect. The display of these messages is handled in /modules/user/user.js where the minimum length it tests for is hardcoded at line 110.

This results in conflicting messages being displayed to the user (see screenshot).

CommentFileSizeAuthor
#11 user-password_length-setting.patch1.7 KBscottalan
#11 logintoboggan-pass_length_override-1142808-11.patch3.74 KBscottalan
#10 logintoboggan-pass_length_override-1142808-10.patch1.63 KBscottalan
#9 logintoboggan-minpasswordlength-1142808-9.patch963 bytespuddyglum
#8 minpasswordlength-1142808-3.patch3.86 KBstevecowie
#4 minpasslength-1142808-4.patch12.81 KBsteveparks
logintoboggan_minpwdlength.jpg101.1 KBsteveparks

Comments

hunmonk’s picture

hunmonk commented

not really sure what to do about this one -- we can hook_js_alter i suppose, but that seems like overkill. suggestions?

steveparks’s picture

steveparks commented

I looked at whether it would be possible to override Drupal.behaviors.password in logintoboggan.js, and then call a different version of Drupal.evaluatePasswordStrength -- but then that would have overridden the majority of user.js anyway!

So yes, I think hook_js_alter is the only way to go on this, but I agree it seems like overkill - and may well create a maintenance problem going forward as we have to track the original.

The bigger question is, if core is now enforcing a minimum password length (even if that's not user customisable), then is there a need for logintoboggan to provide this functionality?

So the options as I seem them:
1. use hook_js_alter to substitute a custom logintoboggan_user.js
2. move the minimum password length feature from the main LT module and into a contrib module, and do (1)
3. remove this functionality from the d7 branch of LT

In the case of 3, we could offer a patch in the core issue queue to provide for user-customisable password lengths. Also, there is already a contrib module that offers this functionality: http://drupal.org/project/password_policy and they have been working on their D7 release: #985374: Drupal 7 Port

hunmonk’s picture

hunmonk commented

removing this feature now that we have a stable release is not a good idea in my opinion. however, since core will not allow anything less than a 6 character password, we should adjust the feature to remove options less than 6. this would involve:

  1. changing the select element for 'logintoboggan_minimum_password_length'
  2. adjusting the default value of all variable_get()'s for the variable to 6, and any checks against it
  3. providing an update function which sets that value to 6 if it's less than six currently.

all that's left after this is a slight UI weirdness, which is livable -- we can even adjust our description if the value is higher than 6 to say something like "further password hardening has been enabled, your password must be at least X characters long."

we can deal with smoothing the rest of this out in 8.x where things are more flexible.

steveparks’s picture

steveparks commented
Assigned: Unassigned » steveparks
Status: Active » Needs review
StatusFileSize
new minpasslength-1142808-4.patch12.81 KB

Here's a first attempt at a patch for this.

Background:
It turns out that although core _recommends_ a minimum password length of 6, it doesn't enforce this. Logintoboggan's role is now to offer to enforce the default level of 6, or a higher number set by the admin - and to communicate this clearly to the user.

This patch:
1. Allows the admin to either stick with the default core policy of recommending a password length of 6, but not enforcing it; or adding enforcement to that minimum; or setting a higher minimum and enforcing that.
2. Communicates this to the user by overriding the default javascript password security helper on registration and change password screens
3. Communicates any _required_ length of password to the user in the instructions below the password fields on registration or password change

To achieve this the patch:
1. Leaves the default on the variable_get()'s at 0 - so that the core default behaviour can remain if so selected by the admin
2. An upgrade hook checks for any historic value of 1-5 and changes it to 6
3. Overrides modules/user/user.js to provide a customised password security helper
4. Adds help messages to the password fields on the relevant forms
5. changes the select element on the logintoboggan settings screen to remove lengths 1-5.
6. abstracts the functionality to communicate any password length requirements into a separate helper function

I feel the end result for the admin and user is the best possible - but maybe the code isn't the cleanest approach just yet. Guidance on refactoring welcome.

hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work

while i appreciate the effort to create the best admin/user experience possible, this approach does so at too high of a code maintenance cost. look at all that code in user.js that we would then have to track to keep it functioning the same as core's user.js in the case of changes/bugfixes. we're already suffering this kind of issue with the form submit handler in user registration, and the js code that does the auto checkbox'ing on the permissions page. with those our hand is forced, as major pieces of LT's functionality would be compromised otherwise.

however, i'm not willing to make this situation even worse just because of a potentially confusing user message -- i think it's helpful to keep in mind that this is core's limitation we're stuck with... ;) so to be clear, i will not commit any approach that includes taking over user.js -- we need to find a more maintainable way to solve this in an acceptable manner in 7.x, and look towards 8.x to help fix core's limitation here.

what are your thoughts on my suggested approach in #3?

Leeteq’s picture

Leeteq commented
Version: 7.x-1.0 » 7.x-1.x-dev

Does this already exist as an issue in the core issue queue as mentioned in #2, to get rid of that hardcoded value?
IMO that is the best way to move forward. We should not have such things hardcoded anywhere.
This issues should be postponed here while being fixed in core, then continued for LT.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented

+1 to have this fixed!

stevecowie’s picture

stevecowie commented
Assigned: steveparks » stevecowie
Status: Needs work » Needs review
StatusFileSize
new minpasswordlength-1142808-3.patch3.86 KB

Here's a patch for this tested against dev. It essentially does the steps described at #3 above:
change variable_set/get to 6 throughout
add an update hook function to set to 6
make 6 minimum value for admin form
alter text of message so that it explains some extra hardening has been applied.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
StatusFileSize
new logintoboggan-minpasswordlength-1142808-9.patch963 bytes

Here is a patch that does not set any minimum length from 0 to 6, but instead allows logintoboggan to alter the help text to match the length specified by logintoboggan.

This is what we were looking for, and it seems to address the core issue of this bug report.

scottalan’s picture

scottalan commented
StatusFileSize
new logintoboggan-pass_length_override-1142808-10.patch1.63 KB

Guess I should have looked at the issue queue first...

The patch I'm using, just another take but basically the same.

scottalan’s picture

scottalan commented
StatusFileSize
new logintoboggan-pass_length_override-1142808-11.patch3.74 KB
new user-password_length-setting.patch1.7 KB

Update: It seems that modules/user/user.js needs a fix, or rather it would be nice. Not sure they will go for this but I'm in the process of submitting a patch for the user.module. Since there isn't an 8.x branch started for logintoboggan I'll post the patch here for the 7.x branch. Any feedback would be appreciated. I'll also include an updated patch for logintoboggan that will work if the D7 core patch is implemented.

Leeteq’s picture

Leeteq commented

@scottalan; is there a related issue filed against core to adjust user.js?

scottalan’s picture

scottalan commented

I found several that had to do with "Password length" but each seemed to be for different reasons. I chose this one: https://drupal.org/node/1824800 as it was labeled simply 'Password length'.

stefan.r’s picture

stefan.r commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

RTBCing #9, we have been using this on dozens of sites for 2+ years without any issues

  • stevecowie committed 9602c18 on 7.x-1.x 
    Issue #1142808 by jmonkfish,scottalan: Added smarter minimum password...
stevecowie’s picture

stevecowie commented

Committed. I'll leave this issue open until hopefully @scottalan and @stefan.r can confirm it's working for them fine in the dev branch.

wuffers’s picture

wuffers commented

Just applied dev branch on an affected site and can confirm the help text is now consistent with password length setting in module.

chellman’s picture

chellman commented

Dev branch has solved the problem here is well.

stevecowie’s picture

stevecowie commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.