From: Max Semenik <maxsem.wiki@gmail.com>
Date: Wed, 26 Jun 2019 06:00:07 +0000 (-0700)
Subject: Defer cookie block checks to resolve a circular dependency
X-Git-Tag: 1.34.0-rc.0~1228^2
X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin//%22%24encUrl/%22?a=commitdiff_plain;h=c8733333;p=lhc%2Fweb%2Fwiklou.git

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 abd2db24af..4e613692ec 100644
--- 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 6025d3cf1a..f91f438048 100644
--- 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 40fe4c881d..892add9786 100644
--- 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 ],
+		];
+	}
 }