From: Thalia Date: Thu, 6 Jun 2019 18:00:20 +0000 (-0400) Subject: Move cookie-blocking methods to BlockManager X-Git-Tag: 1.34.0-rc.0~1455^2 X-Git-Url: http://git.cyclocoop.org/%7B%24admin_url%7Dmes_infos.php?a=commitdiff_plain;h=c5991f614ff3183ca4272ea33b08ec87e779a786;p=lhc%2Fweb%2Fwiklou.git Move cookie-blocking methods to BlockManager 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 --- diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index dca64bd195..aab90b6413 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -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 === * … diff --git a/includes/EditPage.php b/includes/EditPage.php index 0bcc893ba9..f016a40b47 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -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() ) { diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index c5764d2262..e371b5a5c8 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -102,6 +102,7 @@ return [ $config->get( 'EnableDnsBlacklist' ), $config->get( 'ProxyList' ), $config->get( 'ProxyWhitelist' ), + $config->get( 'SecretKey' ), $config->get( 'SoftBlockRanges' ) ); }, diff --git a/includes/block/AbstractBlock.php b/includes/block/AbstractBlock.php index fb49dfcf72..c7ba68daa2 100644 --- a/includes/block/AbstractBlock.php +++ b/includes/block/AbstractBlock.php @@ -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; } diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php index 8d2fe0c8cd..be240ca10b 100644 --- a/includes/block/BlockManager.php +++ b/includes/block/BlockManager.php @@ -20,10 +20,13 @@ 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; + } + } diff --git a/includes/block/DatabaseBlock.php b/includes/block/DatabaseBlock.php index df9eebefec..876a81f861 100644 --- a/includes/block/DatabaseBlock.php +++ b/includes/block/DatabaseBlock.php @@ -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: diff --git a/includes/specials/SpecialCreateAccount.php b/includes/specials/SpecialCreateAccount.php index 2f87c478bb..8396b4dec1 100644 --- a/includes/specials/SpecialCreateAccount.php +++ b/includes/specials/SpecialCreateAccount.php @@ -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() ); } diff --git a/includes/user/User.php b/includes/user/User.php index c41697f961..4008ff3a55 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -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 ); } /** diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php index aec25c18c0..39a5534027 100644 --- a/tests/phpunit/includes/block/BlockManagerTest.php +++ b/tests/phpunit/includes/block/BlockManagerTest.php @@ -29,6 +29,7 @@ class BlockManagerTest extends MediaWikiTestCase { 'wgEnableDnsBlacklist' => true, 'wgProxyList' => [], 'wgProxyWhitelist' => [], + 'wgSecretKey' => false, 'wgSoftBlockRanges' => [], ]; } diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index aa6ae48ca1..14ddd9f7b6 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -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();