mediawiki wrote:

Patch to add the hook to User.php

Patch with improvements suggested by Duesentrieb

attachment patchUserIsValidPassword.diff ignored as obsolete

robchur wrote:

I think I'd prefer to see a more generic hook name, e.g. "IsValidPassword".

mediawiki wrote:

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)

mediawiki wrote:

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

attachment patchUserIsValidPassword.diff ignored as obsolete

robchur wrote:

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.

mediawiki wrote:

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.

robchur wrote:

Whoops, this giving-up-the-coffee thing isn't doing me any good...

mediawiki wrote:

Replacement without as much (any) comments.

Attached:

patch committed in r20195.

@rob: you complain about too *much* documentation? Did it confuse you so much
that the code became unclear to you?...

robchur wrote:

Over-commenting is no more helpful than under commenting...with over-commenting,
the culture of commenting the *wrong thing* often carries forward.

robchur wrote:

*** 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;

}

1. 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.

ayg wrote:

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.