Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F32358495
08-T260485-2-REL1_34.patch
Reedy (Sam Reed)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Authored By
Reedy
Sep 22 2020, 12:40 AM
2020-09-22 00:40:14 (UTC+0)
Size
5 KB
Referenced Files
None
Subscribers
None
08-T260485-2-REL1_34.patch
View Options
From c3e12826a474f23bd2f9d4af96a25969bafb21b3 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 | 70 +++++++++++++++++++++++++++++++------
includes/user/User.php | 39 ++-------------------
2 files changed, 63 insertions(+), 46 deletions(-)
diff --git a/includes/ActorMigration.php b/includes/ActorMigration.php
index 27bff92361..b6287b0fc6 100644
--- a/includes/ActorMigration.php
+++ b/includes/ActorMigration.php
@@ -238,8 +238,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() ],
@@ -249,7 +249,61 @@ 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.
+ *
+ * @since 1.34.3
+ *
+ * @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 ( $q['actor_user'] === null && User::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;
}
/**
@@ -275,17 +329,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 96620583fe..28faa5700a 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -2406,42 +2406,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
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
8573655
Default Alt Text
08-T260485-2-REL1_34.patch (5 KB)
Attached To
Mode
T256335: Tracking bug for MediaWiki 1.31.9/1.34.3/1.35.0
Attached
Detach File
T260485: CentralAuth uses wrong actor ID when locally suppressing the user (CVE-2020-25869)
Attached
Detach File
Event Timeline
Log In to Comment