Pass the user and request into BlockManager::getUserBlock

Blocks are checked from the User object. Specifically,
User::getBlockedStatus instantiates a BlockManager and calls
BlockManager::getUserBlock. However, checking the block often depends
on knowing more about the state than the User should know. As a result,
the global user and request objects were passed into the block manager
on construction.

Whether the global request object should be passed into a service
constructor is still up for debate, so this moves the check for the
global state back to User::getBlockedStatus for now. (Note that it
reintroduces the problem of the User knowing more about state than it
should.)

This change also makes clearer the cases in which
BlockManager::getUserBlock is called from the User.

Different blocks may be sought, depending on the user and their
permissions. The user may be:
(1) The global user (and can be affected by IP blocks). The global
    request object is needed for checking the IP address, the XFF
    header and the cookies.
(2) The global user (and exempt from IP blocks). The global request
    object is needed for checking the cookies.
(3) Another user (not the global user). No request object is available
    or needed; just look for a block against the user account.

Cases #1 and #2 check whether the global user is blocked in practice;
the block may due to their user account being blocked or to an IP
address block or cookie block (or multiple of these). Case #3 simply
checks whether a user's account is blocked, and does not determine
whether the person using that account is affected in practice by any
IP address or cookie blocks.

Bug: T231919
Change-Id: I3f51fd3579514b83b567dfe20926df2f0930dc85

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index f42ef31..3be84a5 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -109,13 +109,10 @@ return [
        },
 
        'BlockManager' => function ( MediaWikiServices $services ) : BlockManager {
-               $context = RequestContext::getMain();
                return new BlockManager(
                        new ServiceOptions(
                                BlockManager::$constructorOptions, $services->getMainConfig()
                        ),
-                       $context->getUser(),
-                       $context->getRequest(),
                        $services->getPermissionManager()
                );
        },

diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
index e27ebac..2e20529 100644 (file)
--- a/includes/block/BlockManager.php
+++ b/includes/block/BlockManager.php
@@ -41,16 +41,6 @@ use Wikimedia\IPSet;
  * @since 1.34 Refactored from User and Block.
  */
 class BlockManager {
-       // TODO: This should be UserIdentity instead of User
-       /** @var User */
-       private $currentUser;
-
-       /** @var WebRequest */
-       private $currentRequest;
-
-       /** @var PermissionManager */
-       private $permissionManager;
-
        /**
         * TODO Make this a const when HHVM support is dropped (T192166)
         *
@@ -71,20 +61,14 @@ class BlockManager {
 
        /**
         * @param ServiceOptions $options
-        * @param User $currentUser
-        * @param WebRequest $currentRequest
         * @param PermissionManager $permissionManager
         */
        public function __construct(
                ServiceOptions $options,
-               User $currentUser,
-               WebRequest $currentRequest,
                PermissionManager $permissionManager
        ) {
                $options->assertRequiredOptions( self::$constructorOptions );
                $this->options = $options;
-               $this->currentUser = $currentUser;
-               $this->currentRequest = $currentRequest;
                $this->permissionManager = $permissionManager;
        }
 
@@ -93,51 +77,116 @@ class BlockManager {
         * return a composite block that combines the strictest features of the applicable
         * blocks.
         *
-        * TODO: $user should be UserIdentity instead of User
+        * Different blocks may be sought, depending on the user and their permissions. The
+        * user may be:
+        * (1) The global user (and can be affected by IP blocks). The global request object
+        * is needed for checking the IP address, the XFF header and the cookies.
+        * (2) The global user (and exempt from IP blocks). The global request object is
+        * needed for checking the cookies.
+        * (3) Another user (not the global user). No request object is available or needed;
+        * just look for a block against the user account.
+        *
+        * Cases #1 and #2 check whether the global user is blocked in practice; the block
+        * may due to their user account being blocked or to an IP address block or cookie
+        * block (or multiple of these). Case #3 simply checks whether a user's account is
+        * blocked, and does not determine whether the person using that account is affected
+        * in practice by any IP address or cookie blocks.
         *
         * @internal This should only be called by User::getBlockedStatus
         * @param User $user
+        * @param WebRequest|null $request The global request object if the user is the
+        *  global user (cases #1 and #2), otherwise null (case #3). The IP address and
+        *  information from the request header are needed to find some types of blocks.
         * @param bool $fromReplica Whether to check the replica DB first.
         *  To improve performance, non-critical checks are done against replica DBs.
         *  Check when actually saving should be done against master.
         * @return AbstractBlock|null The most relevant block, or null if there is no block.
         */
-       public function getUserBlock( User $user, $fromReplica ) {
-               $isAnon = $user->getId() === 0;
+       public function getUserBlock( User $user, $request, $fromReplica ) {
                $fromMaster = !$fromReplica;
+               $ip = null;
 
-               // TODO: If $user is the current user, we should use the current request. Otherwise,
-               // we should not look for XFF or cookie blocks.
-               $request = $user->getRequest();
+               // If this is the global user, they may be affected by IP blocks (case #1),
+               // or they may be exempt (case #2). If affected, look for additional blocks
+               // against the IP address.
+               $checkIpBlocks = $request &&
+                       !$this->permissionManager->userHasRight( $user, 'ipblock-exempt' );
 
-               # We only need to worry about passing the IP address to the block generator if the
-               # user is not immune to autoblocks/hardblocks, and they are the current user so we
-               # know which IP address they're actually coming from
-               $ip = null;
-               $sessionUser = $this->currentUser;
-               // the session user is set up towards the end of Setup.php. Until then,
-               // assume it's a logged-out user.
-               $globalUserName = $sessionUser->isSafeToLoad()
-                       ? $sessionUser->getName()
-                       : IP::sanitizeIP( $this->currentRequest->getIP() );
-               if ( $user->getName() === $globalUserName &&
-                        !$this->permissionManager->userHasRight( $user, 'ipblock-exempt' ) ) {
-                       $ip = $this->currentRequest->getIP();
+               if ( $request && $checkIpBlocks ) {
+
+                       // Case #1: checking the global user, including IP blocks
+                       $ip = $request->getIP();
+                       // TODO: remove dependency on DatabaseBlock (T221075)
+                       $blocks = DatabaseBlock::newListFromTarget( $user, $ip, $fromMaster );
+                       $this->getAdditionalIpBlocks( $blocks, $request, !$user->isRegistered(), $fromMaster );
+                       $this->getCookieBlock( $blocks, $user, $request );
+
+               } elseif ( $request ) {
+
+                       // Case #2: checking the global user, but they are exempt from IP blocks
+                       // TODO: remove dependency on DatabaseBlock (T221075)
+                       $blocks = DatabaseBlock::newListFromTarget( $user, null, $fromMaster );
+                       $this->getCookieBlock( $blocks, $user, $request );
+
+               } else {
+
+                       // Case #3: checking whether a user's account is blocked
+                       // TODO: remove dependency on DatabaseBlock (T221075)
+                       $blocks = DatabaseBlock::newListFromTarget( $user, null, $fromMaster );
+
+               }
+
+               // Filter out any duplicated blocks, e.g. from the cookie
+               $blocks = $this->getUniqueBlocks( $blocks );
+
+               $block = null;
+               if ( count( $blocks ) > 0 ) {
+                       if ( count( $blocks ) === 1 ) {
+                               $block = $blocks[ 0 ];
+                       } else {
+                               $block = new CompositeBlock( [
+                                       'address' => $ip,
+                                       'byText' => 'MediaWiki default',
+                                       'reason' => wfMessage( 'blockedtext-composite-reason' )->plain(),
+                                       'originalBlocks' => $blocks,
+                               ] );
+                       }
                }
 
-               // User/IP blocking
-               // After this, $blocks is an array of blocks or an empty array
-               // TODO: remove dependency on DatabaseBlock
-               $blocks = DatabaseBlock::newListFromTarget( $user, $ip, $fromMaster );
+               Hooks::run( 'GetUserBlock', [ clone $user, $ip, &$block ] );
+
+               return $block;
+       }
 
-               // Cookie blocking
+       /**
+        * Get the cookie block, if there is one.
+        *
+        * @param AbstractBlock[] &$blocks
+        * @param UserIdentity $user
+        * @param WebRequest $request
+        * @return void
+        */
+       private function getCookieBlock( &$blocks, UserIdentity $user, WebRequest $request ) {
                $cookieBlock = $this->getBlockFromCookieValue( $user, $request );
-               if ( $cookieBlock instanceof AbstractBlock ) {
+               if ( $cookieBlock instanceof DatabaseBlock ) {
                        $blocks[] = $cookieBlock;
                }
+       }
+
+       /**
+        * Check for any additional blocks against the IP address or any IPs in the XFF header.
+        *
+        * @param AbstractBlock[] &$blocks Blocks found so far
+        * @param WebRequest $request
+        * @param bool $isAnon The user is logged out
+        * @param bool $fromMaster
+        * @return void
+        */
+       private function getAdditionalIpBlocks( &$blocks, WebRequest $request, $isAnon, $fromMaster ) {
+               $ip = $request->getIP();
 
                // Proxy blocking
-               if ( $ip !== null && !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) ) ) {
+               if ( !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) ) ) {
                        // Local list
                        if ( $this->isLocallyBlockedProxy( $ip ) ) {
                                $blocks[] = new SystemBlock( [
@@ -156,24 +205,8 @@ class BlockManager {
                        }
                }
 
-               // (T25343) Apply IP blocks to the contents of XFF headers, if enabled
-               if ( $this->options->get( 'ApplyIpBlocksToXff' )
-                       && $ip !== null
-                       && !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) )
-               ) {
-                       $xff = $request->getHeader( 'X-Forwarded-For' );
-                       $xff = array_map( 'trim', explode( ',', $xff ) );
-                       $xff = array_diff( $xff, [ $ip ] );
-                       // TODO: remove dependency on DatabaseBlock
-                       $xffblocks = DatabaseBlock::getBlocksForIPList( $xff, $isAnon, $fromMaster );
-                       $blocks = array_merge( $blocks, $xffblocks );
-               }
-
                // Soft blocking
-               if ( $ip !== null
-                       && $isAnon
-                       && IP::isInRanges( $ip, $this->options->get( 'SoftBlockRanges' ) )
-               ) {
+               if ( $isAnon && IP::isInRanges( $ip, $this->options->get( 'SoftBlockRanges' ) ) ) {
                        $blocks[] = new SystemBlock( [
                                'address' => $ip,
                                'byText' => 'MediaWiki default',
@@ -183,26 +216,17 @@ class BlockManager {
                        ] );
                }
 
-               // Filter out any duplicated blocks, e.g. from the cookie
-               $blocks = $this->getUniqueBlocks( $blocks );
-
-               $block = null;
-               if ( count( $blocks ) > 0 ) {
-                       if ( count( $blocks ) === 1 ) {
-                               $block = $blocks[ 0 ];
-                       } else {
-                               $block = new CompositeBlock( [
-                                       'address' => $ip,
-                                       'byText' => 'MediaWiki default',
-                                       'reason' => wfMessage( 'blockedtext-composite-reason' )->plain(),
-                                       'originalBlocks' => $blocks,
-                               ] );
-                       }
+               // (T25343) Apply IP blocks to the contents of XFF headers, if enabled
+               if ( $this->options->get( 'ApplyIpBlocksToXff' )
+                       && !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) )
+               ) {
+                       $xff = $request->getHeader( 'X-Forwarded-For' );
+                       $xff = array_map( 'trim', explode( ',', $xff ) );
+                       $xff = array_diff( $xff, [ $ip ] );
+                       // TODO: remove dependency on DatabaseBlock (T221075)
+                       $xffblocks = DatabaseBlock::getBlocksForIPList( $xff, $isAnon, $fromMaster );
+                       $blocks = array_merge( $blocks, $xffblocks );
                }
-
-               Hooks::run( 'GetUserBlock', [ clone $user, $ip, &$block ] );
-
-               return $block;
        }
 
        /**
@@ -255,7 +279,7 @@ class BlockManager {
 
                $blockCookieId = $this->getIdFromCookieValue( $cookieValue );
                if ( !is_null( $blockCookieId ) ) {
-                       // TODO: remove dependency on DatabaseBlock
+                       // TODO: remove dependency on DatabaseBlock (T221075)
                        $block = DatabaseBlock::newFromID( $blockCookieId );
                        if (
                                $block instanceof DatabaseBlock &&

diff --git a/includes/user/User.php b/includes/user/User.php
index 7068879..8bf73c2 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1753,9 +1753,31 @@ class User implements IDBAccessObject, UserIdentity {
                // overwriting mBlockedby, surely?
                $this->load();
 
+               // TODO: Block checking shouldn't really be done from the User object. Block
+               // checking can involve checking for IP blocks, cookie blocks, and/or XFF blocks,
+               // which need more knowledge of the request context than the User should have.
+               // Since we do currently check blocks from the User, we have to do the following
+               // here:
+               // - Check if this is the user associated with the main request
+               // - If so, pass the relevant request information to the block manager
+               $request = null;
+
+               // The session user is set up towards the end of Setup.php. Until then,
+               // assume it's a logged-out user.
+               $sessionUser = RequestContext::getMain()->getUser();
+               $globalUserName = $sessionUser->isSafeToLoad()
+                       ? $sessionUser->getName()
+                       : IP::sanitizeIP( $sessionUser->getRequest()->getIP() );
+
+               if ( $this->getName() === $globalUserName ) {
+                       // This is the global user, so we need to pass the request
+                       $request = $this->getRequest();
+               }
+
                // @phan-suppress-next-line PhanAccessMethodInternal It's the only allowed use
                $block = MediaWikiServices::getInstance()->getBlockManager()->getUserBlock(
                        $this,
+                       $request,
                        $fromReplica
                );
 

diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
index d4133b7..6a00e67 100644 (file)
--- a/tests/phpunit/includes/block/BlockManagerTest.php
+++ b/tests/phpunit/includes/block/BlockManagerTest.php
@@ -54,8 +54,6 @@ class BlockManagerTest extends MediaWikiTestCase {
                                BlockManager::$constructorOptions,
                                MediaWikiServices::getInstance()->getMainConfig()
                        ),
-                       $this->user,
-                       $this->user->getRequest(),
                        MediaWikiServices::getInstance()->getPermissionManager()
                ];
        }

diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php
index 2d87c78..5d4606d 100644 (file)
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -1566,10 +1566,6 @@ class UserTest extends MediaWikiTestCase {
                // logged in users should be inmune to cookie block of type ip/range
                $this->assertNull( $user->getBlock() );
 
-               // cookie is being cleared
-               $cookies = $request->response()->getCookies();
-               $this->assertEquals( '', $cookies['wikiBlockID']['value'] );
-
                // clean up
                $block->delete();
        }