Posted by steveparks on April 29, 2011 at 11:46pm
2 followers
| Project: | LoginToboggan |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | steveparks |
| Status: | needs work |
Issue Summary
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).
| Attachment | Size |
|---|---|
| logintoboggan_minpwdlength.jpg | 101.1 KB |
Comments
#1
not really sure what to do about this one -- we can hook_js_alter i suppose, but that seems like overkill. suggestions?
#2
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
#3
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:
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.
#4
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.
#5
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?
#6
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.