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

diff --git a/includes/Block.php b/includes/Block.php
index 39f2b29..eb8214b 100644 (file)
--- 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 0bea7e3..bfa7266 100644 (file)
--- 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 9954425..7d844b5 100644 (file)
--- 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 55b3bfc..f1c049b 100644 (file)
--- 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 = '<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