User: use ActorNormalization to insert actor

While creating users, we have several interesting corner cases:

- When creating a new User, we actually rely on the 'unique'
constraint on actor_name. This is important if something calls
'User::createNew' with a name that is already occupied by an
existing anon actor with no user. This is quite a weird corner case,
but there's a test for that. We could probably assimilate this
nicly in actor store by checking whether the user id in the database
for the actor we found is the same as user id in the passed in user identity.
- Even more interesting use-case is 'subsuming' existing actors with
reserved user names. When we call User::newSystemUser, and there is
already an actor with the same reserved name, we 'subsume' that actor
and take over it's actor_id for our new system user. This can now be
done with an upsert. This state of having reserved actor with no user
is not easy to cause, but imports or updating from old MW versions
seem to be able to produce this state. Archeology revealed that
'subsuming' existing actor was added for installer.

Change-Id: I16b2f088217db0283215fc2ef5fb04a3742e1626

2 files changed, 198 insertions, 96 deletions

diff --git a/includes/user/ActorStore.php b/includes/user/ActorStore.php
index 675dc2007740..97ed6afbfe2b 100644
--- a/includes/user/ActorStore.php
+++ b/includes/user/ActorStore.php
@@ -31,6 +31,7 @@ use stdClass;
 use User;
 use Wikimedia\Assert\Assert;
 use Wikimedia\IPUtils;
+use Wikimedia\Rdbms\DBQueryError;
 use Wikimedia\Rdbms\IDatabase;
 use Wikimedia\Rdbms\ILoadBalancer;
 
@@ -423,18 +424,15 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 		if ( !$row || !$row->actor_id ) {
 			return null;
 		}
-
-		$id = (int)$row->actor_id;
-
 		// to cache row
 		$this->newActorFromRow( $row );
 
-		return $id;
+		return (int)$row->actor_id;
 	}
 
 	/**
-	 * Attempt to assign an actor ID to the given $user
-	 * If it is already assigned, return the existing ID.
+	 * Attempt to assign an actor ID to the given $user. If it is already assigned,
+	 * return the existing ID.
 	 *
 	 * @note If called within a transaction, the returned ID might become invalid
 	 * if the transaction is rolled back, so it should not be passed outside of the
@@ -447,7 +445,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 	 *        LoadBalancer provided to the constructor.
 	 *        Not providing a database connection triggers a deprecation warning!
 	 *        In the future, this parameter will be required.
-	 * @return int greater then 0
+	 * @return int actor ID greater then 0
 	 * @throws CannotCreateActorException if no actor ID has been assigned to this $user
 	 */
 	public function acquireActorId( UserIdentity $user, IDatabase $dbw = null ): int {
@@ -462,20 +460,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 			);
 			[ $dbw, ] = $this->getDBConnectionRefForQueryFlags( self::READ_LATEST );
 		}
-		// TODO: we want to assert this user belongs to the correct wiki,
-		// but User objects are always local and we used to use them
-		// on a non-local DB connection. We need to first deprecate this
-		// possibility and then throw on mismatching User object - T273972
-		// $user->assertWiki( $this->wikiId );
-
-		$userName = $this->normalizeUserName( $user->getName() );
-		if ( $userName === null || $userName === '' ) {
-			$userIdForErrorMessage = $user->getId( $this->wikiId );
-			throw new CannotCreateActorException(
-				'Cannot create an actor for a user with no name: ' .
-				"user_id={$userIdForErrorMessage} user_name=\"{$user->getName()}\""
-			);
-		}
+		[ $userId, $userName ] = $this->validateActorForInsertion( $user );
 
 		// allow cache to be used, because if it is in the cache, it already has an actor ID
 		$existingActorId = $this->findActorIdInternal( $userName, $dbw );
@@ -484,14 +469,6 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 			return $existingActorId;
 		}
 
-		$userId = $user->getId( $this->wikiId ) ?: null;
-		if ( $userId === null && $this->userNameUtils->isUsable( $user->getName() ) ) {
-			throw new CannotCreateActorException(
-				'Cannot create an actor for a usable name that is not an existing user: ' .
-				"user_name=\"{$user->getName()}\""
-			);
-		}
-
 		$dbw->insert(
 			'actor',
 			[
@@ -499,9 +476,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 				'actor_name' => $userName,
 			],
 			__METHOD__,
-			[ 'IGNORE' ] );
+			[ 'IGNORE' ]
+		);
+
 		if ( $dbw->affectedRows() ) {
-			$actorId = (int)$dbw->insertId();
+			$actorId = $dbw->insertId();
 		} else {
 			// Outdated cache?
 			// Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot.
@@ -518,25 +497,115 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 			}
 		}
 
