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

A crude solution for the acquireTarget() race condition. Use SQL
GET_LOCK() to lock the target from the acquireTarget() call until the
transaction is committed.

Add FOR UPDATE to the acquireTarget() SELECT, otherwise it just sees the
snapshot version of the row and inserts a new row anyway.

Add a test which reliably failed prior to the change.

Reword the ipb-block-not-found message. This is normal for simultaneous
blocks of the same target. Don't contact us. In the API, remap it to
"alreadyblocked".

Bug: T389028
Change-Id: I1fa35bf08d456a93930194786f77df389217ba61

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' );
+	} );
+} );