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

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