aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--RELEASE-NOTES-1.362
-rw-r--r--includes/specials/SpecialBotPasswords.php42
-rw-r--r--includes/user/BotPassword.php49
-rw-r--r--languages/i18n/en.json2
-rw-r--r--languages/i18n/qqq.json2
-rw-r--r--maintenance/createBotPassword.php17
-rw-r--r--tests/phpunit/includes/user/BotPasswordTest.php60
7 files changed, 140 insertions, 34 deletions
diff --git a/RELEASE-NOTES-1.36 b/RELEASE-NOTES-1.36
index 9e7eee2102aa..35ba54cacb56 100644
--- a/RELEASE-NOTES-1.36
+++ b/RELEASE-NOTES-1.36
@@ -322,6 +322,8 @@ because of Phabricator reports.
It had no known callers.
* wfForeignMemcKey() and wfMemcKey(), deprecated in 1.35, have been removed.
* MediaWiki now also requires the php-intl extension.
+* BotPassword::save() now returns a Status object for the result rather than
+ a bool.
* …
=== Deprecations in 1.36 ===
diff --git a/includes/specials/SpecialBotPasswords.php b/includes/specials/SpecialBotPasswords.php
index 19a9cf009d4a..f9bdc4a42aa7 100644
--- a/includes/specials/SpecialBotPasswords.php
+++ b/includes/specials/SpecialBotPasswords.php
@@ -330,6 +330,11 @@ class SpecialBotPasswords extends FormSpecialPage {
)
] );
+ if ( $bp === null ) {
+ // Messages: botpasswords-insert-failed, botpasswords-update-failed
+ return Status::newFatal( "botpasswords-{$this->operation}-failed", $this->par );
+ }
+
if ( $this->operation === 'insert' || !empty( $data['resetPassword'] ) ) {
$this->password = BotPassword::generatePassword( $this->getConfig() );
$password = $this->passwordFactory->newFromPlaintext( $this->password );
@@ -337,24 +342,25 @@ class SpecialBotPasswords extends FormSpecialPage {
$password = null;
}
- if ( $bp && $bp->save( $this->operation, $password ) ) {
- $this->logger->info(
- "Bot password {op} for {user}@{app_id}",
- [
- 'op' => $this->operation,
- 'user' => $this->getUser()->getName(),
- 'app_id' => $this->par,
- 'centralId' => $this->userId,
- 'restrictions' => $data['restrictions'],
- 'grants' => $bp->getGrants(),
- 'client_ip' => $this->getRequest()->getIP()
- ]
- );
- return Status::newGood();
- }
-
- // Messages: botpasswords-insert-failed, botpasswords-update-failed
- return Status::newFatal( "botpasswords-{$this->operation}-failed", $this->par );
+ $res = $bp->save( $this->operation, $password );
+
+ $success = $res->isGood();
+
+ $this->logger->info(
+ 'Bot password {op} for {user}@{app_id} ' . ( $success ? 'succeeded' : 'failed' ),
+ [
+ 'op' => $this->operation,
+ 'user' => $this->getUser()->getName(),
+ 'app_id' => $this->par,
+ 'centralId' => $this->userId,
+ 'restrictions' => $data['restrictions'],
+ 'grants' => $bp->getGrants(),
+ 'client_ip' => $this->getRequest()->getIP(),
+ 'success' => $success,
+ ]
+ );
+
+ return $res;
}
public function onSuccess() {
diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php
index 54536f4fbd02..9b37179364a7 100644
--- a/includes/user/BotPassword.php
+++ b/includes/user/BotPassword.php
@@ -38,6 +38,18 @@ class BotPassword implements IDBAccessObject {
*/
public const PASSWORD_MINLENGTH = 32;
+ /**
+ * Maximum length of the json representation of restrictions
+ * @since 1.36
+ */
+ public const RESTRICTIONS_MAXLENGTH = 65535;
+
+ /**
+ * Maximum length of the json representation of grants
+ * @since 1.36
+ */
+ public const GRANTS_MAXLENGTH = 65535;
+
/** @var bool */
private $isSaved;
@@ -277,22 +289,44 @@ class BotPassword implements IDBAccessObject {
* Save the BotPassword to the database
* @param string $operation 'update' or 'insert'
* @param Password|null $password Password to set.
- * @return bool Success
+ * @return Status
+ * @throws UnexpectedValueException
*/
public function save( $operation, Password $password = null ) {
// Ensure operation is valid
if ( $operation !== 'insert' && $operation !== 'update' ) {
- return false;
+ throw new UnexpectedValueException(
+ "Expected 'insert' or 'update'; got '{$operation}'."
+ );
}
$conds = [
'bp_user' => $this->centralId,
'bp_app_id' => $this->appId,
];
+
+ $res = Status::newGood();
+
+ $restrictions = $this->restrictions->toJson();
+
+ if ( strlen( $restrictions ) > self::RESTRICTIONS_MAXLENGTH ) {
+ $res->fatal( 'botpasswords-toolong-restrictions' );
+ }
+
+ $grants = FormatJson::encode( $this->grants );
+
+ if ( strlen( $grants ) > self::GRANTS_MAXLENGTH ) {
+ $res->fatal( 'botpasswords-toolong-grants' );
+ }
+
+ if ( !$res->isGood() ) {
+ return $res;
+ }
+
$fields = [
'bp_token' => MWCryptRand::generateHex( User::TOKEN_LENGTH ),
- 'bp_restrictions' => $this->restrictions->toJson(),
- 'bp_grants' => FormatJson::encode( $this->grants ),
+ 'bp_restrictions' => $restrictions,
+ 'bp_grants' => $grants,
];
if ( $password !== null ) {
@@ -302,6 +336,7 @@ class BotPassword implements IDBAccessObject {
}
$dbw = self::getDB( DB_MASTER );
+
if ( $operation === 'insert' ) {
$dbw->insert( 'bot_passwords', $fields + $conds, __METHOD__, [ 'IGNORE' ] );
} else {
@@ -313,8 +348,12 @@ class BotPassword implements IDBAccessObject {
if ( $ok ) {
$this->token = $dbw->selectField( 'bot_passwords', 'bp_token', $conds, __METHOD__ );
$this->isSaved = true;
+
+ return $res;
}
- return $ok;
+
+ // Messages: botpasswords-insert-failed, botpasswords-update-failed
+ return Status::newFatal( "botpasswords-{$operation}-failed", $this->appId );
}
/**
diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 7ed8ab7d6a1e..d46e003514f7 100644
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -571,6 +571,8 @@
"botpasswords-help-grants": "Grants allow access to rights already held by your user account. Enabling a grant here does not provide access to any rights that your user account would not otherwise have. See the [[Special:ListGrants|table of grants]] for more information.",
"botpasswords-label-grants-column": "Granted",
"botpasswords-bad-appid": "The bot name \"$1\" is not valid.",
+ "botpasswords-toolong-restrictions": "There are too many IP addresses or ranges entered.",
+ "botpasswords-toolong-grants": "There are too many grants selected.",
"botpasswords-insert-failed": "Failed to add bot name \"$1\". Was it already added?",
"botpasswords-update-failed": "Failed to update bot name \"$1\". Was it deleted?",
"botpasswords-created-title": "Bot password created",
diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 86ac01bf18c7..c69aef6bc61f 100644
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -790,6 +790,8 @@
"botpasswords-help-grants": "Help text for the grant selection checkmatrix.\n\nIdentical:\n* {{msg-mw|Mwoauth-consumer-grantshelp}}",
"botpasswords-label-grants-column": "Label for the checkbox column on the checkmatrix for selecting grants allowed when the bot password is used.",
"botpasswords-bad-appid": "Used as an error message when an invalid \"bot name\" is supplied on [[Special:BotPasswords]]. Parameters:\n* $1 - The rejected bot name.",
+ "botpasswords-toolong-restrictions": "Used when the bot password allowed IP ranges are too long when turned into JSON.",
+ "botpasswords-toolong-grants": "Error message when the selected grants are too long (usually means too many selected) when turned into JSON.",
"botpasswords-insert-failed": "Error message when saving a new bot password failed. It's likely that the failure was because the user resubmitted the form after a previous successful save. Parameters:\n* $1 - Bot name",
"botpasswords-update-failed": "Error message when saving changes to an existing bot password failed. It's likely that the failure was because the user deleted the bot password in another browser window. Parameters:\n* $1 - Bot name",
"botpasswords-created-title": "Title of the success page when a new bot password is created.",
diff --git a/maintenance/createBotPassword.php b/maintenance/createBotPassword.php
index cf35d883dbff..987fd1b50c99 100644
--- a/maintenance/createBotPassword.php
+++ b/maintenance/createBotPassword.php
@@ -26,7 +26,7 @@ require_once __DIR__ . '/Maintenance.php';
class CreateBotPassword extends Maintenance {
/**
- * width of initial column of --showgrants output
+ * Width of initial column of --showgrants output
*/
private const SHOWGRANTS_COLUMN_WIDTH = 20;
@@ -58,7 +58,7 @@ class CreateBotPassword extends Maintenance {
public function execute() {
if ( $this->hasOption( 'showgrants' ) ) {
$this->showGrants();
- exit;
+ return;
}
$username = $this->getArg( 0 );
@@ -112,14 +112,21 @@ class CreateBotPassword extends Maintenance {
'grants' => $grants
] );
+ if ( $bp === null ) {
+ $this->fatalError( "Bot password creation failed." );
+ }
+
$passwordInstance = $passwordFactory->newFromPlaintext( $password );
- $success = $bp->save( 'insert', $passwordInstance );
+ $status = $bp->save( 'insert', $passwordInstance );
- if ( $success ) {
+ if ( $status->isGood() ) {
$this->output( "Success.\n" );
$this->output( "Log in using username:'${username}@${appId}' and password:'${password}'.\n" );
} else {
- $this->fatalError( "Bot password creation failed. Does this appid already exist for the user perhaps?" );
+ $this->fatalError(
+ "Bot password creation failed. Does this appid already exist for the user perhaps?\n\nErrors:\n" .
+ print_r( $status->getErrors(), true )
+ );
}
}
diff --git a/tests/phpunit/includes/user/BotPasswordTest.php b/tests/phpunit/includes/user/BotPasswordTest.php
index d6a37b7d6aaa..002e5dfe4f9d 100644
--- a/tests/phpunit/includes/user/BotPasswordTest.php
+++ b/tests/phpunit/includes/user/BotPasswordTest.php
@@ -365,8 +365,9 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
);
$passwordHash = $password ? $passwordFactory->newFromPlaintext( $password ) : null;
- $this->assertFalse( $bp->save( 'update', $passwordHash ) );
- $this->assertTrue( $bp->save( 'insert', $passwordHash ) );
+ $this->assertFalse( $bp->save( 'update', $passwordHash )->isGood() );
+ $this->assertTrue( $bp->save( 'insert', $passwordHash )->isGood() );
+
$bp2 = BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST );
$this->assertInstanceOf( BotPassword::class, $bp2 );
$this->assertEquals( $bp->getUserCentralId(), $bp2->getUserCentralId() );
@@ -374,6 +375,7 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
$this->assertEquals( $bp->getToken(), $bp2->getToken() );
$this->assertEquals( $bp->getRestrictions(), $bp2->getRestrictions() );
$this->assertEquals( $bp->getGrants(), $bp2->getGrants() );
+
/** @var Password $pw */
$pw = TestingAccessWrapper::newFromObject( $bp )->getPassword();
if ( $password === null ) {
@@ -385,9 +387,10 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
$token = $bp->getToken();
$this->assertEquals( 42, $bp->getUserCentralId() );
$this->assertEquals( 'TestSave', $bp->getAppId() );
- $this->assertFalse( $bp->save( 'insert' ) );
- $this->assertTrue( $bp->save( 'update' ) );
+ $this->assertFalse( $bp->save( 'insert' )->isGood() );
+ $this->assertTrue( $bp->save( 'update' )->isGood() );
$this->assertNotEquals( $token, $bp->getToken() );
+
$bp2 = BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST );
$this->assertInstanceOf( BotPassword::class, $bp2 );
$this->assertEquals( $bp->getToken(), $bp2->getToken() );
@@ -401,8 +404,9 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
$passwordHash = $passwordFactory->newFromPlaintext( 'XXX' );
$token = $bp->getToken();
- $this->assertTrue( $bp->save( 'update', $passwordHash ) );
+ $this->assertTrue( $bp->save( 'update', $passwordHash )->isGood() );
$this->assertNotEquals( $token, $bp->getToken() );
+
/** @var Password $pw */
$pw = TestingAccessWrapper::newFromObject( $bp )->getPassword();
$this->assertTrue( $pw->verify( 'XXX' ) );
@@ -411,7 +415,8 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
$this->assertFalse( $bp->isSaved() );
$this->assertNull( BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST ) );
- $this->assertFalse( $bp->save( 'foobar' ) );
+ $this->expectException( UnexpectedValueException::class );
+ $bp->save( 'foobar' )->isGood();
}
public static function provideSave() {
@@ -420,4 +425,47 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
[ 'foobar' ],
];
}
+
+ /**
+ * Tests for error handling when bp_restrictions and bp_grants are too long
+ */
+ public function testSaveValidation() {
+ $lotsOfIPs = [
+ 'IPAddresses' => array_fill(
+ 0,
+ 5000,
+ "127.0.0.0/8"
+ )
+ ];
+
+ $bp = BotPassword::newUnsaved( [
+ 'centralId' => 42,
+ 'appId' => 'TestSave',
+ // When this becomes JSON, it'll be 70,017 characters, which is
+ // greater than BotPassword::GRANTS_MAXLENGTH, so it will cause an error.
+ 'restrictions' => MWRestrictions::newFromArray( $lotsOfIPs ),
+ 'grants' => [
+ // Maximum length of the JSON is BotPassword::RESTRICTIONS_MAXLENGTH characters.
+ // So one long grant name should be good. Turning it into JSON will add
+ // a couple of extra characters, taking it over BotPassword::RESTRICTIONS_MAXLENGTH
+ // characters long, so it will cause an error.
+ str_repeat( '*', BotPassword::RESTRICTIONS_MAXLENGTH )
+ ],
+ ] );
+
+ $status = $bp->save( 'insert' );
+
+ $this->assertFalse( $status->isGood() );
+ $this->assertNotEmpty( $status->getErrors() );
+
+ $this->assertSame(
+ 'botpasswords-toolong-restrictions',
+ $status->getErrors()[0]['message']
+ );
+
+ $this->assertSame(
+ 'botpasswords-toolong-grants',
+ $status->getErrors()[1]['message']
+ );
+ }
}
generated by cgit v1.2.3 (git 2.25.1) at 2025-04-27 22:08:43 +0000