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

Clear block cookie when tracking block, not when checking block

Browse source 
Before this commit, clearing a block cookie happens as a
side-effect of checking for a block cookie. After, a block cookie
is cleared if trackBlockWithCookie finds that there should be no
block cookie, but there is one.

Bug: T233595
Change-Id: Id5777361f95c60d2849cacba82f2ed9add647abf
This commit is contained in:
Thalia 2019-09-16 13:18:56 +01:00
parent 66f65e3a22
commit 235cade5a3
2 changed files with 50 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
includes/block/BlockManager.php
View file

@ -272,8 +272,11 @@ class BlockManager {
}
/**
* Try to load a block from an ID given in a cookie value. If the block is invalid
* doesn't exist, or the cookie value is malformed, remove the cookie.
* Try to load a block from an ID given in a cookie value.
*
* If the block is invalid, doesn't exist, or the cookie value is malformed, no
* block will be loaded. In these cases the cookie will either (1) be replaced
* with a valid cookie or (2) removed, next time trackBlockWithCookie is called.
*
* @param UserIdentity $user
* @param WebRequest $request
@ -300,8 +303,6 @@ class BlockManager {
}
}
$this->clearBlockCookie( $request->response() );
return false;
}
@ -430,6 +431,16 @@ class BlockManager {
/**
* Set the 'BlockID' cookie depending on block type and user authentication status.
*
* If a block cookie is already set, this will check the block that the cookie references
* and do the following:
* - If the block is a valid block that should be tracked with a cookie, do nothing and
* return early. This ensures that the cookie's expiry time is based on the time of
* the first page load or attempt. (See discussion on T233595.)
* - If the block is invalid (e.g. has expired), clear the cookie and continue to check
* whether there is another block that should be tracked.
* - If the block is a valid block, but should not be tracked by a cookie, clear the
* cookie and continue to check whether there is another block that should be tracked.
*
* @since 1.34
* @param User $user
* @param WebResponse $response The response on which to set the cookie.
@ -438,9 +449,14 @@ class BlockManager {
*/
public function trackBlockWithCookie( User $user, WebResponse $response ) {
$request = $user->getRequest();
if ( $request->getCookie( 'BlockID' ) !== null ) {
// User already has a block cookie
return;
$cookieBlock = $this->getBlockFromCookieValue( $user, $request );
if ( $cookieBlock && $this->shouldTrackBlockWithCookie( $cookieBlock, $user->isAnon() ) ) {
return;
}
// The block pointed to by the cookie is invalid or should not be tracked.
$this->clearBlockCookie( $response );
}
if ( !$user->isSafeToLoad() ) {

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

@ -305,7 +305,7 @@ class BlockManagerTest extends MediaWikiTestCase {
* @dataProvider provideTrackBlockWithCookie
* @covers ::trackBlockWithCookie
*/
public function testTrackBlockWithCookie( $options, $expectedVal ) {
public function testTrackBlockWithCookie( $options, $expected ) {
$this->setMwGlobals( 'wgCookiePrefix', '' );
$request = new FauxRequest();
@ -331,8 +331,8 @@ class BlockManagerTest extends MediaWikiTestCase {
] );
$blockManager->trackBlockWithCookie( $user, $response );
$this->assertCount( $expectedVal ? 1 : 0, $response->getCookies() );
$this->assertEquals( $expectedVal ?: null, $response->getCookie( 'BlockID' ) );
$this->assertCount( $expected['count'], $response->getCookies() );
$this->assertEquals( $expected['value'], $response->getCookie( 'BlockID' ) );
}
public function provideTrackBlockWithCookie() {
@ -343,28 +343,41 @@ class BlockManagerTest extends MediaWikiTestCase {
'cookieSet' => true,
'block' => $this->getTrackableBlock( $blockId ),
],
null,
[
'count' => 1,
'value' => $blockId,
]
],
'Block cookie is already set; there is no block' => [
[
'cookieSet' => true,
'block' => null,
],
null,
[
// Cookie is cleared by setting it to empty value
'count' => 1,
'value' => '',
]
],
'Block cookie is not yet set; there is no block' => [
[
'cookieSet' => false,
'block' => null,
],
null,
[
'count' => 0,
'value' => null,
]
],
'Block cookie is not yet set; there is a trackable block' => [
[
'cookieSet' => false,
'block' => $this->getTrackableBlock( $blockId ),
],
$blockId,
[
'count' => 1,
'value' => $blockId,
]
],
'Block cookie is not yet set; there is a composite block with a trackable block' => [
[
@ -376,7 +389,10 @@ class BlockManagerTest extends MediaWikiTestCase {
]
] ),
],
$blockId,
[
'count' => 1,
'value' => $blockId,
]
],
'Block cookie is not yet set; there is a composite block but no trackable block' => [
[
@ -388,7 +404,10 @@ class BlockManagerTest extends MediaWikiTestCase {
]
] ),
],
null,
[
'count' => 0,
'value' => null,
]
],
];
}