Pass in ServiceOptions to BlockManager

Change-Id: Ic63d7ff35a71e36c4e6157e9d472e2870f95f00d

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 96baf14..7d2b3cb 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -94,17 +94,11 @@ return [
                $config = $services->getMainConfig();
                $context = RequestContext::getMain();
                return new BlockManager(
+                       new ServiceOptions(
+                               BlockManager::$constructorOptions, $services->getMainConfig()
+                       ),
                        $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( 'SecretKey' ),
-                       $config->get( 'SoftBlockRanges' )
+                       $context->getRequest()
                );
        },
 

diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
index 814171f..68141a1 100644 (file)
--- a/includes/block/BlockManager.php
+++ b/includes/block/BlockManager.php
@@ -23,6 +23,7 @@ namespace MediaWiki\Block;
 use DateTime;
 use DeferredUpdates;
 use IP;
+use MediaWiki\Config\ServiceOptions;
 use MediaWiki\User\UserIdentity;
 use MWCryptHash;
 use User;
@@ -44,70 +45,38 @@ class BlockManager {
        /** @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 string|bool */
-       private $secretKey;
-
-       /** @var array */
-       private $softBlockRanges;
+       /**
+        * TODO Make this a const when HHVM support is dropped (T192166)
+        *
+        * @var array
+        * @since 1.34
+        * */
+       public static $constructorOptions = [
+               'ApplyIpBlocksToXff',
+               'CookieSetOnAutoblock',
+               'CookieSetOnIpBlock',
+               'DnsBlacklistUrls',
+               'EnableDnsBlacklist',
+               'ProxyList',
+               'ProxyWhitelist',
+               'SecretKey',
+               'SoftBlockRanges',
+       ];
 
        /**
+        * @param ServiceOptions $options
         * @param User $currentUser
         * @param WebRequest $currentRequest
-        * @param bool $applyIpBlocksToXff
-        * @param bool $cookieSetOnAutoblock
-        * @param bool $cookieSetOnIpBlock
-        * @param string[] $dnsBlacklistUrls
-        * @param bool $enableDnsBlacklist
-        * @param string[] $proxyList
-        * @param string[] $proxyWhitelist
-        * @param string $secretKey
-        * @param array $softBlockRanges
         */
        public function __construct(
+               ServiceOptions $options,
                User $currentUser,
-               WebRequest $currentRequest,
-               $applyIpBlocksToXff,
-               $cookieSetOnAutoblock,
-               $cookieSetOnIpBlock,
-               array $dnsBlacklistUrls,
-               $enableDnsBlacklist,
-               array $proxyList,
-               array $proxyWhitelist,
-               $secretKey,
-               $softBlockRanges
+               WebRequest $currentRequest
        ) {
+               $options->assertRequiredOptions( self::$constructorOptions );
+               $this->options = $options;
                $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->secretKey = $secretKey;
-               $this->softBlockRanges = $softBlockRanges;
        }
 
        /**
@@ -157,7 +126,7 @@ class BlockManager {
                }
 
                // Proxy blocking
-               if ( $ip !== null && !in_array( $ip, $this->proxyWhitelist ) ) {
+               if ( $ip !== null && !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) ) ) {
                        // Local list
                        if ( $this->isLocallyBlockedProxy( $ip ) ) {
                                $blocks[] = new SystemBlock( [
@@ -177,9 +146,9 @@ class BlockManager {
                }
 
                // (T25343) Apply IP blocks to the contents of XFF headers, if enabled
-               if ( $this->applyIpBlocksToXff
+               if ( $this->options->get( 'ApplyIpBlocksToXff' )
                        && $ip !== null
-                       && !in_array( $ip, $this->proxyWhitelist )
+                       && !in_array( $ip, $this->options->get( 'ProxyWhitelist' ) )
                ) {
                        $xff = $request->getHeader( 'X-Forwarded-For' );
                        $xff = array_map( 'trim', explode( ',', $xff ) );
@@ -192,7 +161,7 @@ class BlockManager {
                // Soft blocking
                if ( $ip !== null
                        && $isAnon
-                       && IP::isInRanges( $ip, $this->softBlockRanges )
+                       && IP::isInRanges( $ip, $this->options->get( 'SoftBlockRanges' ) )
                ) {
                        $blocks[] = new SystemBlock( [
                                'address' => $ip,
@@ -295,9 +264,11 @@ class BlockManager {
                                case DatabaseBlock::TYPE_RANGE:
                                        // If block is type IP or IP range, load only
                                        // if user is not logged in (T152462)
-                                       return $isAnon && $this->cookieSetOnIpBlock;
+                                       return $isAnon &&
+                                               $this->options->get( 'CookieSetOnIpBlock' );
                                case DatabaseBlock::TYPE_USER:
-                                       return $this->cookieSetOnAutoblock && $block->isAutoblocking();
+                                       return $block->isAutoblocking() &&
+                                               $this->options->get( 'CookieSetOnAutoblock' );
                                default:
                                        return false;
                        }
@@ -312,20 +283,21 @@ class BlockManager {
         * @return bool
         */
        private function isLocallyBlockedProxy( $ip ) {
-               if ( !$this->proxyList ) {
+               $proxyList = $this->options->get( 'ProxyList' );
+               if ( !$proxyList ) {
                        return false;
                }
 
-               if ( !is_array( $this->proxyList ) ) {
+               if ( !is_array( $proxyList ) ) {
                        // Load values from the specified file
-                       $this->proxyList = array_map( 'trim', file( $this->proxyList ) );
+                       $proxyList = array_map( 'trim', file( $proxyList ) );
                }
 
                $resultProxyList = [];
                $deprecatedIPEntries = [];
 
                // backward compatibility: move all ip addresses in keys to values
-               foreach ( $this->proxyList as $key => $value ) {
+               foreach ( $proxyList as $key => $value ) {
                        $keyIsIP = IP::isIPAddress( $key );
                        $valueIsIP = IP::isIPAddress( $value );
                        if ( $keyIsIP && !$valueIsIP ) {
@@ -358,13 +330,13 @@ class BlockManager {
         * @return bool True if blacklisted.
         */
        public function isDnsBlacklisted( $ip, $checkWhitelist = false ) {
-               if ( !$this->enableDnsBlacklist ||
-                       ( $checkWhitelist && in_array( $ip, $this->proxyWhitelist ) )
+               if ( !$this->options->get( 'EnableDnsBlacklist' ) ||
+                       ( $checkWhitelist && in_array( $ip, $this->options->get( 'ProxyWhitelist' ) ) )
                ) {
                        return false;
                }
 
-               return $this->inDnsBlacklist( $ip, $this->dnsBlacklistUrls );
+               return $this->inDnsBlacklist( $ip, $this->options->get( 'DnsBlacklistUrls' ) );
        }
 
        /**
@@ -506,9 +478,11 @@ class BlockManager {
                        switch ( $block->getType() ) {
                                case DatabaseBlock::TYPE_IP:
                                case DatabaseBlock::TYPE_RANGE:
-                                       return $isAnon && $this->cookieSetOnIpBlock;
+                                       return $isAnon && $this->options->get( 'CookieSetOnIpBlock' );
                                case DatabaseBlock::TYPE_USER:
-                                       return !$isAnon && $this->cookieSetOnAutoblock && $block->isAutoblocking();
+                                       return !$isAnon &&
+                                               $this->options->get( 'CookieSetOnAutoblock' ) &&
+                                               $block->isAutoblocking();
                                default:
                                        return false;
                        }
@@ -545,12 +519,12 @@ class BlockManager {
                // Extract the ID prefix from the cookie value (may be the whole value, if no bang found).
                $bangPos = strpos( $cookieValue, '!' );
                $id = ( $bangPos === false ) ? $cookieValue : substr( $cookieValue, 0, $bangPos );
-               if ( !$this->secretKey ) {
+               if ( !$this->options->get( 'SecretKey' ) ) {
                        // If there's no secret key, just use the ID as given.
                        return $id;
                }
                $storedHmac = substr( $cookieValue, $bangPos + 1 );
-               $calculatedHmac = MWCryptHash::hmac( $id, $this->secretKey, false );
+               $calculatedHmac = MWCryptHash::hmac( $id, $this->options->get( 'SecretKey' ), false );
                if ( $calculatedHmac === $storedHmac ) {
                        return $id;
                } else {
@@ -571,11 +545,11 @@ class BlockManager {
         */
        public function getCookieValue( DatabaseBlock $block ) {
                $id = $block->getId();
-               if ( !$this->secretKey ) {
+               if ( !$this->options->get( 'SecretKey' ) ) {
                        // If there's no secret key, don't append a HMAC.
                        return $id;
                }
-               $hmac = MWCryptHash::hmac( $id, $this->secretKey, false );
+               $hmac = MWCryptHash::hmac( $id, $this->options->get( 'SecretKey' ), false );
                $cookieValue = $id . '!' . $hmac;
                return $cookieValue;
        }

diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
index 0ed5cd6..fe3bb88 100644 (file)
--- a/tests/phpunit/includes/block/BlockManagerTest.php
+++ b/tests/phpunit/includes/block/BlockManagerTest.php
@@ -3,6 +3,8 @@
 use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\DatabaseBlock;
 use MediaWiki\Block\SystemBlock;
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\MediaWikiServices;
 
 /**
  * @group Blocking
@@ -36,14 +38,25 @@ class BlockManagerTest extends MediaWikiTestCase {
        }
 
        private function getBlockManager( $overrideConfig ) {
-               $blockManagerConfig = array_merge( $this->blockManagerConfig, $overrideConfig );
                return new BlockManager(
-                       $this->user,
-                       $this->user->getRequest(),
-                       ...array_values( $blockManagerConfig )
+                       ...$this->getBlockManagerConstructorArgs( $overrideConfig )
                );
        }
 
+       private function getBlockManagerConstructorArgs( $overrideConfig ) {
+               $blockManagerConfig = array_merge( $this->blockManagerConfig, $overrideConfig );
+               $this->setMwGlobals( $blockManagerConfig );
+               $this->overrideMwServices();
+               return [
+                       new ServiceOptions(
+                               BlockManager::$constructorOptions,
+                               MediaWikiServices::getInstance()->getMainConfig()
+                       ),
+                       $this->user,
+                       $this->user->getRequest()
+               ];
+       }
+
        /**
         * @dataProvider provideGetBlockFromCookieValue
         * @covers ::getBlockFromCookieValue
@@ -179,18 +192,14 @@ class BlockManagerTest extends MediaWikiTestCase {
         * @covers ::inDnsBlacklist
         */
        public function testIsDnsBlacklisted( $options, $expected ) {
-               $blockManagerConfig = array_merge( $this->blockManagerConfig, [
+               $blockManagerConfig = [
                        'wgEnableDnsBlacklist' => true,
                        'wgDnsBlacklistUrls' => $options['blacklist'],
                        'wgProxyWhitelist' => $options['whitelist'],
-               ] );
+               ];
 
                $blockManager = $this->getMockBuilder( BlockManager::class )
-                       ->setConstructorArgs(
-                               array_merge( [
-                                       $this->user,
-                                       $this->user->getRequest(),
-                               ], $blockManagerConfig ) )
+                       ->setConstructorArgs( $this->getBlockManagerConstructorArgs( $blockManagerConfig ) )
                        ->setMethods( [ 'checkHost' ] )
                        ->getMock();