From f3bdd2f4a1796c95490796a521f0d31f4a4ee9af Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 28 Oct 2019 11:51:05 -0400 Subject: [PATCH] User: Allow newSystemUser() to create over anonymous actors Various maintenance scripts assume reserved usernames like "MediaWiki default" exist, but since they're reserved User::isUsableName() returns false and therefore the actor migration created them as anonymous actors. Which would then prevent those maintenance scripts from using User::newSystemUser() to ensure they actually exist. This adjusts User::newSystemUser() to be able to create users for those anonymous actors. This also adjusts uses of "MediaWiki default" in core to create it as a system user. Bug: T236444 Change-Id: I59a646df36ff9343cc43c05aa20b2b69b2ee124a (cherry picked from commit 685b505628099a027ab5c9451502f522b489c109) --- includes/installer/Installer.php | 2 +- includes/user/User.php | 57 ++++++++++++-- maintenance/deleteDefaultMessages.php | 2 +- tests/phpunit/includes/user/UserTest.php | 97 ++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 10 deletions(-) diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php index f84b974071..dcf05518fd 100644 --- a/includes/installer/Installer.php +++ b/includes/installer/Installer.php @@ -1771,7 +1771,7 @@ abstract class Installer { '', EDIT_NEW, false, - User::newFromName( 'MediaWiki default' ) + User::newSystemUser( 'MediaWiki default' ) ); } catch ( Exception $e ) { // using raw, because $wgShowExceptionDetails can not be set yet diff --git a/includes/user/User.php b/includes/user/User.php index a677edec2a..b596f027ea 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -110,6 +110,9 @@ class User implements IDBAccessObject, UserIdentity { 'mActorId', ]; + /** @var string[]|false Cache for self::isUsableName() */ + private static $reservedUsernames = false; + /** Cache variables */ // @{ /** @var int */ @@ -771,9 +774,48 @@ class User implements IDBAccessObject, UserIdentity { if ( !$row ) { // No user. Create it? - return $options['create'] - ? self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] ) - : null; + if ( !$options['create'] ) { + // No. + return null; + } + + // If it's a reserved user that had an anonymous actor created for it at + // some point, we need special handling. + if ( !self::isValidUserName( $name ) || self::isUsableName( $name ) ) { + // Not reserved, so just create it. + return self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] ); + } + + // It is reserved. Check for an anonymous actor row. + $dbw = wfGetDB( DB_MASTER ); + return $dbw->doAtomicSection( __METHOD__, function ( IDatabase $dbw, $fname ) use ( $name ) { + $row = $dbw->selectRow( + 'actor', + [ 'actor_id' ], + [ 'actor_name' => $name, 'actor_user' => null ], + $fname, + [ 'FOR UPDATE' ] + ); + if ( !$row ) { + // No anonymous actor. + return self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] ); + } + + // There is an anonymous actor. Delete the actor row so we can create the user, + // then restore the old actor_id so as to not break existing references. + // @todo If MediaWiki ever starts using foreign keys for `actor`, this will break things. + $dbw->delete( 'actor', [ 'actor_id' => $row->actor_id ], $fname ); + $user = self::createNew( $name, [ 'token' => self::INVALID_TOKEN ] ); + $dbw->update( + 'actor', + [ 'actor_id' => $row->actor_id ], + [ 'actor_id' => $user->getActorId() ], + $fname + ); + $user->clearInstanceCache( 'id' ); + $user->invalidateCache(); + return $user; + } ); } $user = self::newFromRow( $row ); @@ -970,14 +1012,13 @@ class User implements IDBAccessObject, UserIdentity { return false; } - static $reservedUsernames = false; - if ( !$reservedUsernames ) { - $reservedUsernames = $wgReservedUsernames; - Hooks::run( 'UserGetReservedNames', [ &$reservedUsernames ] ); + if ( !self::$reservedUsernames ) { + self::$reservedUsernames = $wgReservedUsernames; + Hooks::run( 'UserGetReservedNames', [ &self::$reservedUsernames ] ); } // Certain names may be reserved for batch processes. - foreach ( $reservedUsernames as $reserved ) { + foreach ( self::$reservedUsernames as $reserved ) { if ( substr( $reserved, 0, 4 ) == 'msg:' ) { $reserved = wfMessage( substr( $reserved, 4 ) )->inContentLanguage()->plain(); } diff --git a/maintenance/deleteDefaultMessages.php b/maintenance/deleteDefaultMessages.php index 326073f1a2..d93be2b79d 100644 --- a/maintenance/deleteDefaultMessages.php +++ b/maintenance/deleteDefaultMessages.php @@ -76,7 +76,7 @@ class DeleteDefaultMessages extends Maintenance { // Deletions will be made by $user temporarly added to the bot group // in order to hide it in RecentChanges. - $user = User::newFromName( 'MediaWiki default' ); + $user = User::newSystemUser( 'MediaWiki default', [ 'steal' => true ] ); if ( !$user ) { $this->fatalError( "Invalid username" ); } diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index f24255b8da..db42d5ad00 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -36,6 +36,13 @@ class UserTest extends MediaWikiTestCase { $this->setUpPermissionGlobals(); $this->user = $this->getTestUser( [ 'unittesters' ] )->getUser(); + + TestingAccessWrapper::newFromClass( User::class )->reservedUsernames = false; + } + + protected function tearDown() : void { + parent::tearDown(); + TestingAccessWrapper::newFromClass( User::class )->reservedUsernames = false; } private function setUpPermissionGlobals() { @@ -1563,4 +1570,94 @@ class UserTest extends MediaWikiTestCase { ); $this->assertSame( null, User::idFromName( 'NotExisitngUser' ) ); } + + /** + * @covers User::newSystemUser + * @dataProvider provideNewSystemUser + * @param string $exists How/whether to create the user before calling User::newSystemUser + * - 'missing': Do not create the user + * - 'actor': Create an anonymous actor + * - 'user': Create a non-system user + * - 'system': Create a system user + * @param string $options Options to User::newSystemUser + * @param array $testOpts Test options + * @param string $expect 'user', 'exception', or 'null' + */ + public function testNewSystemUser( $exists, $options, $testOpts, $expect ) { + $origUser = null; + $actorId = null; + + switch ( $exists ) { + case 'missing': + $name = 'TestNewSystemUser ' . TestUserRegistry::getNextId(); + break; + + case 'actor': + $name = 'TestNewSystemUser ' . TestUserRegistry::getNextId(); + $this->db->insert( 'actor', [ 'actor_name' => $name ] ); + $actorId = (int)$this->db->insertId(); + break; + + case 'user': + $origUser = $this->getMutableTestUser()->getUser(); + $name = $origUser->getName(); + $actorId = $origUser->getActorId(); + break; + + case 'system': + $name = 'TestNewSystemUser ' . TestUserRegistry::getNextId(); + $user = User::newSystemUser( $name ); // Heh. + $actorId = $user->getActorId(); + // Use this hook as a proxy for detecting when a "steal" happens. + $this->setTemporaryHook( 'InvalidateEmailComplete', function () { + $this->fail( 'InvalidateEmailComplete hook should not have been called' ); + } ); + break; + } + + $globals = $testOpts['globals'] ?? []; + if ( !empty( $testOpts['reserved'] ) ) { + $globals['wgReservedUsernames'] = [ $name ]; + } + $this->setMwGlobals( $globals ); + $this->assertTrue( User::isValidUserName( $name ) ); + $this->assertSame( empty( $testOpts['reserved'] ), User::isUsableName( $name ) ); + + if ( $expect === 'exception' ) { + $this->expectException( Exception::class ); + } + $user = User::newSystemUser( $name, $options ); + if ( $expect === 'null' ) { + $this->assertNull( $user ); + if ( $origUser ) { + $this->assertNotSame( + User::INVALID_TOKEN, TestingAccessWrapper::newFromObject( $origUser )->mToken + ); + $this->assertNotSame( '', $origUser->getEmail() ); + } + } else { + $this->assertInstanceOf( User::class, $user ); + $this->assertSame( $name, $user->getName() ); + if ( $actorId !== null ) { + $this->assertSame( $actorId, $user->getActorId() ); + } + $this->assertSame( User::INVALID_TOKEN, TestingAccessWrapper::newFromObject( $user )->mToken ); + $this->assertSame( '', $user->getEmail() ); + } + } + + public static function provideNewSystemUser() { + return [ + 'Basic creation' => [ 'missing', [], [], 'user' ], + 'No creation' => [ 'missing', [ 'create' => false ], [], 'null' ], + 'Validation fail' => [ 'missing', [ 'validate' => 'usable' ], [ 'reserved' => true ], 'null' ], + 'No stealing' => [ 'user', [], [], 'null' ], + 'Stealing allowed' => [ 'user', [ 'steal' => true ], [], 'user' ], + 'Stealing an already-system user' => [ 'system', [ 'steal' => true ], [], 'user' ], + 'Anonymous actor (T236444)' => [ 'actor', [], [ 'reserved' => true ], 'user' ], + 'Reserved but no anonymous actor' => [ 'missing', [], [ 'reserved' => true ], 'user' ], + 'Anonymous actor but no creation' => [ 'actor', [ 'create' => false ], [], 'null' ], + 'Anonymous actor but not reserved' => [ 'actor', [], [], 'exception' ], + ]; + } } -- 2.20.1