From 4b90befa9a76ce692db9b201006654a3b55a8bf1 Mon Sep 17 00:00:00 2001
From: Thalia <thalia.e.chan@googlemail.com>
Date: Wed, 26 Jun 2019 11:04:50 +0100
Subject: [PATCH] Tidy up conditions for applying a block from a cookie

Change-Id: Id9dd6ae395f5bb811db4c741be9db8aa2eb6fb70
---
 includes/block/BlockManager.php               | 72 ++++++++++---------
 .../includes/block/BlockManagerTest.php       |  1 +
 2 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
index c58537e388..814171f5d1 100644
--- a/includes/block/BlockManager.php
+++ b/includes/block/BlockManager.php
@@ -253,7 +253,8 @@ class BlockManager {
 	}
 
 	/**
-	 * Try to load a block from an ID given in a cookie value.
+	 * Try to load a block from an ID given in a cookie value. If the block is invalid
+	 * or doesn't exist, remove the cookie.
 	 *
 	 * @param UserIdentity $user
 	 * @param WebRequest $request
@@ -263,43 +264,43 @@ class BlockManager {
 		UserIdentity $user,
 		WebRequest $request
 	) {
-		$blockCookieVal = $request->getCookie( 'BlockID' );
-		$response = $request->response();
+		$blockCookieId = $this->getIdFromCookieValue( $request->getCookie( 'BlockID' ) );
 
-		// 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 = $this->getIdFromCookieValue( $blockCookieVal );
 		if ( $blockCookieId !== null ) {
-			// An ID was found in the cookie.
 			// TODO: remove dependency on DatabaseBlock
-			$tmpBlock = DatabaseBlock::newFromID( $blockCookieId );
-			if ( $tmpBlock instanceof DatabaseBlock ) {
-				switch ( $tmpBlock->getType() ) {
-					case DatabaseBlock::TYPE_USER:
-						$blockIsValid = !$tmpBlock->isExpired() && $tmpBlock->isAutoblocking();
-						$useBlockCookie = ( $this->cookieSetOnAutoblock === true );
-						break;
-					case DatabaseBlock::TYPE_IP:
-					case DatabaseBlock::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;
-				}
+			$block = DatabaseBlock::newFromID( $blockCookieId );
+			if (
+				$block instanceof DatabaseBlock &&
+				$this->shouldApplyCookieBlock( $block, $user->isAnon() )
+			) {
+				return $block;
+			}
+			$this->clearBlockCookie( $request->response() );
+		}
 
-				if ( $blockIsValid && $useBlockCookie ) {
-					// Use the block.
-					return $tmpBlock;
-				}
+		return false;
+	}
+
+	/**
+	 * Check if the block loaded from the cookie should be applied.
+	 *
+	 * @param DatabaseBlock $block
+	 * @param bool $isAnon The user is logged out
+	 * @return bool The block sould be applied
+	 */
+	private function shouldApplyCookieBlock( DatabaseBlock $block, $isAnon ) {
+		if ( !$block->isExpired() ) {
+			switch ( $block->getType() ) {
+				case DatabaseBlock::TYPE_IP:
+				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;
+				case DatabaseBlock::TYPE_USER:
+					return $this->cookieSetOnAutoblock && $block->isAutoblocking();
+				default:
+					return false;
 			}
-			// If the block is invalid or doesn't exist, remove the cookie.
-			$this->clearBlockCookie( $response );
 		}
 		return false;
 	}
@@ -536,6 +537,11 @@ class BlockManager {
 	 * @return int|null The block ID, or null if the HMAC is present and invalid.
 	 */
 	public function getIdFromCookieValue( $cookieValue ) {
+		// The cookie value must start with a number
+		if ( !is_numeric( substr( $cookieValue, 0, 1 ) ) ) {
+			return null;
+		}
+
 		// 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 );
diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
index 892add9786..0ed5cd6133 100644
--- a/tests/phpunit/includes/block/BlockManagerTest.php
+++ b/tests/phpunit/includes/block/BlockManagerTest.php
@@ -47,6 +47,7 @@ class BlockManagerTest extends MediaWikiTestCase {
 	/**
 	 * @dataProvider provideGetBlockFromCookieValue
 	 * @covers ::getBlockFromCookieValue
+	 * @covers ::shouldApplyCookieBlock
 	 */
 	public function testGetBlockFromCookieValue( $options, $expected ) {
 		$blockManager = $this->getBlockManager( [
-- 
2.20.1