From: Thalia Date: Tue, 19 Mar 2019 18:56:10 +0000 (+0000) Subject: Add CompositeBlock class for enforcing multiple blocks X-Git-Tag: 1.34.0-rc.0~1439^2 X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin//%22%24encUrl/%22?a=commitdiff_plain;h=1eaf65d0a5f551216668a9e434ce323f0ba7dafe;p=lhc%2Fweb%2Fwiklou.git Add CompositeBlock class for enforcing multiple blocks Create a CompositeBlock class which extends AbstractBlock and adds the property $originalBlocks. This is for situations where more than one block applies to a user/IP, and avoids the need to choose just one of these blocks to enforce. Behaviour of the resulting block is determined by combining the strictest parameters of the original blocks. Also add DatabaseBlock::newListFromTarget, which is similar to DatabaseBlock::newFromTarget, but returns all relevant blocks, rather than choosing the most specific one. For tracking a CompositeBlock with a cookie, examine the original blocks and only track the first trackable block that is found. Bug: T206163 Change-Id: I088401105ac8ceb2c6117c6d2fcdb277c754d882 --- diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php index be240ca10b..60ae2f8677 100644 --- a/includes/block/BlockManager.php +++ b/includes/block/BlockManager.php @@ -110,7 +110,9 @@ class BlockManager { } /** - * Get the blocks that apply to a user and return the most relevant one. + * Get the blocks that apply to a user. If there is only one, return that, otherwise + * return a composite block that combines the strictest features of the applicable + * blocks. * * TODO: $user should be UserIdentity instead of User * @@ -143,29 +145,28 @@ class BlockManager { } // User/IP blocking + // After this, $blocks is an array of blocks or an empty array // TODO: remove dependency on DatabaseBlock - $block = DatabaseBlock::newFromTarget( $user, $ip, !$fromReplica ); + $blocks = DatabaseBlock::newListFromTarget( $user, $ip, !$fromReplica ); // Cookie blocking - if ( !$block instanceof AbstractBlock ) { - $block = $this->getBlockFromCookieValue( $user, $request ); + $cookieBlock = $this->getBlockFromCookieValue( $user, $request ); + if ( $cookieBlock instanceof AbstractBlock ) { + $blocks[] = $cookieBlock; } // Proxy blocking - if ( !$block instanceof AbstractBlock - && $ip !== null - && !in_array( $ip, $this->proxyWhitelist ) - ) { + if ( $ip !== null && !in_array( $ip, $this->proxyWhitelist ) ) { // Local list if ( $this->isLocallyBlockedProxy( $ip ) ) { - $block = new SystemBlock( [ + $blocks[] = new SystemBlock( [ 'byText' => wfMessage( 'proxyblocker' )->text(), 'reason' => wfMessage( 'proxyblockreason' )->plain(), 'address' => $ip, 'systemBlock' => 'proxy', ] ); } elseif ( $isAnon && $this->isDnsBlacklisted( $ip ) ) { - $block = new SystemBlock( [ + $blocks[] = new SystemBlock( [ 'byText' => wfMessage( 'sorbs' )->text(), 'reason' => wfMessage( 'sorbsreason' )->plain(), 'address' => $ip, @@ -175,8 +176,7 @@ class BlockManager { } // (T25343) Apply IP blocks to the contents of XFF headers, if enabled - if ( !$block instanceof AbstractBlock - && $this->applyIpBlocksToXff + if ( $this->applyIpBlocksToXff && $ip !== null && !in_array( $ip, $this->proxyWhitelist ) ) { @@ -185,21 +185,15 @@ class BlockManager { $xff = array_diff( $xff, [ $ip ] ); // TODO: remove dependency on DatabaseBlock $xffblocks = DatabaseBlock::getBlocksForIPList( $xff, $isAnon, !$fromReplica ); - // TODO: remove dependency on DatabaseBlock - $block = DatabaseBlock::chooseBlock( $xffblocks, $xff ); - if ( $block instanceof AbstractBlock ) { - # Mangle the reason to alert the user that the block - # originated from matching the X-Forwarded-For header. - $block->setReason( wfMessage( 'xffblockreason', $block->getReason() )->plain() ); - } + $blocks = array_merge( $blocks, $xffblocks ); } - if ( !$block instanceof AbstractBlock - && $ip !== null + // Soft blocking + if ( $ip !== null && $isAnon && IP::isInRanges( $ip, $this->softBlockRanges ) ) { - $block = new SystemBlock( [ + $blocks[] = new SystemBlock( [ 'address' => $ip, 'byText' => 'MediaWiki default', 'reason' => wfMessage( 'softblockrangesreason', $ip )->plain(), @@ -208,7 +202,19 @@ class BlockManager { ] ); } - return $block; + if ( count( $blocks ) > 0 ) { + if ( count( $blocks ) === 1 ) { + $block = $blocks[ 0 ]; + } else { + $block = new CompositeBlock( [ + 'address' => $ip, + 'originalBlocks' => $blocks, + ] ); + } + return $block; + } + + return null; } /** @@ -393,13 +399,23 @@ class BlockManager { public function trackBlockWithCookie( User $user ) { $block = $user->getBlock(); $request = $user->getRequest(); - - if ( - $block && - $request->getCookie( 'BlockID' ) === null && - $this->shouldTrackBlockWithCookie( $block, $user->isAnon() ) - ) { - $this->setBlockCookie( $block, $request->response() ); + $response = $request->response(); + $isAnon = $user->isAnon(); + + if ( $block && $request->getCookie( 'BlockID' ) === null ) { + if ( $block instanceof CompositeBlock ) { + // TODO: Improve on simply tracking the first trackable block (T225654) + foreach ( $block->getOriginalBlocks() as $originalBlock ) { + if ( $this->shouldTrackBlockWithCookie( $originalBlock, $isAnon ) ) { + $this->setBlockCookie( $originalBlock, $response ); + return; + } + } + } else { + if ( $this->shouldTrackBlockWithCookie( $block, $isAnon ) ) { + $this->setBlockCookie( $block, $response ); + } + } } } diff --git a/includes/block/CompositeBlock.php b/includes/block/CompositeBlock.php new file mode 100644 index 0000000000..fda15058b2 --- /dev/null +++ b/includes/block/CompositeBlock.php @@ -0,0 +1,164 @@ + [], + ]; + + $options += $defaults; + + $this->originalBlocks = $options[ 'originalBlocks' ]; + + $this->setHideName( $this->propHasValue( 'mHideName', true ) ); + $this->isSitewide( $this->propHasValue( 'isSitewide', true ) ); + $this->isEmailBlocked( $this->propHasValue( 'mBlockEmail', true ) ); + $this->isCreateAccountBlocked( $this->propHasValue( 'blockCreateAccount', true ) ); + $this->isUsertalkEditAllowed( !$this->propHasValue( 'allowUsertalk', false ) ); + } + + /** + * Determine whether any original blocks have a particular property set to a + * particular value. + * + * @param string $prop + * @param mixed $value + * @return bool At least one block has the property set to the value + */ + private function propHasValue( $prop, $value ) { + foreach ( $this->originalBlocks as $block ) { + if ( $block->$prop == $value ) { + return true; + } + } + return false; + } + + /** + * Determine whether any original blocks have a particular method returning a + * particular value. + * + * @param string $method + * @param mixed $value + * @param mixed ...$params + * @return bool At least one block has the method returning the value + */ + private function methodReturnsValue( $method, $value, ...$params ) { + foreach ( $this->originalBlocks as $block ) { + if ( $block->$method( ...$params ) == $value ) { + return true; + } + } + return false; + } + + /** + * Get the original blocks from which this block is composed + * + * @since 1.34 + * @return AbstractBlock[] + */ + public function getOriginalBlocks() { + return $this->originalBlocks; + } + + /** + * @inheritDoc + */ + public function getPermissionsError( IContextSource $context ) { + $params = $this->getBlockErrorParams( $context ); + + $msg = $this->isSitewide() ? 'blockedtext' : 'blockedtext-partial'; + + array_unshift( $params, $msg ); + + return $params; + } + + /** + * @inheritDoc + */ + public function appliesToRight( $right ) { + return $this->methodReturnsValue( __FUNCTION__, true, $right ); + } + + /** + * @inheritDoc + */ + public function appliesToUsertalk( Title $usertalk = null ) { + return $this->methodReturnsValue( __FUNCTION__, true, $usertalk ); + } + + /** + * @inheritDoc + */ + public function appliesToTitle( Title $title ) { + return $this->methodReturnsValue( __FUNCTION__, true, $title ); + } + + /** + * @inheritDoc + */ + public function appliesToNamespace( $ns ) { + return $this->methodReturnsValue( __FUNCTION__, true, $ns ); + } + + /** + * @inheritDoc + */ + public function appliesToPage( $pageId ) { + return $this->methodReturnsValue( __FUNCTION__, true, $pageId ); + } + + /** + * @inheritDoc + */ + public function appliesToPasswordReset() { + return $this->methodReturnsValue( __FUNCTION__, true ); + } + +} diff --git a/includes/block/DatabaseBlock.php b/includes/block/DatabaseBlock.php index 876a81f861..ba08d5461e 100644 --- a/includes/block/DatabaseBlock.php +++ b/includes/block/DatabaseBlock.php @@ -1159,26 +1159,40 @@ class DatabaseBlock extends AbstractBlock { * not be the same as the target you gave if you used $vagueTarget! */ public static function newFromTarget( $specificTarget, $vagueTarget = null, $fromMaster = false ) { + $blocks = self::newListFromTarget( $specificTarget, $vagueTarget, $fromMaster ); + return self::chooseMostSpecificBlock( $blocks ); + } + + /** + * This is similar to DatabaseBlock::newFromTarget, but it returns all the relevant blocks. + * + * @since 1.34 + * @param string|User|int|null $specificTarget + * @param string|User|int|null $vagueTarget + * @param bool $fromMaster + * @return DatabaseBlock[] Any relevant blocks + */ + public static function newListFromTarget( + $specificTarget, + $vagueTarget = null, + $fromMaster = false + ) { list( $target, $type ) = self::parseTarget( $specificTarget ); if ( $type == self::TYPE_ID || $type == self::TYPE_AUTO ) { - return self::newFromID( $target ); - + $block = self::newFromID( $target ); + return $block ? [ $block ] : []; } elseif ( $target === null && $vagueTarget == '' ) { # We're not going to find anything useful here # Be aware that the == '' check is explicit, since empty values will be # passed by some callers (T31116) - return null; - + return []; } elseif ( in_array( $type, [ self::TYPE_USER, self::TYPE_IP, self::TYPE_RANGE, null ] ) ) { - $blocks = self::newLoad( $target, $type, $fromMaster, $vagueTarget ); - if ( !empty( $blocks ) ) { - return self::chooseMostSpecificBlock( $blocks ); - } + return self::newLoad( $target, $type, $fromMaster, $vagueTarget ); } - return null; + return []; } /** diff --git a/includes/user/User.php b/includes/user/User.php index bdcb17b9b3..e5dfcebe5e 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -1822,8 +1822,7 @@ class User implements IDBAccessObject, UserIdentity { $fromReplica ); - if ( $block instanceof AbstractBlock ) { - wfDebug( __METHOD__ . ": Found block.\n" ); + if ( $block ) { $this->mBlock = $block; $this->mBlockedby = $block->getByName(); $this->mBlockreason = $block->getReason(); diff --git a/tests/phpunit/includes/block/CompositeBlockTest.php b/tests/phpunit/includes/block/CompositeBlockTest.php new file mode 100644 index 0000000000..5cd86b80b4 --- /dev/null +++ b/tests/phpunit/includes/block/CompositeBlockTest.php @@ -0,0 +1,254 @@ +getTestSysop()->getUser()->getId(); + + $userBlock = new Block( [ + 'address' => $this->getTestUser()->getUser(), + 'by' => $sysopId, + 'sitewide' => false, + ] ); + $ipBlock = new Block( [ + 'address' => '127.0.0.1', + 'by' => $sysopId, + 'sitewide' => false, + ] ); + + $userBlock->insert(); + $ipBlock->insert(); + + return [ + 'user' => $userBlock, + 'ip' => $ipBlock, + ]; + } + + private function deleteBlocks( $blocks ) { + foreach ( $blocks as $block ) { + $block->delete(); + } + } + + /** + * @covers ::__construct + * @dataProvider provideTestStrictestParametersApplied + */ + public function testStrictestParametersApplied( $blocks, $expected ) { + $this->setMwGlobals( [ + 'wgBlockDisablesLogin' => false, + 'wgBlockAllowsUTEdit' => true, + ] ); + + $block = new CompositeBlock( [ + 'originalBlocks' => $blocks, + ] ); + + $this->assertSame( $expected[ 'hideName' ], $block->getHideName() ); + $this->assertSame( $expected[ 'sitewide' ], $block->isSitewide() ); + $this->assertSame( $expected[ 'blockEmail' ], $block->isEmailBlocked() ); + $this->assertSame( $expected[ 'allowUsertalk' ], $block->isUsertalkEditAllowed() ); + } + + public static function provideTestStrictestParametersApplied() { + return [ + 'Sitewide block and partial block' => [ + [ + new Block( [ + 'sitewide' => false, + 'blockEmail' => true, + 'allowUsertalk' => true, + ] ), + new Block( [ + 'sitewide' => true, + 'blockEmail' => false, + 'allowUsertalk' => false, + ] ), + ], + [ + 'hideName' => false, + 'sitewide' => true, + 'blockEmail' => true, + 'allowUsertalk' => false, + ], + ], + 'Partial block and system block' => [ + [ + new Block( [ + 'sitewide' => false, + 'blockEmail' => true, + 'allowUsertalk' => false, + ] ), + new SystemBlock( [ + 'systemBlock' => 'proxy', + ] ), + ], + [ + 'hideName' => false, + 'sitewide' => true, + 'blockEmail' => true, + 'allowUsertalk' => false, + ], + ], + 'System block and user name hiding block' => [ + [ + new Block( [ + 'hideName' => true, + 'sitewide' => true, + 'blockEmail' => true, + 'allowUsertalk' => false, + ] ), + new SystemBlock( [ + 'systemBlock' => 'proxy', + ] ), + ], + [ + 'hideName' => true, + 'sitewide' => true, + 'blockEmail' => true, + 'allowUsertalk' => false, + ], + ], + 'Two lenient partial blocks' => [ + [ + new Block( [ + 'sitewide' => false, + 'blockEmail' => false, + 'allowUsertalk' => true, + ] ), + new Block( [ + 'sitewide' => false, + 'blockEmail' => false, + 'allowUsertalk' => true, + ] ), + ], + [ + 'hideName' => false, + 'sitewide' => false, + 'blockEmail' => false, + 'allowUsertalk' => true, + ], + ], + ]; + } + + /** + * @covers ::appliesToTitle + */ + public function testBlockAppliesToTitle() { + $this->setMwGlobals( [ + 'wgBlockDisablesLogin' => false, + ] ); + + $blocks = $this->getPartialBlocks(); + + $block = new CompositeBlock( [ + 'originalBlocks' => $blocks, + ] ); + + $pageFoo = $this->getExistingTestPage( 'Foo' ); + $pageBar = $this->getExistingTestPage( 'User:Bar' ); + + $this->getBlockRestrictionStore()->insert( [ + new PageRestriction( $blocks[ 'user' ]->getId(), $pageFoo->getId() ), + new NamespaceRestriction( $blocks[ 'ip' ]->getId(), NS_USER ), + ] ); + + $this->assertTrue( $block->appliesToTitle( $pageFoo->getTitle() ) ); + $this->assertTrue( $block->appliesToTitle( $pageBar->getTitle() ) ); + + $this->deleteBlocks( $blocks ); + } + + /** + * @covers ::appliesToUsertalk + * @covers ::appliesToPage + * @covers ::appliesToNamespace + */ + public function testBlockAppliesToUsertalk() { + $this->setMwGlobals( [ + 'wgBlockAllowsUTEdit' => true, + 'wgBlockDisablesLogin' => false, + ] ); + + $blocks = $this->getPartialBlocks(); + + $block = new CompositeBlock( [ + 'originalBlocks' => $blocks, + ] ); + + $title = $blocks[ 'user' ]->getTarget()->getTalkPage(); + $page = $this->getExistingTestPage( 'User talk:' . $title->getText() ); + + $this->getBlockRestrictionStore()->insert( [ + new PageRestriction( $blocks[ 'user' ]->getId(), $page->getId() ), + new NamespaceRestriction( $blocks[ 'ip' ]->getId(), NS_USER ), + ] ); + + $this->assertTrue( $block->appliesToUsertalk( $blocks[ 'user' ]->getTarget()->getTalkPage() ) ); + + $this->deleteBlocks( $blocks ); + } + + /** + * @covers ::appliesToRight + * @dataProvider provideTestBlockAppliesToRight + */ + public function testBlockAppliesToRight( $blocks, $right, $expected ) { + $this->setMwGlobals( [ + 'wgBlockDisablesLogin' => false, + ] ); + + $block = new CompositeBlock( [ + 'originalBlocks' => $blocks, + ] ); + + $this->assertSame( $block->appliesToRight( $right ), $expected ); + } + + public static function provideTestBlockAppliesToRight() { + return [ + 'Read is not blocked' => [ + [ + new Block(), + new Block(), + ], + 'read', + false, + ], + 'Email is blocked if blocked by any blocks' => [ + [ + new Block( [ + 'blockEmail' => true, + ] ), + new Block( [ + 'blockEmail' => false, + ] ), + ], + 'sendemail', + true, + ], + ]; + } + + /** + * Get an instance of BlockRestrictionStore + * + * @return BlockRestrictionStore + */ + protected function getBlockRestrictionStore() : BlockRestrictionStore { + return MediaWikiServices::getInstance()->getBlockRestrictionStore(); + } +} diff --git a/tests/phpunit/includes/user/PasswordResetTest.php b/tests/phpunit/includes/user/PasswordResetTest.php index 55a29e364e..b0c0fec6b2 100644 --- a/tests/phpunit/includes/user/PasswordResetTest.php +++ b/tests/phpunit/includes/user/PasswordResetTest.php @@ -2,6 +2,7 @@ use MediaWiki\Auth\AuthManager; use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Block\CompositeBlock; use MediaWiki\Block\SystemBlock; /** @@ -141,6 +142,34 @@ class PasswordResetTest extends MediaWikiTestCase { 'globalBlock' => null, 'isAllowed' => false, ], + 'blocked with multiple blocks, all allowing password reset' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => new CompositeBlock( [ + 'originalBlocks' => [ + new SystemBlock( [ 'systemBlock' => 'wgSoftBlockRanges', 'anonOnly' => true ] ), + new Block( [] ), + ] + ] ), + 'globalBlock' => null, + 'isAllowed' => true, + ], + 'blocked with multiple blocks, not all allowing password reset' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => new CompositeBlock( [ + 'originalBlocks' => [ + new SystemBlock( [ 'systemBlock' => 'wgSoftBlockRanges', 'anonOnly' => true ] ), + new SystemBlock( [ 'systemBlock' => 'proxy' ] ), + ] + ] ), + 'globalBlock' => null, + 'isAllowed' => false, + ], 'all OK' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 14ddd9f7b6..79c6e966af 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -4,6 +4,7 @@ define( 'NS_UNITTEST', 5600 ); define( 'NS_UNITTEST_TALK', 5601 ); use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Block\CompositeBlock; use MediaWiki\Block\Restriction\PageRestriction; use MediaWiki\Block\Restriction\NamespaceRestriction; use MediaWiki\Block\SystemBlock; @@ -66,6 +67,15 @@ class UserTest extends MediaWikiTestCase { ]; } + private function setSessionUser( User $user, WebRequest $request ) { + $this->setMwGlobals( 'wgUser', $user ); + RequestContext::getMain()->setUser( $user ); + RequestContext::getMain()->setRequest( $request ); + TestingAccessWrapper::newFromObject( $user )->mRequest = $request; + $request->getSession()->setUser( $user ); + $this->overrideMwServices(); + } + /** * @covers User::getGroupPermissions */ @@ -779,28 +789,20 @@ class UserTest extends MediaWikiTestCase { * @covers User::getBlockedStatus */ public function testSoftBlockRanges() { - $setSessionUser = function ( User $user, WebRequest $request ) { - $this->setMwGlobals( 'wgUser', $user ); - RequestContext::getMain()->setUser( $user ); - RequestContext::getMain()->setRequest( $request ); - TestingAccessWrapper::newFromObject( $user )->mRequest = $request; - $request->getSession()->setUser( $user ); - $this->overrideMwServices(); - }; $this->setMwGlobals( 'wgSoftBlockRanges', [ '10.0.0.0/8' ] ); // IP isn't in $wgSoftBlockRanges $wgUser = new User(); $request = new FauxRequest(); $request->setIP( '192.168.0.1' ); - $setSessionUser( $wgUser, $request ); + $this->setSessionUser( $wgUser, $request ); $this->assertNull( $wgUser->getBlock() ); // IP is in $wgSoftBlockRanges $wgUser = new User(); $request = new FauxRequest(); $request->setIP( '10.20.30.40' ); - $setSessionUser( $wgUser, $request ); + $this->setSessionUser( $wgUser, $request ); $block = $wgUser->getBlock(); $this->assertInstanceOf( SystemBlock::class, $block ); $this->assertSame( 'wgSoftBlockRanges', $block->getSystemBlockType() ); @@ -809,7 +811,7 @@ class UserTest extends MediaWikiTestCase { $wgUser = $this->getTestUser()->getUser(); $request = new FauxRequest(); $request->setIP( '10.20.30.40' ); - $setSessionUser( $wgUser, $request ); + $this->setSessionUser( $wgUser, $request ); $this->assertFalse( $wgUser->isAnon(), 'sanity check' ); $this->assertNull( $wgUser->getBlock() ); } @@ -1316,6 +1318,35 @@ class UserTest extends MediaWikiTestCase { $this->assertFalse( $user->isBlockedFrom( $ut ) ); } + /** + * @covers User::getBlockedStatus + */ + public function testCompositeBlocks() { + $user = $this->getMutableTestUser()->getUser(); + $request = $user->getRequest(); + $this->setSessionUser( $user, $request ); + + $ipBlock = new Block( [ + 'address' => $user->getRequest()->getIP(), + 'by' => $this->getTestSysop()->getUser()->getId(), + 'createAccount' => true, + ] ); + $ipBlock->insert(); + + $userBlock = new Block( [ + 'address' => $user, + 'by' => $this->getTestSysop()->getUser()->getId(), + 'createAccount' => false, + ] ); + $userBlock->insert(); + + $block = $user->getBlock(); + $this->assertInstanceOf( CompositeBlock::class, $block ); + $this->assertTrue( $block->isCreateAccountBlocked() ); + $this->assertTrue( $block->appliesToPasswordReset() ); + $this->assertTrue( $block->appliesToNamespace( NS_MAIN ) ); + } + /** * @covers User::isBlockedFrom * @dataProvider provideIsBlockedFrom