From 66f85fa125f9c92fbdffda4a077ac0991c95ce2f Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 16 Oct 2023 15:57:40 +1100 Subject: [PATCH] AuthManager: deny auto-creation for globally blocked users * In AuthManager::autoCreateUser(), check the permissions of the performer instead of relying on the secondary providers. This means that auto-creation will be denied when the anonymous user is globally IP-blocked. * Remove create account block check from CheckBlocksSecondaryAuthenticationProvider. testUserForCreation() is supposed to only do target name checks, but it's not actually possible to block a non-existent local name. So we don't need this code. * Add a $performer parameter to autoCreateUser() so that Special:CreateLocalAccount can have elevated permissions when it creates an account with IP block exemption. * When a performer is passed, don't use the session as a cache. * Since we are passing autocreateaccount as the action to PermissionManager instead of createaccount, some special cases need to be tweaked. Previously AuthManager checked for either autocreateaccount or createaccount rights. Now PermissionManager does that when the action is autocreateaccount. By removing redundant checks from testUserForCreation(), the number of ipblocks queries during a normal Special:CreateAccount post request is reduced from 8 to 6. The CentralAuth change I7e7a7fc8bcd86285f857063a38de02b41b5175d0 should be merged immediately after this one. Bug: T234371 Bug: T345683 Change-Id: If2937c7d717d2adc249f608d4585122b02a43fff --- includes/Permissions/PermissionManager.php | 11 ++- includes/auth/AuthManager.php | 87 +++++++++++++----- ...kBlocksSecondaryAuthenticationProvider.php | 52 ----------- includes/block/AbstractBlock.php | 1 + .../phpunit/includes/auth/AuthManagerTest.php | 92 ++++++++++++++++++- ...cksSecondaryAuthenticationProviderTest.php | 60 ------------ 6 files changed, 163 insertions(+), 140 deletions(-) diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index b6bc5f3f732..d8215d2bfbd 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -879,7 +879,7 @@ class PermissionManager { $useReplica = $rigor !== self::RIGOR_SECURE; // Create account blocks are implemented separately due to weird IP exemption rules - if ( $action === 'createaccount' ) { + if ( in_array( $action, [ 'createaccount', 'autocreateaccount' ], true ) ) { $isExempt = $this->userHasRight( $user, 'ipblock-exempt' ); return $this->blockManager->getCreateAccountBlock( $user, @@ -1078,6 +1078,11 @@ class PermissionManager { // Show category page-specific message only if the user can move other pages $errors[] = [ 'cant-move-to-category-page' ]; } + } elseif ( $action === 'autocreateaccount' ) { + // createaccount implies autocreateaccount + if ( !$this->userHasAnyRight( $user, 'autocreateaccount', 'createaccount' ) ) { + $errors[] = $this->missingPermissionError( $action, $short ); + } } elseif ( !$this->userHasRight( $user, $action ) ) { $errors[] = $this->missingPermissionError( $action, $short ); } @@ -1339,7 +1344,9 @@ class PermissionManager { // Only 'createaccount' can be performed on special pages, // which don't actually exist in the DB. - if ( $title->getNamespace() === NS_SPECIAL && $action !== 'createaccount' ) { + if ( $title->getNamespace() === NS_SPECIAL + && !in_array( $action, [ 'createaccount', 'autocreateaccount' ], true ) + ) { $errors[] = [ 'ns-specialprotected' ]; } diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php index b43127460f0..43316ad592b 100644 --- a/includes/auth/AuthManager.php +++ b/includes/auth/AuthManager.php @@ -1112,10 +1112,12 @@ class AuthManager implements LoggerAwareInterface { /** * @param callable $authorizer ( string $action, PageIdentity $target, PermissionStatus $status ) + * @param string $action * @return StatusValue */ private function authorizeInternal( - callable $authorizer + callable $authorizer, + string $action ): StatusValue { // Wiki is read-only? if ( $this->readOnlyMode->isReadOnly() ) { @@ -1124,7 +1126,7 @@ class AuthManager implements LoggerAwareInterface { $permStatus = new PermissionStatus(); if ( !$authorizer( - 'createaccount', + $action, SpecialPage::getTitleFor( 'CreateAccount' ), $permStatus ) ) { @@ -1158,7 +1160,8 @@ class AuthManager implements LoggerAwareInterface { PermissionStatus $status ) use ( $creator ) { return $creator->probablyCan( $action, $target, $status ); - } + }, + 'createaccount' ); } @@ -1181,7 +1184,8 @@ class AuthManager implements LoggerAwareInterface { PermissionStatus $status ) use ( $creator ) { return $creator->authorizeWrite( $action, $target, $status ); - } + }, + 'createaccount' ); } @@ -1712,9 +1716,19 @@ class AuthManager implements LoggerAwareInterface { * - one of the self::AUTOCREATE_SOURCE_* constants * @param bool $login Whether to also log the user in * @param bool $log Whether to generate a user creation log entry (since 1.36) + * @param Authority|null $performer The performer of the action to use for user rights + * checking. Normally null to indicate an anonymous performer. Added in 1.42 for + * Special:CreateLocalAccount (T234371). + * * @return Status Good if user was created, Ok if user already existed, otherwise Fatal */ - public function autoCreateUser( User $user, $source, $login = true, $log = true ) { + public function autoCreateUser( + User $user, + $source, + $login = true, + $log = true, + ?Authority $performer = null + ) { $validSources = [ self::AUTOCREATE_SOURCE_SESSION, self::AUTOCREATE_SOURCE_MAINT, @@ -1777,10 +1791,12 @@ class AuthManager implements LoggerAwareInterface { return Status::newFatal( wfMessage( 'readonlytext', $reason ) ); } + // If there is a non-anonymous performer, don't use their session + $session = $performer ? null : $this->request->getSession(); + // Check the session, if we tried to create this user already there's // no point in retrying. - $session = $this->request->getSession(); - if ( $session->get( self::AUTOCREATE_BLOCKLIST ) ) { + if ( $session && $session->get( self::AUTOCREATE_BLOCKLIST ) ) { $this->logger->debug( __METHOD__ . ': blacklisted in session {sessionid}', [ 'username' => $username, 'sessionid' => $session->getId(), @@ -1801,26 +1817,31 @@ class AuthManager implements LoggerAwareInterface { $this->logger->debug( __METHOD__ . ': name "{username}" is not usable', [ 'username' => $username, ] ); - $session->set( self::AUTOCREATE_BLOCKLIST, 'noname' ); + if ( $session ) { + $session->set( self::AUTOCREATE_BLOCKLIST, 'noname' ); + } $user->setId( 0 ); $user->loadFromId(); return Status::newFatal( 'noname' ); } // Is the IP user able to create accounts? - $anon = $this->userFactory->newAnonymous(); - if ( $source !== self::AUTOCREATE_SOURCE_MAINT && - !$anon->isAllowedAny( 'createaccount', 'autocreateaccount' ) - ) { - $this->logger->debug( __METHOD__ . ': IP lacks the ability to create or autocreate accounts', [ - 'username' => $username, - 'clientip' => $anon->getName(), - ] ); - $session->set( self::AUTOCREATE_BLOCKLIST, 'authmanager-autocreate-noperm' ); - $session->persist(); - $user->setId( 0 ); - $user->loadFromId(); - return Status::newFatal( 'authmanager-autocreate-noperm' ); + $performer ??= $this->userFactory->newAnonymous(); + if ( $source !== self::AUTOCREATE_SOURCE_MAINT ) { + $status = $this->authorizeAutoCreateAccount( $performer ); + if ( !$status->isOK() ) { + $this->logger->debug( __METHOD__ . ': cannot create or autocreate accounts', [ + 'username' => $username, + 'creator' => $performer->getName(), + ] ); + if ( $session ) { + $session->set( self::AUTOCREATE_BLOCKLIST, $status ); + $session->persist(); + } + $user->setId( 0 ); + $user->loadFromId(); + return Status::wrap( $status ); + } } // Avoid account creation races on double submissions @@ -1851,7 +1872,9 @@ class AuthManager implements LoggerAwareInterface { 'username' => $username, 'reason' => $ret->getWikiText( false, false, 'en' ), ] ); - $session->set( self::AUTOCREATE_BLOCKLIST, $status ); + if ( $session ) { + $session->set( self::AUTOCREATE_BLOCKLIST, $status ); + } $user->setId( 0 ); $user->loadFromId(); return $ret; @@ -1951,6 +1974,26 @@ class AuthManager implements LoggerAwareInterface { return Status::newGood(); } + /** + * Authorize automatic account creation. This is like account creation but + * checks the autocreateaccount right instead of the createaccount right. + * + * @param Authority $creator + * @return StatusValue + */ + private function authorizeAutoCreateAccount( Authority $creator ) { + return $this->authorizeInternal( + static function ( + string $action, + PageIdentity $target, + PermissionStatus $status + ) use ( $creator ) { + return $creator->authorizeWrite( $action, $target, $status ); + }, + 'autocreateaccount' + ); + } + // endregion -- end of Account creation /***************************************************************************/ diff --git a/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php b/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php index b7ac4782404..5e5f61b3d1e 100644 --- a/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php +++ b/includes/auth/CheckBlocksSecondaryAuthenticationProvider.php @@ -21,10 +21,7 @@ namespace MediaWiki\Auth; -use MediaWiki\Block\AbstractBlock; use MediaWiki\MainConfigNames; -use MediaWiki\MediaWikiServices; -use StatusValue; /** * Check if the user is blocked, and prevent authentication if so. @@ -83,53 +80,4 @@ class CheckBlocksSecondaryAuthenticationProvider extends AbstractSecondaryAuthen return AuthenticationResponse::newAbstain(); } - /** @inheritDoc */ - public function testUserForCreation( $user, $autocreate, array $options = [] ) { - // isBlockedFromCreateAccount() does not return non-accountcreation blocks, but we need them - // in the $wgBlockDisablesLogin case; getBlock() is unreliable for IP blocks. So we need both. - $blocks = [ - 'local-createaccount' => $user->isBlockedFromCreateAccount(), - 'local' => $user->getBlock(), - ]; - foreach ( $blocks as $block ) { - /** @var AbstractBlock $block */ - if ( $block && $block->isSitewide() - // This method is for checking a given account/username, not the current user, so - // ignore IP blocks; they will be checked elsewhere via authorizeCreateAccount(). - // FIXME: special-case autocreation which doesn't do that check. Should it? - && ( $block->isBlocking( $user ) || $autocreate ) - && ( - // Should blocks that prevent account creation also prevent autocreation? - // We'll go with yes here. - $block->isCreateAccountBlocked() - // A successful autocreation means the user is logged in, so we must make sure to - // honor $wgBlockDisablesLogin. If it's enabled, sitewide blocks are expected to - // prevent login regardless of their flags. - || ( $autocreate && $this->blockDisablesLogin ) - ) - // FIXME: ideally on autocreation we'd figure out if the user has the ipblock-exempt - // or globalblock-exempt right via some central authorization system like - // CentralAuth global groups. But at this point the local account doesn't exist - // yet so there is no way to do that. There should probably be some separate hook - // to fetch user rights for a central user. - // FIXME: T249444: there should probably be a way to force autocreation through blocks - ) { - $formatter = MediaWikiServices::getInstance()->getBlockErrorFormatter(); - - $context = \RequestContext::getMain(); - - $language = $context->getUser()->isSafeToLoad() ? - \RequestContext::getMain()->getLanguage() : - MediaWikiServices::getInstance()->getContentLanguage(); - - $ip = $context->getRequest()->getIP(); - - return StatusValue::newFatal( - $formatter->getMessage( $block, $user, $language, $ip ) - ); - } - } - return StatusValue::newGood(); - } - } diff --git a/includes/block/AbstractBlock.php b/includes/block/AbstractBlock.php index fd7eb4321b4..75da85fcf77 100644 --- a/includes/block/AbstractBlock.php +++ b/includes/block/AbstractBlock.php @@ -284,6 +284,7 @@ abstract class AbstractBlock implements Block { $res = null; switch ( $right ) { + case 'autocreateaccount': case 'createaccount': $res = $this->isCreateAccountBlocked(); break; diff --git a/tests/phpunit/includes/auth/AuthManagerTest.php b/tests/phpunit/includes/auth/AuthManagerTest.php index 43ca1e6ce8f..2e0e571e4f7 100644 --- a/tests/phpunit/includes/auth/AuthManagerTest.php +++ b/tests/phpunit/includes/auth/AuthManagerTest.php @@ -10,6 +10,8 @@ use MediaWiki\Auth\Hook\SecuritySensitiveOperationStatusHook; use MediaWiki\Auth\Hook\UserLoggedInHook; use MediaWiki\Block\BlockManager; use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Block\Restriction\PageRestriction; +use MediaWiki\Block\SystemBlock; use MediaWiki\Config\Config; use MediaWiki\Config\HashConfig; use MediaWiki\Config\ServiceOptions; @@ -106,6 +108,7 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase { protected function setUp(): void { parent::setUp(); $this->tablesUsed[] = 'ipblocks'; + $this->tablesUsed[] = 'user'; } /** @@ -2796,16 +2799,16 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase { $this->hook( 'LocalUserCreated', LocalUserCreatedHook::class, $this->never() ); $ret = $this->manager->autoCreateUser( $user, AuthManager::AUTOCREATE_SOURCE_SESSION, true, true ); $this->unhook( 'LocalUserCreated' ); - $this->assertEquals( Status::newFatal( 'authmanager-autocreate-noperm' ), $ret ); + $this->assertTrue( $ret->hasMessage( 'badaccess-group0' ) ); $this->assertSame( 0, $user->getId() ); $this->assertNotEquals( $username, $user->getName() ); $this->assertSame( 0, $session->getUser()->getId() ); $this->assertSame( [ - [ LogLevel::DEBUG, 'IP lacks the ability to create or autocreate accounts' ], + [ LogLevel::DEBUG, 'cannot create or autocreate accounts' ], ], $logger->getBuffer() ); $logger->clearBuffer(); - $this->assertSame( - 'authmanager-autocreate-noperm', $session->get( AuthManager::AUTOCREATE_BLOCKLIST ) + $this->assertEquals( + (string)$ret, (string)$session->get( AuthManager::AUTOCREATE_BLOCKLIST ) ); // maintenance scripts always work @@ -3076,6 +3079,87 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase { $workaroundPHPUnitBug = true; } + /** + * @dataProvider provideAutoCreateUserBlocks + */ + public function testAutoCreateUserBlocks( + string $blockType, + array $blockOptions, + string $performerType, + bool $expectedStatus + + ) { + if ( $blockType === 'ip' ) { + $blockOptions['address'] = '127.0.0.0/24'; + } elseif ( $blockType === 'global-ip' ) { + $this->setTemporaryHook( 'GetUserBlock', + static function ( $user, $ip, &$block ) use ( $blockOptions ) { + $block = new SystemBlock( $blockOptions ); + $block->isCreateAccountBlocked( true ); + } + ); + $blockOptions = null; + } elseif ( $blockType === 'none' ) { + $blockOptions = null; + } else { + $this->fail( "Unknown block type \"$blockType\"" ); + } + + if ( $blockOptions !== null ) { + $blockOptions += [ + 'by' => $this->getTestSysop()->getUser(), + 'reason' => __METHOD__, + 'expiry' => time() + 100500, + ]; + $blockStore = $this->getServiceContainer()->getDatabaseBlockStore(); + $block = new DatabaseBlock( $blockOptions ); + $blockStore->insertBlock( $block ); + } + + if ( $performerType === 'sysop' ) { + $performer = $this->getTestSysop()->getUser(); + } elseif ( $performerType === 'anon' ) { + $performer = null; + } else { + $this->fail( "Unknown performer type \"$performerType\"" ); + } + + $this->logger = LoggerFactory::getInstance( 'AuthManagerTest' ); + $this->initializeManager( true ); + + $user = $this->userFactory->newFromName( 'NewUser' ); + $status = $this->manager->autoCreateUser( $user, + AuthManager::AUTOCREATE_SOURCE_SESSION, true, true, $performer ); + $this->assertSame( $expectedStatus, $status->isGood() ); + } + + public static function provideAutoCreateUserBlocks() { + return [ + // block type (ip/global/none), block options, performer, expected status + 'not blocked' => [ 'none', [], 'anon', true ], + 'ip-blocked' => [ 'ip', [], 'anon', true ], + 'ip-blocked with createAccount' => [ + 'ip', + [ 'createAccount' => true ], + 'anon', + false + ], + 'partially ip-blocked' => [ + 'ip', + [ 'restrictions' => new PageRestriction( 0, 1 ) ], + 'anon', + true + ], + 'ip-blocked with sysop performer' => [ + 'ip', + [ 'createAccount' => true ], + 'sysop', + true + ], + 'globally blocked' => [ 'global-ip', [], 'anon', false ], + ]; + } + /** * @dataProvider provideGetAuthenticationRequests * @param string $action diff --git a/tests/phpunit/includes/auth/CheckBlocksSecondaryAuthenticationProviderTest.php b/tests/phpunit/includes/auth/CheckBlocksSecondaryAuthenticationProviderTest.php index 595108d8940..13aa1f587f9 100644 --- a/tests/phpunit/includes/auth/CheckBlocksSecondaryAuthenticationProviderTest.php +++ b/tests/phpunit/includes/auth/CheckBlocksSecondaryAuthenticationProviderTest.php @@ -185,64 +185,4 @@ class CheckBlocksSecondaryAuthenticationProviderTest extends \MediaWikiIntegrati ]; } - /** - * @dataProvider provideTestUserForCreation - */ - public function testTestUserForCreation( - bool $autocreate, - string $blockType, - array $blockOptions, - bool $blockDisablesLogin, - bool $expectedStatus - - ) { - /** @var AuthManager|MockObject $authManager */ - $authManager = $this->createNoOpMock( AuthManager::class, [ 'getRequest' ] ); - $authManager->method( 'getRequest' )->willReturn( new FauxRequest() ); - $provider = new CheckBlocksSecondaryAuthenticationProvider( - [ 'blockDisablesLogin' => $blockDisablesLogin ] - ); - $this->initProvider( $provider, new HashConfig(), null, $authManager ); - - $user = $this->getAnyBlockedUser( $blockType, $blockOptions ); - - $status = $provider->testUserForCreation( $user, - $autocreate ? AuthManager::AUTOCREATE_SOURCE_SESSION : false ); - $this->assertSame( $expectedStatus, $status->isGood() ); - } - - public static function provideTestUserForCreation() { - // Tests for normal signup: only prevent if the user is blocked, the block is specifically - // targeted to the username, not partial, and the block prevents account creation. - $signupTests = [ - // block type (user/ip/global/none), block options, wgBlockDisablesLogin, expected status - 'not blocked' => [ 'none', [], true, true ], - 'blocked' => [ 'user', [], false, true ], - 'createaccount-blocked' => [ 'user', [ 'createAccount' => true ], false, false ], - 'blocked with wgBlockDisablesLogin' => [ 'user', [], true, true ], - 'ip-blocked' => [ 'ip', [], false, true ], - 'createaccount-ip-blocked' => [ 'ip', [ 'createAccount' => true ], false, true ], - 'ip-blocked with wgBlockDisablesLogin' => [ 'ip', [], true, true ], - 'partially blocked' => [ 'user', [ 'createAccount' => true, 'sitewide' => false ], true, true ], - 'globally blocked' => [ 'global-ip', [], false, true ], - 'globally blocked with wgBlockDisablesLogin' => [ 'global-ip', [], true, true ], - ]; - - // Tests for autocreation: in addition, also prevent on blocks without the - // createaccount flag if $wgBlockDisablesLogin is set, and also prevent on IP blocks - // when either the createaccount flag or $wgBlockDisablesLogin is set. - $autocreateTests = $signupTests; - $autocreateTests['blocked with wgBlockDisablesLogin'][3] = false; - $autocreateTests['createaccount-ip-blocked'][3] = false; - $autocreateTests['ip-blocked with wgBlockDisablesLogin'][3] = false; - - foreach ( $signupTests as $name => $test ) { - // add autocreate parameter - yield 'signup, ' . $name => array_merge( [ false ], $test ); - } - foreach ( $autocreateTests as $name => $test ) { - yield 'autocreate, ' . $name => array_merge( [ true ], $test ); - } - } - }