Introduce a BlockManager service
authorThalia <thalia.e.chan@googlemail.com>
Fri, 5 Apr 2019 19:13:17 +0000 (20:13 +0100)
committerThalia <thalia.e.chan@googlemail.com>
Mon, 29 Apr 2019 16:47:55 +0000 (17:47 +0100)
This introduces a minimal BlockManager service, for getting blocks
that apply to a User.

Move the part of User::getBlockedStatus that checks for the blocks
into BlockManager::getUserBlock, and move the related helper
methods from User to BlockManager.

Hard deprecate or remove these helper methods, and move to private
methods in the BlockManager:
* User::getBlockFromCookieValue
* User::isLocallyBlockedProxy
* User::inDnsBlacklist

Soft deprecate these helper methods, and move to public methods in
the BlockManager:
* User::isDnsBlacklisted

Add tests to cover the methods moved to BlockManager.

Bug: T219441
Change-Id: I0af658d71288376735cebe541215383b56bb72e5

RELEASE-NOTES-1.34
includes/MediaWikiServices.php
includes/ServiceWiring.php
includes/auth/AuthManager.php
includes/block/BlockManager.php [new file with mode: 0644]
includes/user/User.php
tests/phpunit/includes/auth/AuthManagerTest.php
tests/phpunit/includes/block/BlockManagerTest.php [new file with mode: 0644]
tests/phpunit/includes/user/UserTest.php

index 126e00b..834ec89 100644 (file)
@@ -116,6 +116,10 @@ because of Phabricator reports.
 * User::isBlocked() is deprecated since it does not tell you if the user is
   blocked from editing a particular page. Use User::getBlock() or
   PermissionManager::isBlockedFrom() or PermissionManager::userCan() instead.
+* User::isLocallyBlockedProxy and User::inDnsBlacklist are deprecated and moved
+  to the BlockManager as private helper methods.
+* User::isDnsBlacklisted is deprecated. Use BlockManager::isDnsBlacklisted
+  instead.
 * …
 
 === Other changes in 1.34 ===
index 3590633..c374a62 100644 (file)
@@ -13,6 +13,7 @@ use GlobalVarConfig;
 use Hooks;
 use IBufferingStatsdDataFactory;
 use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
+use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\BlockRestrictionStore;
 use MediaWiki\Http\HttpRequestFactory;
 use MediaWiki\Permissions\PermissionManager;
@@ -437,6 +438,14 @@ class MediaWikiServices extends ServiceContainer {
                return $this->getService( 'BlobStoreFactory' );
        }
 
