Merge "Improve ApiLogin test coverage"

diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php
index 14491da..8f2e759 100644 (file)
--- 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 5762120..0c4b425 100644 (file)
--- 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 4492141..12ca2b8 100644 (file)
--- 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 0d22b21..bc0946f 100644 (file)
--- 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' ] ],
                ];
        }