T260485-new-actor.patch
daniel (Daniel Kinzler)
Actions

Authored By

	daniel
	Aug 31 2020, 3:00 PM

Size

5 KB

Referenced Files

None

Subscribers

None

T260485-new-actor.patch
View Options

	From 84b37d0d9d588b8fd16e5dcb0007702fc255a28d Mon Sep 17 00:00:00 2001
	From: daniel <dkinzler@wikimedia.org>
	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 \| 71 +++++++++++++++++++++++++++++++------
	includes/user/User.php \| 39 ++------------------
	2 files changed, 64 insertions(+), 46 deletions(-)

	diff --git a/includes/ActorMigration.php b/includes/ActorMigration.php
	index 192e0409c6..e6a6107ad4 100644
	--- a/includes/ActorMigration.php
	+++ b/includes/ActorMigration.php
	@@ -242,8 +242,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() ],
	@@ -253,7 +253,62 @@ 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 ) {
	+ // TODO: inject
	+ $userNameUtils = MediaWikiServices::getInstance()->getUserNameUtils();
	+
	+ $q = [
	+ 'actor_user' => $user->getId() ?: null,
	+ 'actor_name' => (string)$user->getName(),
	+ ];
	+ if ( $q['actor_user'] === null && $userNameUtils->isUsable( $q['actor_name'] ) ) {
	+ throw new CannotCreateActorException(
	+ 'Cannot create an actor for a usable name that is not an existing user: ' .
	+ "user_id={$user->getId()} user_name=\"{$user->getName()}\""
	+ );
	+ }
	+ if ( $q['actor_name'] === '' ) {
	+ throw new CannotCreateActorException(
	+ 'Cannot create an actor for a user with no name: ' .
	+ "user_id={$user->getId()} user_name=\"{$user->getName()}\""
	+ );
	+ }
	+
	+ $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(
	+ "Failed to create actor ID for " .
	+ "user_id={$user->getId()} user_name=\"{$user->getName()}\""
	+ );
	+ }
	+ }
	+
	+ return $actorId;
	}

	/**
	@@ -279,17 +334,13 @@ class ActorMigration {
	$ret[$text] = $user->getName();
	}
	if ( $this->stage & SCHEMA_COMPAT_WRITE_NEW ) {
	- // 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 517d96a7e6..511b493f92 100644
	--- a/includes/user/User.php
	+++ b/includes/user/User.php
	@@ -1996,42 +1996,9 @@ class User implements IDBAccessObject, UserIdentity {
	}

	if ( !$this->mActorId && $dbw ) {
	- $q = [
	- 'actor_user' => $this->getId() ?: null,
	- 'actor_name' => (string)$this->getName(),
	- ];
	- 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: ' .
	- "user_id={$this->getId()} user_name=\"{$this->getName()}\""
	- );
	- }
	- if ( $q['actor_name'] === '' ) {
	- throw new CannotCreateActorException(
	- 'Cannot create an actor for a user with no name: ' .
	- "user_id={$this->getId()} user_name=\"{$this->getName()}\""
	- );
	- }
	- $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(
	- "Failed to create actor ID for " .
	- "user_id={$this->getId()} user_name=\"{$this->getName()}\""
	- );
	- }
	- }
	+ $migration = MediaWikiServices::getInstance()->getActorMigration();
	+ $this->mActorId = $migration->getNewActorId( $dbw, $this );
	+
	$this->invalidateCache();
	$this->setItemLoaded( 'actor' );
	}
	--
	2.25.1

File Metadata

Mime Type: text/x-diff
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 8524528
Default Alt Text: T260485-new-actor.patch (5 KB)

T260485-new-actor.patchdaniel (Daniel Kinzler)Actions

T260485-new-actor.patchView Options

File Metadata

Event Timeline

T260485-new-actor.patch
daniel (Daniel Kinzler)
Actions

T260485-new-actor.patch
View Options