Merge "block: Add autoblock filtering parameters"

9 files changed, 118 insertions, 62 deletions

diff --git a/includes/api/ApiBlock.php b/includes/api/ApiBlock.php
index 22417b4c2c9f..d7f3342b5b4c 100644
--- a/includes/api/ApiBlock.php
+++ b/includes/api/ApiBlock.php
@@ -138,14 +138,8 @@ class ApiBlock extends ApiBase {
 			if ( $params['newblock'] ) {
 				$status = $this->insertBlock( $target, $params );
 			} else {
-				// Get non-auto blocks
-				// TODO: factor out to DatabaseBlockStore
-				$blocks = [];
-				foreach ( $this->blockStore->newListFromTarget( $target ) as $block ) {
-					if ( $block->getType() !== AbstractBlock::TYPE_AUTO ) {
-						$blocks[] = $block;
-					}
-				}
+				$blocks = $this->blockStore->newListFromTarget(
+					$target, null, false, DatabaseBlockStore::AUTO_NONE );
 				if ( count( $blocks ) === 0 ) {
 					$status = $this->insertBlock( $target, $params );
 				} elseif ( count( $blocks ) === 1 ) {
diff --git a/includes/block/BlockUser.php b/includes/block/BlockUser.php
index 22cbc02c0701..aa9ac9269fd4 100644
--- a/includes/block/BlockUser.php
+++ b/includes/block/BlockUser.php
@@ -388,17 +388,11 @@ class BlockUser {
 	 */
 	private function getPriorBlocksForTarget() {
 		if ( $this->priorBlocksForTarget === null ) {
-			$priorBlocks = $this->blockStore->newListFromTarget( $this->target, null, true );
-			foreach ( $priorBlocks as $i => $block ) {
+			$this->priorBlocksForTarget = $this->blockStore->newListFromTarget(
+				$this->target, null, true,
 				// If we're blocking an IP, ignore any matching autoblocks (T287798)
-				// TODO: put this in the query conditions
-				if ( $this->target->getType() !== Block::TYPE_AUTO
-					&& $block->getType() === Block::TYPE_AUTO
-				) {
-					unset( $priorBlocks[$i] );
-				}
-			}
-			$this->priorBlocksForTarget = array_values( $priorBlocks );
+				DatabaseBlockStore::AUTO_SPECIFIED
+			);
 		}
 		return $this->priorBlocksForTarget;
 	}
diff --git a/includes/block/DatabaseBlockStore.php b/includes/block/DatabaseBlockStore.php
index d8c8d71e12d9..4ecd8024d155 100644
--- a/includes/block/DatabaseBlockStore.php
+++ b/includes/block/DatabaseBlockStore.php
@@ -57,6 +57,13 @@ use function array_key_exists;
  * @author DannyS712
  */
 class DatabaseBlockStore {
+	/** Load all autoblocks */
+	public const AUTO_ALL = 'all';
+	/** Load only autoblocks specified by ID */
+	public const AUTO_SPECIFIED = 'specified';
+	/** Do not load autoblocks */
+	public const AUTO_NONE = 'none';
+
 	/**
 	 * @internal For use by ServiceWiring
 	 */
@@ -195,12 +202,14 @@ class DatabaseBlockStore {
 	 * @param BlockTarget|null $vagueTarget Also search for blocks affecting
 	 *     this target. Doesn't make any sense to use TYPE_AUTO here. Leave blank to
 	 *     skip IP lookups.
+	 * @param string $auto One of the self::AUTO_* constants
 	 * @return DatabaseBlock[] Any relevant blocks
 	 */
 	private function newLoad(
 		$specificTarget,
 		$fromPrimary,
-		$vagueTarget = null
+		$vagueTarget = null,
+		$auto = self::AUTO_ALL
 	) {
 		if ( $fromPrimary ) {
 			$db = $this->getPrimaryDB();
@@ -268,10 +277,14 @@ class DatabaseBlockStore {
 			return [];
 		}
 
+		// Exclude autoblocks unless AUTO_ALL was requested.
+		$autoConds = $auto === self::AUTO_ALL ? [] : [ 'bt_auto' => 0 ];
+
 		$blockQuery = $this->getQueryInfo();
 		$res = $db->newSelectQueryBuilder()
 			->queryInfo( $blockQuery )
 			->where( $db->orExpr( $orConds ) )
+			->andWhere( $autoConds )
 			->caller( __METHOD__ )
 			->fetchResultSet();
 
@@ -450,6 +463,10 @@ class DatabaseBlockStore {
 	 *     block which affects that target (so for an IP address, get ranges containing that IP;
 	 *     and also get any relevant autoblocks). Leave empty or blank to skip IP-based lookups.
 	 * @param bool $fromPrimary Whether to use the DB_PRIMARY database
+	 * @param string $auto Since 1.44. One of the self::AUTO_* constants:
+	 *    - AUTO_ALL: always load autoblocks
+	 *    - AUTO_SPECIFIED: load only autoblocks specified in the input by ID
+	 *    - AUTO_NONE: do not load autoblocks
 	 * @return DatabaseBlock|null (null if no relevant block could be found). The target and type
 	 *     of the returned block will refer to the actual block which was found, which might
 	 *     not be the same as the target you gave if you used $vagueTarget!
@@ -457,9 +474,10 @@ class DatabaseBlockStore {
 	public function newFromTarget(
 		$specificTarget,
 		$vagueTarget = null,
-		$fromPrimary = false
+		$fromPrimary = false,
+		$auto = self::AUTO_ALL
 	) {
-		$blocks = $this->newListFromTarget( $specificTarget, $vagueTarget, $fromPrimary );
+		$blocks = $this->newListFromTarget( $specificTarget, $vagueTarget, $fromPrimary, $auto );
 		return $this->chooseMostSpecificBlock( $blocks );
 	}
 
@@ -470,12 +488,17 @@ class DatabaseBlockStore {
 	 * @param BlockTarget|string|UserIdentity|int|null $specificTarget
 	 * @param BlockTarget|string|UserIdentity|int|null $vagueTarget
 	 * @param bool $fromPrimary
+	 * @param string $auto Since 1.44. One of the self::AUTO_* constants:
+	 *   - AUTO_ALL: always load autoblocks
+	 *   - AUTO_SPECIFIED: load only autoblocks specified in the input by ID
+	 *   - AUTO_NONE: do not load autoblocks
 	 * @return DatabaseBlock[] Any relevant blocks
 	 */
 	public function newListFromTarget(
 		$specificTarget,
 		$vagueTarget = null,
-		$fromPrimary = false
+		$fromPrimary = false,
+		$auto = self::AUTO_ALL
 	) {
 		if ( !( $specificTarget instanceof BlockTarget ) ) {
 			$specificTarget = $this->blockTargetFactory->newFromLegacyUnion( $specificTarget );
@@ -484,13 +507,16 @@ class DatabaseBlockStore {
 			$vagueTarget = $this->blockTargetFactory->newFromLegacyUnion( $vagueTarget );
 		}
 		if ( $specificTarget instanceof AutoBlockTarget ) {
+			if ( $auto === self::AUTO_NONE ) {
+				return [];
+			}
 			$block = $this->newFromID( $specificTarget->getId() );
 			return $block ? [ $block ] : [];
 		} elseif ( $specificTarget === null && $vagueTarget === null ) {
 			// We're not going to find anything useful here
 			return [];
 		} else {
-			return $this->newLoad( $specificTarget, $fromPrimary, $vagueTarget );
+			return $this->newLoad( $specificTarget, $fromPrimary, $vagueTarget, $auto );
 		}
 	}
 
@@ -962,7 +988,8 @@ class DatabaseBlockStore {
 		DatabaseBlock $block,
 		IDatabase $dbw
 	) {
-		$targetConds = $this->getTargetConds( $block );
+		// @phan-suppress-next-line PhanTypeMismatchArgumentNullable
+		$targetConds = $this->getTargetConds( $block->getTarget() );
 		return $dbw->newSelectQueryBuilder()
 			->select( [ 'bl_id', 'bl_target' ] )
 			->from( 'block' )
@@ -975,17 +1002,18 @@ class DatabaseBlockStore {
 	/**
 	 * Get conditions matching an existing block's block_target row
 	 *
-	 * @param DatabaseBlock $block
+	 * @param BlockTarget $target
 	 * @return array
 	 */
-	private function getTargetConds( DatabaseBlock $block ) {
-		$target = $block->getTarget();
+	private function getTargetConds( BlockTarget $target ) {
 		if ( $target instanceof UserBlockTarget ) {
 			return [
 				'bt_user' => $target->getUserIdentity()->getId( $this->wikiId )
 			];
-		} else {
+		} elseif ( $target instanceof AnonIpBlockTarget || $target instanceof RangeBlockTarget ) {
 			return [ 'bt_address' => $target->toString() ];
+		} else {
+			throw new \InvalidArgumentException( 'Invalid target type' );
 		}
 	}
 
@@ -1207,7 +1235,8 @@ class DatabaseBlockStore {
 			$newTarget = $this->blockTargetFactory->newFromLegacyUnion( $newTarget );
 		}
 
-		$oldTargetConds = $this->getTargetConds( $block );
+		// @phan-suppress-next-line PhanTypeMismatchArgumentNullable
+		$oldTargetConds = $this->getTargetConds( $block->getTarget() );
 		$block->setTarget( $newTarget );
 
 		$dbw->startAtomic( __METHOD__ );
diff --git a/includes/block/UnblockUser.php b/includes/block/UnblockUser.php
index fc56f9dc8e1e..f4f071656f85 100644
--- a/includes/block/UnblockUser.php
+++ b/includes/block/UnblockUser.php
@@ -228,7 +228,8 @@ class UnblockUser {
 			return $this->blockToRemove;
 		}
 
-		$activeBlocks = $this->blockStore->newListFromTarget( $this->target );
+		$activeBlocks = $this->blockStore->newListFromTarget(
+			$this->target, null, false, DatabaseBlockStore::AUTO_SPECIFIED );
 		if ( !$activeBlocks ) {
 			$status->fatal( 'ipb_cant_unblock', $this->target );
 			return null;
diff --git a/includes/logging/LogEventsList.php b/includes/logging/LogEventsList.php
index 67787ab929ab..e2d9c57e435f 100644
--- a/includes/logging/LogEventsList.php
+++ b/includes/logging/LogEventsList.php
@@ -27,7 +27,6 @@ namespace MediaWiki\Logging;
 
 use InvalidArgumentException;
 use MapCacheLRU;
-use MediaWiki\Block\Block;
 use MediaWiki\Block\DatabaseBlockStore;
 use MediaWiki\ChangeTags\ChangeTags;
 use MediaWiki\Context\ContextSource;
@@ -841,28 +840,26 @@ class LogEventsList extends ContextSource {
 		if ( !$user ) {
 			return null;
 		}
-		$numBlocks = 0;
 		$appliesToTitle = false;
 		$logTargetPage = '';
 		$blockTargetName = '';
-		foreach ( $blockStore->newListFromTarget( $user, $user ) as $block ) {
-			if ( $block->getType() !== Block::TYPE_AUTO ) {
-				$numBlocks++;
-				if ( $block->appliesToTitle( $title ) ) {
-					$appliesToTitle = true;
-				}
-				$blockTargetName = $block->getTargetName();
-				$logTargetPage = $namespaceInfo->getCanonicalName( NS_USER ) .
-					':' . $blockTargetName;
+		$blocks = $blockStore->newListFromTarget( $user, $user, false,
+			DatabaseBlockStore::AUTO_NONE );
+		foreach ( $blocks as $block ) {
+			if ( $block->appliesToTitle( $title ) ) {
+				$appliesToTitle = true;
 			}
+			$blockTargetName = $block->getTargetName();
+			$logTargetPage = $namespaceInfo->getCanonicalName( NS_USER ) .
+				':' . $blockTargetName;
 		}
 
 		// Show log extract if the user is sitewide blocked or is partially
 		// blocked and not allowed to edit their user page or user talk page
-		if ( !$numBlocks || !$appliesToTitle ) {
+		if ( !count( $blocks ) || !$appliesToTitle ) {
 			return null;
 		}
-		$msgKey = $numBlocks === 1
+		$msgKey = count( $blocks ) === 1
 			? 'blocked-notice-logextract' : 'blocked-notice-logextract-multi';
 		$params = [
 			'lim' => 1,
@@ -870,10 +867,10 @@ class LogEventsList extends ContextSource {
 			'msgKey' => [
 				$msgKey,
 				$user->getName(), # Support GENDER in notice
-				$numBlocks
+				count( $blocks )
 			],
 		];
-		if ( $numBlocks > 1 ) {
+		if ( count( $blocks ) > 1 ) {
 			$params['footerHtmlItems'] = [
 				$linkRenderer->makeKnownLink(
 					SpecialPage::getTitleFor( 'BlockList' ),
diff --git a/includes/page/Article.php b/includes/page/Article.php
index ac0fec742a51..c6ff94f24503 100644
--- a/includes/page/Article.php
+++ b/includes/page/Article.php
@@ -1121,7 +1121,9 @@ class Article implements Page {
 			} else {
 				$specificTarget = $title->getRootText();
 			}
-			if ( $this->blockStore->newFromTarget( $specificTarget, $vagueTarget ) instanceof DatabaseBlock ) {
+			$block = $this->blockStore->newFromTarget(
+				$specificTarget, $vagueTarget, false, DatabaseBlockStore::AUTO_NONE );
+			if ( $block instanceof DatabaseBlock ) {
 				return [
 					'index' => 'noindex',
 					'follow' => 'nofollow'
diff --git a/includes/specialpage/ContributionsSpecialPage.php b/includes/specialpage/ContributionsSpecialPage.php
index cb9a1eb564ab..7a5a43811f0f 100644
--- a/includes/specialpage/ContributionsSpecialPage.php
+++ b/includes/specialpage/ContributionsSpecialPage.php
@@ -434,28 +434,26 @@ class ContributionsSpecialPage extends IncludableSpecialPage {
 				// For IP ranges you must give DatabaseBlock::newFromTarget the CIDR string
 				// and not a user object.
 				if ( IPUtils::isValidRange( $userObj->getName() ) ) {
-					$blocks = $this->blockStore
-						->newListFromTarget( $userObj->getName(), $userObj->getName() );
+					$blocks = $this->blockStore->newListFromTarget(
+						$userObj->getName(), $userObj->getName(), false,
+						DatabaseBlockStore::AUTO_NONE );
 				} else {
-					$blocks = $this->blockStore->newListFromTarget( $userObj, $userObj );
+					$blocks = $this->blockStore->newListFromTarget(
+						$userObj, $userObj, false, DatabaseBlockStore::AUTO_NONE );
 				}
 
-				$numBlocks = 0;
 				$sitewide = false;
 				$logTargetPage = '';
 				foreach ( $blocks as $block ) {
-					if ( $block->getType() !== Block::TYPE_AUTO ) {
-						$numBlocks++;
-						if ( $block->isSitewide() ) {
-							$sitewide = true;
-						}
-						$logTargetPage = $this->namespaceInfo->getCanonicalName( NS_USER ) .
-							':' . $block->getTargetName();
+					if ( $block->isSitewide() ) {
+						$sitewide = true;
 					}
+					$logTargetPage = $this->namespaceInfo->getCanonicalName( NS_USER ) .
+						':' . $block->getTargetName();
 				}
 
-				if ( $numBlocks ) {
-					if ( $numBlocks === 1 ) {
+				if ( count( $blocks ) ) {
+					if ( count( $blocks ) === 1 ) {
 						if ( $userObj->isAnon() ) {
 							$msgKey = $sitewide ?
 								'sp-contributions-blocked-notice-anon' :
@@ -487,7 +485,7 @@ class ContributionsSpecialPage extends IncludableSpecialPage {
 							'msgKey' => [
 								$msgKey,
 								$userObj->getName(), # Support GENDER in 'sp-contributions-blocked-notice'
-								$numBlocks
+								count( $blocks )
 							],
 							'offset' => '', # don't use WebRequest parameter offset
 							'wrap' => Html::rawElement(
diff --git a/includes/specials/SpecialBlock.php b/includes/specials/SpecialBlock.php
index eb1efce26617..b2ba1aa079b0 100644
--- a/includes/specials/SpecialBlock.php
+++ b/includes/specials/SpecialBlock.php
@@ -658,11 +658,12 @@ class SpecialBlock extends FormSpecialPage {
 		// This won't be
 		$fields['PreviousTarget']['default'] = (string)$this->target;
 
-		$block = $this->blockStore->newFromTarget( $this->target );
+		$block = $this->blockStore->newFromTarget(
+			$this->target, null, false, DatabaseBlockStore::AUTO_NONE );
 
 		// Populate fields if there is a block that is not an autoblock; if it is a range
 		// block, only populate the fields if the range is the same as $this->target
-		if ( $block instanceof DatabaseBlock && $block->getType() !== DatabaseBlock::TYPE_AUTO
+		if ( $block instanceof DatabaseBlock
 			&& ( !( $this->target instanceof RangeBlockTarget )
 				|| $block->isBlocking( $this->target ) )
 		) {
diff --git a/tests/phpunit/integration/includes/block/DatabaseBlockStoreTest.php b/tests/phpunit/integration/includes/block/DatabaseBlockStoreTest.php
index a014a1dd8b5c..d55633800e81 100644
--- a/tests/phpunit/integration/includes/block/DatabaseBlockStoreTest.php
+++ b/tests/phpunit/integration/includes/block/DatabaseBlockStoreTest.php
@@ -317,6 +317,46 @@ class DatabaseBlockStoreTest extends MediaWikiIntegrationTestCase {
 		);
 	}
 
+	/**
+	 * @covers ::newListFromTarget
+	 * @covers ::newLoad
+	 */
+	public function testNewListFromTargetAuto() {
+		// Set up once and do multiple tests, faster than a provider
+		$store = $this->getStore();
+		$ip = '1.2.3.4';
+		$inserted = $store->insertBlockWithParams( [
+			'address' => $ip,
+			'by' => $this->getTestSysop()->getUser(),
+			'auto' => true,
+		] );
+
+		// ALL
+		$list = $store->newListFromTarget( $ip,
+			null, false, DatabaseBlockStore::AUTO_ALL );
+		$this->assertCount( 1, $list );
+
+		// Specified with IP
+		$list = $store->newListFromTarget( $ip,
+			null, false, DatabaseBlockStore::AUTO_SPECIFIED );
+		$this->assertCount( 0, $list );
+
+		// Specified autoblock ID
+		$list = $store->newListFromTarget( "#{$inserted->getId()}",
+			null, false, DatabaseBlockStore::AUTO_SPECIFIED );
+		$this->assertCount( 1, $list );
+
+		// None with IP
+		$list = $store->newListFromTarget( $ip,
+			null, false, DatabaseBlockStore::AUTO_NONE );
+		$this->assertCount( 0, $list );
+
+		// None with autoblock ID
+		$list = $store->newListFromTarget( "#{$inserted->getId()}",
+			null, false, DatabaseBlockStore::AUTO_NONE );
+		$this->assertCount( 0, $list );
+	}
+
 	public static function provideGetRangeCond() {
 		// $start, $end, $expect
 		$hex1 = IPUtils::toHex( '1.2.3.4' );