API: Check assert parameters earlier in the request

Specifically, check the assert and assertuser parameters before setting
up the action module, so errors in parsing the module's parameters due
to being logged out don't override the client's intended "am I logged
in?" check.

Note this means that assertion failures will no longer use custom module
output formatters. This seems like an acceptable tradeoff: on Wikimedia
sites in May 2018 there were no requests that would have been affected
by this change.

Bug: T197672
Change-Id: I02a71395d5ed9f445e57162f2136292825f8dbb5

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index 731f874..261f4b8 100644 (file)
--- a/RELEASE-NOTES-1.32
+++ b/RELEASE-NOTES-1.32
@@ -78,6 +78,9 @@ production.
     templated parameters.
 * It is now an error to submit too many values for a multi-valued parameter.
   This has generated a warning since MediaWiki 1.14.
+* Assertion failures from the 'assert' and 'assertuser' parameters will no
+  longer use the action module's custom response format, for the few modules
+  that use custom formatters that handle errors.
 
 === Action API internal changes in 1.32 ===
 * Added 'ApiParseMakeOutputPage' hook.

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index c0c4895..f324eff 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -1554,6 +1554,11 @@ class ApiMain extends ApiBase {
         */
        protected function executeAction() {
                $params = $this->setupExecuteAction();
+
+               // Check asserts early so e.g. errors in parsing a module's parameters due to being
+               // logged out don't override the client's intended "am I logged in?" check.
+               $this->checkAsserts( $params );
+
                $module = $this->setupModule();
                $this->mModule = $module;
 
@@ -1575,8 +1580,6 @@ class ApiMain extends ApiBase {
                        $this->setupExternalResponse( $module, $params );
                }
 
-               $this->checkAsserts( $params );
-
                // Execute
                $module->execute();
                Hooks::run( 'APIAfterExecute', [ &$module ] );

diff --git a/tests/phpunit/includes/api/ApiMainTest.php b/tests/phpunit/includes/api/ApiMainTest.php
index 584c60c..f06d97e 100644 (file)
--- a/tests/phpunit/includes/api/ApiMainTest.php
+++ b/tests/phpunit/includes/api/ApiMainTest.php
@@ -434,6 +434,35 @@ class ApiMainTest extends ApiTestCase {
                }
        }
 
+       /**
+        * Test that 'assert' is processed before module errors
+        */
+       public function testAssertBeforeModule() {
+               // Sanity check that the query without assert throws too-many-titles
+               try {
+                       $this->doApiRequest( [
+                               'action' => 'query',
+                               'titles' => implode( '|', range( 1, ApiBase::LIMIT_SML1 + 1 ) ),
+                       ], null, null, new User );
+                       $this->fail( 'Expected exception not thrown' );
+               } catch ( ApiUsageException $e ) {
+                       $this->assertTrue( self::apiExceptionHasCode( $e, 'too-many-titles' ), 'sanity check' );
+               }
+
+               // Now test that the assert happens first
+               try {
+                       $this->doApiRequest( [
+                               'action' => 'query',
+                               'titles' => implode( '|', range( 1, ApiBase::LIMIT_SML1 + 1 ) ),
+                               'assert' => 'user',
+                       ], null, null, new User );
+                       $this->fail( 'Expected exception not thrown' );
+               } catch ( ApiUsageException $e ) {
+                       $this->assertTrue( self::apiExceptionHasCode( $e, 'assertuserfailed' ),
+                               "Error '{$e->getMessage()}' matched expected 'assertuserfailed'" );
+               }
+       }
+
        /**
         * Test if all classes in the main module manager exists
         */