-		// Set the actor ID in the User object. To be removed, see T274148.
-		if ( $user instanceof User ) {
-			$user->setActorId( $actorId );
+		$this->attachActorId( $user, $actorId );
+		// Cache row we've just created
+		$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
+		$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
+		return $actorId;
+	}
+
+	/**
+	 * Create a new actor for the given $user. If an actor with this name already exists,
+	 * this method throws.
+	 *
+	 * @note If called within a transaction, the returned ID might become invalid
+	 * if the transaction is rolled back, so it should not be passed outside of the
+	 * transaction context.
+	 *
+	 * @param UserIdentity $user
+	 * @param IDatabase $dbw
+	 * @return int actor ID greater then 0
+	 * @throws CannotCreateActorException if an actor with this name already exist.
+	 * @internal for use in user account creation only.
+	 */
+	public function createNewActor( UserIdentity $user, IDatabase $dbw ) : int {
+		$this->checkDatabaseDomain( $dbw );
+		[ $userId, $userName ] = $this->validateActorForInsertion( $user );
+
+		try {
+			$dbw->insert(
+				'actor',
+				[
+					'actor_user' => $userId,
+					'actor_name' => $userName,
+				],
+				__METHOD__
+			);
+		} catch ( DBQueryError $e ) {
+			// We rely on the database to crash on unique actor_name constraint.
+			throw new CannotCreateActorException( $e->getMessage() );
 		}
+		$actorId = $dbw->insertId();
 
+		$this->attachActorId( $user, $actorId );
 		// Cache row we've just created
 		$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
-		if ( $dbw->trxLevel() ) {
-			// If called within a transaction and it was rolled back, the cached actor ID
-			// becomes invalid, so cache needs to be invalidated as well. See T277795.
-			$dbw->onTransactionResolution(
-				function ( int $trigger ) use ( $actorId, $cachedUserIdentity, $user ) {
-					if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) {
-						$this->deleteUserIdentityFromCache( $actorId, $cachedUserIdentity );
-						$this->detachActorId( $user );
-					}
-				} );
+		$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
+
+		return $actorId;
+	}
+
+	/**
+	 * Attempt to assign an ID to an actor for a system user. If an actor ID already
+	 * exists, return it.
+	 *
+	 * @note For reserved user names this method will overwrite the user ID of the
+	 * existing anon actor.
+	 *
+	 * @note If called within a transaction, the returned ID might become invalid
+	 * if the transaction is rolled back, so it should not be passed outside of the
+	 * transaction context.
+	 *
+	 * @param UserIdentity $user
+	 * @param IDatabase $dbw
+	 * @return int actor ID greater then zero
+	 * @throws CannotCreateActorException if the existing actor is associated with registered user.
+	 * @internal for use in user account creation only.
+	 */
+	public function acquireSystemActorId( UserIdentity $user, IDatabase $dbw ) : int {
+		$this->checkDatabaseDomain( $dbw );
+		[ $userId, $userName ] = $this->validateActorForInsertion( $user );
+
+		$existingActorId = $this->findActorIdInternal( $userName, $dbw );
+		if ( $existingActorId ) {
+			// It certainly will be cached if we just found it.
+			$existingActor = $this->actorsByActorId->get( $existingActorId )[0];
+
+			// If we already have an existing actor with a matching user ID
+			// just return it, nothing to do here.
+			if ( $existingActor->getId( $this->wikiId ) === $user->getId( $this->wikiId ) ) {
+				return $existingActorId;
+			}
+
+			// Allow overwriting user ID for already existing actor with reserved user name, see T236444
+			if ( $this->userNameUtils->isUsable( $userName ) || $existingActor->isRegistered() ) {
+				throw new CannotCreateActorException(
+					'Cannot replace user for existing actor: ' .
+					"actor_id=$existingActorId, new user_id=$userId"
+				);
+			}
+		}
+
+		$dbw->upsert(
+			'actor',
+			[ 'actor_name' => $userName, 'actor_user' => $userId ],
+			[ 'actor_name' ],
+			[ 'actor_user' => $userId ],
+			__METHOD__
+		);
+		if ( !$dbw->affectedRows() ) {
+			throw new CannotCreateActorException(
+				'Failed to replace user for actor: ' .
+				"actor_id=$existingActorId, new user_id=$userId"
+			);
 		}
+		$actorId = $dbw->insertId() ?: $existingActorId;
 
+		$this->deleteUserIdentityFromCache( $actorId, $user );
+		$this->attachActorId( $user, $actorId );
+		// Cache row we've just created
+		$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
+		$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
 		return $actorId;
 	}
 
