From 86db28715f8683acb7dc41952f9de0826734117f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Thu, 20 Dec 2018 14:44:04 -0800 Subject: [PATCH] Deprecate User::getPasswordValidity() Unused, the return format does not seem useful. Also improve the documentation of $wgPasswordPolicy and PasswordPolicyChecks. Change-Id: Ic01e80cfefc4cfb0eee1eccc6a66942f692278a0 --- RELEASE-NOTES-1.33 | 2 + includes/DefaultSettings.php | 46 ++++++++++++++++------ includes/password/PasswordPolicyChecks.php | 28 +++++++++---- includes/user/User.php | 7 +++- tests/phpunit/includes/user/UserTest.php | 1 + 5 files changed, 61 insertions(+), 23 deletions(-) diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index 5b6f6b2120..75672524d7 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -189,6 +189,8 @@ because of Phabricator reports. domain ID as a key component and use makeGlobalKey(). * (T202094) Title::getUserCaseDBKey() is deprecated; instead, please use Title::getDBKey(), which doesn't vary case. +* User::getPasswordValidity() is now deprecated. User::checkPasswordValidity() + returns the same information in a more useful format. * … === Other changes in 1.33 === diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 2f1efbf549..c76b8c1629 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4451,29 +4451,49 @@ $wgCentralIdLookupProviders = [ $wgCentralIdLookupProvider = 'local'; /** - * Password policy for local wiki users. A user's effective policy - * is the superset of all policy statements from the policies for the - * groups where the user is a member. If more than one group policy - * include the same policy statement, the value is the max() of the - * values. Note true > false. The 'default' policy group is required, - * and serves as the minimum policy for all users. New statements can - * be added by appending to $wgPasswordPolicy['checks']. - * Statements: - * - MinimalPasswordLength - minimum length a user can set - * - MinimumPasswordLengthToLogin - passwords shorter than this will + * Password policy for the wiki. + * Structured as + * [ + * 'policies' => [ => [ => , ... ], ... ], + * 'checks' => [ => , ... ], + * ] + * where is a user group, is a password policy name + * (arbitrary string) defined in the 'checks' part, is the + * PHP callable implementing the policy check, is a number, + * boolean or null that gets passed to the callback. + * + * A user's effective policy is the superset of all policy statements + * from the policies for the groups where the user is a member. If more + * than one group policy include the same policy statement, the value is + * the max() of the values. Note true > false. The 'default' policy group + * is required, and serves as the minimum policy for all users. + * + * Callbacks receive three arguments: the policy value, the User object + * and the password; and must return a StatusValue. A non-good status + * means the password will not be accepted for new accounts, and existing + * accounts will be prompted for password change or barred from logging in + * (depending on whether the status is a fatal or merely error/warning). + * + * The checks supported by core are: + * - MinimalPasswordLength - Minimum length a user can set. + * - MinimumPasswordLengthToLogin - Passwords shorter than this will * not be allowed to login, regardless if it is correct. * - MaximalPasswordLength - maximum length password a user is allowed * to attempt. Prevents DoS attacks with pbkdf2. - * - PasswordCannotMatchUsername - Password cannot match username to + * - PasswordCannotMatchUsername - Password cannot match the username. * - PasswordCannotMatchBlacklist - Username/password combination cannot - * match a specific, hardcoded blacklist. + * match a blacklist of default passwords used by MediaWiki in the past. * - PasswordCannotBePopular - Blacklist passwords which are known to be * commonly chosen. Set to integer n to ban the top n passwords. * If you want to ban all common passwords on file, use the * PHP_INT_MAX constant. * - PasswordNotInLargeBlacklist - Password not in best practices list of - * 100,000 commonly used passwords. + * 100,000 commonly used passwords. Due to the size of the list this + * is a probabilistic test. + * * @since 1.26 + * @see PasswordPolicyChecks + * @see User::checkPasswordValidity() */ $wgPasswordPolicy = [ 'policies' => [ diff --git a/includes/password/PasswordPolicyChecks.php b/includes/password/PasswordPolicyChecks.php index 3c565359d9..81b8a0d3ba 100644 --- a/includes/password/PasswordPolicyChecks.php +++ b/includes/password/PasswordPolicyChecks.php @@ -25,13 +25,20 @@ use MediaWiki\MediaWikiServices; use Wikimedia\PasswordBlacklist; /** - * Functions to check passwords against a policy requirement + * Functions to check passwords against a policy requirement. + * + * $policyVal is the value configured in $wgPasswordPolicy. If the return status is fatal, + * the user won't be allowed to login. If the status is not good but not fatal, the user + * will not be allowed to set the given password (on registration or password change), + * but can still log in after bypassing a warning. + * * @since 1.26 + * @see $wgPasswordPolicy */ class PasswordPolicyChecks { /** - * Check password is longer than minimum, not fatal + * Check password is longer than minimum, not fatal. * @param int $policyVal minimal length * @param User $user * @param string $password @@ -46,7 +53,7 @@ class PasswordPolicyChecks { } /** - * Check password is longer than minimum, fatal + * Check password is longer than minimum, fatal. * @param int $policyVal minimal length * @param User $user * @param string $password @@ -61,7 +68,8 @@ class PasswordPolicyChecks { } /** - * Check password is shorter than maximum, fatal + * Check password is shorter than maximum, fatal. + * Intended for preventing DoS attacks when using a more expensive password hash like PBKDF2. * @param int $policyVal maximum length * @param User $user * @param string $password @@ -76,7 +84,7 @@ class PasswordPolicyChecks { } /** - * Check if username and password match + * Check if username and password are a (case-insensitive) match. * @param bool $policyVal true to force compliance. * @param User $user * @param string $password @@ -95,7 +103,7 @@ class PasswordPolicyChecks { } /** - * Check if username and password are on a blacklist + * Check if username and password are on a blacklist of past MediaWiki default passwords. * @param bool $policyVal true to force compliance. * @param User $user * @param string $password @@ -126,7 +134,8 @@ class PasswordPolicyChecks { } /** - * Ensure that password isn't in top X most popular passwords + * Ensure that password isn't in top X most popular passwords, as defined by + * $wgPopularPasswordFile. * * @param int $policyVal Cut off to use. Will automatically shrink to the max * supported for error messages if set to more than max number of passwords on file, @@ -135,6 +144,7 @@ class PasswordPolicyChecks { * @param string $password * @since 1.27 * @return Status + * @see $wgPopularPasswordFile */ public static function checkPopularPasswordBlacklist( $policyVal, User $user, $password ) { global $wgPopularPasswordFile, $wgSitename; @@ -173,7 +183,9 @@ class PasswordPolicyChecks { /** * Ensure the password isn't in the list of passwords blacklisted by the - * wikimedia/password-blacklist library + * wikimedia/password-blacklist library, which contains (as of 0.1.4) the + * 100.000 top passwords from SecLists (as a Bloom filter, with an + * 0.000001 false positive ratio). * * @param bool $policyVal Whether to apply this policy * @param User $user diff --git a/includes/user/User.php b/includes/user/User.php index 14122158f8..ec75f052e5 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -1144,8 +1144,8 @@ class User implements IDBAccessObject, UserIdentity { * @return bool */ public function isValidPassword( $password ) { - // simple boolean wrapper for getPasswordValidity - return $this->getPasswordValidity( $password ) === true; + // simple boolean wrapper for checkPasswordValidity + return $this->checkPasswordValidity( $password )->isGood(); } /** @@ -1153,8 +1153,11 @@ class User implements IDBAccessObject, UserIdentity { * * @param string $password Desired password * @return bool|string|array True on success, string or array of error message on failure + * @deprecated since 1.33, use checkPasswordValidity */ public function getPasswordValidity( $password ) { + wfDeprecated( __METHOD__, '1.33' ); + $result = $this->checkPasswordValidity( $password ); if ( $result->isGood() ) { return true; diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 4f8a7da3af..50a9e50a9d 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -392,6 +392,7 @@ class UserTest extends MediaWikiTestCase { ], ], ] ); + $this->hideDeprecated( 'User::getPasswordValidity' ); $user = static::getTestUser()->getUser(); -- 2.20.1