From 74ff87d291e6daddfd791270c6ee95ca587d3d46 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 1 Nov 2018 10:11:03 -0400 Subject: [PATCH] Block: Clean up handling of non-User targets The fix applied in d67121f6d took care of the immediate issue in T208398, but after further analysis it was not a correct fix. * Near line 770, the method shouldn't even be called unless the target is TYPE_USER. * Near line 1598, it isn't dealing with a target at all. * Near line 1813, you're not going to get a sensible result trying to call `$user->getTalkPage()` for a range or auto-block ID. What you would really need there to handle range and auto-blocks correctly is to pass in the User actually making the edit. But after some pushback in code review about passing the User into Block::preventsEdit() to make line 1813 work, we'll instead replace the method with Block::appliesToTitle() and put the check for user talk pages back into User::isBlockedFrom(). Bug: T208398 Bug: T208472 Change-Id: I23d3a3a1925e97f0cabe328c1cc74e978cb4d24a --- includes/Block.php | 50 ++++++--------- includes/user/User.php | 13 +++- tests/phpunit/includes/BlockTest.php | 80 ++++------------------- tests/phpunit/includes/user/UserTest.php | 81 ++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 101 deletions(-) diff --git a/includes/Block.php b/includes/Block.php index 39f2b2990b..eb8214bae1 100644 --- a/includes/Block.php +++ b/includes/Block.php @@ -766,11 +766,11 @@ class Block { return; } - $target = $block->getTarget(); - // FIXME: Push this into getTargetActor() or whatever to reuse - if ( is_string( $target ) ) { - $target = User::newFromName( $target, false ); + // Autoblocks only apply to TYPE_USER + if ( $block->getType() !== self::TYPE_USER ) { + return; } + $target = $block->getTarget(); // TYPE_USER => always a User object $dbr = wfGetDB( DB_REPLICA ); $rcQuery = ActorMigration::newMigration()->getWhere( $dbr, 'rc_user', $target, false ); @@ -1595,7 +1595,6 @@ class Block { * @param User|string $user Local User object or username string */ public function setBlocker( $user ) { - // FIXME: Push this into getTargetActor() or whatever to reuse if ( is_string( $user ) ) { $user = User::newFromName( $user, false ); } @@ -1796,39 +1795,28 @@ class Block { } /** - * Checks if a block prevents an edit on a given article + * Checks if a block applies to a particular title + * + * This check does not consider whether `$this->prevents( 'editownusertalk' )` + * returns false, as the identity of the user making the hypothetical edit + * isn't known here (particularly in the case of IP hardblocks, range + * blocks, and auto-blocks). * - * @param \Title $title + * @param Title $title * @return bool */ - public function preventsEdit( \Title $title ) { - $blocked = $this->isSitewide(); - - // user talk page has its own rules - // This check happens before partial blocks because the flag - // to allow user to edit their user talk page could be - // overwritten by a partial block restriction (E.g. user talk namespace) - $user = $this->getTarget(); - - // Not all blocked `$user`s are self::TYPE_USER - // FIXME: Push this into getTargetActor() or whatever to reuse - if ( is_string( $user ) ) { - $user = User::newFromName( $user, false ); - } - - if ( $title->equals( $user->getTalkPage() ) ) { - $blocked = $this->prevents( 'editownusertalk' ); + public function appliesToTitle( Title $title ) { + if ( $this->isSitewide() ) { + return true; } - if ( !$this->isSitewide() ) { - $restrictions = $this->getRestrictions(); - foreach ( $restrictions as $restriction ) { - if ( $restriction->matches( $title ) ) { - $blocked = true; - } + $restrictions = $this->getRestrictions(); + foreach ( $restrictions as $restriction ) { + if ( $restriction->matches( $title ) ) { + return true; } } - return $blocked; + return false; } } diff --git a/includes/user/User.php b/includes/user/User.php index 0bea7e3cce..bfa7266203 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -2304,7 +2304,18 @@ class User implements IDBAccessObject, UserIdentity { if ( !$blocked ) { $block = $this->getBlock( $fromSlave ); if ( $block ) { - $blocked = $block->preventsEdit( $title ); + // A sitewide block covers everything except maybe the user's + // talk page. A partial block covering the user's talk page + // overrides the editownusertalk flag, however. + // TODO: Do we really want a partial block on the user talk + // namespace to ignore editownusertalk? + $blocked = $block->isSitewide(); + if ( $blocked && $title->equals( $this->getTalkPage() ) ) { + $blocked = $block->prevents( 'editownusertalk' ); + } + if ( !$block->isSitewide() ) { + $blocked = $block->appliesToTitle( $title ); + } } } diff --git a/tests/phpunit/includes/BlockTest.php b/tests/phpunit/includes/BlockTest.php index 9954425651..7d844b5869 100644 --- a/tests/phpunit/includes/BlockTest.php +++ b/tests/phpunit/includes/BlockTest.php @@ -612,9 +612,9 @@ class BlockTest extends MediaWikiLangTestCase { } /** - * @covers Block::preventsEdit + * @covers Block::appliesToTitle */ - public function testPreventsEditReturnsTrueOnSitewideBlock() { + public function testAppliesToTitleReturnsTrueOnSitewideBlock() { $user = $this->getTestUser()->getUser(); $block = new Block( [ 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), @@ -628,15 +628,19 @@ class BlockTest extends MediaWikiLangTestCase { $title = $this->getExistingTestPage( 'Foo' )->getTitle(); - $this->assertTrue( $block->preventsEdit( $title ) ); + $this->assertTrue( $block->appliesToTitle( $title ) ); + + // appliesToTitle() ignores allowUsertalk + $title = $user->getTalkPage(); + $this->assertTrue( $block->appliesToTitle( $title ) ); $block->delete(); } /** - * @covers Block::preventsEdit + * @covers Block::appliesToTitle */ - public function testPreventsEditOnPartialBlock() { + public function testAppliesToTitleOnPartialBlock() { $user = $this->getTestUser()->getUser(); $block = new Block( [ 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), @@ -654,72 +658,10 @@ class BlockTest extends MediaWikiLangTestCase { $pageRestriction = new PageRestriction( $block->getId(), $pageFoo->getId() ); BlockRestriction::insert( [ $pageRestriction ] ); - $this->assertTrue( $block->preventsEdit( $pageFoo->getTitle() ) ); - $this->assertFalse( $block->preventsEdit( $pageBar->getTitle() ) ); - - $block->delete(); - } - - /** - * @covers Block::preventsEdit - * @dataProvider preventsEditOnUserTalkProvider - */ - public function testPreventsEditOnUserTalkPage( - $allowUsertalk, $sitewide, $result, $blockAllowsUTEdit = true - ) { - $this->setMwGlobals( [ - 'wgBlockAllowsUTEdit' => $blockAllowsUTEdit, - ] ); - - $user = $this->getTestUser()->getUser(); - $block = new Block( [ - 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), - 'allowUsertalk' => $allowUsertalk, - 'sitewide' => $sitewide - ] ); - - $block->setTarget( $user ); - $block->setBlocker( $this->getTestSysop()->getUser() ); - $block->insert(); + $this->assertTrue( $block->appliesToTitle( $pageFoo->getTitle() ) ); + $this->assertFalse( $block->appliesToTitle( $pageBar->getTitle() ) ); - $this->assertEquals( $result, $block->preventsEdit( $user->getTalkPage() ) ); $block->delete(); } - public function preventsEditOnUserTalkProvider() { - return [ - [ - 'allowUsertalk' => false, - 'sitewide' => true, - 'result' => true, - ], - [ - 'allowUsertalk' => true, - 'sitewide' => true, - 'result' => false, - ], - [ - 'allowUsertalk' => true, - 'sitewide' => false, - 'result' => false, - ], - [ - 'allowUsertalk' => false, - 'sitewide' => false, - 'result' => true, - ], - [ - 'allowUsertalk' => true, - 'sitewide' => true, - 'result' => true, - 'blockAllowsUTEdit' => false - ], - [ - 'allowUsertalk' => true, - 'sitewide' => false, - 'result' => true, - 'blockAllowsUTEdit' => false - ], - ]; - } } diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 55b3bfc261..f1c049b86b 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -3,6 +3,7 @@ define( 'NS_UNITTEST', 5600 ); define( 'NS_UNITTEST_TALK', 5601 ); +use MediaWiki\Block\Restriction\PageRestriction; use MediaWiki\MediaWikiServices; use MediaWiki\User\UserIdentityValue; use Wikimedia\TestingAccessWrapper; @@ -11,6 +12,10 @@ use Wikimedia\TestingAccessWrapper; * @group Database */ class UserTest extends MediaWikiTestCase { + + /** Constant for self::testIsBlockedFrom */ + const USER_TALK_PAGE = ''; + /** * @var User */ @@ -1209,6 +1214,82 @@ class UserTest extends MediaWikiTestCase { $this->assertFalse( $user->isBlockedFrom( $ut ) ); } + /** + * @covers User::isBlockedFrom + * @dataProvider provideIsBlockedFrom + * @param string|null $title Title to test. + * @param bool $expect Expected result from User::isBlockedFrom() + * @param array $options Additional test options: + * - 'blockAllowsUTEdit': (bool, default true) Value for $wgBlockAllowsUTEdit + * - 'allowUsertalk': (bool, default false) Passed to Block::__construct() + * - 'pageRestrictions': (array|null) If non-empty, page restriction titles for the block. + */ + public function testIsBlockedFrom( $title, $expect, array $options = [] ) { + $this->setMwGlobals( [ + 'wgBlockAllowsUTEdit' => $options['blockAllowsUTEdit'] ?? true, + ] ); + + $user = $this->getTestUser()->getUser(); + + if ( $title === self::USER_TALK_PAGE ) { + $title = $user->getTalkPage(); + } else { + $title = Title::newFromText( $title ); + } + + $restrictions = []; + foreach ( $options['pageRestrictions'] ?? [] as $pagestr ) { + $page = $this->getExistingTestPage( + $pagestr === self::USER_TALK_PAGE ? $user->getTalkPage() : $pagestr + ); + $restrictions[] = new PageRestriction( 0, $page->getId() ); + } + + $block = new Block( [ + 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), + 'allowUsertalk' => $options['allowUsertalk'] ?? false, + 'sitewide' => !$restrictions, + ] ); + $block->setTarget( $user ); + $block->setBlocker( $this->getTestSysop()->getUser() ); + if ( $restrictions ) { + $block->setRestrictions( $restrictions ); + } + $block->insert(); + + try { + $this->assertSame( $expect, $user->isBlockedFrom( $title ) ); + } finally { + $block->delete(); + } + } + + public static function provideIsBlockedFrom() { + return [ + 'Basic operation' => [ 'Test page', true ], + 'User talk page, not allowed' => [ self::USER_TALK_PAGE, true, [ + 'allowUsertalk' => false, + ] ], + 'User talk page, allowed' => [ self::USER_TALK_PAGE, false, [ + 'allowUsertalk' => true, + ] ], + 'User talk page, allowed but $wgBlockAllowsUTEdit is false' => [ self::USER_TALK_PAGE, true, [ + 'allowUsertalk' => true, + 'blockAllowsUTEdit' => false, + ] ], + + 'Partial block, blocking the page' => [ 'Test page', true, [ + 'pageRestrictions' => [ 'Test page' ], + ] ], + 'Partial block, not blocking the page' => [ 'Test page 2', false, [ + 'pageRestrictions' => [ 'Test page' ], + ] ], + 'Partial block, overriding allowUsertalk' => [ self::USER_TALK_PAGE, true, [ + 'pageRestrictions' => [ self::USER_TALK_PAGE ], + ] ], + ]; + } + /** * Block cookie should be set for IP Blocks if * wgCookieSetOnIpBlock is set to true -- 2.20.1