@@ -566,6 +635,65 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
 	}
 
 	/**
+	 * Validates actor before insertion.
+	 *
+	 * @param UserIdentity $user
+	 * @return array [ $normalizedUserId, $normalizedName ]
+	 */
+	private function validateActorForInsertion( UserIdentity $user ): array {
+		// TODO: we want to assert this user belongs to the correct wiki,
+		// but User objects are always local and we used to use them
+		// on a non-local DB connection. We need to first deprecate this
+		// possibility and then throw on mismatching User object - T273972
+		// $user->assertWiki( $this->wikiId );
+
+		$userName = $this->normalizeUserName( $user->getName() );
+		if ( $userName === null || $userName === '' ) {
+			$userIdForErrorMessage = $user->getId( $this->wikiId );
+			throw new CannotCreateActorException(
+				'Cannot create an actor for a user with no name: ' .
+				"user_id={$userIdForErrorMessage} user_name=\"{$user->getName()}\""
+			);
+		}
+
+		$userId = $user->getId( $this->wikiId ) ?: null;
+		if ( $userId === null && $this->userNameUtils->isUsable( $user->getName() ) ) {
+			throw new CannotCreateActorException(
+				'Cannot create an actor for a usable name that is not an existing user: ' .
+				"user_name=\"{$user->getName()}\""
+			);
+		}
+		return [ $userId, $userName ];
+	}
+
+	/**
+	 * Clear in-process caches if transaction gets rolled back.
+	 *
+	 * @param IDatabase $dbw
+	 * @param int $actorId
+	 * @param UserIdentity $cachedActor
+	 * @param UserIdentity $originalActor
+	 */
+	private function setUpRollbackHandler(
+		IDatabase $dbw,
+		int $actorId,
+		UserIdentity $cachedActor,
+		UserIdentity $originalActor
+	) {
+		if ( $dbw->trxLevel() ) {
+			// If called within a transaction and it was rolled back, the cached actor ID
+			// becomes invalid, so cache needs to be invalidated as well. See T277795.
+			$dbw->onTransactionResolution(
+				function ( int $trigger ) use ( $actorId, $cachedActor, $originalActor ) {
+					if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) {
+						$this->deleteUserIdentityFromCache( $actorId, $cachedActor );
+						$this->detachActorId( $originalActor );
+					}
+				} );
+		}
+	}
+
+	/**
 	 * @param int $queryFlags a bit field composed of READ_XXX flags
 	 * @return array [ IDatabase $db, array $options ]
 	 */
diff --git a/includes/user/User.php b/includes/user/User.php
index 779ed28bd0b0..b12e13d15678 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -39,6 +39,7 @@ use MediaWiki\Session\SessionManager;
 use MediaWiki\Session\Token;
 use MediaWiki\User\UserFactory;
 use MediaWiki\User\UserIdentity;
+use MediaWiki\User\UserIdentityValue;
 use MediaWiki\User\UserNameUtils;
 use MediaWiki\User\UserOptionsLookup;
 use Wikimedia\Assert\Assert;
@@ -862,41 +863,9 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
 
 			// If it's a reserved user that had an anonymous actor created for it at
 			// some point, we need special handling.
