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

diff --git a/includes/specials/SpecialBlock.php b/includes/specials/SpecialBlock.php
index ea4cd22..03006f3 100644 (file)
--- 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 080c6e4..40fe1e3 100644 (file)
--- 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();