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)

diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php
index f84b974..dcf0551 100644 (file)
--- 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 a677ede..b596f02 100644 (file)
--- 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 326073f..d93be2b 100644 (file)
--- 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 f24255b..db42d5a 100644 (file)
--- 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' ],
+               ];
+       }
 }