From a387a375a10e02fe51dda09a288abc5b031b5744 Mon Sep 17 00:00:00 2001 From: Thalia Date: Fri, 24 May 2019 14:02:32 +0100 Subject: [PATCH] Decouple DatabaseBlock::newFromTarget from DatabaseBlock::newLoad Before this, DatabaseBlock:newFromTarget initialises a new block and calls DatabaseBlock::newLoad on that instance, passing through the target and type via that instance. However, newLoad returns a brand new block instance. This patch makes newLoad into a static method, with the target and type passed as method parameters. It also separates the block-choosing logic in newLoad into a separate method, DatabaseBlock::chooseMostSpecificBlock. Doing this (1) makes it more transparent that Block uses two different ways to choose a block (see also Block::chooseBlock), and (2) makes it possible to re-use newLoad to get multiple blocks. Also, filter out any duplicate autoblocks that are found by newLoad. Bug: T206163 Change-Id: Iefa3aaadf2954c3b86f5c691096af31de40fae6c --- includes/block/DatabaseBlock.php | 103 +++++++++++++++++++++---------- 1 file changed, 69 insertions(+), 34 deletions(-) diff --git a/includes/block/DatabaseBlock.php b/includes/block/DatabaseBlock.php index 6d9dd13c21..df9eebefec 100644 --- a/includes/block/DatabaseBlock.php +++ b/includes/block/DatabaseBlock.php @@ -264,21 +264,28 @@ class DatabaseBlock extends AbstractBlock { } /** - * Load a block from the database which affects the already-set $this->target: - * 1) A block directly on the given user or IP - * 2) A rangeblock encompassing the given IP (smallest first) - * 3) An autoblock on the given IP + * Load blocks from the database which target the specific target exactly, or which cover the + * vague target. + * + * @param User|String|null $specificTarget + * @param int|null $specificType + * @param bool $fromMaster * @param User|string|null $vagueTarget Also search for blocks affecting this target. Doesn't * make any sense to use TYPE_AUTO / TYPE_ID here. Leave blank to skip IP lookups. * @throws MWException - * @return bool Whether a relevant block was found + * @return DatabaseBlock[] Any relevant blocks */ - protected function newLoad( $vagueTarget = null ) { - $db = wfGetDB( $this->mFromMaster ? DB_MASTER : DB_REPLICA ); - - if ( $this->type !== null ) { + protected static function newLoad( + $specificTarget, + $specificType, + $fromMaster, + $vagueTarget = null + ) { + $db = wfGetDB( $fromMaster ? DB_MASTER : DB_REPLICA ); + + if ( $specificType !== null ) { $conds = [ - 'ipb_address' => [ (string)$this->target ], + 'ipb_address' => [ (string)$specificTarget ], ]; } else { $conds = [ 'ipb_address' => [] ]; @@ -317,13 +324,9 @@ class DatabaseBlock extends AbstractBlock { $blockQuery['tables'], $blockQuery['fields'], $conds, __METHOD__, [], $blockQuery['joins'] ); - # This result could contain a block on the user, a block on the IP, and a russian-doll - # set of rangeblocks. We want to choose the most specific one, so keep a leader board. - $bestRow = null; - - # Lower will be better - $bestBlockScore = 100; - + $blocks = []; + $blockIds = []; + $autoBlocks = []; foreach ( $res as $row ) { $block = self::newFromRow( $row ); @@ -333,10 +336,53 @@ class DatabaseBlock extends AbstractBlock { } # Don't use anon only blocks on users - if ( $this->type == self::TYPE_USER && !$block->isHardblock() ) { + if ( $specificType == self::TYPE_USER && !$block->isHardblock() ) { continue; } + // Check for duplicate autoblocks + if ( $block->getType() === self::TYPE_AUTO ) { + $autoBlocks[] = $block; + } else { + $blocks[] = $block; + $blockIds[] = $block->getId(); + } + } + + // Only add autoblocks that aren't duplicates + foreach ( $autoBlocks as $block ) { + if ( !in_array( $block->mParentBlockId, $blockIds ) ) { + $blocks[] = $block; + } + } + + return $blocks; + } + + /** + * Choose the most specific block from some combination of user, IP and IP range + * blocks. Decreasing order of specificity: user > IP > narrower IP range > wider IP + * range. A range that encompasses one IP address is ranked equally to a singe IP. + * + * Note that DatabaseBlock::chooseBlocks chooses blocks in a different way. + * + * This is refactored out from DatabaseBlock::newLoad. + * + * @param DatabaseBlock[] $blocks These should not include autoblocks or ID blocks + * @return DatabaseBlock|null The block with the most specific target + */ + protected static function chooseMostSpecificBlock( $blocks ) { + if ( count( $blocks ) === 1 ) { + return $blocks[0]; + } + + # This result could contain a block on the user, a block on the IP, and a russian-doll + # set of rangeblocks. We want to choose the most specific one, so keep a leader board. + $bestBlock = null; + + # Lower will be better + $bestBlockScore = 100; + foreach ( $blocks as $block ) { if ( $block->getType() == self::TYPE_RANGE ) { # This is the number of bits that are allowed to vary in the block, give # or take some floating point errors @@ -354,16 +400,11 @@ class DatabaseBlock extends AbstractBlock { if ( $score < $bestBlockScore ) { $bestBlockScore = $score; - $bestRow = $row; + $bestBlock = $block; } } - if ( $bestRow !== null ) { - $this->initFromRow( $bestRow ); - return true; - } else { - return false; - } + return $bestBlock; } /** @@ -1134,15 +1175,9 @@ class DatabaseBlock extends AbstractBlock { $type, [ self::TYPE_USER, self::TYPE_IP, self::TYPE_RANGE, null ] ) ) { - $block = new DatabaseBlock(); - $block->fromMaster( $fromMaster ); - - if ( $type !== null ) { - $block->setTarget( $target ); - } - - if ( $block->newLoad( $vagueTarget ) ) { - return $block; + $blocks = self::newLoad( $target, $type, $fromMaster, $vagueTarget ); + if ( !empty( $blocks ) ) { + return self::chooseMostSpecificBlock( $blocks ); } } return null; -- 2.20.1