Last modified: 2010-05-15 15:48:25 UTC
I'd like a hook in User->isValidPassword, to enable the enforcement of strong passwords. Here's the diff.. --- mediawiki-1.9.3/includes/User.php 2007-02-21 03:20:31.000000000 +0100 +++ wiki/includes/User.php 2007-03-06 07:43:15.000000000 +0100 @@ -466,7 +466,17 @@ */ static function isValidPassword( $password ) { global $wgMinimalPasswordLength; - return strlen( $password ) >= $wgMinimalPasswordLength; + if (strlen( $password ) >= $wgMinimalPasswordLength) { + //call hook + if( !wfRunHooks( 'EnforceStrongPassword', array( $password ) ) ) { + return false; + } + return true; + } + else { + //password is false, no need to bother with the hook. + return false; + } } /** Many thanks! Ger Apeldoorn
Created attachment 3298 [details] Patch to add the hook to User.php Patch with improvements suggested by Duesentrieb
I think I'd prefer to see a more generic hook name, e.g. "IsValidPassword".
Yes, if you look at the attachment, you'll find the generic name there. I've talked to Duesentrieb and he suggested some additional modifications that I hope to insert tomorrow. Ger. PS I cannot change the original report (to get the diff out)
Created attachment 3307 [details] Update to make use of the return value of a hook. This implements the use of the return value of a hook. Thanks to Duesentrieb
I don't quite get the purpose of a second check against $return - it more or less ignores the return value from the hook chain in the case that $return is modified to be false. A regular hook should work fine here. You also don't need to comment things in as much detail.
I've talked to Duesentrieb @irc. As I understood, the return value of the wfRunHooks call should determine if the original code is run afterwards. If the wfRunHooks return value is true, the second check is necessary to determine that the hook thinks that the password is invalid.
Whoops, this giving-up-the-coffee thing isn't doing me any good...
Created attachment 3308 [details] Replacement without as much (any) comments.
patch committed in r20195. @rob: you complain about too *much* documentation? Did it confuse you so much that the code became unclear to you?...
Over-commenting is no more helpful than under commenting...with over-commenting, the culture of commenting the *wrong thing* often carries forward.
*** Bug 10617 has been marked as a duplicate of this bug. ***
Thanks for adding this. There should probably also be a hook in User::randomPassword to make sure that a password is not generated that doesn't meet the requirements of isValidPassword.
Also, in SpecialUserlogin.php around line 264, if isValidPassword fails, the wiki outputs the message 'passwordtoshort'. Ideally this message would be made more generic ('passwordnotvalid'?) or an extension would somehow be able to specify the message returned.
Yeah now I'm really confused. So if an extension returns false from this hook, then the function User::isValidPassword() returns $result. So presumably, result is the reason that the password is not valid. But, in SpecialUserlogin.php, there is a loop that looks like this: if ( !$u->isValidPassword( $this->mPassword ) ) { $this->mainLoginForm( wfMsg( 'passwordtooshort', $wgMinimalPasswordLength ) ); return false; } So if the result is a string or anything other then false or null, this check will wrongly assume the password is valid. The check in SpecialUserlogin.php -> LoginForm::addNewAccountInternal() should probably look like this: $result = $u->isValidPassword( $this->mPassword ); if ( $result !== true ) { $this->mainLoginForm( wfMsg( $result ) ); return false; } This change would require that User.php -> User::isValidPassword() be modified so that $result is set to 'passwordtooshort' before the function returns false. Something like this: function isValidPassword( $password ) { global $wgMinimalPasswordLength, $wgContLang; $result = null; if( !wfRunHooks( 'isValidPassword', array( $password, &$result ) ) ) return $result; if ($result === false) { $result = 'genericbadpasswordmessage'; return false; } # check if the password is long enough and if not set $result before returning false; $longenough = (strlen( $password ) >= $wgMinimalPasswordLength) && ($wgContLang->lc( $password ) !== $wgContLang->lc( $this->mName )); if(!$longenough) { $result = 'passwordtooshort'; return false; } return true; } Sorry I don't have any diffing skills. Please let me know if I'm missing something here.
The bug is still fixed. $result is meant to be boolean. If you'd like a $reason field or some other change to the hook, open a different bug, please.
Your hook's return value is not the return value from $user->isValidPassword(). $user->isValidPassword() returns either: true -- the password is valid or false -- the password is invalid. Your hook returns "true" to indicate that further processing should continue or "false" to indicate that processing should stop at that point, as all $wgHooks functions do. You may also modify the $result variable, which should contain the values: true -- the password is valid or false -- the password is invalid. *That* value is passed on out from the user-level API function $user->isValidPassword().
Thanks. My mistake.