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 992e70f..89a22f8 100644 (file)
--- 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 788d304..f679f63 100644 (file)
--- 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 );