From cfb62c605f55553db1588bd1b81f8c2ae06e81b2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Tue, 9 Oct 2018 18:21:54 -0700 Subject: [PATCH] Hard-deprecate LegacyHookPreAuthenticationProvider class The hooks that used to be called by this class will be removed in I24d6fa963. The only reason to keep this class around is that someone might have added it to $wgAuthManagerConfig so removing it would trigger class lookup failures, so make sure any use of the class triggers a deprecation warning. Change-Id: I9755288eda7461ecf3dcd35de2081fbb3eb04ae3 --- RELEASE-NOTES-1.33 | 3 + includes/DefaultSettings.php | 4 - .../LegacyHookPreAuthenticationProvider.php | 150 +------ ...egacyHookPreAuthenticationProviderTest.php | 375 ------------------ 4 files changed, 5 insertions(+), 527 deletions(-) delete mode 100644 tests/phpunit/includes/auth/LegacyHookPreAuthenticationProviderTest.php diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index d48322eeaf..4433ed923e 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -66,6 +66,9 @@ because of Phabricator reports. applied for Arabic and Malayalam in the future. Please enable these on your local wiki (if you have them explicitly set to false) and run maintenance/cleanupTitles.php to fix any existing page titles. +* The LegacyHookPreAuthenticationProvider class, deprecated since its creation + in 1.27 as part of the AuthManager re-write, now emits deprecation warnings. + This will help identify the issue if you added it to $wgAuthManagerConfig. * … === Other changes in 1.33 === diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 20d965daf8..4a3b6db607 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4571,10 +4571,6 @@ $wgAuthManagerConfig = null; */ $wgAuthManagerAutoConfig = [ 'preauth' => [ - MediaWiki\Auth\LegacyHookPreAuthenticationProvider::class => [ - 'class' => MediaWiki\Auth\LegacyHookPreAuthenticationProvider::class, - 'sort' => 0, - ], MediaWiki\Auth\ThrottlePreAuthenticationProvider::class => [ 'class' => MediaWiki\Auth\ThrottlePreAuthenticationProvider::class, 'sort' => 0, diff --git a/includes/auth/LegacyHookPreAuthenticationProvider.php b/includes/auth/LegacyHookPreAuthenticationProvider.php index ad8856491f..5f55ec5ea0 100644 --- a/includes/auth/LegacyHookPreAuthenticationProvider.php +++ b/includes/auth/LegacyHookPreAuthenticationProvider.php @@ -21,10 +21,6 @@ namespace MediaWiki\Auth; -use LoginForm; -use StatusValue; -use User; - /** * A pre-authentication provider to call some legacy hooks. * @ingroup Auth @@ -32,149 +28,7 @@ use User; * @deprecated since 1.27 */ class LegacyHookPreAuthenticationProvider extends AbstractPreAuthenticationProvider { - - public function testForAuthentication( array $reqs ) { - $req = AuthenticationRequest::getRequestByClass( $reqs, PasswordAuthenticationRequest::class ); - if ( $req ) { - $user = User::newFromName( $req->username ); - $password = $req->password; - } else { - $user = null; - foreach ( $reqs as $req ) { - if ( $req->username !== null ) { - $user = User::newFromName( $req->username ); - break; - } - } - if ( !$user ) { - $this->logger->debug( __METHOD__ . ': No username in $reqs, skipping hooks' ); - return StatusValue::newGood(); - } - - // Something random for the 'AbortLogin' hook. - $password = wfRandomString( 32 ); - } - - $msg = null; - if ( !\Hooks::run( 'LoginUserMigrated', [ $user, &$msg ], '1.27' ) ) { - return $this->makeFailResponse( - $user, LoginForm::USER_MIGRATED, $msg, 'LoginUserMigrated' - ); - } - - $abort = LoginForm::ABORTED; - $msg = null; - if ( !\Hooks::run( 'AbortLogin', [ $user, $password, &$abort, &$msg ], '1.27' ) ) { - return $this->makeFailResponse( $user, $abort, $msg, 'AbortLogin' ); - } - - return StatusValue::newGood(); - } - - public function testForAccountCreation( $user, $creator, array $reqs ) { - $abortError = ''; - $abortStatus = null; - if ( !\Hooks::run( 'AbortNewAccount', [ $user, &$abortError, &$abortStatus ], '1.27' ) ) { - // Hook point to add extra creation throttles and blocks - $this->logger->debug( __METHOD__ . ': a hook blocked creation' ); - if ( $abortStatus === null ) { - // Report back the old string as a raw message status. - // This will report the error back as 'createaccount-hook-aborted' - // with the given string as the message. - // To return a different error code, return a StatusValue object. - $msg = wfMessage( 'createaccount-hook-aborted' )->rawParams( $abortError ); - return StatusValue::newFatal( $msg ); - } else { - // For MediaWiki 1.23+ and updated hooks, return the Status object - // returned from the hook. - $ret = StatusValue::newGood(); - $ret->merge( $abortStatus ); - return $ret; - } - } - - return StatusValue::newGood(); - } - - public function testUserForCreation( $user, $autocreate, array $options = [] ) { - if ( $autocreate !== false ) { - $abortError = ''; - if ( !\Hooks::run( 'AbortAutoAccount', [ $user, &$abortError ], '1.27' ) ) { - // Hook point to add extra creation throttles and blocks - $this->logger->debug( __METHOD__ . ": a hook blocked auto-creation: $abortError\n" ); - return $this->makeFailResponse( - $user, LoginForm::ABORTED, $abortError, 'AbortAutoAccount' - ); - } - } - - return StatusValue::newGood(); - } - - /** - * Construct an appropriate failure response - * @param User $user - * @param int $constant One of the LoginForm::… constants - * @param string|null $msg Optional message key, will be derived from $constant otherwise - * @param string $hook Name of the hook for error logging and exception messages - * @return StatusValue - */ - private function makeFailResponse( User $user, $constant, $msg, $hook ) { - switch ( $constant ) { - case LoginForm::SUCCESS: - // WTF? - $this->logger->debug( "$hook is SUCCESS?!" ); - return StatusValue::newGood(); - - case LoginForm::NEED_TOKEN: - return StatusValue::newFatal( $msg ?: 'nocookiesforlogin' ); - - case LoginForm::WRONG_TOKEN: - return StatusValue::newFatal( $msg ?: 'sessionfailure' ); - - case LoginForm::NO_NAME: - case LoginForm::ILLEGAL: - return StatusValue::newFatal( $msg ?: 'noname' ); - - case LoginForm::WRONG_PLUGIN_PASS: - case LoginForm::WRONG_PASS: - return StatusValue::newFatal( $msg ?: 'wrongpassword' ); - - case LoginForm::NOT_EXISTS: - return StatusValue::newFatal( $msg ?: 'nosuchusershort', wfEscapeWikiText( $user->getName() ) ); - - case LoginForm::EMPTY_PASS: - return StatusValue::newFatal( $msg ?: 'wrongpasswordempty' ); - - case LoginForm::RESET_PASS: - return StatusValue::newFatal( $msg ?: 'resetpass_announce' ); - - case LoginForm::THROTTLED: - $throttle = $this->config->get( 'PasswordAttemptThrottle' ); - return StatusValue::newFatal( - $msg ?: 'login-throttled', - \Message::durationParam( $throttle['seconds'] ) - ); - - case LoginForm::USER_BLOCKED: - return StatusValue::newFatal( - $msg ?: 'login-userblocked', wfEscapeWikiText( $user->getName() ) - ); - - case LoginForm::ABORTED: - return StatusValue::newFatal( - $msg ?: 'login-abort-generic', wfEscapeWikiText( $user->getName() ) - ); - - case LoginForm::USER_MIGRATED: - $error = $msg ?: 'login-migrated-generic'; - return StatusValue::newFatal( ...(array)$error ); - - // @codeCoverageIgnoreStart - case LoginForm::CREATE_BLOCKED: // Can never happen - default: - throw new \DomainException( __METHOD__ . ": Unhandled case value from $hook" ); - } - // @codeCoverageIgnoreEnd + public function __construct() { + wfDeprecated( self::class, '1.27' ); } } diff --git a/tests/phpunit/includes/auth/LegacyHookPreAuthenticationProviderTest.php b/tests/phpunit/includes/auth/LegacyHookPreAuthenticationProviderTest.php deleted file mode 100644 index 4b89d254e2..0000000000 --- a/tests/phpunit/includes/auth/LegacyHookPreAuthenticationProviderTest.php +++ /dev/null @@ -1,375 +0,0 @@ -getMockBuilder( \FauxRequest::class ) - ->setMethods( [ 'getIP' ] )->getMock(); - $request->expects( $this->any() )->method( 'getIP' )->will( $this->returnValue( '127.0.0.42' ) ); - - $manager = new AuthManager( - $request, - MediaWikiServices::getInstance()->getMainConfig() - ); - - $provider = new LegacyHookPreAuthenticationProvider(); - $provider->setManager( $manager ); - $provider->setLogger( new \Psr\Log\NullLogger() ); - $provider->setConfig( new \HashConfig( [ - 'PasswordAttemptThrottle' => [ 'count' => 23, 'seconds' => 42 ], - ] ) ); - return $provider; - } - - /** - * Sets a mock on a hook - * @param string $hook - * @param object $expect From $this->once(), $this->never(), etc. - * @return object $mock->expects( $expect )->method( ... ). - */ - protected function hook( $hook, $expect ) { - $mock = $this->getMockBuilder( __CLASS__ )->setMethods( [ "on$hook" ] )->getMock(); - $this->mergeMwGlobalArrayValue( 'wgHooks', [ - $hook => [ $mock ], - ] ); - $mockClass = get_class( $mock ); - $this->hideDeprecated( "$hook hook (used in $mockClass::on$hook)" ); - return $mock->expects( $expect )->method( "on$hook" ); - } - - /** - * Unsets a hook - * @param string $hook - */ - protected function unhook( $hook ) { - $this->mergeMwGlobalArrayValue( 'wgHooks', [ - $hook => [], - ] ); - } - - // Stubs for hooks taking reference parameters - public function onLoginUserMigrated( $user, &$msg ) { - } - public function onAbortLogin( $user, $password, &$abort, &$msg ) { - } - public function onAbortNewAccount( $user, &$abortError, &$abortStatus ) { - } - public function onAbortAutoAccount( $user, &$abortError ) { - } - - /** - * @dataProvider provideTestForAuthentication - * @param string|null $username - * @param string|null $password - * @param string|null $msgForLoginUserMigrated - * @param int|null $abortForAbortLogin - * @param string|null $msgForAbortLogin - * @param string|null $failMsg - * @param array $failParams - */ - public function testTestForAuthentication( - $username, $password, - $msgForLoginUserMigrated, $abortForAbortLogin, $msgForAbortLogin, - $failMsg, $failParams = [] - ) { - $reqs = []; - if ( $username === null ) { - $this->hook( 'LoginUserMigrated', $this->never() ); - $this->hook( 'AbortLogin', $this->never() ); - } else { - if ( $password === null ) { - $req = $this->getMockForAbstractClass( AuthenticationRequest::class ); - } else { - $req = new PasswordAuthenticationRequest(); - $req->action = AuthManager::ACTION_LOGIN; - $req->password = $password; - } - $req->username = $username; - $reqs[get_class( $req )] = $req; - - $h = $this->hook( 'LoginUserMigrated', $this->once() ); - if ( $msgForLoginUserMigrated !== null ) { - $h->will( $this->returnCallback( - function ( $user, &$msg ) use ( $username, $msgForLoginUserMigrated ) { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( $username, $user->getName() ); - $msg = $msgForLoginUserMigrated; - return false; - } - ) ); - $this->hook( 'AbortLogin', $this->never() ); - } else { - $h->will( $this->returnCallback( - function ( $user, &$msg ) use ( $username ) { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( $username, $user->getName() ); - return true; - } - ) ); - $h2 = $this->hook( 'AbortLogin', $this->once() ); - if ( $abortForAbortLogin !== null ) { - $h2->will( $this->returnCallback( - function ( $user, $pass, &$abort, &$msg ) - use ( $username, $password, $abortForAbortLogin, $msgForAbortLogin ) - { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( $username, $user->getName() ); - if ( $password !== null ) { - $this->assertSame( $password, $pass ); - } else { - $this->assertInternalType( 'string', $pass ); - } - $abort = $abortForAbortLogin; - $msg = $msgForAbortLogin; - return false; - } - ) ); - } else { - $h2->will( $this->returnCallback( - function ( $user, $pass, &$abort, &$msg ) use ( $username, $password ) { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( $username, $user->getName() ); - if ( $password !== null ) { - $this->assertSame( $password, $pass ); - } else { - $this->assertInternalType( 'string', $pass ); - } - return true; - } - ) ); - } - } - } - unset( $h, $h2 ); - - $status = $this->getProvider()->testForAuthentication( $reqs ); - - $this->unhook( 'LoginUserMigrated' ); - $this->unhook( 'AbortLogin' ); - - if ( $failMsg === null ) { - $this->assertEquals( \StatusValue::newGood(), $status, 'should succeed' ); - } else { - $this->assertInstanceOf( \StatusValue::class, $status, 'should fail (type)' ); - $this->assertFalse( $status->isOk(), 'should fail (ok)' ); - $errors = $status->getErrors(); - $this->assertEquals( $failMsg, $errors[0]['message'], 'should fail (message)' ); - $this->assertEquals( $failParams, $errors[0]['params'], 'should fail (params)' ); - } - } - - public static function provideTestForAuthentication() { - return [ - 'No valid requests' => [ - null, null, null, null, null, null - ], - 'No hook errors' => [ - 'User', 'PaSsWoRd', null, null, null, null - ], - 'No hook errors, no password' => [ - 'User', null, null, null, null, null - ], - 'LoginUserMigrated no message' => [ - 'User', 'PaSsWoRd', false, null, null, 'login-migrated-generic' - ], - 'LoginUserMigrated with message' => [ - 'User', 'PaSsWoRd', 'LUM-abort', null, null, 'LUM-abort' - ], - 'LoginUserMigrated with message and params' => [ - 'User', 'PaSsWoRd', [ 'LUM-abort', 'foo' ], null, null, 'LUM-abort', [ 'foo' ] - ], - 'AbortLogin, SUCCESS' => [ - 'User', 'PaSsWoRd', null, \LoginForm::SUCCESS, null, null - ], - 'AbortLogin, NEED_TOKEN, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::NEED_TOKEN, null, 'nocookiesforlogin' - ], - 'AbortLogin, NEED_TOKEN, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::NEED_TOKEN, 'needtoken', 'needtoken' - ], - 'AbortLogin, WRONG_TOKEN, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_TOKEN, null, 'sessionfailure' - ], - 'AbortLogin, WRONG_TOKEN, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_TOKEN, 'wrongtoken', 'wrongtoken' - ], - 'AbortLogin, ILLEGAL, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::ILLEGAL, null, 'noname' - ], - 'AbortLogin, ILLEGAL, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::ILLEGAL, 'badname', 'badname' - ], - 'AbortLogin, NO_NAME, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::NO_NAME, null, 'noname' - ], - 'AbortLogin, NO_NAME, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::NO_NAME, 'badname', 'badname' - ], - 'AbortLogin, WRONG_PASS, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_PASS, null, 'wrongpassword' - ], - 'AbortLogin, WRONG_PASS, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_PASS, 'badpass', 'badpass' - ], - 'AbortLogin, WRONG_PLUGIN_PASS, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_PLUGIN_PASS, null, 'wrongpassword' - ], - 'AbortLogin, WRONG_PLUGIN_PASS, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::WRONG_PLUGIN_PASS, 'badpass', 'badpass' - ], - 'AbortLogin, NOT_EXISTS, no message' => [ - "User'", 'A', null, \LoginForm::NOT_EXISTS, null, 'nosuchusershort', [ 'User'' ] - ], - 'AbortLogin, NOT_EXISTS, with message' => [ - "User'", 'A', null, \LoginForm::NOT_EXISTS, 'badname', 'badname', [ 'User'' ] - ], - 'AbortLogin, EMPTY_PASS, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::EMPTY_PASS, null, 'wrongpasswordempty' - ], - 'AbortLogin, EMPTY_PASS, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::EMPTY_PASS, 'badpass', 'badpass' - ], - 'AbortLogin, RESET_PASS, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::RESET_PASS, null, 'resetpass_announce' - ], - 'AbortLogin, RESET_PASS, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::RESET_PASS, 'resetpass', 'resetpass' - ], - 'AbortLogin, THROTTLED, no message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::THROTTLED, null, 'login-throttled', - [ \Message::durationParam( 42 ) ] - ], - 'AbortLogin, THROTTLED, with message' => [ - 'User', 'PaSsWoRd', null, \LoginForm::THROTTLED, 't', 't', - [ \Message::durationParam( 42 ) ] - ], - 'AbortLogin, USER_BLOCKED, no message' => [ - "User'", 'P', null, \LoginForm::USER_BLOCKED, null, 'login-userblocked', [ 'User'' ] - ], - 'AbortLogin, USER_BLOCKED, with message' => [ - "User'", 'P', null, \LoginForm::USER_BLOCKED, 'blocked', 'blocked', [ 'User'' ] - ], - 'AbortLogin, ABORTED, no message' => [ - "User'", 'P', null, \LoginForm::ABORTED, null, 'login-abort-generic', [ 'User'' ] - ], - 'AbortLogin, ABORTED, with message' => [ - "User'", 'P', null, \LoginForm::ABORTED, 'aborted', 'aborted', [ 'User'' ] - ], - 'AbortLogin, USER_MIGRATED, no message' => [ - 'User', 'P', null, \LoginForm::USER_MIGRATED, null, 'login-migrated-generic' - ], - 'AbortLogin, USER_MIGRATED, with message' => [ - 'User', 'P', null, \LoginForm::USER_MIGRATED, 'migrated', 'migrated' - ], - 'AbortLogin, USER_MIGRATED, with message and params' => [ - 'User', 'P', null, \LoginForm::USER_MIGRATED, [ 'migrated', 'foo' ], - 'migrated', [ 'foo' ] - ], - ]; - } - - /** - * @dataProvider provideTestForAccountCreation - * @param string $msg - * @param Status|null $status - * @param StatusValue $result - */ - public function testTestForAccountCreation( $msg, $status, $result ) { - $this->hook( 'AbortNewAccount', $this->once() ) - ->will( $this->returnCallback( function ( $user, &$error, &$abortStatus ) - use ( $msg, $status ) - { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( 'User', $user->getName() ); - $error = $msg; - $abortStatus = $status; - return $error === null && $status === null; - } ) ); - - $user = \User::newFromName( 'User' ); - $creator = \User::newFromName( 'UTSysop' ); - $ret = $this->getProvider()->testForAccountCreation( $user, $creator, [] ); - - $this->unhook( 'AbortNewAccount' ); - - $this->assertEquals( $result, $ret ); - } - - public static function provideTestForAccountCreation() { - return [ - 'No hook errors' => [ - null, null, \StatusValue::newGood() - ], - 'AbortNewAccount, old style' => [ - 'foobar', null, \StatusValue::newFatal( - \Message::newFromKey( 'createaccount-hook-aborted' )->rawParams( 'foobar' ) - ) - ], - 'AbortNewAccount, new style' => [ - 'foobar', - \Status::newFatal( 'aborted!', 'param' ), - \StatusValue::newFatal( 'aborted!', 'param' ) - ], - ]; - } - - /** - * @dataProvider provideTestUserForCreation - * @param string|null $error - * @param string|null $failMsg - */ - public function testTestUserForCreation( $error, $failMsg ) { - $testUser = self::getTestUser()->getUser(); - $provider = $this->getProvider(); - $options = [ 'flags' => \User::READ_LOCKING, 'creating' => true ]; - - $this->hook( 'AbortNewAccount', $this->never() ); - $this->hook( 'AbortAutoAccount', $this->once() ) - ->will( $this->returnCallback( function ( $user, &$abortError ) use ( $testUser, $error ) { - $this->assertInstanceOf( \User::class, $user ); - $this->assertSame( $testUser->getName(), $user->getName() ); - $abortError = $error; - return $error === null; - } ) ); - $status = $provider->testUserForCreation( - $testUser, AuthManager::AUTOCREATE_SOURCE_SESSION, $options - ); - $this->unhook( 'AbortNewAccount' ); - $this->unhook( 'AbortAutoAccount' ); - if ( $failMsg === null ) { - $this->assertEquals( \StatusValue::newGood(), $status, 'should succeed' ); - } else { - $this->assertInstanceOf( \StatusValue::class, $status, 'should fail (type)' ); - $this->assertFalse( $status->isOk(), 'should fail (ok)' ); - $errors = $status->getErrors(); - $this->assertEquals( $failMsg, $errors[0]['message'], 'should fail (message)' ); - } - - $this->hook( 'AbortAutoAccount', $this->never() ); - $this->hook( 'AbortNewAccount', $this->never() ); - $status = $provider->testUserForCreation( $testUser, false, $options ); - $this->unhook( 'AbortNewAccount' ); - $this->unhook( 'AbortAutoAccount' ); - $this->assertEquals( \StatusValue::newGood(), $status, 'should succeed' ); - } - - public static function provideTestUserForCreation() { - return [ - 'Success' => [ null, null ], - 'Fail, no message' => [ false, 'login-abort-generic' ], - 'Fail, with message' => [ 'fail', 'fail' ], - ]; - } -} -- 2.20.1