Merge "block: Fix DBS::acquireTarget() race using GET_LOCK()"

4 files changed, 44 insertions, 1 deletions

diff --git a/includes/api/ApiMessageTrait.php b/includes/api/ApiMessageTrait.php
index 8f796101ce2b..9c2bd7575117 100644
--- a/includes/api/ApiMessageTrait.php
+++ b/includes/api/ApiMessageTrait.php
@@ -67,6 +67,7 @@ trait ApiMessageTrait {
 		'importuploaderrorpartial' => 'partialupload',
 		'importuploaderrorsize' => 'filetoobig',
 		'importuploaderrortemp' => 'notempdir',
+		'ipb-block-not-found' => 'alreadyblocked',
 		'ipb_already_blocked' => 'alreadyblocked',
 		'ipb_blocked_as_range' => 'blockedasrange',
 		'ipb_cant_unblock' => 'cantunblock',
diff --git a/includes/block/DatabaseBlockStore.php b/includes/block/DatabaseBlockStore.php
index c33157b58157..20f662e044d5 100644
--- a/includes/block/DatabaseBlockStore.php
+++ b/includes/block/DatabaseBlockStore.php
@@ -1044,6 +1044,7 @@ class DatabaseBlockStore {
 			$targetUserName = (string)$target;
 			$targetUserId = $target->getUserIdentity()->getId( $this->wikiId );
 			$targetConds = [ 'bt_user' => $targetUserId ];
+			$targetLockKey = $dbw->getDomainID() . ':block:u:' . $targetUserId;
 		} else {
 			$targetAddress = (string)$target;
 			$targetUserName = null;
@@ -1052,6 +1053,8 @@ class DatabaseBlockStore {
 				'bt_address' => $targetAddress,
 				'bt_auto' => $isAuto,
 			];
+			$targetLockKey = $dbw->getDomainID() . ':block:' .
+				( $isAuto ? 'a' : 'i' ) . ':' . $targetAddress;
 		}
 
 		$condsWithCount = $targetConds;
@@ -1059,6 +1062,15 @@ class DatabaseBlockStore {
 			$condsWithCount['bt_count'] = $expectedTargetCount;
 		}
 
+		$dbw->lock( $targetLockKey, __METHOD__ );
+		$func = __METHOD__;
+		$dbw->onTransactionCommitOrIdle(
+			static function () use ( $dbw, $targetLockKey, $func ) {
+				$dbw->unlock( $targetLockKey, "$func.closure" );
+			},
+			__METHOD__
+		);
+
 		// This query locks the index gap when the target doesn't exist yet,
 		// so there is a risk of throttling adjacent block insertions,
 		// especially on small wikis which have larger gaps. If this proves to
@@ -1076,6 +1088,7 @@ class DatabaseBlockStore {
 			->select( [ 'bt_id', 'bt_count' ] )
 			->from( 'block_target' )
 			->where( $targetConds )
+			->forUpdate()
 			->caller( __METHOD__ )
 			->fetchResultSet();
 		if ( $res->numRows() > 1 ) {
diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index a2f807404712..2393d99a6656 100644
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -2621,7 +2621,7 @@
 	"blockipsuccesssub": "Block succeeded",
 	"blockipsuccesstext": "[[Special:Contributions/$1|$1]] has been blocked.<br />\nSee the [[Special:BlockList|block list]] to review blocks.",
 	"ipb-empty-block": "The block submitted has no restrictions enabled.",
-	"ipb-block-not-found": "The block could not be made, but no existing block was found for \"$1\". If this problem persists, please [https://www.mediawiki.org/wiki/Special:MyLanguage/Help_talk:Blocking_users report it].",
+	"ipb-block-not-found": "The block could not be made, probably because another administrator tried to block this user at the same time. Check the block status and try again.",
 	"ipb-blockingself": "You are about to block yourself! Are you sure you want to do that?",
 	"ipb-confirmhideuser": "You are about to block a user with \"hide user\" enabled. This will suppress the user's name in all lists and log entries. Are you sure you want to do that?",
 	"ipb-confirmaction": "If you are sure you really want to do it, please check the \"{{int:ipb-confirm}}\" field at the bottom.",
diff --git a/tests/api-testing/action/Block.js b/tests/api-testing/action/Block.js
new file mode 100644
index 000000000000..8053137494b5
--- /dev/null
+++ b/tests/api-testing/action/Block.js
@@ -0,0 +1,29 @@
+'use strict';
+
+const { action, assert } = require( 'api-testing' );
+
+describe( 'Block', () => {
+	const ip = '::' + Math.floor( Math.random() * 65534 ).toString( 16 );
+	it( 'should not allow multiblocks without newblock (T389028)', async () => {
+		const mindy = await action.mindy();
+		const token = await mindy.token();
+		const promises = [
+			mindy.request( { action: 'block', user: ip, token: token }, 'POST' ),
+			mindy.request( { action: 'block', user: ip, token: token }, 'POST' )
+		];
+		const res = await Promise.all( promises );
+		assert.lengthOf( res, 2 );
+		assert.equal( res[ 0 ].status, 200 );
+		assert.equal( res[ 1 ].status, 200 );
+
+		const goodIndex = 'block' in res[ 0 ].body ? 0 : 1;
+		const goodBody = res[ goodIndex ].body;
+		const badBody = res[ +!goodIndex ].body;
+
+		assert.property( goodBody, 'block' );
+		assert.isOk( goodBody.block.id );
+
+		assert.property( badBody, 'error' );
+		assert.equal( badBody.error.code, 'alreadyblocked' );
+	} );
+} );