Handle collisions from SerialProvider::acquireIndex

Why:
* When using the TempUserCreator::create or ::acquireAndStashName to
  get temporary account when the chosen username already existed, that
  temporary account is treated as if it doesn't exist. This causes
  confusing "userexists" errors and also causes the user to be logged
  into an already existing temporary account.
* Furthermore, because the user existence check in AuthManager::auto
  CreateUser only checks the local wiki, if an existing temporary
  account exists globally but not on the local wiki then the code
  sign a new user into an existing temporary account.
* This is very bad behaviour, though shouldn't happen unless the
  serialMapping configuration uses a SerialMapping class that could
  provide duplicates and/or the configuration has been changed to
  use a different SerialMapping class.
* There is a need to change the SerialMapping class in use to a
  different class, which means that the code will attempt to use
  temporary account usernames which already exist.
* As such, the code that is generating the temporary account usernames
  based on the SerialMapping and SerialProvider should be aware that
  it may produce an already existing temporary account username, even
  if the SerialMapping class being used is asserted to never provide
  duplicates.
* Therefore, the code that generates temporary account usernames
  should always attempt to verify that a automatically generated
  temporary account name does not already exist on the wiki.

What:
* Update TempUserCreator::acquireName to check to see if the username
  it generates already exists centrally using the CentralIdLookup.
  If it does, then the method returns null. Otherwise, the username
  that hasn't been used yet is returned.
* Create the private method TempUserCreator::attemptAutoCreate that
  attempts an autocreate for a temporary account name, and optionally
  logs the account in.
* Update TempUserCreator::create to use ::attemptAutoCreate to
  first to check if the account can be created and then again once
  the account is created to actually login to that temporary account.
  This is done to prevent logins to existing temporary accounts on
  the local wiki. The second call to actually perform the login is
  necessary as there is no other way to login to a temporary account.
* Update TempUserCreator::acquireAndStashName to respond to the changes
  to ::acquireName, such that it returns null if ::acquireName returns
  null and also does not modify the session.
* Update EditPage::maybeActivateTempUserCreate to return a Status and
  return a good status in all cases except when a temporary account
  name could not be acquired.
* Add IEditObject::AS_UNABLE_TO_ACQUIRE_TEMP_ACCOUNT, and use it as
  the value of the fatal status returned by EditPage
  ::internalAttemptSave if a temporary account name could not be
  acquired. This will cause the display of a useful error to the
  user on edit.
* Update ApiEditPage and ApiAcquireTempUserName to die with an error
  if a temporary account username was unable to be acquired.
* Provide tests for the untested ApiAcquireTempUserName.php file
  including testing the new behaviour.
* Add and update tests for TempUserCreator.php

Bug: T353390
Change-Id: Id3a316ea0eba544d51d4ffcdfb03e35f4b3c54cc

includes/ServiceWiring.php

