From 3aec9274851c43c4f7bd1f28e256c34c088852ec Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 31 Aug 2020 16:03:36 +0200 Subject: [PATCH] SECURITY: ensure actor ID from correct wiki is used. This builds on top of Urbanecm's patch, now also covering the case where the actor ID does not exist in the target DB, but does exist in the local DB. Bug: T260485 Change-Id: I2336954c665366a99f9995df9b08071d4de6db79 --- includes/ActorMigration.php | 68 +++++++++++++++++++++++++++++++------ includes/user/User.php | 49 +++----------------------- 2 files changed, 63 insertions(+), 54 deletions(-) diff --git a/includes/ActorMigration.php b/includes/ActorMigration.php index b0aeeaf8ae..c348f987f0 100644 --- a/includes/ActorMigration.php +++ b/includes/ActorMigration.php @@ -184,8 +184,8 @@ class ActorMigration { /** * Get actor ID from UserIdentity, if it exists */ - private function getExistingActorId( IDatabase $dbw, UserIdentity $user ) { - $row = $dbw->selectRow( + public function getExistingActorId( IDatabase $db, UserIdentity $user ) { + $row = $db->selectRow( 'actor', [ 'actor_id' ], [ 'actor_name' => $user->getName() ], @@ -195,7 +195,59 @@ class ActorMigration { return false; } - return $row->actor_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. + * + * @param IDatabase $dbw + * @param UserIdentity $user + * + * @return int The new actor ID + */ + public function getNewActorId( IDatabase $dbw, UserIdentity $user ) { + $q = [ + 'actor_user' => $user->getId() ?: null, + 'actor_name' => (string)$user->getName(), + ]; + if ( $dbw ) { + if ( $q['actor_user'] === null && User::isUsableName( $q['actor_name'] ) ) { + throw new CannotCreateActorException( + 'Cannot create an actor for a usable name that is not an existing user' + ); + } + if ( $q['actor_name'] === '' ) { + throw new CannotCreateActorException( 'Cannot create an actor for a user with no name' ); + } + $dbw->insert( 'actor', $q, __METHOD__, [ 'IGNORE' ] ); + if ( $dbw->affectedRows() ) { + $actorId = (int)$dbw->insertId(); + } else { + // Outdated cache? + // Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot. + $actorId = (int)$dbw->selectField( + 'actor', + 'actor_id', + $q, + __METHOD__, + [ 'LOCK IN SHARE MODE' ] + ); + if ( !$actorId ) { + throw new CannotCreateActorException( + "Cannot create actor ID for user_id={$this->getId()} user_name={$this->getName()}" + ); + } + } + $this->invalidateCache(); + } else { + list( $index, $options ) = DBAccessObjectUtils::getDBOptions( $this->queryFlagsUsed ); + $db = wfGetDB( $index ); + $actorId = (int)$db->selectField( 'actor', 'actor_id', $q, __METHOD__, $options ); + } + + return $actorId; } /** @@ -219,17 +271,13 @@ class ActorMigration { $ret[$text] = $user->getName(); } if ( $this->stage >= MIGRATION_WRITE_BOTH ) { - // UBN fix for T260485 - do not let User object to handle existing actor IDs - // TODO: Make User object wiki-aware and let it handle all cases. + // NOTE: Don't use $user->getActorId(), since that may be for the wrong wiki (T260485) + // TODO: Make User object wiki-aware and let it handle all cases (T260933) $existingActorId = $this->getExistingActorId( $dbw, $user ); if ( $existingActorId !== false ) { $ret[$actor] = $existingActorId; } else { - // We need to be able to assign an actor ID if none exists - if ( !$user instanceof User && !$user->getActorId() ) { - $user = User::newFromAnyId( $user->getId(), $user->getName(), null ); - } - $ret[$actor] = $user->getActorId( $dbw ); + $ret[$actor] = $this->getNewActorId( $dbw, $user ); } } return $ret; diff --git a/includes/user/User.php b/includes/user/User.php index c60d6cee77..0bd7c1536b 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -2528,50 +2528,11 @@ class User implements IDBAccessObject, UserIdentity { $this->load(); } - // Currently $this->mActorId might be null if $this was loaded from a - // cache entry that was written when $wgActorTableSchemaMigrationStage - // was MIGRATION_OLD. Once that is no longer a possibility (i.e. when - // User::VERSION is incremented after $wgActorTableSchemaMigrationStage - // has been removed), that condition may be removed. - if ( $this->mActorId === null || !$this->mActorId && $dbw ) { - $q = [ - 'actor_user' => $this->getId() ?: null, - 'actor_name' => (string)$this->getName(), - ]; - if ( $dbw ) { - if ( $q['actor_user'] === null && self::isUsableName( $q['actor_name'] ) ) { - throw new CannotCreateActorException( - 'Cannot create an actor for a usable name that is not an existing user' - ); - } - if ( $q['actor_name'] === '' ) { - throw new CannotCreateActorException( 'Cannot create an actor for a user with no name' ); - } - $dbw->insert( 'actor', $q, __METHOD__, [ 'IGNORE' ] ); - if ( $dbw->affectedRows() ) { - $this->mActorId = (int)$dbw->insertId(); - } else { - // Outdated cache? - // Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot. - $this->mActorId = (int)$dbw->selectField( - 'actor', - 'actor_id', - $q, - __METHOD__, - [ 'LOCK IN SHARE MODE' ] - ); - if ( !$this->mActorId ) { - throw new CannotCreateActorException( - "Cannot create actor ID for user_id={$this->getId()} user_name={$this->getName()}" - ); - } - } - $this->invalidateCache(); - } else { - list( $index, $options ) = DBAccessObjectUtils::getDBOptions( $this->queryFlagsUsed ); - $db = wfGetDB( $index ); - $this->mActorId = (int)$db->selectField( 'actor', 'actor_id', $q, __METHOD__, $options ); - } + if ( !$this->mActorId && $dbw ) { + $migration = MediaWikiServices::getInstance()->getActorMigration(); + $this->mActorId = $migration->getNewActorId( $dbw, $this ); + + $this->invalidateCache(); $this->setItemLoaded( 'actor' ); } -- 2.25.1