From e3ac564e2d05d747aa80dc3c8db9f8c79ef0ebbb Mon Sep 17 00:00:00 2001
From: Petr Pchelko <ppchelko@wikimedia.org>
Date: Tue, 20 Aug 2019 13:59:49 -0700
Subject: [PATCH] PermissionManager should not cache anonymous rights under ID
 0

Bug: T228253
Change-Id: I8a54830842f220ff1ac4402a3380c2229a99b619
---
 includes/Permissions/PermissionManager.php    | 40 ++++++++++++-------
 .../Permissions/PermissionManagerTest.php     | 22 ++++++++++
 tests/phpunit/includes/user/UserTest.php      |  9 +----
 3 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php
index 43d57a7748..f9ad3ebb93 100644
--- a/includes/Permissions/PermissionManager.php
+++ b/includes/Permissions/PermissionManager.php
@@ -1244,11 +1244,12 @@ class PermissionManager {
 	 */
 	public function getUserPermissions( UserIdentity $user ) {
 		$user = User::newFromIdentity( $user );
-		if ( !isset( $this->usersRights[ $user->getId() ] ) ) {
-			$this->usersRights[ $user->getId() ] = $this->getGroupPermissions(
+		$rightsCacheKey = $this->getRightsCacheKey( $user );
+		if ( !isset( $this->usersRights[ $rightsCacheKey ] ) ) {
+			$this->usersRights[ $rightsCacheKey ] = $this->getGroupPermissions(
 				$user->getEffectiveGroups()
 			);
-			Hooks::run( 'UserGetRights', [ $user, &$this->usersRights[ $user->getId() ] ] );
+			Hooks::run( 'UserGetRights', [ $user, &$this->usersRights[ $rightsCacheKey ] ] );
 
 			// Deny any rights denied by the user's session, unless this
 			// endpoint has no sessions.
@@ -1256,17 +1257,17 @@ class PermissionManager {
 				// FIXME: $user->getRequest().. need to be replaced with something else
 				$allowedRights = $user->getRequest()->getSession()->getAllowedUserRights();
 				if ( $allowedRights !== null ) {
-					$this->usersRights[ $user->getId() ] = array_intersect(
-						$this->usersRights[ $user->getId() ],
+					$this->usersRights[ $rightsCacheKey ] = array_intersect(
+						$this->usersRights[ $rightsCacheKey ],
 						$allowedRights
 					);
 				}
 			}
 
-			Hooks::run( 'UserGetRightsRemove', [ $user, &$this->usersRights[ $user->getId() ] ] );
+			Hooks::run( 'UserGetRightsRemove', [ $user, &$this->usersRights[ $rightsCacheKey ] ] );
 			// Force reindexation of rights when a hook has unset one of them
-			$this->usersRights[ $user->getId() ] = array_values(
-				array_unique( $this->usersRights[ $user->getId() ] )
+			$this->usersRights[ $rightsCacheKey ] = array_values(
+				array_unique( $this->usersRights[ $rightsCacheKey ] )
 			);
 
 			if (
@@ -1275,13 +1276,13 @@ class PermissionManager {
 				$user->getBlock()
 			) {
 				$anon = new User;
-				$this->usersRights[ $user->getId() ] = array_intersect(
-					$this->usersRights[ $user->getId() ],
+				$this->usersRights[ $rightsCacheKey ] = array_intersect(
+					$this->usersRights[ $rightsCacheKey ],
 					$this->getUserPermissions( $anon )
 				);
 			}
 		}
-		$rights = $this->usersRights[ $user->getId() ];
+		$rights = $this->usersRights[ $rightsCacheKey ];
 		foreach ( $this->temporaryUserRights[ $user->getId() ] ?? [] as $overrides ) {
 			$rights = array_values( array_unique( array_merge( $rights, $overrides ) ) );
 		}
@@ -1298,14 +1299,24 @@ class PermissionManager {
 	 */
 	public function invalidateUsersRightsCache( $user = null ) {
 		if ( $user !== null ) {
-			if ( isset( $this->usersRights[ $user->getId() ] ) ) {
-				unset( $this->usersRights[$user->getId()] );
+			$rightsCacheKey = $this->getRightsCacheKey( $user );
+			if ( isset( $this->usersRights[ $rightsCacheKey ] ) ) {
+				unset( $this->usersRights[ $rightsCacheKey ] );
 			}
 		} else {
 			$this->usersRights = null;
 		}
 	}
 
+	/**
+	 * Gets a unique key for user rights cache.
+	 * @param UserIdentity $user
+	 * @return string
+	 */
+	private function getRightsCacheKey( UserIdentity $user ) {
+		return $user->isRegistered() ? "u:{$user->getId()}" : "anon:{$user->getName()}";
+	}
+
 	/**
 	 * Check, if the given group has the given permission
 	 *
@@ -1583,7 +1594,8 @@ class PermissionManager {
 		if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
 			throw new Exception( __METHOD__ . ' can not be called outside of tests' );
 		}
-		$this->usersRights[ $user->getId() ] = is_array( $rights ) ? $rights : [ $rights ];
+		$this->usersRights[ $this->getRightsCacheKey( $user ) ] =
+			is_array( $rights ) ? $rights : [ $rights ];
 	}
 
 }
diff --git a/tests/phpunit/includes/Permissions/PermissionManagerTest.php b/tests/phpunit/includes/Permissions/PermissionManagerTest.php
index 122f377674..44b7f67a31 100644
--- a/tests/phpunit/includes/Permissions/PermissionManagerTest.php
+++ b/tests/phpunit/includes/Permissions/PermissionManagerTest.php
@@ -1865,4 +1865,26 @@ class PermissionManagerTest extends MediaWikiLangTestCase {
 			->getPermissionManager()
 			->getNamespaceRestrictionLevels( $ns, $user ) );
 	}
+
+	/**
+	 * @covers \MediaWiki\Permissions\PermissionManager::getRightsCacheKey
+	 * @throws \Exception
+	 */
+	public function testAnonPermissionsNotClash() {
+		$user1 = User::newFromName( 'User1' );
+		$user2 = User::newFromName( 'User2' );
+		$pm = MediaWikiServices::getInstance()->getPermissionManager();
+		$pm->overrideUserRightsForTesting( $user2, [] );
+		$this->assertNotSame( $pm->getUserPermissions( $user1 ), $pm->getUserPermissions( $user2 ) );
+	}
+
+	/**
+	 * @covers \MediaWiki\Permissions\PermissionManager::getRightsCacheKey
+	 */
+	public function testAnonPermissionsNotClashOneRegistered() {
+		$user1 = User::newFromName( 'User1' );
+		$user2 = $this->getTestSysop()->getUser();
+		$pm = MediaWikiServices::getInstance()->getPermissionManager();
+		$this->assertNotSame( $pm->getUserPermissions( $user1 ), $pm->getUserPermissions( $user2 ) );
+	}
 }
diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php
index f48385d17a..9ff052142f 100644
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -929,13 +929,8 @@ class UserTest extends MediaWikiTestCase {
 		$this->assertFalse( $user->isPingLimitable() );
 
 		$this->setMwGlobals( 'wgRateLimitsExcludedIPs', [] );
-		$noRateLimitUser = $this->getMockBuilder( User::class )->disableOriginalConstructor()
-			->setMethods( [ 'getIP', 'getId', 'getGroups' ] )->getMock();
-		$noRateLimitUser->expects( $this->any() )->method( 'getIP' )->willReturn( '1.2.3.4' );
-		$noRateLimitUser->expects( $this->any() )->method( 'getId' )->willReturn( 0 );
-		$noRateLimitUser->expects( $this->any() )->method( 'getGroups' )->willReturn( [] );
-		$this->overrideUserPermissions( $noRateLimitUser, 'noratelimit' );
-		$this->assertFalse( $noRateLimitUser->isPingLimitable() );
+		$this->overrideUserPermissions( $user, 'noratelimit' );
+		$this->assertFalse( $user->isPingLimitable() );
 	}
 
 	public function provideExperienceLevel() {
-- 
2.20.1