From: Gergő Tisza <gtisza@wikimedia.org>
Date: Thu, 8 Sep 2016 22:40:20 +0000 (+0000)
Subject: Avoid user autocreation race condition caused by repeatable read
X-Git-Tag: 1.31.0-rc.0~5696^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=55572b08a6845e892870abcac4b8b0718b7c9075

Avoid user autocreation race condition caused by repeatable read

AuthManager tries to check whether the user already exists if
User::addToDatabase fails in autocreation, but since the same DB row
was already checked a few lines earlier and this method is typically
wrapped in an implicit transaction, it will just re-read the same
snapshot and not do anything useful. addToDatabase already has
a check for that so let's rely on that instead.

Bug: T145131
Change-Id: I94a5e8b851dcf994f5f9e773edf4e9153a4a3535
---

diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php
index 992e70f118..89a22f8402 100644
--- a/includes/auth/AuthManager.php
+++ b/includes/auth/AuthManager.php
@@ -1679,14 +1679,12 @@ class AuthManager implements LoggerAwareInterface {
 		try {
 			$status = $user->addToDatabase();
 			if ( !$status->isOk() ) {
-				// double-check for a race condition (T70012)
-				$localId = User::idFromName( $username, User::READ_LATEST );
-				if ( $localId ) {
+				// Double-check for a race condition (T70012). We make use of the fact that when
+				// addToDatabase fails due to the user already existing, the user object gets loaded.
+				if ( $user->getId() ) {
 					$this->logger->info( __METHOD__ . ': {username} already exists locally (race)', [
 						'username' => $username,
 					] );
-					$user->setId( $localId );
-					$user->loadFromId( User::READ_LATEST );
 					if ( $login ) {
 						$this->setSessionDataForUser( $user );
 					}
diff --git a/tests/phpunit/includes/auth/AuthManagerTest.php b/tests/phpunit/includes/auth/AuthManagerTest.php
index 788d304295..f679f63345 100644
--- a/tests/phpunit/includes/auth/AuthManagerTest.php
+++ b/tests/phpunit/includes/auth/AuthManagerTest.php
@@ -2709,9 +2709,11 @@ class AuthManagerTest extends \MediaWikiTestCase {
 		$session->clear();
 		$user = $this->getMock( 'User', [ 'addToDatabase' ] );
 		$user->expects( $this->once() )->method( 'addToDatabase' )
-			->will( $this->returnCallback( function () use ( $username ) {
-				$status = \User::newFromName( $username )->addToDatabase();
+			->will( $this->returnCallback( function () use ( $username, &$user ) {
+				$oldUser = \User::newFromName( $username );
+				$status = $oldUser->addToDatabase();
 				$this->assertTrue( $status->isOK(), 'sanity check' );
+				$user->setId( $oldUser->getId() );
 				return \Status::newFatal( 'userexists' );
 			} ) );
 		$user->setName( $username );