Defer cookie block checks to resolve a circular dependency

User needs to load user preferences to get preferred language, which
calls User::load() which calls User::loadFromSession().

User::loadFromSession() tries to load blocks which might use messages
which need user language which calls User::load() because the previous
call to it haven't completed yet...

We have a protection against infinite recursion to prevent this from
completely crashing MW, however this patch fixes the main issue: loading
too much stuff when a User is initialized.

Bug: T180050
Change-Id: I63af6d2239b36124d5ed382b8d2aab82c8d54d69

diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
index abd2db2..4e61369 100644 (file)
--- a/includes/block/BlockManager.php
+++ b/includes/block/BlockManager.php
@@ -21,6 +21,7 @@
 namespace MediaWiki\Block;
 
 use DateTime;
+use DeferredUpdates;
 use IP;
 use MediaWiki\User\UserIdentity;
 use MWCryptHash;
@@ -431,26 +432,38 @@ class BlockManager {
         * @param User $user
         */
        public function trackBlockWithCookie( User $user ) {
-               $block = $user->getBlock();
                $request = $user->getRequest();
-               $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;
+               if ( $request->getCookie( 'BlockID' ) !== null ) {
+                       // User already has a block cookie
+                       return;
+               }
+
+               // Defer checks until the user has been fully loaded to avoid circular dependency
+               // of User on itself
+               DeferredUpdates::addCallableUpdate(
+                       function () use ( $user, $request ) {
+                               $block = $user->getBlock();
+                               $response = $request->response();
+                               $isAnon = $user->isAnon();
+
+                               if ( $block ) {
+                                       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 );
+                                               }
                                        }
                                }
-                       } else {
-                               if ( $this->shouldTrackBlockWithCookie( $block, $isAnon ) ) {
-                                       $this->setBlockCookie( $block, $response );
-                               }
-                       }
-               }
+                       },
+                       DeferredUpdates::PRESEND
+               );
        }
 
        /**

diff --git a/includes/user/User.php b/includes/user/User.php
index 6025d3c..f91f438 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1352,12 +1352,11 @@ class User implements IDBAccessObject, UserIdentity {
                $user = $session->getUser();
                if ( $user->isLoggedIn() ) {
                        $this->loadFromUserObject( $user );
-                       if ( $user->getBlock() ) {
-                               // If this user is autoblocked, set a cookie to track the block. This has to be done on
-                               // every session load, because an autoblocked editor might not edit again from the same
-                               // IP address after being blocked.
-                               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $this );
-                       }
+
+                       // If this user is autoblocked, set a cookie to track the block. This has to be done on
+                       // every session load, because an autoblocked editor might not edit again from the same
+                       // IP address after being blocked.
+                       MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $this );
 
                        // Other code expects these to be set in the session, so set them.
                        $session->set( 'wsUserID', $this->getId() );

diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
index 40fe4c8..892add9 100644 (file)
--- a/tests/phpunit/includes/block/BlockManagerTest.php
+++ b/tests/phpunit/includes/block/BlockManagerTest.php
@@ -323,4 +323,62 @@ class BlockManagerTest extends MediaWikiTestCase {
 
                $this->assertSame( 2, count( $method->invoke( $blockManager, $blocks ) ) );
        }
+
+       /**
+        * @covers ::trackBlockWithCookie
+        * @dataProvider provideTrackBlockWithCookie
+        * @param bool $expectCookieSet
+        * @param bool $hasCookie
+        * @param bool $isBlocked
+        */
+       public function testTrackBlockWithCookie( $expectCookieSet, $hasCookie, $isBlocked ) {
+               $blockID = 123;
+               $this->setMwGlobals( 'wgCookiePrefix', '' );
+
+               $request = new FauxRequest();
+               if ( $hasCookie ) {
+                       $request->setCookie( 'BlockID', 'the value does not matter' );
+               }
+
+               if ( $isBlocked ) {
+                       $block = $this->getMockBuilder( DatabaseBlock::class )
+                               ->setMethods( [ 'getType', 'getId' ] )
+                               ->getMock();
+                       $block->method( 'getType' )
+                               ->willReturn( DatabaseBlock::TYPE_IP );
+                       $block->method( 'getId' )
+                               ->willReturn( $blockID );
+               } else {
+                       $block = null;
+               }
+
+               $user = $this->getMockBuilder( User::class )
+                       ->setMethods( [ 'getBlock', 'getRequest' ] )
+                       ->getMock();
+               $user->method( 'getBlock' )
+                       ->willReturn( $block );
+               $user->method( 'getRequest' )
+                       ->willReturn( $request );
+               /** @var User $user */
+
+               // Although the block cookie is set via DeferredUpdates, in command line mode updates are
+               // processed immediately
+               $blockManager = $this->getBlockManager( [] );
+               $blockManager->trackBlockWithCookie( $user );
+
+               /** @var FauxResponse $response */
+               $response = $request->response();
+               $this->assertCount( $expectCookieSet ? 1 : 0, $response->getCookies() );
+               $this->assertEquals( $expectCookieSet ? $blockID : null, $response->getCookie( 'BlockID' ) );
+       }
+
+       public function provideTrackBlockWithCookie() {
+               return [
+                       // $expectCookieSet, $hasCookie, $isBlocked
+                       [ false, false, false ],
+                       [ false, true, false ],
+                       [ true, false, true ],
+                       [ false, true, true ],
+               ];
+       }
 }