From 4f29c961970ae2cac6925d8574efd1b91c60639b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Tue, 23 Aug 2016 03:02:23 +0000 Subject: [PATCH] Allow putting the app ID in the password for bot passwords Bot passwords allow backwards-compatible login (with grants, for API usage only) with "@" for username plus a random-generated password. This doesn't work well with some bot frameworks (including Pywikibot, the most popular one) which assume that the text that goes into the username field of the login API is the username that they will be logged in with afterwards (and so the @-postfix causes all kinds of errors). Since the goal of bot passwords is compatibility with old unmaintained API clients, this patch adds an alternative format which does not cause problems with old bots: use the username normally, and use "@" as password. Since this is technically a valid normal password, there is some ambiguity, but bot passwords have a distintive format so it's easy to check and it is extremely unlikely that someone would use the exact same format for their normal password; and if the bot password login fails we can simply retry it as a normal password, just in case. Bug: T142304 Change-Id: Ib59a6fbe0e65d80d5e7d19ff37cec5e011c00539 --- includes/api/ApiLogin.php | 9 +++-- includes/specials/SpecialBotPasswords.php | 8 ++-- includes/user/BotPassword.php | 38 +++++++++++++++++++ languages/i18n/en.json | 2 +- languages/i18n/qqq.json | 2 +- .../phpunit/includes/user/BotPasswordTest.php | 27 +++++++++++++ 6 files changed, 76 insertions(+), 10 deletions(-) diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php index 28937f7873..5f6e34acdf 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -102,17 +102,18 @@ class ApiLogin extends ApiBase { } // Try bot passwords - if ( $authRes === false && $this->getConfig()->get( 'EnableBotPasswords' ) && - strpos( $params['name'], BotPassword::getSeparator() ) !== false + if ( + $authRes === false && $this->getConfig()->get( 'EnableBotPasswords' ) && + ( $botLoginData = BotPassword::canonicalizeLoginData( $params['name'], $params['password'] ) ) ) { $status = BotPassword::login( - $params['name'], $params['password'], $this->getRequest() + $botLoginData[0], $botLoginData[1], $this->getRequest() ); if ( $status->isOK() ) { $session = $status->getValue(); $authRes = 'Success'; $loginType = 'BotPassword'; - } else { + } elseif ( !$botLoginData[2] ) { $authRes = 'Failed'; $message = $status->getMessage(); LoggerFactory::getInstance( 'authentication' )->info( diff --git a/includes/specials/SpecialBotPasswords.php b/includes/specials/SpecialBotPasswords.php index 7e330aa63f..f2ea3e45c2 100644 --- a/includes/specials/SpecialBotPasswords.php +++ b/includes/specials/SpecialBotPasswords.php @@ -290,9 +290,7 @@ class SpecialBotPasswords extends FormSpecialPage { ] ); if ( $this->operation === 'insert' || !empty( $data['resetPassword'] ) ) { - $this->password = PasswordFactory::generateRandomPasswordString( - max( 32, $this->getConfig()->get( 'MinimalPasswordLength' ) ) - ); + $this->password = BotPassword::generatePassword( $this->getConfig() ); $passwordFactory = new PasswordFactory(); $passwordFactory->init( RequestContext::getMain()->getConfig() ); $password = $passwordFactory->newFromPlaintext( $this->password ); @@ -335,7 +333,9 @@ class SpecialBotPasswords extends FormSpecialPage { $out->addWikiMsg( 'botpasswords-newpassword', htmlspecialchars( $username . $sep . $this->par ), - htmlspecialchars( $this->password ) + htmlspecialchars( $this->password ), + htmlspecialchars( $username ), + htmlspecialchars( $this->par . $sep . $this->password ) ); $this->password = null; } diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php index 49a71633c7..df1cb773d7 100644 --- a/includes/user/BotPassword.php +++ b/includes/user/BotPassword.php @@ -388,6 +388,44 @@ class BotPassword implements IDBAccessObject { return (bool)$dbw->affectedRows(); } + /** + * Returns a (raw, unhashed) random password string. + * @param Config $config + * @return string + */ + public static function generatePassword( $config ) { + return PasswordFactory::generateRandomPasswordString( + max( 32, $config->get( 'MinimalPasswordLength' ) ) ); + } + + /** + * There are two ways to login with a bot password: "username@appId", "password" and + * "username", "appId@password". Transform it so it is always in the first form. + * Returns [bot username, bot password, could be normal password?] where the last one is a flag + * meaning this could either be a bot password or a normal password, it cannot be decided for + * certain (although in such cases it almost always will be a bot password). + * If this cannot be a bot password login just return false. + * @param string $username + * @param string $password + * @return array|false + */ + public static function canonicalizeLoginData( $username, $password ) { + $sep = BotPassword::getSeparator(); + if ( strpos( $username, $sep ) !== false ) { + // the separator is not valid in usernames so this must be a bot login + return [ $username, $password, false ]; + } elseif ( strlen( $password ) > 32 && strpos( $password, $sep ) !== false ) { + // the strlen check helps minimize the password information obtainable from timing + $segments = explode( $sep, $password ); + $password = array_pop( $segments ); + $appId = implode( $sep, $segments ); + if ( preg_match( '/^[0-9a-w]{32,}$/', $password ) ) { + return [ $username . $sep . $appId, $password, true ]; + } + } + return false; + } + /** * Try to log the user in * @param string $username Combined user name and app ID diff --git a/languages/i18n/en.json b/languages/i18n/en.json index e1c37c88b5..7b424ee8d4 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -571,7 +571,7 @@ "botpasswords-updated-body": "The bot password for bot name \"$1\" of user \"$2\" was updated.", "botpasswords-deleted-title": "Bot password deleted", "botpasswords-deleted-body": "The bot password for bot name \"$1\" of user \"$2\" was deleted.", - "botpasswords-newpassword": "The new password to log in with $1 is $2. Please record this for future reference.", + "botpasswords-newpassword": "The new password to log in with $1 is $2. Please record this for future reference.
(For old bots which require the login name to be the same as the eventual username, you can also use $3 as username and $4 as password.)", "botpasswords-no-provider": "BotPasswordsSessionProvider is not available.", "botpasswords-restriction-failed": "Bot password restrictions prevent this login.", "botpasswords-invalid-name": "The username specified does not contain the bot password separator (\"$1\").", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 6a3d604ae7..e02761858b 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -754,7 +754,7 @@ "botpasswords-updated-body": "Success message when a bot password is updated. Parameters:\n* $1 - Bot name\n* $2 - User name", "botpasswords-deleted-title": "Title of the success page when a bot password is deleted.", "botpasswords-deleted-body": "Success message when a bot password is deleted. Parameters:\n* $1 - Bot name\n* $2 - User name", - "botpasswords-newpassword": "Success message to display the new password when a bot password is created or updated. Parameters:\n* $1 - User name to be used for login.\n* $2 - Password to be used for login.", + "botpasswords-newpassword": "Success message to display the new password when a bot password is created or updated. Parameters:\n* $1 - User name to be used for login.\n* $2 - Password to be used for login.\n* $3, $4 - an alternative version of the user name and password, respectively, which is less preferred, but more compatible with old bots.", "botpasswords-no-provider": "Error message when login is attempted but the BotPasswordsSessionProvider is not included in $wgSessionProviders.", "botpasswords-restriction-failed": "Error message when login is rejected because the configured restrictions were not satisfied.", "botpasswords-invalid-name": "Error message when a username lacking the separator character is passed to BotPassword. Parameters:\n* $1 - The separator character.", diff --git a/tests/phpunit/includes/user/BotPasswordTest.php b/tests/phpunit/includes/user/BotPasswordTest.php index cfd5f7890a..d63770418c 100644 --- a/tests/phpunit/includes/user/BotPasswordTest.php +++ b/tests/phpunit/includes/user/BotPasswordTest.php @@ -228,6 +228,33 @@ class BotPasswordTest extends MediaWikiTestCase { $this->assertNotNull( BotPassword::newFromCentralId( 43, 'BotPassword' ) ); } + /** + * @dataProvider provideCanonicalizeLoginData + */ + public function testCanonicalizeLoginData( $username, $password, $expectedResult ) { + $result = BotPassword::canonicalizeLoginData( $username, $password ); + if ( is_array( $expectedResult ) ) { + $this->assertArrayEquals( $expectedResult, $result, true, true ); + } else { + $this->assertSame( $expectedResult, $result ); + } + } + + public function provideCanonicalizeLoginData() { + return [ + [ 'user', 'pass', false ], + [ 'user', 'abc@def', false ], + [ 'user@bot', '12345678901234567890123456789012', + [ 'user@bot', '12345678901234567890123456789012', false ] ], + [ 'user', 'bot@12345678901234567890123456789012', + [ 'user@bot', '12345678901234567890123456789012', true ] ], + [ 'user', 'bot@12345678901234567890123456789012345', + [ 'user@bot', '12345678901234567890123456789012345', true ] ], + [ 'user', 'bot@x@12345678901234567890123456789012', + [ 'user@bot@x', '12345678901234567890123456789012', true ] ], + ]; + } + public function testLogin() { // Test failure when bot passwords aren't enabled $this->setMwGlobals( 'wgEnableBotPasswords', false ); -- 2.20.1