+       /**
+        * @since 1.34
+        * @return BlockManager
+        */
+       public function getBlockManager() : BlockManager {
+               return $this->getService( 'BlockManager' );
+       }
+
        /**
         * @since 1.33
         * @return BlockRestrictionStore
index 832cee8..93ddee9 100644 (file)
@@ -39,6 +39,7 @@
 
 use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
 use MediaWiki\Auth\AuthManager;
+use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\BlockRestrictionStore;
 use MediaWiki\Config\ConfigRepository;
 use MediaWiki\Interwiki\ClassicInterwikiLookup;
@@ -85,6 +86,23 @@ return [
                );
        },
 
+       'BlockManager' => function ( MediaWikiServices $services ) : BlockManager {
+               $config = $services->getMainConfig();
+               $context = RequestContext::getMain();
+               return new BlockManager(
+                       $context->getUser(),
+                       $context->getRequest(),
+                       $config->get( 'ApplyIpBlocksToXff' ),
+                       $config->get( 'CookieSetOnAutoblock' ),
+                       $config->get( 'CookieSetOnIpBlock' ),
+                       $config->get( 'DnsBlacklistUrls' ),
+                       $config->get( 'EnableDnsBlacklist' ),
+                       $config->get( 'ProxyList' ),
+                       $config->get( 'ProxyWhitelist' ),
+                       $config->get( 'SoftBlockRanges' )
+               );
+       },
+
        'BlockRestrictionStore' => function ( MediaWikiServices $services ) : BlockRestrictionStore {
                return new BlockRestrictionStore(
                        $services->getDBLoadBalancer()
index 3515a70..5915d35 100644 (file)
@@ -1021,7 +1021,10 @@ class AuthManager implements LoggerAwareInterface {
                }
 
                $ip = $this->getRequest()->getIP();
-               if ( $creator->isDnsBlacklisted( $ip, true /* check $wgProxyWhitelist */ ) ) {
+               if (
+                       MediaWikiServices::getInstance()->getBlockManager()
+                               ->isDnsBlacklisted( $ip, true /* check $wgProxyWhitelist */ )
+               ) {
                        return Status::newFatal( 'sorbs_create_account_reason' );
                }
 
diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
new file mode 100644 (file)
index 0000000..3ef35d7
--- /dev/null
@@ -0,0 +1,370 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+namespace MediaWiki\Block;
+
+use Block;
+use IP;
+use User;
+use WebRequest;
+use Wikimedia\IPSet;
+use MediaWiki\User\UserIdentity;
+
+/**
+ * A service class for checking blocks.
+ * To obtain an instance, use MediaWikiServices::getInstance()->getBlockManager().
+ *
+ * @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 bool */
+       private $applyIpBlocksToXff;
+
+       /** @var bool */
+       private $cookieSetOnAutoblock;
+
+       /** @var bool */
+       private $cookieSetOnIpBlock;
+
+       /** @var array */
+       private $dnsBlacklistUrls;
+
+       /** @var bool */
+       private $enableDnsBlacklist;
+
+       /** @var array */
+       private $proxyList;
+
+       /** @var array */
+       private $proxyWhitelist;
+
+       /** @var array */
+       private $softBlockRanges;
+
+       /**
+        * @param User $currentUser
+        * @param WebRequest $currentRequest
+        * @param bool $applyIpBlocksToXff
+        * @param bool $cookieSetOnAutoblock
+        * @param bool $cookieSetOnIpBlock
+        * @param array $dnsBlacklistUrls
+        * @param bool $enableDnsBlacklist
+        * @param array $proxyList
+        * @param array $proxyWhitelist
+        * @param array $softBlockRanges
+        */
+       public function __construct(
+               $currentUser,
+               $currentRequest,
+               $applyIpBlocksToXff,
+               $cookieSetOnAutoblock,
+               $cookieSetOnIpBlock,
+               $dnsBlacklistUrls,
+               $enableDnsBlacklist,
+               $proxyList,
+               $proxyWhitelist,
+               $softBlockRanges
+       ) {
+               $this->currentUser = $currentUser;
+               $this->currentRequest = $currentRequest;
+               $this->applyIpBlocksToXff = $applyIpBlocksToXff;
+               $this->cookieSetOnAutoblock = $cookieSetOnAutoblock;
+               $this->cookieSetOnIpBlock = $cookieSetOnIpBlock;
+               $this->dnsBlacklistUrls = $dnsBlacklistUrls;
+               $this->enableDnsBlacklist = $enableDnsBlacklist;
+               $this->proxyList = $proxyList;
+               $this->proxyWhitelist = $proxyWhitelist;
+               $this->softBlockRanges = $softBlockRanges;
+       }
+
+       /**
+        * Get the blocks that apply to a user and return the most relevant one.
+        *
+        * TODO: $user should be UserIdentity instead of User
+        *
+        * @internal This should only be called by User::getBlockedStatus
+        * @param User $user
+        * @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 Block|null The most relevant block, or null if there is no block.
+        */
+       public function getUserBlock( User $user, $fromReplica ) {
+               $isAnon = $user->getId() === 0;
+
+               // 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();
+
+               # 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 && !$user->isAllowed( 'ipblock-exempt' ) ) {
+                       $ip = $this->currentRequest->getIP();
+               }
+
+               // User/IP blocking
+               // TODO: remove dependency on Block
+               $block = Block::newFromTarget( $user, $ip, !$fromReplica );
+
+               // Cookie blocking
+               if ( !$block instanceof Block ) {
+                       $block = $this->getBlockFromCookieValue( $user, $request );
+               }
+
+               // Proxy blocking
+               if ( !$block instanceof Block && $ip !== null && !in_array( $ip, $this->proxyWhitelist ) ) {
+                       // Local list
+                       if ( $this->isLocallyBlockedProxy( $ip ) ) {
+                               $block = new Block( [
+                                       'byText' => wfMessage( 'proxyblocker' )->text(),
+                                       'reason' => wfMessage( 'proxyblockreason' )->plain(),
+                                       'address' => $ip,
+                                       'systemBlock' => 'proxy',
+                               ] );
+                       } elseif ( $isAnon && $this->isDnsBlacklisted( $ip ) ) {
+                               $block = new Block( [
+                                       'byText' => wfMessage( 'sorbs' )->text(),
+                                       'reason' => wfMessage( 'sorbsreason' )->plain(),
+                                       'address' => $ip,
+                                       'systemBlock' => 'dnsbl',
+                               ] );
+                       }
+               }
+
+               // (T25343) Apply IP blocks to the contents of XFF headers, if enabled
+               if ( !$block instanceof Block
+                       && $this->applyIpBlocksToXff
+                       && $ip !== null
+                       && !in_array( $ip, $this->proxyWhitelist )
+               ) {
+                       $xff = $request->getHeader( 'X-Forwarded-For' );
+                       $xff = array_map( 'trim', explode( ',', $xff ) );
+                       $xff = array_diff( $xff, [ $ip ] );
+                       // TODO: remove dependency on Block
+                       $xffblocks = Block::getBlocksForIPList( $xff, $isAnon, !$fromReplica );
+                       // TODO: remove dependency on Block
+                       $block = Block::chooseBlock( $xffblocks, $xff );
+                       if ( $block instanceof Block ) {
+                               # Mangle the reason to alert the user that the block
+                               # originated from matching the X-Forwarded-For header.
+                               $block->setReason( wfMessage( 'xffblockreason', $block->getReason() )->plain() );
+                       }
+               }
+
+               if ( !$block instanceof Block
+                       && $ip !== null
+                       && $isAnon
+                       && IP::isInRanges( $ip, $this->softBlockRanges )
+               ) {
+                       $block = new Block( [
+                               'address' => $ip,
+                               'byText' => 'MediaWiki default',
+                               'reason' => wfMessage( 'softblockrangesreason', $ip )->plain(),
+                               'anonOnly' => true,
+                               'systemBlock' => 'wgSoftBlockRanges',
+                       ] );
+               }
+
+               return $block;
+       }
+
+       /**
+        * Try to load a Block from an ID given in a cookie value.
+        *
+        * @param UserIdentity $user
+        * @param WebRequest $request
+        * @return Block|bool The Block object, or false if none could be loaded.
+        */
+       private function getBlockFromCookieValue(
+               UserIdentity $user,
+               WebRequest $request
+       ) {
+               $blockCookieVal = $request->getCookie( 'BlockID' );
+               $response = $request->response();
+
+               // Make sure there's something to check. The cookie value must start with a number.
+               if ( strlen( $blockCookieVal ) < 1 || !is_numeric( substr( $blockCookieVal, 0, 1 ) ) ) {
+                       return false;
+               }
+               // Load the Block from the ID in the cookie.
+               // TODO: remove dependency on Block
+               $blockCookieId = Block::getIdFromCookieValue( $blockCookieVal );
+               if ( $blockCookieId !== null ) {
+                       // An ID was found in the cookie.
+                       // TODO: remove dependency on Block
+                       $tmpBlock = Block::newFromID( $blockCookieId );
+                       if ( $tmpBlock instanceof Block ) {
+                               switch ( $tmpBlock->getType() ) {
+                                       case Block::TYPE_USER:
+                                               $blockIsValid = !$tmpBlock->isExpired() && $tmpBlock->isAutoblocking();
+                                               $useBlockCookie = ( $this->cookieSetOnAutoblock === true );
+                                               break;
+                                       case Block::TYPE_IP:
+                                       case Block::TYPE_RANGE:
+                                               // If block is type IP or IP range, load only if user is not logged in (T152462)
+                                               $blockIsValid = !$tmpBlock->isExpired() && $user->getId() === 0;
+                                               $useBlockCookie = ( $this->cookieSetOnIpBlock === true );
+                                               break;
+                                       default:
+                                               $blockIsValid = false;
+                                               $useBlockCookie = false;
+                               }
+
+                               if ( $blockIsValid && $useBlockCookie ) {
+                                       // Use the block.
+                                       return $tmpBlock;
+                               }
+
+                               // If the block is not valid, remove the cookie.
+                               // TODO: remove dependency on Block
+                               Block::clearCookie( $response );
+                       } else {
+                               // If the block doesn't exist, remove the cookie.
+                               // TODO: remove dependency on Block
+                               Block::clearCookie( $response );
+                       }
+               }
+               return false;
+       }
+
+       /**
+        * Check if an IP address is in the local proxy list
+        *
+        * @param string $ip
+        * @return bool
+        */
+       private function isLocallyBlockedProxy( $ip ) {
+               if ( !$this->proxyList ) {
+                       return false;
+               }
+
+               if ( !is_array( $this->proxyList ) ) {
+                       // Load values from the specified file
+                       $this->proxyList = array_map( 'trim', file( $this->proxyList ) );
+               }
+
+               $resultProxyList = [];
+               $deprecatedIPEntries = [];
+
+               // backward compatibility: move all ip addresses in keys to values
+               foreach ( $this->proxyList as $key => $value ) {
+                       $keyIsIP = IP::isIPAddress( $key );
+                       $valueIsIP = IP::isIPAddress( $value );
+                       if ( $keyIsIP && !$valueIsIP ) {
+                               $deprecatedIPEntries[] = $key;
+                               $resultProxyList[] = $key;
+                       } elseif ( $keyIsIP && $valueIsIP ) {
+                               $deprecatedIPEntries[] = $key;
+                               $resultProxyList[] = $key;
+                               $resultProxyList[] = $value;
+                       } else {
+                               $resultProxyList[] = $value;
+                       }
+               }
+
+               if ( $deprecatedIPEntries ) {
+                       wfDeprecated(
+                               'IP addresses in the keys of $wgProxyList (found the following IP addresses in keys: ' .
+                               implode( ', ', $deprecatedIPEntries ) . ', please move them to values)', '1.30' );
+               }
+
+               $proxyListIPSet = new IPSet( $resultProxyList );
+               return $proxyListIPSet->match( $ip );
+       }
+
+       /**
+        * Whether the given IP is in a DNS blacklist.
+        *
+        * @param string $ip IP to check
+        * @param bool $checkWhitelist Whether to check the whitelist first
+        * @return bool True if blacklisted.
+        */
+       public function isDnsBlacklisted( $ip, $checkWhitelist = false ) {
+               if ( !$this->enableDnsBlacklist ||
+                       ( $checkWhitelist && in_array( $ip, $this->proxyWhitelist ) )
+               ) {
+                       return false;
+               }
+
+               return $this->inDnsBlacklist( $ip, $this->dnsBlacklistUrls );
+       }
+
+       /**
+        * Whether the given IP is in a given DNS blacklist.
+        *
+        * @param string $ip IP to check
+        * @param array $bases Array of Strings: URL of the DNS blacklist
+        * @return bool True if blacklisted.
+        */
+       private function inDnsBlacklist( $ip, array $bases ) {
+               $found = false;
+               // @todo FIXME: IPv6 ???  (https://bugs.php.net/bug.php?id=33170)
+               if ( IP::isIPv4( $ip ) ) {
+                       // Reverse IP, T23255
+                       $ipReversed = implode( '.', array_reverse( explode( '.', $ip ) ) );
+
+                       foreach ( $bases as $base ) {
+                               // Make hostname
+                               // If we have an access key, use that too (ProjectHoneypot, etc.)
+                               $basename = $base;
+                               if ( is_array( $base ) ) {
+                                       if ( count( $base ) >= 2 ) {
+                                               // Access key is 1, base URL is 0
+                                               $host = "{$base[1]}.$ipReversed.{$base[0]}";
+                                       } else {
+                                               $host = "$ipReversed.{$base[0]}";
+                                       }
+                                       $basename = $base[0];
+                               } else {
+                                       $host = "$ipReversed.$base";
+                               }
+
+                               // Send query
+                               $ipList = gethostbynamel( $host );
+
+                               if ( $ipList ) {
+                                       wfDebugLog( 'dnsblacklist', "Hostname $host is {$ipList[0]}, it's a proxy says $basename!" );
+                                       $found = true;
+                                       break;
+                               }
+
+                               wfDebugLog( 'dnsblacklist', "Requested $host, not found in $basename." );
+                       }
+               }
+
+               return $found;
+       }
+
+}
index 13467a4..d2a0707 100644 (file)
@@ -1813,13 +1813,14 @@ class User implements IDBAccessObject, UserIdentity {
 
        /**
         * Get blocking information
+        *
+        * TODO: Move this into the BlockManager, along with block-related properties.
+        *
         * @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.
         */
        private function getBlockedStatus( $fromReplica = true ) {
-               global $wgProxyWhitelist, $wgApplyIpBlocksToXff, $wgSoftBlockRanges;
-
                if ( $this->mBlockedby != -1 ) {
                        return;
                }
@@ -1833,79 +1834,10 @@ class User implements IDBAccessObject, UserIdentity {
                // overwriting mBlockedby, surely?
                $this->load();
 
-               # 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 = RequestContext::getMain()->getUser();
-               // 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( $sessionUser->getRequest()->getIP() );
-               if ( $this->getName() === $globalUserName && !$this->isAllowed( 'ipblock-exempt' ) ) {
-                       $ip = $this->getRequest()->getIP();
-               }
-
-               // User/IP blocking
-               $block = Block::newFromTarget( $this, $ip, !$fromReplica );
-
-               // Cookie blocking
-               if ( !$block instanceof Block ) {
-                       $block = $this->getBlockFromCookieValue( $this->getRequest()->getCookie( 'BlockID' ) );
-               }
-
-               // Proxy blocking
-               if ( !$block instanceof Block && $ip !== null && !in_array( $ip, $wgProxyWhitelist ) ) {
-                       // Local list
-                       if ( self::isLocallyBlockedProxy( $ip ) ) {
-                               $block = new Block( [
-                                       'byText' => wfMessage( 'proxyblocker' )->text(),
-                                       'reason' => wfMessage( 'proxyblockreason' )->plain(),
-                                       'address' => $ip,
-                                       'systemBlock' => 'proxy',
-                               ] );
-                       } elseif ( $this->isAnon() && $this->isDnsBlacklisted( $ip ) ) {
-                               $block = new Block( [
-                                       'byText' => wfMessage( 'sorbs' )->text(),
-                                       'reason' => wfMessage( 'sorbsreason' )->plain(),
-                                       'address' => $ip,
-                                       'systemBlock' => 'dnsbl',
-                               ] );
-                       }
-               }
-
-               // (T25343) Apply IP blocks to the contents of XFF headers, if enabled
-               if ( !$block instanceof Block
-                       && $wgApplyIpBlocksToXff
-                       && $ip !== null
-                       && !in_array( $ip, $wgProxyWhitelist )
-               ) {
-                       $xff = $this->getRequest()->getHeader( 'X-Forwarded-For' );
-                       $xff = array_map( 'trim', explode( ',', $xff ) );
-                       $xff = array_diff( $xff, [ $ip ] );
-                       $xffblocks = Block::getBlocksForIPList( $xff, $this->isAnon(), !$fromReplica );
-                       $block = Block::chooseBlock( $xffblocks, $xff );
-                       if ( $block instanceof Block ) {
-                               # Mangle the reason to alert the user that the block
-                               # originated from matching the X-Forwarded-For header.
-                               $block->setReason( wfMessage( 'xffblockreason', $block->getReason() )->plain() );
-                       }
-               }
-
-               if ( !$block instanceof Block
-                       && $ip !== null
-                       && $this->isAnon()
-                       && IP::isInRanges( $ip, $wgSoftBlockRanges )
-               ) {
-                       $block = new Block( [
-                               'address' => $ip,
-                               'byText' => 'MediaWiki default',
-                               'reason' => wfMessage( 'softblockrangesreason', $ip )->plain(),
-                               'anonOnly' => true,
-                               'systemBlock' => 'wgSoftBlockRanges',
-                       ] );
-               }
+               $block = MediaWikiServices::getInstance()->getBlockManager()->getUserBlock(
+                       $this,
+                       $fromReplica
+               );
 
                if ( $block instanceof Block ) {
                        wfDebug( __METHOD__ . ": Found block.\n" );
@@ -1928,82 +1860,30 @@ class User implements IDBAccessObject, UserIdentity {
                Hooks::run( 'GetBlockedStatus', [ &$thisUser ] );
        }
 
-       /**
-        * Try to load a Block from an ID given in a cookie value.
-        * @param string|null $blockCookieVal The cookie value to check.
-        * @return Block|bool The Block object, or false if none could be loaded.
-        */
-       protected function getBlockFromCookieValue( $blockCookieVal ) {
-               // Make sure there's something to check. The cookie value must start with a number.
-               if ( strlen( $blockCookieVal ) < 1 || !is_numeric( substr( $blockCookieVal, 0, 1 ) ) ) {
-                       return false;
-               }
-               // Load the Block from the ID in the cookie.
-               $blockCookieId = Block::getIdFromCookieValue( $blockCookieVal );
-               if ( $blockCookieId !== null ) {
-                       // An ID was found in the cookie.
-                       $tmpBlock = Block::newFromID( $blockCookieId );
-                       if ( $tmpBlock instanceof Block ) {
-                               $config = RequestContext::getMain()->getConfig();
-
-                               switch ( $tmpBlock->getType() ) {
-                                       case Block::TYPE_USER:
-                                               $blockIsValid = !$tmpBlock->isExpired() && $tmpBlock->isAutoblocking();
-                                               $useBlockCookie = ( $config->get( 'CookieSetOnAutoblock' ) === true );
-                                               break;
-                                       case Block::TYPE_IP:
-                                       case Block::TYPE_RANGE:
-                                               // If block is type IP or IP range, load only if user is not logged in (T152462)
-                                               $blockIsValid = !$tmpBlock->isExpired() && !$this->isLoggedIn();
-                                               $useBlockCookie = ( $config->get( 'CookieSetOnIpBlock' ) === true );
-                                               break;
-                                       default:
-                                               $blockIsValid = false;
-                                               $useBlockCookie = false;
-                               }
-
-                               if ( $blockIsValid && $useBlockCookie ) {
-                                       // Use the block.
-                                       return $tmpBlock;
-                               }
-
-                               // If the block is not valid, remove the cookie.
-                               Block::clearCookie( $this->getRequest()->response() );
-                       } else {
-                               // If the block doesn't exist, remove the cookie.
-                               Block::clearCookie( $this->getRequest()->response() );
-                       }
-               }
-               return false;
-       }
-
        /**
         * Whether the given IP is in a DNS blacklist.
         *
+        * @deprecated since 1.34 Use BlockManager::isDnsBlacklisted.
         * @param string $ip IP to check
         * @param bool $checkWhitelist Whether to check the whitelist first
         * @return bool True if blacklisted.
         */
        public function isDnsBlacklisted( $ip, $checkWhitelist = false ) {
-               global $wgEnableDnsBlacklist, $wgDnsBlacklistUrls, $wgProxyWhitelist;
-
-               if ( !$wgEnableDnsBlacklist ||
-                       ( $checkWhitelist && in_array( $ip, $wgProxyWhitelist ) )
-               ) {
-                       return false;
-               }
-
-               return $this->inDnsBlacklist( $ip, $wgDnsBlacklistUrls );
+               return MediaWikiServices::getInstance()->getBlockManager()
+                       ->isDnsBlacklisted( $ip, $checkWhitelist );
        }
 
        /**
         * Whether the given IP is in a given DNS blacklist.
         *
+        * @deprecated since 1.34 Check via BlockManager::isDnsBlacklisted instead.
         * @param string $ip IP to check
         * @param string|array $bases Array of Strings: URL of the DNS blacklist
         * @return bool True if blacklisted.
         */
        public function inDnsBlacklist( $ip, $bases ) {
+               wfDeprecated( __METHOD__, '1.34' );
+
                $found = false;
                // @todo FIXME: IPv6 ???  (https://bugs.php.net/bug.php?id=33170)
                if ( IP::isIPv4( $ip ) ) {
@@ -2045,11 +1925,13 @@ class User implements IDBAccessObject, UserIdentity {
        /**
         * Check if an IP address is in the local proxy list
         *
+        * @deprecated since 1.34 Use BlockManager::getUserBlock instead.
         * @param string $ip
-        *
         * @return bool
         */
        public static function isLocallyBlockedProxy( $ip ) {
+               wfDeprecated( __METHOD__, '1.34' );
+
                global $wgProxyList;
 
                if ( !$wgProxyList ) {
index d5e1879..209ed55 100644 (file)
@@ -1471,10 +1471,12 @@ class AuthManagerTest extends \MediaWikiTestCase {
                        ],
                        'wgProxyWhitelist' => [],
                ] );
+               $this->overrideMwServices();
                $status = $this->manager->checkAccountCreatePermissions( new \User );
                $this->assertFalse( $status->isOK() );
                $this->assertTrue( $status->hasMessage( 'sorbs_create_account_reason' ) );
                $this->setMwGlobals( 'wgProxyWhitelist', [ '127.0.0.1' ] );
+               $this->overrideMwServices();
                $status = $this->manager->checkAccountCreatePermissions( new \User );
                $this->assertTrue( $status->isGood() );
        }
diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
new file mode 100644 (file)
index 0000000..4145665
--- /dev/null
@@ -0,0 +1,226 @@
+<?php
+
+use MediaWiki\Block\BlockManager;
+
+/**
+ * @group Blocking
+ * @group Database
+ * @coversDefaultClass \MediaWiki\Block\BlockManager
+ */
+class BlockManagerTest extends MediaWikiTestCase {
+
+       /** @var User */
+       protected $user;
+
+       /** @var int */
+       protected $sysopId;
+
+       protected function setUp() {
+               parent::setUp();
+
+               $this->user = $this->getTestUser()->getUser();
+               $this->sysopId = $this->getTestSysop()->getUser()->getId();
+       }
+
+       private function getBlockManager( $overrideConfig ) {
+               $blockManagerConfig = array_merge( [
+                       'wgApplyIpBlocksToXff' => true,
+                       'wgCookieSetOnAutoblock' => true,
+                       'wgCookieSetOnIpBlock' => true,
+                       'wgDnsBlacklistUrls' => [],
+                       'wgEnableDnsBlacklist' => true,
+                       'wgProxyList' => [],
+                       'wgProxyWhitelist' => [],
+                       'wgSoftBlockRanges' => [],
+               ], $overrideConfig );
+               return new BlockManager(
+                       $this->user,
+                       $this->user->getRequest(),
+                       ...array_values( $blockManagerConfig )
+               );
+       }
+
+       /**
+        * @dataProvider provideGetBlockFromCookieValue
+        * @covers ::getBlockFromCookieValue
+        */
+       public function testGetBlockFromCookieValue( $options, $expected ) {
+               $blockManager = $this->getBlockManager( [
+                       'wgCookieSetOnAutoblock' => true,
+                       'wgCookieSetOnIpBlock' => true,
+               ] );
+
+               $block = new Block( array_merge( [
+                       'address' => $options[ 'target' ] ?: $this->user,
+                       'by' => $this->sysopId,
+               ], $options[ 'blockOptions' ] ) );
+               $block->insert();
+
+               $class = new ReflectionClass( BlockManager::class );
+               $method = $class->getMethod( 'getBlockFromCookieValue' );
+               $method->setAccessible( true );
+
+               $user = $options[ 'loggedIn' ] ? $this->user : new User();
+               $user->getRequest()->setCookie( 'BlockID', $block->getCookieValue() );
+
+               $this->assertSame( $expected, (bool)$method->invoke(
+                       $blockManager,
+                       $user,
+                       $user->getRequest()
+               ) );
+
+               $block->delete();
+       }
+
+       public static function provideGetBlockFromCookieValue() {
+               return [
+                       'Autoblocking user block' => [
+                               [
+                                       'target' => '',
+                                       'loggedIn' => true,
+                                       'blockOptions' => [
+                                               'enableAutoblock' => true
+                                       ],
+                               ],
+                               true,
+                       ],
+                       'Non-autoblocking user block' => [
+                               [
+                                       'target' => '',
+                                       'loggedIn' => true,
+                                       'blockOptions' => [],
+                               ],
+                               false,
+                       ],
+                       'IP block for anonymous user' => [
+                               [
+                                       'target' => '127.0.0.1',
+                                       'loggedIn' => false,
+                                       'blockOptions' => [],
+                               ],
+                               true,
+                       ],
+                       'IP block for logged in user' => [
+                               [
+                                       'target' => '127.0.0.1',
+                                       'loggedIn' => true,
+                                       'blockOptions' => [],
+                               ],
+                               false,
+                       ],
+                       'IP range block for anonymous user' => [
+                               [
+                                       'target' => '127.0.0.0/8',
+                                       'loggedIn' => false,
+                                       'blockOptions' => [],
+                               ],
+                               true,
+                       ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideIsLocallyBlockedProxy
+        * @covers ::isLocallyBlockedProxy
+        */
+       public function testIsLocallyBlockedProxy( $proxyList, $expected ) {
+               $class = new ReflectionClass( BlockManager::class );
+               $method = $class->getMethod( 'isLocallyBlockedProxy' );
+               $method->setAccessible( true );
+
+               $blockManager = $this->getBlockManager( [
+                       'wgProxyList' => $proxyList
+               ] );
+
+               $ip = '1.2.3.4';
+               $this->assertSame( $expected, $method->invoke( $blockManager, $ip ) );
+       }
+
+       public static function provideIsLocallyBlockedProxy() {
+               return [
+                       'Proxy list is empty' => [ [], false ],
+                       'Proxy list contains IP' => [ [ '1.2.3.4' ], true ],
+                       'Proxy list contains IP as value' => [ [ 'test' => '1.2.3.4' ], true ],
+                       'Proxy list contains range that covers IP' => [ [ '1.2.3.0/16' ], true ],
+               ];
+       }
+
+       /**
+        * @covers ::isLocallyBlockedProxy
+        */
+       public function testIsLocallyBlockedProxyDeprecated() {
+               $proxy = '1.2.3.4';
+
+               $this->hideDeprecated(
+                       'IP addresses in the keys of $wgProxyList (found the following IP ' .
+                       'addresses in keys: ' . $proxy . ', please move them to values)'
+               );
+
+               $class = new ReflectionClass( BlockManager::class );
+               $method = $class->getMethod( 'isLocallyBlockedProxy' );
+               $method->setAccessible( true );
+
+               $blockManager = $this->getBlockManager( [
+                       'wgProxyList' => [ $proxy => 'test' ]
+               ] );
+
+               $ip = '1.2.3.4';
+               $this->assertSame( true, $method->invoke( $blockManager, $ip ) );
+       }
+
+       /**
+        * @dataProvider provideIsDnsBlacklisted
+        * @covers ::isDnsBlacklisted
+        * @covers ::inDnsBlacklist
+        */
+       public function testIsDnsBlacklisted( $options, $expected ) {
+               $blockManager = $this->getBlockManager( [
+                       'wgEnableDnsBlacklist' => true,
+                       'wgDnsBlacklistUrls' => $options[ 'inBlacklist' ] ? [ 'local.wmftest.net' ] : [],
+                       'wgProxyWhitelist' => $options[ 'inWhitelist' ] ? [ '127.0.0.1' ] : [],
+               ] );
+
+               $ip = '127.0.0.1';
+               $this->assertSame(
+                       $expected,
+                       $blockManager->isDnsBlacklisted( $ip, $options[ 'check' ] )
+               );
+       }
+
+       public static function provideIsDnsBlacklisted() {
+               return [
+                       'IP is blacklisted' => [
+                               [
+                                       'inBlacklist' => true,
+                                       'inWhitelist' => false,
+                                       'check' => false,
+                               ],
+                               true,
+                       ],
+                       'IP is not blacklisted' => [
+                               [
+                                       'inBlacklist' => false,
+                                       'inWhitelist' => false,
+                                       'check' => false,
+                               ],
+                               false,
+                       ],
+                       'IP is blacklisted and whitelisted; whitelist is checked' => [
+                               [
+                                       'inBlacklist' => true,
+                                       'inWhitelist' => true,
+                                       'check' => false,
+                               ],
+                               true,
+                       ],
+                       'IP is blacklisted and whitelisted; whitelist is not checked' => [
+                               [
+                                       'inBlacklist' => true,
+                                       'inWhitelist' => true,
+                                       'check' => true,
+                               ],
+                               false,
+                       ],
+               ];
+       }
+}
index e7bedc2..ec127af 100644 (file)
@@ -783,6 +783,7 @@ class UserTest extends MediaWikiTestCase {
                        RequestContext::getMain()->setRequest( $request );
                        TestingAccessWrapper::newFromObject( $user )->mRequest = $request;
                        $request->getSession()->setUser( $user );
+                       $this->overrideMwServices();
                };
                $this->setMwGlobals( 'wgSoftBlockRanges', [ '10.0.0.0/8' ] );
 
@@ -980,7 +981,7 @@ class UserTest extends MediaWikiTestCase {
                $this->assertFalse( $user->getExperienceLevel() );
        }
 
-       public static function provideIsLocallBlockedProxy() {
+       public static function provideIsLocallyBlockedProxy() {
                return [
                        [ '1.2.3.4', '1.2.3.4' ],
                        [ '1.2.3.4', '1.2.3.0/16' ],
@@ -988,10 +989,12 @@ class UserTest extends MediaWikiTestCase {
        }
 
        /**
-        * @dataProvider provideIsLocallBlockedProxy
+        * @dataProvider provideIsLocallyBlockedProxy
         * @covers User::isLocallyBlockedProxy
         */
        public function testIsLocallyBlockedProxy( $ip, $blockListEntry ) {
+               $this->hideDeprecated( 'User::isLocallyBlockedProxy' );
+
                $this->setMwGlobals(
                        'wgProxyList', []
                );