Merge "Pass the user and request into BlockManager::getUserBlock"

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 0b0aaf5..c7ad75c 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -107,13 +107,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 4445e1d..d71750b 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1718,9 +1718,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 3005ae9..2a4ba4b 100644 (file)
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -1488,10 +1488,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();
        }