Move cookie-blocking methods to BlockManager
authorThalia <thalia.e.chan@googlemail.com>
Thu, 6 Jun 2019 18:00:20 +0000 (14:00 -0400)
committerThalia <thalia.e.chan@googlemail.com>
Tue, 11 Jun 2019 14:08:21 +0000 (15:08 +0100)
Move the cookie blocking logic into one place. Specifically, move
these methods to the BlockManager:
* User::trackBlockWithCookie
* DatabaseBlock::setCookie
* DatabaseBlock::clearCookie
* DatabaseBlock::getCookieValue
* DatabaseBlock::getIdFromCookieValue
* AbstractBlock::shouldTrackWithCookie

After this, BlockManager::trackBlockWithCookie should be called to
track a block, and BlockManager::clearBlockCookie should be called
to unset the cookie. The other methods in the above list are
helper methods that are made private or marked internal.

Also update places in core that call User::trackBlockWithCookie to
BlockManager::trackBlockWithCookie

Bug: T225141
Change-Id: I818962c6932c01c841a549a101637e00a7593e48

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 dca64bd..aab90b6 100644 (file)
@@ -241,6 +241,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 0bcc893..f016a40 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 c41697f..4008ff3 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();