From 6585277528ba75d52ccecbe8226fd3461907d662 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Tue, 27 Nov 2018 18:38:14 +0000 Subject: [PATCH] Allow users to block the user that blocked them. This is to make it so that blocking all other admins is not a succesful attack plan, as the blocked admins can block the blocker, and then it ends in a stalemate with everyone blocked. This also allows users with unblock-self right to adjust their own blocks. The code already existed for this but was broken. Credit for this idea goes to Tgr. Bug: T150826 Change-Id: I0418279fdb2a59f8f1d7eeb8931d874123d03e4f --- includes/specials/SpecialBlock.php | 30 +++++++++- .../includes/specials/SpecialBlockTest.php | 57 +++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/includes/specials/SpecialBlock.php b/includes/specials/SpecialBlock.php index ea4cd2292a..03006f3c44 100644 --- a/includes/specials/SpecialBlock.php +++ b/includes/specials/SpecialBlock.php @@ -66,7 +66,6 @@ class SpecialBlock extends FormSpecialPage { */ protected function checkExecutePermissions( User $user ) { parent::checkExecutePermissions( $user ); - # T17810: blocked admins should have limited access here $status = self::checkUnblockSelf( $this->target, $user ); if ( $status !== true ) { @@ -74,6 +73,15 @@ class SpecialBlock extends FormSpecialPage { } } + /** + * We allow certain special cases where user is blocked + * + * @return bool + */ + public function requiresUnblock() { + return false; + } + /** * Handle some magic here * @@ -1021,7 +1029,11 @@ class SpecialBlock extends FormSpecialPage { * T17810: blocked admins should not be able to block/unblock * others, and probably shouldn't be able to unblock themselves * either. - * @param User|int|string $user + * + * Exception: Users can block the user who blocked them, to reduce + * advantage of a malicious account blocking all admins (T150826) + * + * @param User|int|string $user Target to block or unblock * @param User $performer User doing the request * @return bool|string True or error message key */ @@ -1031,7 +1043,6 @@ class SpecialBlock extends FormSpecialPage { } elseif ( is_string( $user ) ) { $user = User::newFromName( $user ); } - if ( $performer->isBlocked() ) { if ( $user instanceof User && $user->getId() == $performer->getId() ) { # User is trying to unblock themselves @@ -1043,6 +1054,19 @@ class SpecialBlock extends FormSpecialPage { } else { return 'ipbnounblockself'; } + } elseif ( + $user instanceof User && + $performer->getBlock() instanceof Block && + $performer->getBlock()->getBy() && + $performer->getBlock()->getBy() === $user->getId() + ) { + // Allow users to block the user that blocked them. + // This is to prevent a situation where a malicious user + // blocks all other users. This way, the non-malicious + // user can block the malicious user back, resulting + // in a stalemate. + return true; + } else { # User is trying to block/unblock someone else return 'ipbblocked'; diff --git a/tests/phpunit/includes/specials/SpecialBlockTest.php b/tests/phpunit/includes/specials/SpecialBlockTest.php index 080c6e4c43..40fe1e382b 100644 --- a/tests/phpunit/includes/specials/SpecialBlockTest.php +++ b/tests/phpunit/includes/specials/SpecialBlockTest.php @@ -380,6 +380,63 @@ class SpecialBlockTest extends SpecialPageTestBase { $this->assertSame( 0, $count ); } + /** + * @dataProvider provideCheckUnblockSelf + * @covers ::checkUnblockSelf + */ + public function testCheckUnblockSelf( + $blockedUser, + $blockPerformer, + $adjustPerformer, + $adjustTarget, + $expectedResult, + $reason + ) { + $this->setGroupPermissions( 'sysop', 'unblockself', true ); + $this->setGroupPermissions( 'user', 'block', true ); + // Getting errors about creating users in db in provider. + // Need to use mutable to ensure different named testusers. + $users = [ + 'u1' => TestUserRegistry::getMutableTestUser( __CLASS__, 'sysop' )->getUser(), + 'u2' => TestUserRegistry::getMutableTestUser( __CLASS__, 'sysop' )->getUser(), + 'u3' => TestUserRegistry::getMutableTestUser( __CLASS__, 'sysop' )->getUser(), + 'u4' => TestUserRegistry::getMutableTestUser( __CLASS__, 'sysop' )->getUser(), + 'nonsysop' => $this->getTestUser()->getUser() + ]; + foreach ( [ 'blockedUser', 'blockPerformer', 'adjustPerformer', 'adjustTarget' ] as $var ) { + $$var = $users[$$var]; + } + + $block = new \Block( [ + 'address' => $blockedUser->getName(), + 'user' => $blockedUser->getId(), + 'by' => $blockPerformer->getId(), + 'expiry' => 'infinity', + 'sitewide' => 1, + 'enableAutoblock' => true, + ] ); + + $block->insert(); + + $this->assertSame( + SpecialBlock::checkUnblockSelf( $adjustTarget, $adjustPerformer ), + $expectedResult, + $reason + ); + } + + public function provideCheckUnblockSelf() { + // 'blockedUser', 'blockPerformer', 'adjustPerformer', 'adjustTarget' + return [ + [ 'u1', 'u2', 'u3', 'u4', true, 'Unrelated users' ], + [ 'u1', 'u2', 'u1', 'u4', 'ipbblocked', 'Block unrelated while blocked' ], + [ 'u1', 'u2', 'u1', 'u1', true, 'Has unblockself' ], + [ 'nonsysop', 'u2', 'nonsysop', 'nonsysop', 'ipbnounblockself', 'no unblockself' ], + [ 'nonsysop', 'nonsysop', 'nonsysop', 'nonsysop', true, 'no unblockself but can de-selfblock' ], + [ 'u1', 'u2', 'u1', 'u2', true, 'Can block user who blocked' ], + ]; + } + protected function insertBlock() { $badActor = $this->getTestUser()->getUser(); $sysop = $this->getTestSysop()->getUser(); -- 2.20.1