-			if ( !$userNameUtils->isValid( $name ) || $userNameUtils->isUsable( $name ) ) {
-				// Not reserved, so just create it.
-				return self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] );
-			}
-
-			// It is reserved. Check for an anonymous actor row.
-			$dbw = $loadBalancer->getConnectionRef( DB_MASTER );
-			return $dbw->doAtomicSection( __METHOD__, function ( IDatabase $dbw, $fname ) use ( $name ) {
-				$row = $dbw->selectRow(
-					'actor',
-					[ 'actor_id' ],
-					[ 'actor_name' => $name, 'actor_user' => null ],
-					$fname,
-					[ 'FOR UPDATE' ]
-				);
-				if ( !$row ) {
-					// No anonymous actor.
-					return self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] );
-				}
-
-				// There is an anonymous actor. Delete the actor row so we can create the user,
-				// then restore the old actor_id so as to not break existing references.
-				// @todo If MediaWiki ever starts using foreign keys for `actor`, this will break things.
-				$dbw->delete( 'actor', [ 'actor_id' => $row->actor_id ], $fname );
-				$user = self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] );
-				$dbw->update(
-					'actor',
-					[ 'actor_id' => $row->actor_id ],
-					[ 'actor_id' => $user->getActorId() ],
-					$fname
-				);
-				$user->clearInstanceCache( 'id' );
-				$user->invalidateCache();
-				return $user;
-			} );
+			return self::insertNewUser( function ( UserIdentity $actor, IDatabase $dbw ) {
+				return MediaWikiServices::getInstance()->getActorStore()->acquireSystemActorId( $actor, $dbw );
+			}, $name, [ 'token' => self::INVALID_TOKEN ] );
 		}
 
 		$user = self::newFromRow( $row );
@@ -3518,10 +3487,22 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
 	 *   - options: An associative array of non-default options.
 	 *   - token: Random authentication token. Do not set.
 	 *   - registration: Registration timestamp. Do not set.
-	 *
 	 * @return User|null User object, or null if the username already exists.
 	 */
 	public static function createNew( $name, $params = [] ) {
+		return self::insertNewUser( function ( UserIdentity $actor, IDatabase $dbw ) {
+			return MediaWikiServices::getInstance()->getActorStore()->createNewActor( $actor, $dbw );
+		}, $name, $params );
+	}
+
+	/**
+	 * See ::createNew
+	 * @param callable $insertActor ( UserIdentity $actor, IDatabase $dbw ): int actor ID,
+	 * @param string $name
+	 * @param array $params
+	 * @return User|null
+	 */
+	private static function insertNewUser( callable $insertActor, $name, $params = [] ) {
 		foreach ( [ 'password', 'newpassword', 'newpass_time', 'password_expires' ] as $field ) {
 			if ( isset( $params[$field] ) ) {
 				wfDeprecated( __METHOD__ . " with param '$field'", '1.27' );
@@ -3558,12 +3539,14 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
 			$fields["user_$name"] = $value;
 		}
 
-		return $dbw->doAtomicSection( __METHOD__, function ( IDatabase $dbw, $fname ) use ( $fields ) {
+		return $dbw->doAtomicSection( __METHOD__, function ( IDatabase $dbw, $fname ) use ( $fields, $insertActor ) {
 			$dbw->insert( 'user', $fields, $fname, [ 'IGNORE' ] );
 			if ( $dbw->affectedRows() ) {
 				$newUser = self::newFromId( $dbw->insertId() );
 				$newUser->mName = $fields['user_name'];
-				$newUser->updateActorId( $dbw );
+				// Don't pass $this, since calling ::getId, ::getName might force ::load
+				// and this user might not be ready for the yet.
+				$newUser->mActorId = $insertActor( new UserIdentityValue( $newUser->mId, $newUser->mName ), $dbw );
 				// Load the user from master to avoid replica lag
 				$newUser->load( self::READ_LATEST );
 			} else {
@@ -3650,8 +3633,12 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
 			}
 			$this->mId = $dbw->insertId();
 			self::$idCacheByName[$this->mName] = $this->mId;
-			$this->updateActorId( $dbw );
 
+			// Don't pass $this, since calling ::getId, ::getName might force ::load
+			// and this user might not be ready for the yet.
+			$this->mActorId = MediaWikiServices::getInstance()
+				->getActorNormalization()
+				->acquireActorId( new UserIdentityValue( $this->mId, $this->mName ), $dbw );
 			return Status::newGood();
 		} );
 		if ( !$status->isGood() ) {
@@ -3666,19 +3653,6 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
 	}
 
 	/**
-	 * Update the actor ID after an insert
-	 * @param IDatabase $dbw Writable database handle
-	 */
-	private function updateActorId( IDatabase $dbw ) {
-		$dbw->insert(
-			'actor',
-			[ 'actor_user' => $this->mId, 'actor_name' => $this->mName ],
-			__METHOD__
-		);
-		$this->mActorId = (int)$dbw->insertId();
-	}
-
-	/**
 	 * If this user is logged-in and blocked,
 	 * block any IP address they've successfully logged in from.
 	 * @return bool A block was spread