From 1496fd4b4e569cb52f903f575c0a43dfa6ec8ae7 Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Mon, 8 Oct 2018 16:30:13 +0300 Subject: [PATCH] Improve ApiLogin test coverage Coverage is 100% except for one session-related bit that seems a bit involved to test right now. It looks like it will be easier once SessionManager becomes a service. I removed the third parameter from the return value of canonicalizeLoginData, since af37a4c7 made it always return true. I also removed three lines of dead code from ApiLogin.php. Change-Id: Ia0073eddd27c82827518e0031e3c313f83cfd7cc --- includes/api/ApiLogin.php | 9 +- includes/user/BotPassword.php | 8 +- tests/phpunit/includes/api/ApiLoginTest.php | 172 ++++++++++++++++-- .../phpunit/includes/user/BotPasswordTest.php | 8 +- 4 files changed, 172 insertions(+), 25 deletions(-) diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php index 14491da19f..8f2e759276 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -130,7 +130,7 @@ class ApiLogin extends ApiBase { $session = $status->getValue(); $authRes = 'Success'; $loginType = 'BotPassword'; - } elseif ( !$botLoginData[2] || + } elseif ( $status->hasMessage( 'login-throttled' ) || $status->hasMessage( 'botpasswords-needs-reset' ) || $status->hasMessage( 'botpasswords-locked' ) @@ -141,6 +141,7 @@ class ApiLogin extends ApiBase { 'BotPassword login failed: ' . $status->getWikiText( false, false, 'en' ) ); } + // For other errors, let's see if it's a valid non-bot login } if ( $authRes === false ) { @@ -220,15 +221,15 @@ class ApiLogin extends ApiBase { ); break; + // @codeCoverageIgnoreStart + // Unreachable default: ApiBase::dieDebug( __METHOD__, "Unhandled case value: {$authRes}" ); + // @codeCoverageIgnoreEnd } $this->getResult()->addValue( null, 'login', $result ); - if ( $loginType === 'LoginForm' && isset( LoginForm::$statusCodes[$authRes] ) ) { - $authRes = LoginForm::$statusCodes[$authRes]; - } LoggerFactory::getInstance( 'authevents' )->info( 'Login attempt', [ 'event' => 'login', 'successful' => $authRes === 'Success', diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php index 5762120053..0c4b425020 100644 --- a/includes/user/BotPassword.php +++ b/includes/user/BotPassword.php @@ -410,9 +410,7 @@ class BotPassword implements IDBAccessObject { /** * There are two ways to login with a bot password: "username@appId", "password" and * "username", "appId@password". Transform it so it is always in the first form. - * Returns [bot username, bot password, could be normal password?] where the last one is a flag - * meaning this could either be a bot password or a normal password, it cannot be decided for - * certain (although in such cases it almost always will be a bot password). + * Returns [bot username, bot password]. * If this cannot be a bot password login just return false. * @param string $username * @param string $password @@ -424,14 +422,14 @@ class BotPassword implements IDBAccessObject { if ( strlen( $password ) >= 32 && strpos( $username, $sep ) !== false ) { // the separator is not valid in new usernames but might appear in legacy ones if ( preg_match( '/^[0-9a-w]{32,}$/', $password ) ) { - return [ $username, $password, true ]; + return [ $username, $password ]; } } elseif ( strlen( $password ) > 32 && strpos( $password, $sep ) !== false ) { $segments = explode( $sep, $password ); $password = array_pop( $segments ); $appId = implode( $sep, $segments ); if ( preg_match( '/^[0-9a-w]{32,}$/', $password ) ) { - return [ $username . $sep . $appId, $password, true ]; + return [ $username . $sep . $appId, $password ]; } } return false; diff --git a/tests/phpunit/includes/api/ApiLoginTest.php b/tests/phpunit/includes/api/ApiLoginTest.php index 449214186f..12ca2b89ba 100644 --- a/tests/phpunit/includes/api/ApiLoginTest.php +++ b/tests/phpunit/includes/api/ApiLoginTest.php @@ -13,6 +13,34 @@ use Wikimedia\TestingAccessWrapper; * @covers ApiLogin */ class ApiLoginTest extends ApiTestCase { + public function setUp() { + parent::setUp(); + + $this->tablesUsed[] = 'bot_passwords'; + } + + public static function provideEnableBotPasswords() { + return [ + 'Bot passwords enabled' => [ true ], + 'Bot passwords disabled' => [ false ], + ]; + } + + /** + * @dataProvider provideEnableBotPasswords + */ + public function testExtendedDescription( $enableBotPasswords ) { + $this->setMwGlobals( 'wgEnableBotPasswords', $enableBotPasswords ); + $ret = $this->doApiRequest( [ + 'action' => 'paraminfo', + 'modules' => 'login', + 'helpformat' => 'raw', + ] ); + $this->assertSame( + 'apihelp-login-extended-description' . ( $enableBotPasswords ? '' : '-nobotpasswords' ), + $ret[0]['paraminfo']['modules'][0]['description'][1]['key'] + ); + } /** * Test result of attempted login with an empty username @@ -30,7 +58,16 @@ class ApiLoginTest extends ApiTestCase { $this->assertSame( 'Failed', $ret[0]['login']['result'] ); } - private function doUserLogin( $name, $password ) { + /** + * Attempts to log in with the given name and password, retrieves the returned token, and makes + * a second API request to actually log in with the token. + * + * @param string $name + * @param string $password + * @param array $params To pass to second request + * @return array Result of second doApiRequest + */ + private function doUserLogin( $name, $password, array $params = [] ) { $ret = $this->doApiRequest( [ 'action' => 'login', 'lgname' => $name, @@ -38,12 +75,25 @@ class ApiLoginTest extends ApiTestCase { $this->assertSame( 'NeedToken', $ret[0]['login']['result'] ); - return $this->doApiRequest( [ - 'action' => 'login', - 'lgtoken' => $ret[0]['login']['token'], - 'lgname' => $name, - 'lgpassword' => $password, - ], $ret[2] ); + return $this->doApiRequest( array_merge( + [ + 'action' => 'login', + 'lgtoken' => $ret[0]['login']['token'], + 'lgname' => $name, + 'lgpassword' => $password, + ], $params + ), $ret[2] ); + } + + public function testBadToken() { + $user = self::$users['sysop']; + $userName = $user->getUser()->getName(); + $password = $user->getPassword(); + $user->getUser()->logout(); + + $ret = $this->doUserLogin( $userName, $password, [ 'lgtoken' => 'invalid token' ] ); + + $this->assertSame( 'WrongToken', $ret[0]['login']['result'] ); } public function testBadPass() { @@ -56,7 +106,12 @@ class ApiLoginTest extends ApiTestCase { $this->assertSame( 'Failed', $ret[0]['login']['result'] ); } - public function testGoodPass() { + /** + * @dataProvider provideEnableBotPasswords + */ + public function testGoodPass( $enableBotPasswords ) { + $this->setMwGlobals( 'wgEnableBotPasswords', $enableBotPasswords ); + $user = self::$users['sysop']; $userName = $user->getUser()->getName(); $password = $user->getPassword(); @@ -65,9 +120,56 @@ class ApiLoginTest extends ApiTestCase { $ret = $this->doUserLogin( $userName, $password ); $this->assertSame( 'Success', $ret[0]['login']['result'] ); + $this->assertSame( + [ 'warnings' => ApiErrorFormatter::stripMarkup( wfMessage( + 'apiwarn-deprecation-login-' . ( $enableBotPasswords ? '' : 'no' ) . 'botpw' )-> + text() ) ], + $ret[0]['warnings']['login'] + ); + } + + /** + * @dataProvider provideEnableBotPasswords + */ + public function testUnsupportedAuthResponseType( $enableBotPasswords ) { + $this->setMwGlobals( 'wgEnableBotPasswords', $enableBotPasswords ); + + $mockProvider = $this->createMock( + MediaWiki\Auth\AbstractSecondaryAuthenticationProvider::class ); + $mockProvider->method( 'beginSecondaryAuthentication' )->willReturn( + MediaWiki\Auth\AuthenticationResponse::newUI( + [ new MediaWiki\Auth\UsernameAuthenticationRequest ], + // Slightly silly message here + wfMessage( 'mainpage' ) + ) + ); + $mockProvider->method( 'getAuthenticationRequests' ) + ->willReturn( [] ); + + $this->mergeMwGlobalArrayValue( 'wgAuthManagerConfig', [ + 'secondaryauth' => [ [ + 'factory' => function () use ( $mockProvider ) { + return $mockProvider; + }, + ] ], + ] ); + + $user = self::$users['sysop']; + $userName = $user->getUser()->getName(); + $password = $user->getPassword(); + $user->getUser()->logout(); + + $ret = $this->doUserLogin( $userName, $password ); + + $this->assertSame( [ 'login' => [ + 'result' => 'Aborted', + 'reason' => ApiErrorFormatter::stripMarkup( wfMessage( + 'api-login-fail-aborted' . ( $enableBotPasswords ? '' : '-nobotpw' ) )->text() ), + ] ], $ret[0] ); } /** + * @todo Should this test just be deleted? * @group Broken */ public function testGotCookie() { @@ -114,7 +216,10 @@ class ApiLoginTest extends ApiTestCase { ); } - public function testBotPassword() { + /** + * @return [ $username, $password ] suitable for passing to an API request for successful login + */ + private function setUpForBotPassword() { global $wgSessionProviders; $this->setMwGlobals( [ @@ -171,12 +276,51 @@ class ApiLoginTest extends ApiTestCase { $lgName = $user->getUser()->getName() . BotPassword::getSeparator() . 'foo'; - $ret = $this->doUserLogin( $lgName, $password ); + return [ $lgName, $password ]; + } + + public function testBotPassword() { + $ret = $this->doUserLogin( ...$this->setUpForBotPassword() ); $this->assertSame( 'Success', $ret[0]['login']['result'] ); } - public function testLoginWithNoSameOriginSecurity() { + public function testBotPasswordThrottled() { + global $wgPasswordAttemptThrottle; + + $this->setGroupPermissions( 'sysop', 'noratelimit', false ); + $this->setMwGlobals( 'wgMainCacheType', 'hash' ); + + list( $name, $password ) = $this->setUpForBotPassword(); + + for ( $i = 0; $i < $wgPasswordAttemptThrottle[0]['count']; $i++ ) { + $this->doUserLogin( $name, 'incorrectpasswordincorrectpassword' ); + } + + $ret = $this->doUserLogin( $name, $password ); + + $this->assertSame( [ + 'result' => 'Failed', + 'reason' => ApiErrorFormatter::stripMarkup( wfMessage( 'login-throttled' )-> + durationParams( $wgPasswordAttemptThrottle[0]['seconds'] )->text() ), + ], $ret[0]['login'] ); + } + + public function testBotPasswordLocked() { + $this->setTemporaryHook( 'UserIsLocked', function ( User $unused, &$isLocked ) { + $isLocked = true; + return true; + } ); + + $ret = $this->doUserLogin( ...$this->setUpForBotPassword() ); + + $this->assertSame( [ + 'result' => 'Failed', + 'reason' => wfMessage( 'botpasswords-locked' )->text(), + ], $ret[0]['login'] ); + } + + public function testNoSameOriginSecurity() { $this->setTemporaryHook( 'RequestHasSameOriginSecurity', function () { return false; @@ -185,11 +329,15 @@ class ApiLoginTest extends ApiTestCase { $ret = $this->doApiRequest( [ 'action' => 'login', + 'errorformat' => 'plaintext', ] )[0]['login']; $this->assertSame( [ 'result' => 'Aborted', - 'reason' => 'Cannot log in when the same-origin policy is not applied.', + 'reason' => [ + 'code' => 'api-login-fail-sameorigin', + 'text' => 'Cannot log in when the same-origin policy is not applied.', + ], ], $ret ); } } diff --git a/tests/phpunit/includes/user/BotPasswordTest.php b/tests/phpunit/includes/user/BotPasswordTest.php index 0d22b21592..bc0946fbf7 100644 --- a/tests/phpunit/includes/user/BotPasswordTest.php +++ b/tests/phpunit/includes/user/BotPasswordTest.php @@ -248,13 +248,13 @@ class BotPasswordTest extends MediaWikiTestCase { [ 'user', 'abc@def', false ], [ 'legacy@user', 'pass', false ], [ 'user@bot', '12345678901234567890123456789012', - [ 'user@bot', '12345678901234567890123456789012', true ] ], + [ 'user@bot', '12345678901234567890123456789012' ] ], [ 'user', 'bot@12345678901234567890123456789012', - [ 'user@bot', '12345678901234567890123456789012', true ] ], + [ 'user@bot', '12345678901234567890123456789012' ] ], [ 'user', 'bot@12345678901234567890123456789012345', - [ 'user@bot', '12345678901234567890123456789012345', true ] ], + [ 'user@bot', '12345678901234567890123456789012345' ] ], [ 'user', 'bot@x@12345678901234567890123456789012', - [ 'user@bot@x', '12345678901234567890123456789012', true ] ], + [ 'user@bot@x', '12345678901234567890123456789012' ] ], ]; } -- 2.20.1