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

BlockUser: Provide correct information to the post-block hook

Browse source 
Previously, BlockUser::placeBlockInternal attempted to insert
the new block with possible failure due to an existing block.
Regardless of the result, it would then retrieve the current
block which could be the just now inserted block. This one
was treated as the prior block and incorrectly provided to
the hook as such.

Refactor the method a little by retrieving the prior block
before attempting to insert a new one to make it more
efficient and to ensure the hook will have the correct value
for the prior block.

Change-Id: I70480ce32545f28f8317bb58f9c724d64cc54ccf
This commit is contained in:
Matěj Suchánek 2021-07-08 12:41:16 +02:00
parent 58cef29d69
commit 166ed5cd95
2 changed files with 126 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

45
includes/block/BlockUser.php
View file

@ -549,31 +549,32 @@ class BlockUser {
return $status;
}
// Try to insert block. Is there a conflicting block?
$insertStatus = $this->databaseBlockStore->insertBlock( $block );
$priorBlock = DatabaseBlock::newFromTarget( $this->target );
// Is there a conflicting block?
$priorBlock = DatabaseBlock::newFromTarget( $this->target, null, /*fromPrimary=*/true );
$isReblock = false;
if ( !$insertStatus ) {
// Reblock if the caller wants so
if ( $reblock ) {
if ( $priorBlock === null ) {
$this->logger->warning( 'Block could not be inserted. No existing block was found.' );
return Status::newFatal( 'ipb-block-not-found', $block->getTargetName() );
}
if ( $block->equals( $priorBlock ) ) {
// Block settings are equal => user is already blocked
return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
}
$currentBlock = $this->configureBlock( $priorBlock );
$this->databaseBlockStore->updateBlock( $currentBlock ); // TODO handle failure
$isReblock = true;
$block = $currentBlock;
} else {
if ( $priorBlock !== null ) {
// Reblock only if the caller wants so
if ( !$reblock ) {
return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
}
if ( $block->equals( $priorBlock ) ) {
// Block settings are equal => user is already blocked
return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
}
$currentBlock = $this->configureBlock( $priorBlock );
$this->databaseBlockStore->updateBlock( $currentBlock ); // TODO handle failure
$isReblock = true;
$block = $currentBlock;
} else {
// Try to insert block.
$insertStatus = $this->databaseBlockStore->insertBlock( $block );
if ( !$insertStatus ) {
$this->logger->warning( 'Block could not be inserted. No existing block was found.' );
return Status::newFatal( 'ipb-block-not-found', $block->getTargetName() );
}
}
// Set *_deleted fields if requested

103
tests/phpunit/integration/includes/block/BlockUserTest.php
View file

@ -108,4 +108,107 @@ class BlockUserTest extends MediaWikiIntegrationTestCase {
$this->assertFalse( $status->isOK() );
$this->assertTrue( $status->hasMessage( 'cant-block-nonexistent-page' ) );
}
/**
* @covers MediaWiki\Block\BlockUser::placeBlockInternal
*/
public function testReblock() {
$blockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test block'
)->placeBlockUnsafe();
$this->assertTrue( $blockStatus->isOK() );
$priorBlock = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $priorBlock );
$this->assertSame( 'test block', $priorBlock->getReasonComment()->text );
$blockId = $priorBlock->getId();
$reblockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test reblock'
)->placeBlockUnsafe( /*reblock=*/false );
$this->assertFalse( $reblockStatus->isOK() );
$this->user->clearInstanceCache();
$block = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $block );
$this->assertSame( $blockId, $block->getId() );
$reblockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test block'
)->placeBlockUnsafe( /*reblock=*/true );
$this->assertFalse( $reblockStatus->isOK() );
$this->user->clearInstanceCache();
$block = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $block );
$this->assertSame( $blockId, $block->getId() );
$reblockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test reblock'
)->placeBlockUnsafe( /*reblock=*/true );
$this->assertTrue( $reblockStatus->isOK() );
$this->user->clearInstanceCache();
$block = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $block );
$this->assertSame( 'test reblock', $block->getReasonComment()->text );
}
/**
* @covers MediaWiki\Block\BlockUser::placeBlockInternal
*/
public function testPostHook() {
$hookBlock = false;
$hookPriorBlock = false;
$this->setTemporaryHook(
'BlockIpComplete',
static function ( $block, $legacyUser, $priorBlock )
use ( &$hookBlock, &$hookPriorBlock )
{
$hookBlock = $block;
$hookPriorBlock = $priorBlock;
}
);
$blockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test block'
)->placeBlockUnsafe();
$this->assertTrue( $blockStatus->isOK() );
$priorBlock = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $priorBlock );
$this->assertSame( $priorBlock->getId(), $hookBlock->getId() );
$this->assertNull( $hookPriorBlock );
$hookBlock = false;
$hookPriorBlock = false;
$reblockStatus = $this->blockUserFactory->newBlockUser(
$this->user,
$this->mockAnonUltimateAuthority(),
'infinity',
'test reblock'
)->placeBlockUnsafe( /*reblock=*/true );
$this->assertTrue( $reblockStatus->isOK() );
$this->user->clearInstanceCache();
$newBlock = $this->user->getBlock();
$this->assertInstanceOf( DatabaseBlock::class, $newBlock );
$this->assertSame( $newBlock->getId(), $hookBlock->getId() );
$this->assertSame( $priorBlock->getId(), $hookPriorBlock->getId() );
}
}