UserGroupManager: Consider $queryFlags when caching

Bug: T254282
Change-Id: I41a31ba6d597c11c7d7c8e753aa0e4c29303d5d9

includes/user/User.php

@@ -1391,7 +1391,11 @@ class User implements IDBAccessObject, UserIdentity {
 			if ( isset( $data['user_groups'] ) && is_array( $data['user_groups'] ) ) {
 				MediaWikiServices::getInstance()
 					->getUserGroupManager()
-					->loadGroupMembershipsFromArray( $this, $data['user_groups'] );
+					->loadGroupMembershipsFromArray(
+						$this,
+						$data['user_groups'],
+						$this->queryFlagsUsed
+					);
 			}
 			if ( isset( $data['user_properties'] ) && is_array( $data['user_properties'] ) ) {
 				MediaWikiServices::getInstance()
@@ -3039,7 +3043,7 @@ class User implements IDBAccessObject, UserIdentity {
 	public function getAutomaticGroups( $recache = false ) {
 		return MediaWikiServices::getInstance()
 			->getUserGroupManager()
-			->getUserImplicitGroups( $this, $recache );
+			->getUserImplicitGroups( $this, $this->queryFlagsUsed, $recache );
 	}
 
 	/**

includes/user/UserGroupManager.php

@@ -87,6 +87,19 @@ class UserGroupManager implements IDBAccessObject {
 	 */
 	private $userGroupCache = [];
 
+	/**
+	 * @var array An assoc. array that stores query flags used to retrieve user groups
+	 * from the database and is stored in the following format:
+	 *
+	 * userKey => [
+	 *   'implicit' => implicit groups query flag
+	 *   'effective' => effective groups query flag
+	 *   'membership' => membership groups query flag
+	 *   'former' => former groups query flag
+	 * ]
+	 */
+	private $queryFlagsUsedForCaching = [];
+
 	/**
 	 * @param ServiceOptions $options
 	 * @param ConfiguredReadOnlyMode $configuredReadOnlyMode
@@ -164,19 +177,20 @@ class UserGroupManager implements IDBAccessObject {
 	 * @internal for use by the User object only
 	 * @param UserIdentity $user
 	 * @param array $userGroups an array of database query results
+	 * @param int $queryFlags
 	 */
 	public function loadGroupMembershipsFromArray(
 		UserIdentity $user,
-		array $userGroups
+		array $userGroups,
+		int $queryFlags = self::READ_NORMAL
 	) {
-		$userKey = $this->getCacheKey( $user );
-
-		$this->userGroupCache[$userKey]['membership'] = [];
+		$membershipGroups = [];
 		reset( $userGroups );
 		foreach ( $userGroups as $row ) {
 			$ugm = $this->newGroupMembershipFromRow( $row );
-			$this->userGroupCache[ $userKey ]['membership'][ $ugm->getGroup() ] = $ugm;
+			$membershipGroups[ $ugm->getGroup() ] = $ugm;
 		}
+		$this->setCache( $user, 'membership', $membershipGroups, $queryFlags );
 	}
 
 	/**
@@ -186,12 +200,20 @@ class UserGroupManager implements IDBAccessObject {
 	 * and autopromoted groups
 	 *
 	 * @param UserIdentity $user
+	 * @param int $queryFlags
 	 * @param bool $recache Whether to avoid the cache
 	 * @return string[] internal group names
 	 */
-	public function getUserImplicitGroups( UserIdentity $user, bool $recache = false ) : array {
+	public function getUserImplicitGroups(
+		UserIdentity $user,
+		int $queryFlags = self::READ_NORMAL,
+		bool $recache = false
+	)  : array {
 		$userKey = $this->getCacheKey( $user );
-		if ( $recache || !isset( $this->userGroupCache[$userKey]['implicit'] ) ) {
+		if ( $recache ||
+			!isset( $this->userGroupCache[$userKey]['implicit'] ) ||
+			!$this->canUseCachedValues( $user, 'implicit', $queryFlags )
+		) {
 			$groups = [ '*' ];
 			if ( $user->isRegistered() ) {
 				$groups[] = 'user';
@@ -203,11 +225,11 @@ class UserGroupManager implements IDBAccessObject {
 					Autopromote::getAutopromoteGroups( User::newFromIdentity( $user ) )
 				) );
 			}
-			$this->userGroupCache[$userKey]['implicit'] = $groups;
+			$this->setCache( $user, 'implicit', $groups, $queryFlags );
 			if ( $recache ) {
 				// Assure data consistency with rights/groups,
 				// as getEffectiveGroups() depends on this function
-				unset( $this->userGroupCache[$userKey]['effective'] );
+				$this->clearUserCacheForGroup( $user, 'effective' );
 			}
 		}
 		return $this->userGroupCache[$userKey]['implicit'];
@@ -230,15 +252,15 @@ class UserGroupManager implements IDBAccessObject {
 		bool $recache = false
 	) : array {
 		$userKey = $this->getCacheKey( $user );
-		// Ignore cache is the $recache flag is set, query flags = READ_NORMAL
+		// Ignore cache if the $recache flag is set, cached values can not be used
 		// or the cache value is missing
-		if ( $recache
-			|| $queryFlags !== self::READ_NORMAL
-			|| !isset( $this->userGroupCache[$userKey]['effective'] )
+		if ( $recache ||
+			!$this->canUseCachedValues( $user, 'effective', $queryFlags ) ||
+			!isset( $this->userGroupCache[$userKey]['effective'] )
 		) {
 			$groups = array_unique( array_merge(
 				$this->getUserGroups( $user, $queryFlags ), // explicit groups
-				$this->getUserImplicitGroups( $user, $recache ) // implicit groups
+				$this->getUserImplicitGroups( $user, $queryFlags, $recache ) // implicit groups
 			) );
 			// TODO: Deprecate passing out user object in the hook by introducing
 			// an alternative hook
@@ -249,7 +271,8 @@ class UserGroupManager implements IDBAccessObject {
 				$this->hookRunner->onUserEffectiveGroups( $userObj, $groups );
 			}
 			// Force reindexation of groups when a hook has unset one of them
-			$this->userGroupCache[$userKey]['effective'] = array_values( array_unique( $groups ) );
+			$effectiveGroups = array_values( array_unique( $groups ) );
+			$this->setCache( $user, 'effective', $effectiveGroups, $queryFlags );
 		}
 		return $this->userGroupCache[$userKey]['effective'];
 	}
@@ -271,10 +294,17 @@ class UserGroupManager implements IDBAccessObject {
 	) : array {
 		$userKey = $this->getCacheKey( $user );
 
-		if ( isset( $this->userGroupCache[$userKey]['former'] ) ) {
+		if ( $this->canUseCachedValues( $user, 'former', $queryFlags ) &&
+			isset( $this->userGroupCache[$userKey]['former'] )
+		) {
 			return $this->userGroupCache[$userKey]['former'];
 		}
 
+		if ( !$user->isRegistered() ) {
+			// Anon users don't have groups stored in the database
+			return [];
+		}
+
 		$db = $this->getDBConnectionRefForQueryFlags( $queryFlags );
 		$res = $db->select(
 			'user_former_groups',
@@ -282,10 +312,11 @@ class UserGroupManager implements IDBAccessObject {
 			[ 'ufg_user' => $user->getId() ],
 			__METHOD__
 		);
-		$this->userGroupCache[$userKey]['former'] = [];
+		$formerGroups = [];
 		foreach ( $res as $row ) {
-			$this->userGroupCache[$userKey]['former'][] = $row->ufg_group;
+			$formerGroups[] = $row->ufg_group;
 		}
+		$this->setCache( $user, 'former', $formerGroups, $queryFlags );
 
 		return $this->userGroupCache[$userKey]['former'];
 	}
@@ -319,15 +350,18 @@ class UserGroupManager implements IDBAccessObject {
 	) : array {
 		$userKey = $this->getCacheKey( $user );
 
-		// Return cached value (if any) only if the query flags are for READ_NORMAL
-		// otherwise - ignore cache
-		if ( $queryFlags === self::READ_NORMAL
-			&& isset( $this->userGroupCache[$userKey]['membership'] )
+		if ( $this->canUseCachedValues( $user, 'membership', $queryFlags ) &&
+			isset( $this->userGroupCache[$userKey]['membership'] )
 		) {
 			/** @suppress PhanTypeMismatchReturn */
 			return $this->userGroupCache[$userKey]['membership'];
 		}
 
+		if ( !$user->isRegistered() ) {
+			// Anon users don't have groups stored in the database
+			return [];
+		}
+
 		$db = $this->getDBConnectionRefForQueryFlags( $queryFlags );
 		$queryInfo = $this->getQueryInfo();
 		$res = $db->select(
@@ -348,7 +382,8 @@ class UserGroupManager implements IDBAccessObject {
 		}
 		ksort( $ugms );
 
-		$this->userGroupCache[$userKey]['membership'] = $ugms;
+		$this->setCache( $user, 'membership', $ugms, $queryFlags );
+
 		return $ugms;
 	}
 
@@ -470,6 +505,7 @@ class UserGroupManager implements IDBAccessObject {
 	 *
 	 * @param UserIdentity $user
 	 * @param string $group Name of the group to remove
+	 * @throws InvalidArgumentException
 	 * @return bool
 	 */
 	public function removeUserFromGroup( UserIdentity $user, string $group ) : bool {
@@ -487,6 +523,13 @@ class UserGroupManager implements IDBAccessObject {
 			return false;
 		}
 
+		if ( !$user->isRegistered() ) {
+			throw new InvalidArgumentException(
+				'UserGroupManager::removeUserFromGroup() needs a positive user ID. ' .
+				'Perhaps removeUserFromGroup() was called before the user was added to the database.'
+			);
+		}
+
 		$dbw = $this->loadBalancer->getConnectionRef( DB_MASTER, [], $this->dbDomain );
 		$dbw->delete(
 			'user_groups',
@@ -614,6 +657,33 @@ class UserGroupManager implements IDBAccessObject {
 	public function clearCache( UserIdentity $user ) {
 		$userKey = $this->getCacheKey( $user );
 		unset( $this->userGroupCache[$userKey] );
+		unset( $this->queryFlagsUsedForCaching[$userKey] );
+	}
+
+	/**
+	 * Sets cached group memberships and query flags for a given user
+	 *
+	 * @param UserIdentity $user
+	 * @param string $group
+	 * @param array $groupValue
+	 * @param int $queryFlags
+	 */
+	private function setCache( UserIdentity $user, string $group, array $groupValue, int $queryFlags ) {
+		$userKey = $this->getCacheKey( $user );
+		$this->userGroupCache[$userKey][$group] = $groupValue;
+		$this->queryFlagsUsedForCaching[$userKey][$group] = $queryFlags;
+	}
+
+	/**
+	 * Clears a cached group membership and query key for a given user
+	 *
+	 * @param UserIdentity $user
+	 * @param string $group
+	 */
+	private function clearUserCacheForGroup( UserIdentity $user, string $group ) {
+		$userKey = $this->getCacheKey( $user );
+		unset( $this->userGroupCache[$userKey][$group] );
+		unset( $this->queryFlagsUsedForCaching[$userKey][$group] );
 	}
 
 	/**
@@ -633,4 +703,25 @@ class UserGroupManager implements IDBAccessObject {
 	private function getCacheKey( UserIdentity $user ) : string {
 		return $user->isRegistered() ? "u:{$user->getId()}" : "anon:{$user->getName()}";
 	}
+
+	/**
+	 * Determines if it's ok to use cached options values for a given user and query flags
+	 * @param UserIdentity $user
+	 * @param string $group
+	 * @param int $queryFlags
+	 * @return bool
+	 */
+	private function canUseCachedValues( UserIdentity $user, string $group, int $queryFlags ) : bool {
+		if ( !$user->isRegistered() ) {
+			// Anon users don't have groups stored in the database,
+			// so $queryFlags are ignored.
+			return true;
+		}
+		if ( $queryFlags >= self::READ_LOCKING ) {
+			return false;
+		}
+		$userKey = $this->getCacheKey( $user );
+		$queryFlagsUsed = $this->queryFlagsUsedForCaching[$userKey][$group] ?? self::READ_NONE;
+		return $queryFlagsUsed >= $queryFlags;
+	}
 }

tests/phpunit/includes/user/UserGroupManagerTest.php

@@ -171,7 +171,7 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase {
 		$user->confirmEmail();
 		$this->assertArrayEquals(
 			[ '*', 'user', 'dummy' ],
-			$manager->getUserImplicitGroups( $user, true )
+			$manager->getUserImplicitGroups( $user, UserGroupManager::READ_NORMAL, true )
 		);
 		$this->assertArrayEquals(
 			[ '*', 'user', 'dummy' ],
@@ -307,6 +307,26 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase {
 		$this->assertNull( $manager->getUserGroupMemberships( $user )['from_hook']->getExpiry() );
 	}
 
+	/**
+	 * @covers \MediaWiki\User\UserGroupManager::getUserGroupMemberships
+	 */
+	public function testGetUserGroupMembershipsForAnon() {
+		$manager = $this->getManager();
+		$anon = new UserIdentityValue( 0, 'Anon', 0 );
+
+		$this->assertEmpty( $manager->getUserGroupMemberships( $anon ) );
+	}
+
+	/**
+	 * @covers \MediaWiki\User\UserGroupManager::getUserFormerGroups
+	 */
+	public function testGetUserFormerGroupsForAnon() {
+		$manager = $this->getManager();
+		$anon = new UserIdentityValue( 0, 'Anon', 0 );
+
+		$this->assertEmpty( $manager->getUserFormerGroups( $anon ) );
+	}
+
 	/**
 	 * @covers \MediaWiki\User\UserGroupManager::removeUserFromGroup
 	 * @covers \MediaWiki\User\UserGroupManager::getUserFormerGroups
@@ -386,6 +406,16 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase {
 		$this->assertContains( 'test', $manager->getUserGroups( $user ) );
 	}
 
+	/**
+	 * @covers \MediaWiki\User\UserGroupManager::removeUserFromGroup
+	 */
+	public function testRemoveUserFromGroupAnon() {
+		$manager = $this->getManager();
+		$anon = new UserIdentityValue( 0, 'Anon', 0 );
+		$this->expectException( InvalidArgumentException::class );
+		$manager->removeUserFromGroup( $anon, 'test' );
+	}
+
 	/**
 	 * @covers \MediaWiki\User\UserGroupManager::removeUserFromGroup
 	 */
@@ -467,7 +497,7 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase {
 		$row->ug_user = $user->getId();
 		$row->ug_group = 'test';
 		$row->ug_expiry = null;
-		$manager->loadGroupMembershipsFromArray( $user, [ $row ] );
+		$manager->loadGroupMembershipsFromArray( $user, [ $row ], UserGroupManager::READ_NORMAL );
 		$memberships = $manager->getUserGroupMemberships( $user );
 		$this->assertCount( 1, $memberships );
 		$this->assertArrayHasKey( 'test', $memberships );