Merge "Move cookie-blocking methods to BlockManager"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 11 Jun 2019 15:16:00 +0000 (15:16 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 11 Jun 2019 15:16:00 +0000 (15:16 +0000)
RELEASE-NOTES-1.34
includes/EditPage.php
includes/ServiceWiring.php
includes/block/AbstractBlock.php
includes/block/BlockManager.php
includes/block/DatabaseBlock.php
includes/specials/SpecialCreateAccount.php
includes/user/User.php
tests/phpunit/includes/block/BlockManagerTest.php
tests/phpunit/includes/user/UserTest.php

index 76fa583..3d90ac8 100644 (file)
@@ -250,6 +250,11 @@ because of Phabricator reports.
 * (T62260) Hard deprecate Language::getExtraUserToggles() method.
 * Language::viewPrevNext function is deprecated, use
   PrevNextNavigationRenderer::buildPrevNextNavigation instead
+* User::trackBlockWithCookie and DatabaseBlock::clearCookie are deprecated. Use
+  BlockManager::trackBlockWithCookie and BlockManager::clearCookie instead.
+* DatabaseBlock::setCookie, DatabaseBlock::getCookieValue,
+  DatabaseBlock::getIdFromCookieValue and AbstractBlock::shouldTrackWithCookie
+  are moved to internal helper methods for BlockManager::trackBlockWithCookie.
 
 === Other changes in 1.34 ===
 * …
index 2eeede9..e4adb48 100644 (file)
@@ -622,7 +622,8 @@ class EditPage {
 
                        if ( $this->context->getUser()->getBlock() ) {
                                // track block with a cookie if it doesn't exists already
-                               $this->context->getUser()->trackBlockWithCookie();
+                               MediaWikiServices::getInstance()->getBlockManager()
+                                       ->trackBlockWithCookie( $this->context->getUser() );
 
                                // Auto-block user's IP if the account was "hard" blocked
                                if ( !wfReadOnly() ) {
index c5764d2..e371b5a 100644 (file)
@@ -102,6 +102,7 @@ return [
                        $config->get( 'EnableDnsBlacklist' ),
                        $config->get( 'ProxyList' ),
                        $config->get( 'ProxyWhitelist' ),
+                       $config->get( 'SecretKey' ),
                        $config->get( 'SoftBlockRanges' )
                );
        },
index fb49dfc..c7ba68d 100644 (file)
@@ -655,10 +655,13 @@ abstract class AbstractBlock {
         * Check if the block should be tracked with a cookie.
         *
         * @since 1.33
+        * @deprecated since 1.34 Use BlockManager::trackBlockWithCookie instead
+        *  of calling this directly.
         * @param bool $isAnon The user is logged out
         * @return bool The block should be tracked with a cookie
         */
        public function shouldTrackWithCookie( $isAnon ) {
+               wfDeprecated( __METHOD__, '1.34' );
                return false;
        }
 
index 8d2fe0c..be240ca 100644 (file)
 
 namespace MediaWiki\Block;
 
+use DateTime;
 use IP;
 use MediaWiki\User\UserIdentity;
+use MWCryptHash;
 use User;
 use WebRequest;
+use WebResponse;
 use Wikimedia\IPSet;
 
 /**
@@ -61,6 +64,9 @@ class BlockManager {
        /** @var array */
        private $proxyWhitelist;
 
+       /** @var string|bool */
+       private $secretKey;
+
        /** @var array */
        private $softBlockRanges;
 
@@ -74,6 +80,7 @@ class BlockManager {
         * @param bool $enableDnsBlacklist
         * @param array $proxyList
         * @param array $proxyWhitelist
+        * @param string $secretKey
         * @param array $softBlockRanges
         */
        public function __construct(
@@ -86,6 +93,7 @@ class BlockManager {
                $enableDnsBlacklist,
                $proxyList,
                $proxyWhitelist,
+               $secretKey,
                $softBlockRanges
        ) {
                $this->currentUser = $currentUser;
@@ -97,6 +105,7 @@ class BlockManager {
                $this->enableDnsBlacklist = $enableDnsBlacklist;
                $this->proxyList = $proxyList;
                $this->proxyWhitelist = $proxyWhitelist;
+               $this->secretKey = $secretKey;
                $this->softBlockRanges = $softBlockRanges;
        }
 
@@ -221,8 +230,7 @@ class BlockManager {
                        return false;
                }
                // Load the block from the ID in the cookie.
-               // TODO: remove dependency on DatabaseBlock
-               $blockCookieId = DatabaseBlock::getIdFromCookieValue( $blockCookieVal );
+               $blockCookieId = $this->getIdFromCookieValue( $blockCookieVal );
                if ( $blockCookieId !== null ) {
                        // An ID was found in the cookie.
                        // TODO: remove dependency on DatabaseBlock
@@ -248,15 +256,9 @@ class BlockManager {
                                        // Use the block.
                                        return $tmpBlock;
                                }
-
-                               // If the block is not valid, remove the cookie.
-                               // TODO: remove dependency on DatabaseBlock
-                               DatabaseBlock::clearCookie( $response );
-                       } else {
-                               // If the block doesn't exist, remove the cookie.
-                               // TODO: remove dependency on DatabaseBlock
-                               DatabaseBlock::clearCookie( $response );
                        }
+                       // If the block is invalid or doesn't exist, remove the cookie.
+                       $this->clearBlockCookie( $response );
                }
                return false;
        }
@@ -382,4 +384,131 @@ class BlockManager {
                return gethostbynamel( $hostname );
        }
 
+       /**
+        * Set the 'BlockID' cookie depending on block type and user authentication status.
+        *
+        * @since 1.34
+        * @param User $user
+        */
+       public function trackBlockWithCookie( User $user ) {
+               $block = $user->getBlock();
+               $request = $user->getRequest();
+
+               if (
+                       $block &&
+                       $request->getCookie( 'BlockID' ) === null &&
+                       $this->shouldTrackBlockWithCookie( $block, $user->isAnon() )
+               ) {
+                       $this->setBlockCookie( $block, $request->response() );
+               }
+       }
+
+       /**
+        * Set the 'BlockID' cookie to this block's ID and expiry time. The cookie's expiry will be
+        * the same as the block's, to a maximum of 24 hours.
+        *
+        * @since 1.34
+        * @internal Should be private.
+        *  Left public for backwards compatibility, until DatabaseBlock::setCookie is removed.
+        * @param DatabaseBlock $block
+        * @param WebResponse $response The response on which to set the cookie.
+        */
+       public function setBlockCookie( DatabaseBlock $block, WebResponse $response ) {
+               // Calculate the default expiry time.
+               $maxExpiryTime = wfTimestamp( TS_MW, wfTimestamp() + ( 24 * 60 * 60 ) );
+
+               // Use the block's expiry time only if it's less than the default.
+               $expiryTime = $block->getExpiry();
+               if ( $expiryTime === 'infinity' || $expiryTime > $maxExpiryTime ) {
+                       $expiryTime = $maxExpiryTime;
+               }
+
+               // Set the cookie. Reformat the MediaWiki datetime as a Unix timestamp for the cookie.
+               $expiryValue = DateTime::createFromFormat( 'YmdHis', $expiryTime )->format( 'U' );
+               $cookieOptions = [ 'httpOnly' => false ];
+               $cookieValue = $this->getCookieValue( $block );
+               $response->setCookie( 'BlockID', $cookieValue, $expiryValue, $cookieOptions );
+       }
+
+       /**
+        * Check if the block should be tracked with a cookie.
+        *
+        * @param AbstractBlock $block
+        * @param bool $isAnon The user is logged out
+        * @return bool The block sould be tracked with a cookie
+        */
+       private function shouldTrackBlockWithCookie( AbstractBlock $block, $isAnon ) {
+               if ( $block instanceof DatabaseBlock ) {
+                       switch ( $block->getType() ) {
+                               case DatabaseBlock::TYPE_IP:
+                               case DatabaseBlock::TYPE_RANGE:
+                                       return $isAnon && $this->cookieSetOnIpBlock;
+                               case DatabaseBlock::TYPE_USER:
+                                       return !$isAnon && $this->cookieSetOnAutoblock && $block->isAutoblocking();
+                               default:
+                                       return false;
+                       }
+               }
+               return false;
+       }
+
+       /**
+        * Unset the 'BlockID' cookie.
+        *
+        * @since 1.34
+        * @param WebResponse $response
+        */
+       public static function clearBlockCookie( WebResponse $response ) {
+               $response->clearCookie( 'BlockID', [ 'httpOnly' => false ] );
+       }
+
+       /**
+        * Get the stored ID from the 'BlockID' cookie. The cookie's value is usually a combination of
+        * the ID and a HMAC (see DatabaseBlock::setCookie), but will sometimes only be the ID.
+        *
+        * @since 1.34
+        * @internal Should be private.
+        *  Left public for backwards compatibility, until DatabaseBlock::getIdFromCookieValue is removed.
+        * @param string $cookieValue The string in which to find the ID.
+        * @return int|null The block ID, or null if the HMAC is present and invalid.
+        */
+       public function getIdFromCookieValue( $cookieValue ) {
+               // 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 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 );
+               if ( $calculatedHmac === $storedHmac ) {
+                       return $id;
+               } else {
+                       return null;
+               }
+       }
+
+       /**
+        * Get the BlockID cookie's value for this block. This is usually the block ID concatenated
+        * with an HMAC in order to avoid spoofing (T152951), but if wgSecretKey is not set will just
+        * be the block ID.
+        *
+        * @since 1.34
+        * @internal Should be private.
+        *  Left public for backwards compatibility, until DatabaseBlock::getCookieValue is removed.
+        * @param DatabaseBlock $block
+        * @return string The block ID, probably concatenated with "!" and the HMAC.
+        */
+       public function getCookieValue( DatabaseBlock $block ) {
+               $id = $block->getId();
+               if ( !$this->secretKey ) {
+                       // If there's no secret key, don't append a HMAC.
+                       return $id;
+               }
+               $hmac = MWCryptHash::hmac( $id, $this->secretKey, false );
+               $cookieValue = $id . '!' . $hmac;
+               return $cookieValue;
+       }
+
 }
index df9eebe..876a81f 100644 (file)
@@ -26,7 +26,6 @@ use ActorMigration;
 use AutoCommitUpdate;
 use BadMethodCallException;
 use CommentStore;
-use DateTime;
 use DeferredUpdates;
 use Hooks;
 use Html;
@@ -36,7 +35,6 @@ use MediaWiki\Block\Restriction\NamespaceRestriction;
 use MediaWiki\Block\Restriction\PageRestriction;
 use MediaWiki\Block\Restriction\Restriction;
 use MediaWiki\MediaWikiServices;
-use MWCryptHash;
 use MWException;
 use RequestContext;
 use stdClass;
@@ -1383,35 +1381,22 @@ class DatabaseBlock extends AbstractBlock {
         * the same as the block's, to a maximum of 24 hours.
         *
         * @since 1.29
-        *
+        * @deprecated since 1.34 Set a cookie via BlockManager::trackBlockWithCookie instead.
         * @param WebResponse $response The response on which to set the cookie.
         */
        public function setCookie( WebResponse $response ) {
-               // Calculate the default expiry time.
-               $maxExpiryTime = wfTimestamp( TS_MW, wfTimestamp() + ( 24 * 60 * 60 ) );
-
-               // Use the block's expiry time only if it's less than the default.
-               $expiryTime = $this->getExpiry();
-               if ( $expiryTime === 'infinity' || $expiryTime > $maxExpiryTime ) {
-                       $expiryTime = $maxExpiryTime;
-               }
-
-               // Set the cookie. Reformat the MediaWiki datetime as a Unix timestamp for the cookie.
-               $expiryValue = DateTime::createFromFormat( 'YmdHis', $expiryTime )->format( 'U' );
-               $cookieOptions = [ 'httpOnly' => false ];
-               $cookieValue = $this->getCookieValue();
-               $response->setCookie( 'BlockID', $cookieValue, $expiryValue, $cookieOptions );
+               MediaWikiServices::getInstance()->getBlockManager()->setBlockCookie( $this, $response );
        }
 
        /**
         * Unset the 'BlockID' cookie.
         *
         * @since 1.29
-        *
+        * @deprecated since 1.34 Use BlockManager::clearBlockCookie instead
         * @param WebResponse $response The response on which to unset the cookie.
         */
        public static function clearCookie( WebResponse $response ) {
-               $response->clearCookie( 'BlockID', [ 'httpOnly' => false ] );
+               MediaWikiServices::getInstance()->getBlockManager()->clearBlockCookie( $response );
        }
 
        /**
@@ -1420,20 +1405,12 @@ class DatabaseBlock extends AbstractBlock {
         * be the block ID.
         *
         * @since 1.29
-        *
+        * @deprecated since 1.34 Use BlockManager::trackBlockWithCookie instead of calling this
+        *  directly
         * @return string The block ID, probably concatenated with "!" and the HMAC.
         */
        public function getCookieValue() {
-               $config = RequestContext::getMain()->getConfig();
-               $id = $this->getId();
-               $secretKey = $config->get( 'SecretKey' );
-               if ( !$secretKey ) {
-                       // If there's no secret key, don't append a HMAC.
-                       return $id;
-               }
-               $hmac = MWCryptHash::hmac( $id, $secretKey, false );
-               $cookieValue = $id . '!' . $hmac;
-               return $cookieValue;
+               return MediaWikiServices::getInstance()->getBlockManager()->getCookieValue( $this );
        }
 
        /**
@@ -1441,29 +1418,12 @@ class DatabaseBlock extends AbstractBlock {
         * the ID and a HMAC (see DatabaseBlock::setCookie), but will sometimes only be the ID.
         *
         * @since 1.29
-        *
+        * @deprecated since 1.34 Use BlockManager::getUserBlock instead
         * @param string $cookieValue The string in which to find the ID.
-        *
         * @return int|null The block ID, or null if the HMAC is present and invalid.
         */
        public static function getIdFromCookieValue( $cookieValue ) {
-               // 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 );
-               // Get the site-wide secret key.
-               $config = RequestContext::getMain()->getConfig();
-               $secretKey = $config->get( 'SecretKey' );
-               if ( !$secretKey ) {
-                       // If there's no secret key, just use the ID as given.
-                       return $id;
-               }
-               $storedHmac = substr( $cookieValue, $bangPos + 1 );
-               $calculatedHmac = MWCryptHash::hmac( $id, $secretKey, false );
-               if ( $calculatedHmac === $storedHmac ) {
-                       return $id;
-               } else {
-                       return null;
-               }
+               return MediaWikiServices::getInstance()->getBlockManager()->getIdFromCookieValue( $cookieValue );
        }
 
        /**
@@ -1600,9 +1560,12 @@ class DatabaseBlock extends AbstractBlock {
        }
 
        /**
+        * @deprecated since 1.34 Use BlockManager::trackBlockWithCookie instead of calling this
+        *  directly.
         * @inheritDoc
         */
        public function shouldTrackWithCookie( $isAnon ) {
+               wfDeprecated( __METHOD__, '1.34' );
                $config = RequestContext::getMain()->getConfig();
                switch ( $this->getType() ) {
                        case self::TYPE_IP:
index 2f87c47..8396b4d 100644 (file)
@@ -23,6 +23,7 @@
 
 use MediaWiki\Auth\AuthManager;
 use MediaWiki\Logger\LoggerFactory;
+use MediaWiki\MediaWikiServices;
 
 /**
  * Implements Special:CreateAccount
@@ -65,7 +66,7 @@ class SpecialCreateAccount extends LoginSignupSpecialPage {
                if ( !$status->isGood() ) {
                        // track block with a cookie if it doesn't exists already
                        if ( $user->isBlockedFromCreateAccount() ) {
-                               $user->trackBlockWithCookie();
+                               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $user );
                        }
                        throw new ErrorPageError( 'createacct-error', $status->getMessage() );
                }
index 79b8420..bdcb17b 100644 (file)
@@ -1363,7 +1363,7 @@ class User implements IDBAccessObject, UserIdentity {
                                // If this user is autoblocked, set a cookie to track the block. This has to be done on
                                // every session load, because an autoblocked editor might not edit again from the same
                                // IP address after being blocked.
-                               $this->trackBlockWithCookie();
+                               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $this );
                        }
 
                        // Other code expects these to be set in the session, so set them.
@@ -1379,15 +1379,11 @@ class User implements IDBAccessObject, UserIdentity {
 
        /**
         * Set the 'BlockID' cookie depending on block type and user authentication status.
+        *
+        * @deprecated since 1.34 Use BlockManager::trackBlockWithCookie instead
         */
        public function trackBlockWithCookie() {
-               $block = $this->getBlock();
-
-               if ( $block && $this->getRequest()->getCookie( 'BlockID' ) === null
-                       && $block->shouldTrackWithCookie( $this->isAnon() )
-               ) {
-                       $block->setCookie( $this->getRequest()->response() );
-               }
+               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $this );
        }
 
        /**
index aec25c1..39a5534 100644 (file)
@@ -29,6 +29,7 @@ class BlockManagerTest extends MediaWikiTestCase {
                        'wgEnableDnsBlacklist' => true,
                        'wgProxyList' => [],
                        'wgProxyWhitelist' => [],
+                       'wgSecretKey' => false,
                        'wgSoftBlockRanges' => [],
                ];
        }
index aa6ae48..14ddd9f 100644 (file)
@@ -626,8 +626,10 @@ class UserTest extends MediaWikiTestCase {
                $cookies = $request1->response()->getCookies();
                $this->assertArrayHasKey( 'wmsitetitleBlockID', $cookies );
                $this->assertEquals( $expiryFiveHours, $cookies['wmsitetitleBlockID']['expire'] );
-               $cookieValue = DatabaseBlock::getIdFromCookieValue( $cookies['wmsitetitleBlockID']['value'] );
-               $this->assertEquals( $block->getId(), $cookieValue );
+               $cookieId = MediaWikiServices::getInstance()->getBlockManager()->getIdFromCookieValue(
+                       $cookies['wmsitetitleBlockID']['value']
+               );
+               $this->assertEquals( $block->getId(), $cookieId );
 
                // 2. Create a new request, set the cookies, and see if the (anon) user is blocked.
                $request2 = new FauxRequest();
@@ -1470,7 +1472,7 @@ class UserTest extends MediaWikiTestCase {
 
                // get user
                $user = User::newFromSession( $request );
-               $user->trackBlockWithCookie();
+               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $user );
 
                // test cookie was set
                $cookies = $request->response()->getCookies();
@@ -1506,7 +1508,7 @@ class UserTest extends MediaWikiTestCase {
 
                // get user
                $user = User::newFromSession( $request );
-               $user->trackBlockWithCookie();
+               MediaWikiServices::getInstance()->getBlockManager()->trackBlockWithCookie( $user );
 
                // test cookie was not set
                $cookies = $request->response()->getCookies();