Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Defer cookie block checks to resolve a circular dependency

Browse source 
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
This commit is contained in:
Max Semenik 2019-06-25 23:00:07 -07:00
parent 647e0709ae
commit c873333333
3 changed files with 92 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

45
includes/block/BlockManager.php
View file

@ -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 ( $request->getCookie( 'BlockID' ) !== null ) {
// User already has a block cookie
return;
}
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;
// 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
);
}
/**

11
includes/user/User.php
View file

@ -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() );

58
tests/phpunit/includes/block/BlockManagerTest.php
View file

@ -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 ],
];
}
}