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
(cherry picked from commit 2b65587e4d92e7f27661e8821b14f74ade939cfa)

includes/api/ApiMessageTrait.php

@@ -66,6 +66,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',

includes/block/DatabaseBlockStore.php

@@ -1098,17 +1098,29 @@ class DatabaseBlockStore {
 		// Update bt_count field in existing target, if there is one
 		if ( $isUser ) {
 			$targetConds = [ 'bt_user' => $targetUserId ];
+			$targetLockKey = $dbw->getDomainID() . ':block:u:' . $targetUserId;
 		} else {
 			$targetConds = [
 				'bt_address' => $targetAddress,
 				'bt_auto' => $isAuto,
 			];
+			$targetLockKey = $dbw->getDomainID() . ':block:' .
+				( $isAuto ? 'a' : 'i' ) . ':' . $targetAddress;
 		}
 		$condsWithCount = $targetConds;
 		if ( $expectedTargetCount !== null ) {
 			$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
@@ -1126,6 +1138,7 @@ class DatabaseBlockStore {
 			->select( 'bt_id' )
 			->from( 'block_target' )
 			->where( $targetConds )
+			->forUpdate()
 			->caller( __METHOD__ )
 			->fetchFieldValues();
 		if ( count( $ids ) > 1 ) {

languages/i18n/en.json

@@ -2664,7 +2664,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.",

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