@@ -2211,6 +2211,7 @@ return [
 			$services->getObjectFactory(),
 			$services->getUserFactory(),
 			$services->getAuthManager(),
+			$services->getCentralIdLookup(),
 			// This is supposed to match ThrottlePreAuthenticationProvider
 			new Throttler(
 				$accountCreationThrottle,

includes/api/ApiAcquireTempUserName.php

@@ -58,8 +58,11 @@ class ApiAcquireTempUserName extends ApiBase {
 		// Checks passed, acquire the name
 		$session = $this->getRequest()->getSession();
 		$name = $this->tempUserCreator->acquireAndStashName( $session );
-		$session->persist();
+		if ( $name === null ) {
+			$this->dieWithError( 'apierror-tempuseracquirefailed', 'tempuseracquirefailed' );
+		}
 
+		$session->persist();
 		$this->getResult()->addValue( null, $this->getModuleName(), $name );
 	}
 

includes/api/ApiEditPage.php

@@ -499,7 +499,10 @@ class ApiEditPage extends ApiBase {
 		$ep->setApiEditOverride( true );
 		$ep->setContextTitle( $titleObj );
 		$ep->importFormData( $req );
-		$ep->maybeActivateTempUserCreate( true );
+		$tempUserCreateStatus = $ep->maybeActivateTempUserCreate( true );
+		if ( !$tempUserCreateStatus->isOK() ) {
+			$this->dieWithError( 'apierror-tempuseracquirefailed', 'tempuseracquirefailed' );
+		}
 
 		// T255700: Ensure content models of the base content
 		// and fetched revision remain the same before attempting to save.

includes/api/i18n/en.json

@@ -1971,6 +1971,7 @@
 	"apierror-stashzerolength": "File is of zero length, and could not be stored in the stash: $1.",
 	"apierror-systemblocked": "You have been blocked automatically by MediaWiki.",
 	"apierror-tempuserdisabled": "Temporary user account creation is disabled.",
+	"apierror-tempuseracquirefailed": "Unable to acquire a temporary account username.",
 	"apierror-templateexpansion-notwikitext": "Template expansion is only supported for wikitext content. $1 uses content model $2.",
 	"apierror-toofewexpiries": "$1 expiry {{PLURAL:$1|timestamp was|timestamps were}} provided where $2 {{PLURAL:$2|was|were}} needed.",
 	"apierror-toomanyvalues": "Too many values supplied for parameter <var>$1</var>. The limit is $2.",

includes/api/i18n/qqq.json

@@ -1873,6 +1873,7 @@
 	"apierror-stashzerolength": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text. Currently this is probably English, hopefully we'll fix that in the future.",
 	"apierror-systemblocked": "{{doc-apierror}}",
 	"apierror-tempuserdisabled": "{{doc-apierror}}",
+	"apierror-tempuseracquirefailed": "{{doc-apierror}}",
 	"apierror-templateexpansion-notwikitext": "{{doc-apierror}}\n\nParameters:\n* $1 - Page title.\n* $2 - Content model.",
 	"apierror-toofewexpiries": "{{doc-apierror}}\n\nParameters:\n* $1 - Number provided.\n* $2 - Number needed.",
 	"apierror-toomanyvalues": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.\n* $2 - Maximum number of values allowed.",

includes/editpage/EditPage.php

@@ -463,6 +463,9 @@ class EditPage implements IEditObject {
 	/** @var bool Whether temp user creation was successful */
 	private $tempUserCreateDone = false;
 
+	/** @var bool Whether temp username acquisition failed (false indicates no failure or not attempted) */
+	private $unableToAcquireTempName = false;
+
 	private LinkRenderer $linkRenderer;
 	private LinkBatchFactory $linkBatchFactory;
 	private RestrictionStore $restrictionStore;
@@ -634,7 +637,7 @@ class EditPage implements IEditObject {
 		// This allows anonymous users to edit via a temporary account, if the site is
 		// configured to (1) disallow anonymous editing and (2) autocreate temporary
 		// accounts on edit.
-		$this->maybeActivateTempUserCreate( !$this->firsttime );
+		$this->unableToAcquireTempName = !$this->maybeActivateTempUserCreate( !$this->firsttime )->isOK();
 
 		$permErrors = $this->getEditPermissionErrors(
 			$this->save ? PermissionManager::RIGOR_SECURE : PermissionManager::RIGOR_FULL
@@ -766,17 +769,23 @@ class EditPage implements IEditObject {
 	 * @param bool $doAcquire Whether to acquire a name for the temporary account
 	 *
 	 * @since 1.39
+	 * @return Status Will return a fatal status if $doAcquire was true and the acquire failed.
 	 */
-	public function maybeActivateTempUserCreate( $doAcquire ) {
+	public function maybeActivateTempUserCreate( $doAcquire ): Status {
 		if ( $this->tempUserCreateActive ) {
 			// Already done
-			return;
+			return Status::newGood();
 		}
 		$user = $this->context->getUser();
 		if ( $this->tempUserCreator->shouldAutoCreate( $user, 'edit' ) ) {
 			if ( $doAcquire ) {
 				$name = $this->tempUserCreator->acquireAndStashName(
 					$this->context->getRequest()->getSession() );
+				if ( $name === null ) {
+					$status = Status::newFatal( 'temp-user-unable-to-acquire' );
+					$status->value = self::AS_UNABLE_TO_ACQUIRE_TEMP_ACCOUNT;
+					return $status;
+				}
 				$this->unsavedTempUser = $this->userFactory->newUnsavedTempUser( $name );
 				$this->tempUserName = $name;
 			} else {
@@ -784,6 +793,7 @@ class EditPage implements IEditObject {
 			}
 			$this->tempUserCreateActive = true;
 		}
+		return Status::newGood();
 	}
 
 	/**
@@ -1836,6 +1846,7 @@ class EditPage implements IEditObject {
 
 			case self::AS_PARSE_ERROR:
 			case self::AS_UNICODE_NOT_SUPPORTED:
+			case self::AS_UNABLE_TO_ACQUIRE_TEMP_ACCOUNT:
 				$out->wrapWikiTextAsInterface( 'error',
 					$status->getWikiText( false, false, $this->context->getLanguage() )
 				);
@@ -2023,6 +2034,12 @@ class EditPage implements IEditObject {
 			return $status;
 		}
 
+		if ( $this->unableToAcquireTempName ) {
+			$status = Status::newFatal( 'temp-user-unable-to-acquire' );
+			$status->value = self::AS_UNABLE_TO_ACQUIRE_TEMP_ACCOUNT;
+			return $status;
+		}
+
 		try {
 			# Construct Content object
 			$textbox_content = $this->toEditContent( $this->textbox1 );

includes/editpage/IEditObject.php

@@ -114,4 +114,7 @@ interface IEditObject {
 
 	/** Status: edit rejected because browser doesn't support Unicode. */
 	public const AS_UNICODE_NOT_SUPPORTED = 242;
+
+	/** Status: edit rejected because server was unable to acquire a temporary account name for this user */
+	public const AS_UNABLE_TO_ACQUIRE_TEMP_ACCOUNT = 243;
 }

includes/user/TempUser/TempUserCreator.php

@@ -8,6 +8,7 @@ use MediaWiki\Auth\Throttler;
 use MediaWiki\Permissions\Authority;
 use MediaWiki\Request\WebRequest;
 use MediaWiki\Session\Session;
+use MediaWiki\User\CentralId\CentralIdLookup;
 use MediaWiki\User\UserFactory;
 use MediaWiki\User\UserRigorOptions;
 use UnexpectedValueException;
@@ -27,6 +28,7 @@ class TempUserCreator implements TempUserConfig {
 	private RealTempUserConfig $config;
 	private UserFactory $userFactory;
 	private AuthManager $authManager;
+	private CentralIdLookup $centralIdLookup;
 	private ?Throttler $throttler;
 	private array $serialProviderConfig;
 	private array $serialMappingConfig;
@@ -64,12 +66,14 @@ class TempUserCreator implements TempUserConfig {
 		ObjectFactory $objectFactory,
 		UserFactory $userFactory,
 		AuthManager $authManager,
+		CentralIdLookup $centralIdLookup,
 		?Throttler $throttler
 	) {
 		$this->config = $config;
 		$this->objectFactory = $objectFactory;
 		$this->userFactory = $userFactory;
 		$this->authManager = $authManager;
+		$this->centralIdLookup = $centralIdLookup;
 		$this->throttler = $throttler;
 		$this->serialProviderConfig = $config->getSerialProviderConfig();
 		$this->serialMappingConfig = $config->getSerialMappingConfig();
@@ -99,29 +103,19 @@ class TempUserCreator implements TempUserConfig {
 
 		if ( $name === null ) {
 			$name = $this->acquireName();
+			if ( $name === null ) {
+				// If the $name remains null after calling ::acquireName, then
+				// we cannot generate a username and therefore cannot create a user.
+				// In this case return a CreateStatus indicating no user was created.
+				return CreateStatus::newFatal( 'temp-user-unable-to-acquire' );
+			}
 		}
+		$createStatus = $this->attemptAutoCreate( $name );
 
-		$user = $this->userFactory->newFromName( $name, UserRigorOptions::RIGOR_USABLE );
-		if ( !$user ) {
-			$status->fatal( 'internalerror_info',
-				'Unable to create user with automatically generated name' );
-			return $status;
-		}
-
-		$status = $this->authManager->autoCreateUser(
-			$user,
-			AuthManager::AUTOCREATE_SOURCE_TEMP,
-			true, // login
-			false // log
-		);
-		$createStatus = new CreateStatus;
-		$createStatus->merge( $status );
-		// Make userexists warning be fatal
-		if ( $createStatus->hasMessage( 'userexists' ) ) {
-			$createStatus->fatal( 'userexists' );
-		}
 		if ( $createStatus->isOK() ) {
-			$createStatus->value = $user;
+			// The temporary account name didn't already exist, so now attempt to login
+			// using ::attemptAutoCreate as there isn't a public method to just login.
+			$this->attemptAutoCreate( $name, true );
 		}
 		return $createStatus;
 	}
@@ -162,16 +156,69 @@ class TempUserCreator implements TempUserConfig {
 		return $this->config->getNotifyBeforeExpirationDays();
 	}
 
+	/**
+	 * Attempts to auto create a temporary user using
+	 * AuthManager::autoCreateUser, and optionally log them
+	 * in if $login is true.
+	 *
+	 * @param string $name
+	 * @param bool $login Whether to also log the user in to this temporary account.
+	 * @return CreateStatus
+	 */
+	private function attemptAutoCreate( string $name, bool $login = false ): CreateStatus {
+		$createStatus = new CreateStatus;
+		// Verify the $name is usable.
+		$user = $this->userFactory->newFromName( $name, UserRigorOptions::RIGOR_USABLE );
+		if ( !$user ) {
+			$createStatus->fatal( 'internalerror_info',
+				'Unable to create user with automatically generated name' );
+			return $createStatus;
+		}
+		$status = $this->authManager->autoCreateUser(
+			$user,
+			AuthManager::AUTOCREATE_SOURCE_TEMP,
+			$login, // login
+			false // log
+		);
+		$createStatus->merge( $status );
+		// If a userexists warning is a part of the status, then
+		// add the fatal error temp-user-unable-to-acquire.
+		if ( $createStatus->hasMessage( 'userexists' ) ) {
+			$createStatus->fatal( 'temp-user-unable-to-acquire' );
+		}
+		if ( $createStatus->isOK() ) {
+			$createStatus->value = $user;
+		}
+		return $createStatus;
+	}
+
 	/**
 	 * Acquire a new username and return it. Permanently reserve the ID in
 	 * the database.
 	 *
-	 * @return string
+	 * @return string|null The username, or null if the auto-generated username is
+	 *    already in use.
 	 */
-	private function acquireName(): string {
+	private function acquireName(): ?string {
+		// Check if the temporary account name is already in use as the ID provided
+		// may not be properly collision safe (T353390)
 		$index = $this->getSerialProvider()->acquireIndex();
 		$serialId = $this->getSerialMapping()->getSerialIdForIndex( $index );
-		return $this->config->getGeneratorPattern()->generate( $serialId );
+		$username = $this->config->getGeneratorPattern()->generate( $serialId );
+
+		// Because the ::acquireIndex method may not always return a unique index,
+		// make sure that the temporary account name does not already exist. This
+		// is needed because of the problems discussed in T353390.
+		// The problems discussed at that task should not require the use of a primary lookup.
+		$centralId = $this->centralIdLookup->centralIdFromName(
+			$username,
+			CentralIdLookup::AUDIENCE_RAW
+		);
+		if ( !$centralId ) {
+			// If no user exists with this name centrally, then return the $username.
+			return $username;
+		}
+		return null;
 	}
 
 	/**
@@ -261,7 +308,7 @@ class TempUserCreator implements TempUserConfig {
 	 * previously stashed username instead of acquiring a new one.
 	 *
 	 * @param Session $session
-	 * @return string The username
+	 * @return string|null The username, or null if no username could be acquired
 	 */
 	public function acquireAndStashName( Session $session ) {
 		$name = $session->get( 'TempUser:name' );
@@ -269,8 +316,10 @@ class TempUserCreator implements TempUserConfig {
 			return $name;
 		}
 		$name = $this->acquireName();
-		$session->set( 'TempUser:name', $name );
-		$session->save();
+		if ( $name !== null ) {
+			$session->set( 'TempUser:name', $name );
+			$session->save();
+		}
 		return $name;
 	}
 

languages/i18n/en.json

@@ -4639,5 +4639,6 @@
 	"createacct-benefit-subheading-temp-user": "{{SITENAME}} is made by people like you. With an account you can have a permanent and secure identity with access to features like:",
 	"benefit-1-description": "Watchlists that allow you to keep track of pages that you’re interested in.",
 	"benefit-2-description": "Permanent list of contributions you’ve made to the project.",
-	"benefit-3-description": "Preferences that allow you to customize your experience."
+	"benefit-3-description": "Preferences that allow you to customize your experience.",
+	"temp-user-unable-to-acquire": "Unable to acquire a temporary account username. Please try again."
 }

languages/i18n/qqq.json

@@ -4894,5 +4894,6 @@
 	"createacct-benefit-subheading-temp-user": "Subheading text description for temporary users who want to create an account to make their edits more permanent.",
 	"benefit-1-description": "The text for the first benefit shown on signup for temporary users.",
 	"benefit-2-description": "The text for the second benefit shown on signup for temporary users.",
-	"benefit-3-description": "The text for the third benefit shown on signup for temporary users."
+	"benefit-3-description": "The text for the third benefit shown on signup for temporary users.",
+	"temp-user-unable-to-acquire": "The error message shown when the server was unable to acquire a temporary account username for the user."
 }

tests/phpunit/includes/api/ApiAcquireTempUserNameTest.php

@@ -0,0 +1,107 @@
+<?php
+
+use MediaWiki\MainConfigNames;
+use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait;
+use MediaWiki\User\TempUser\TempUserCreator;
+
+/**
+ * @group API
+ * @group Database
+ * @group medium
+ *
+ * @covers ApiAcquireTempUserName
+ */
+class ApiAcquireTempUserNameTest extends ApiTestCase {
+	use MockAuthorityTrait;
+
+	public function testExecuteDiesWhenNotEnabled() {
+		$this->overrideConfigValue(
+			MainConfigNames::AutoCreateTempUser,
+			[
+				'enabled' => false,
+			]
+		);
+		$this->expectApiErrorCode( 'tempuserdisabled' );
+
+		$this->doApiRequestWithToken( [
+			"action" => "acquiretempusername",
+		] );
+	}
+
+	public function testExecuteDiesWhenUserIsRegistered() {
+		$this->overrideConfigValue(
+			MainConfigNames::AutoCreateTempUser,
+			[
+				'enabled' => true,
+				'expireAfterDays' => null,
+				'actions' => [ 'edit' ],
+				'genPattern' => '*Unregistered $1',
+				'matchPattern' => '*$1',
+				'serialProvider' => [ 'type' => 'local' ],
+				'serialMapping' => [ 'type' => 'plain-numeric' ],
+			]
+		);
+		$this->expectApiErrorCode( 'alreadyregistered' );
+
+		$this->doApiRequestWithToken(
+			[
+				'action' => 'acquiretempusername',
+			],
+			null,
+			$this->mockRegisteredUltimateAuthority()
+		);
+	}
+
+	public function testExecuteDiesWhenNameCannotBeAcquired() {
+		$mockTempUserCreator = $this->createMock( TempUserCreator::class );
+		$mockTempUserCreator->method( 'isEnabled' )
+			->willReturn( true );
+		$mockTempUserCreator->method( 'acquireAndStashName' )
+			->willReturn( null );
+		$this->overrideMwServices(
+			null,
+			[
+				'TempUserCreator' => static function () use ( $mockTempUserCreator ) {
+					return $mockTempUserCreator;
+				}
+			]
+		);
+		$this->expectApiErrorCode( 'tempuseracquirefailed' );
+
+		$this->doApiRequestWithToken(
+			[
+				'action' => 'acquiretempusername',
+			],
+			null,
+			$this->mockAnonUltimateAuthority()
+		);
+	}
+
+	public function testExecuteForSuccessfulCall() {
+		$this->overrideConfigValue(
+			MainConfigNames::AutoCreateTempUser,
+			[
+				'enabled' => true,
+				'expireAfterDays' => null,
+				'actions' => [ 'edit' ],
+				'genPattern' => '*Unregistered $1',
+				'matchPattern' => '*$1',
+				'serialProvider' => [ 'type' => 'local' ],
+				'serialMapping' => [ 'type' => 'plain-numeric' ],
+			]
+		);
+
+		$this->assertArrayEquals(
+			[ 'acquiretempusername' => '*Unregistered 1' ],
+			$this->doApiRequestWithToken(
+				[
+					'action' => 'acquiretempusername',
+				],
+				null,
+				$this->mockAnonUltimateAuthority()
+			)[0],
+			true,
+			true
+		);
+	}
+}

tests/phpunit/integration/includes/user/TempUser/TempUserCreatorTest.php

@@ -6,6 +6,7 @@ use ExtensionRegistry;
 use MediaWiki\Auth\AuthManager;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Session\Session;
+use MediaWiki\User\CentralId\CentralIdLookup;
 use MediaWiki\User\TempUser\RealTempUserConfig;
 use MediaWiki\User\TempUser\SerialMapping;
 use MediaWiki\User\TempUser\SerialProvider;
@@ -98,6 +99,7 @@ class TempUserCreatorTest extends \MediaWikiIntegrationTestCase {
 			$this->createSimpleObjectFactory(),
 			$this->createMock( UserFactory::class ),
 			$this->createMock( AuthManager::class ),
+			$this->createMock( CentralIdLookup::class ),
 			null
 		);
 		return [ $creator, [ $scope1, $scope2 ] ];
@@ -122,6 +124,23 @@ class TempUserCreatorTest extends \MediaWikiIntegrationTestCase {
 		$this->assertSame( '*Unregistered 2', $tuc->acquireName() );
 	}
 
+	public function testAcquireNameOnDuplicate_db() {
+		$this->overrideConfigValue(
+			MainConfigNames::AutoCreateTempUser,
+			[ 'enabled' => true ] + self::DEFAULTS
+		);
+		$tuc = TestingAccessWrapper::newFromObject(
+			$this->getServiceContainer()->getTempUserCreator()
+		);
+		// Create a temporary account
+		$this->assertSame( '*Unregistered 1', $tuc->create()->value->getName() );
+		// Reset the user_autocreate_serial table
+		$this->truncateTable( 'user_autocreate_serial' );
+		// Because user_autocreate_serial was truncated, the ::acquireName method should
+		// return null as the code attempts to return a temporary account that already exists.
+		$this->assertSame( null, $tuc->acquireName() );
+	}
+
 	public function testAcquireAndStashName() {
 		/** @var TempUserCreator $creator */
 		[ $creator, $scope ] = $this->getTempUserCreatorUnit();