Last modified: 2010-05-15 15:48:25 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T11180, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 9180 - Hook for User::isValidPassword()
Hook for User::isValidPassword()
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-reviewed
: 10617 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2007-03-06 07:42 UTC by Ger Apeldoorn
Modified: 2010-05-15 15:48 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---

Patch to add the hook to User.php (556 bytes, patch)
2007-03-06 14:12 UTC, Ger Apeldoorn
Update to make use of the return value of a hook. (927 bytes, patch)
2007-03-07 06:50 UTC, Ger Apeldoorn
Replacement without as much (any) comments. (583 bytes, patch)
2007-03-07 08:30 UTC, Ger Apeldoorn

Description Ger Apeldoorn 2007-03-06 07:42:45 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
Comment 1 Ger Apeldoorn 2007-03-06 14:12:07 UTC
Created attachment 3298 [details]
Patch to add the hook to User.php

Patch with improvements suggested by Duesentrieb
Comment 2 Rob Church 2007-03-06 15:58:51 UTC
I think I'd prefer to see a more generic hook name, e.g. "IsValidPassword".
Comment 3 Ger Apeldoorn 2007-03-06 16:05:30 UTC
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.


PS I cannot change the original report (to get the diff out)
Comment 4 Ger Apeldoorn 2007-03-07 06:50:04 UTC
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
Comment 5 Rob Church 2007-03-07 07:58:27 UTC
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.
Comment 6 Ger Apeldoorn 2007-03-07 08:12:53 UTC
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.
Comment 7 Rob Church 2007-03-07 08:20:50 UTC
Whoops, this giving-up-the-coffee thing isn't doing me any good...
Comment 8 Ger Apeldoorn 2007-03-07 08:30:23 UTC
Created attachment 3308 [details]
Replacement without as much (any) comments.
Comment 9 Daniel Kinzler 2007-03-07 11:26:01 UTC
patch committed in r20195.

@rob: you complain about too *much* documentation? Did it confuse you so much
that the code became unclear to you?...
Comment 10 Rob Church 2007-03-07 11:35:21 UTC
Over-commenting is no more helpful than under commenting...with over-commenting,
the culture of commenting the *wrong thing* often carries forward.
Comment 11 Rob Church 2007-07-17 16:28:00 UTC
*** Bug 10617 has been marked as a duplicate of this bug. ***
Comment 12 Christian Neubauer 2007-07-17 16:59:58 UTC
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.
Comment 13 Christian Neubauer 2007-07-18 20:01:17 UTC
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.
Comment 14 Christian Neubauer 2007-07-18 20:30:29 UTC
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.
Comment 15 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-07-18 20:31:52 UTC
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.
Comment 16 Brion Vibber 2007-07-18 20:35:18 UTC
Your hook's return value is not the return value from $user->isValidPassword().

$user->isValidPassword() returns either:

true -- the password is valid
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
false -- the password is invalid.

*That* value is passed on out from the user-level API function $user->isValidPassword().
Comment 17 Christian Neubauer 2007-07-18 20:50:22 UTC
Thanks.  My mistake.

Note You need to log in before you can comment on or make changes to this bug.