r62482 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62481‎ | r62482 | r62483 >
Date:22:20, 14 February 2010
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Start of "Bug 21991 - Move common query parameter (uc, rc) validation, token requiringness/checking to ApiBase/Similar"

Move token requringness check to the ApiMain

Adding an exception if we're using "gettoken" (block/unblock)

Remove array( 'missingparam', 'token' ), from the getPossibleErrors of modules that set requireToken method to true
Modified paths:
  • /trunk/phase3/includes/api/ApiBase.php (modified) (history)
  • /trunk/phase3/includes/api/ApiBlock.php (modified) (history)
  • /trunk/phase3/includes/api/ApiDelete.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEditPage.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEmailUser.php (modified) (history)
  • /trunk/phase3/includes/api/ApiImport.php (modified) (history)
  • /trunk/phase3/includes/api/ApiMain.php (modified) (history)
  • /trunk/phase3/includes/api/ApiMove.php (modified) (history)
  • /trunk/phase3/includes/api/ApiParse.php (modified) (history)
  • /trunk/phase3/includes/api/ApiPatrol.php (modified) (history)
  • /trunk/phase3/includes/api/ApiProtect.php (modified) (history)
  • /trunk/phase3/includes/api/ApiRollback.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUnblock.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUndelete.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUpload.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUserrights.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiMove.php
——@@ -46,8 +46,6 @@
4747 $this->requireOnlyOneParameter( $params, 'from', 'fromid' );
4848 if ( !isset( $params['to'] ) )
4949 $this->dieUsageMsg( array( 'missingparam', 'to' ) );
50 - if ( !isset( $params['token'] ) )
51 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5250 if ( !$wgUser->matchEditToken( $params['token'] ) )
5351 $this->dieUsageMsg( array( 'sessionfailure' ) );
5452
——@@ -215,7 +213,6 @@
216214 public function getPossibleErrors() {
217215 return array_merge( parent::getPossibleErrors(), array(
218216 array( 'missingparam', 'to' ),
219 - array( 'missingparam', 'token' ),
220217 array( 'sessionfailure' ),
221218 array( 'invalidtitle', 'from' ),
222219 array( 'nosuchpageid', 'fromid' ),
——@@ -224,6 +221,10 @@
225222 array( 'sharedfile-exists' ),
226223 ) );
227224 }
 225+
 226+ public function requiresToken() {
 227+ return true;
 228+ }
228229
229230 protected function getExamples() {
230231 return array (
Index: trunk/phase3/includes/api/ApiParse.php
——@@ -325,4 +325,4 @@
326326 public function getVersion() {
327327 return __CLASS__ . ': $Id$';
328328 }
329 -}
 329+}
\ No newline at end of file
Index: trunk/phase3/includes/api/ApiProtect.php
——@@ -43,8 +43,6 @@
4444 $titleObj = null;
4545 if ( !isset( $params['title'] ) )
4646 $this->dieUsageMsg( array( 'missingparam', 'title' ) );
47 - if ( !isset( $params['token'] ) )
48 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
4947 if ( empty( $params['protections'] ) )
5048 $this->dieUsageMsg( array( 'missingparam', 'protections' ) );
5149
——@@ -177,7 +175,6 @@
178176 public function getPossibleErrors() {
179177 return array_merge( parent::getPossibleErrors(), array(
180178 array( 'missingparam', 'title' ),
181 - array( 'missingparam', 'token' ),
182179 array( 'missingparam', 'protections' ),
183180 array( 'sessionfailure' ),
184181 array( 'invalidtitle', 'title' ),
——@@ -190,6 +187,10 @@
191188 array( 'pastexpiry', 'expiry' ),
192189 ) );
193190 }
 191+
 192+ public function requiresToken() {
 193+ return true;
 194+ }
194195
195196 protected function getExamples() {
196197 return array (
Index: trunk/phase3/includes/api/ApiRollback.php
——@@ -44,8 +44,6 @@
4545 $this->dieUsageMsg( array( 'missingparam', 'title' ) );
4646 if ( !isset( $params['user'] ) )
4747 $this->dieUsageMsg( array( 'missingparam', 'user' ) );
48 - if ( !isset( $params['token'] ) )
49 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5048
5149 $titleObj = Title::newFromText( $params['title'] );
5250 if ( !$titleObj )
——@@ -118,12 +116,15 @@
119117 return array_merge( parent::getPossibleErrors(), array(
120118 array( 'missingparam', 'title' ),
121119 array( 'missingparam', 'user' ),
122 - array( 'missingparam', 'token' ),
123120 array( 'invalidtitle', 'title' ),
124121 array( 'notanarticle' ),
125122 array( 'invaliduser', 'user' ),
126123 ) );
127124 }
 125+
 126+ public function requiresToken() {
 127+ return true;
 128+ }
128129
129130 protected function getExamples() {
130131 return array (
Index: trunk/phase3/includes/api/ApiUserrights.php
——@@ -41,8 +41,6 @@
4242 $params = $this->extractRequestParams();
4343 if ( is_null( $params['user'] ) )
4444 $this->dieUsageMsg( array( 'missingparam', 'user' ) );
45 - if ( is_null( $params['token'] ) )
46 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
4745
4846 $form = new UserrightsPage;
4947 $user = $form->fetchUser( $params['user'] );
——@@ -109,10 +107,13 @@
110108 public function getPossibleErrors() {
111109 return array_merge( parent::getPossibleErrors(), array(
112110 array( 'missingparam', 'user' ),
113 - array( 'missingparam', 'token' ),
114111 array( 'sessionfailure' ),
115112 ) );
116113 }
 114+
 115+ public function requiresToken() {
 116+ return true;
 117+ }
117118
118119 protected function getExamples() {
119120 return array (
Index: trunk/phase3/includes/api/ApiDelete.php
——@@ -51,8 +51,6 @@
5252 $params = $this->extractRequestParams();
5353
5454 $this->requireOnlyOneParameter( $params, 'title', 'pageid' );
55 - if ( !isset( $params['token'] ) )
56 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5755
5856 if ( isset( $params['title'] ) )
5957 {
——@@ -214,13 +212,16 @@
215213
216214 public function getPossibleErrors() {
217215 return array_merge( parent::getPossibleErrors(), array(
218 - array( 'missingparam', 'token' ),
219216 array( 'invalidtitle', 'title' ),
220217 array( 'nosuchpageid', 'pageid' ),
221218 array( 'notanarticle' ),
222219 array( 'hookaborted', 'error' ),
223220 ) );
224221 }
 222+
 223+ public function requiresToken() {
 224+ return true;
 225+ }
225226
226227 protected function getExamples() {
227228 return array (
Index: trunk/phase3/includes/api/ApiImport.php
——@@ -44,8 +44,6 @@
4545 if ( !$wgUser->isAllowed( 'import' ) )
4646 $this->dieUsageMsg( array( 'cantimport' ) );
4747 $params = $this->extractRequestParams();
48 - if ( !isset( $params['token'] ) )
49 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5048 if ( !$wgUser->matchEditToken( $params['token'] ) )
5149 $this->dieUsageMsg( array( 'sessionfailure' ) );
5250
——@@ -146,7 +144,6 @@
147145 public function getPossibleErrors() {
148146 return array_merge( parent::getPossibleErrors(), array(
149147 array( 'cantimport' ),
150 - array( 'missingparam', 'token' ),
151148 array( 'sessionfailure' ),
152149 array( 'missingparam', 'interwikipage' ),
153150 array( 'cantimport-upload' ),
——@@ -154,6 +151,10 @@
155152 array( 'import-unknownerror', 'result' ),
156153 ) );
157154 }
 155+
 156+ public function requiresToken() {
 157+ return true;
 158+ }
158159
159160 protected function getExamples() {
160161 return array(
Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -53,9 +53,6 @@
5454 $params['undo'] == 0 )
5555 $this->dieUsageMsg( array( 'missingtext' ) );
5656
57 - if ( is_null( $params['token'] ) )
58 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
59 -
6057 if ( !$wgUser->matchEditToken( $params['token'] ) )
6158 $this->dieUsageMsg( array( 'sessionfailure' ) );
6259
——@@ -350,7 +347,6 @@
351348 return array_merge( parent::getPossibleErrors(), array(
352349 array( 'missingparam', 'title' ),
353350 array( 'missingtext' ),
354 - array( 'missingparam', 'token' ),
355351 array( 'sessionfailure' ),
356352 array( 'invalidtitle', 'title' ),
357353 array( 'createonly-exists' ),
——@@ -466,6 +462,10 @@
467463 'undoafter' => 'Undo all revisions from undo to this one. If not set, just undo one revision',
468464 );
469465 }
 466+
 467+ public function requiresToken() {
 468+ return true;
 469+ }
470470
471471 protected function getExamples() {
472472 return array (
Index: trunk/phase3/includes/api/ApiUnblock.php
——@@ -57,8 +57,6 @@
5858 $this->dieUsageMsg( array( 'unblock-notarget' ) );
5959 if ( !is_null( $params['id'] ) && !is_null( $params['user'] ) )
6060 $this->dieUsageMsg( array( 'unblock-idanduser' ) );
61 - if ( is_null( $params['token'] ) )
62 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
6361 if ( !$wgUser->matchEditToken( $params['token'] ) )
6462 $this->dieUsageMsg( array( 'sessionfailure' ) );
6563 if ( !$wgUser->isAllowed( 'block' ) )
——@@ -115,11 +113,14 @@
116114 return array_merge( parent::getPossibleErrors(), array(
117115 array( 'unblock-notarget' ),
118116 array( 'unblock-idanduser' ),
119 - array( 'missingparam', 'token' ),
120117 array( 'sessionfailure' ),
121118 array( 'cantunblock' ),
122119 ) );
123120 }
 121+
 122+ public function requiresToken() {
 123+ return true;
 124+ }
124125
125126 protected function getExamples() {
126127 return array (
Index: trunk/phase3/includes/api/ApiMain.php
——@@ -378,11 +378,15 @@
379379 if ( !is_string( $this->mAction ) ) {
380380 $this->dieUsage( "The API requires a valid action parameter", 'unknown_action' );
381381 }
382 -
 382+
383383 // Instantiate the module requested by the user
384384 $module = new $this->mModules[$this->mAction] ( $this, $this->mAction );
385385 $this->mModule = $module;
386386
 387+ //Die if token required, but not provided (unless there is a gettoken parameter)
 388+ if ( $module->requiresToken() && is_null( $params['token'] ) && !is_null( $params['gettoken'] ) )
 389+ $this->dieUsageMsg( array( 'missingparam', 'token' ) );
 390+
387391 if ( $module->shouldCheckMaxlag() && isset( $params['maxlag'] ) ) {
388392 // Check for maxlag
389393 global $wgShowHostnames;
Index: trunk/phase3/includes/api/ApiEmailUser.php
——@@ -48,8 +48,6 @@
4949 $this->dieUsageMsg( array( 'missingparam', 'target' ) );
5050 if ( !isset( $params['text'] ) )
5151 $this->dieUsageMsg( array( 'missingparam', 'text' ) );
52 - if ( !isset( $params['token'] ) )
53 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5452
5553 // Validate target
5654 $targetUser = EmailUserForm::validateEmailTarget( $params['target'] );
——@@ -111,9 +109,12 @@
112110 array( 'usermaildisabled' ),
113111 array( 'missingparam', 'target' ),
114112 array( 'missingparam', 'text' ),
115 - array( 'missingparam', 'token' ),
116113 ) );
117114 }
 115+
 116+ public function requiresToken() {
 117+ return true;
 118+ }
118119
119120 protected function getExamples() {
120121 return array (
Index: trunk/phase3/includes/api/ApiBlock.php
——@@ -61,8 +61,6 @@
6262
6363 if ( is_null( $params['user'] ) )
6464 $this->dieUsageMsg( array( 'missingparam', 'user' ) );
65 - if ( is_null( $params['token'] ) )
66 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
6765 if ( !$wgUser->matchEditToken( $params['token'] ) )
6866 $this->dieUsageMsg( array( 'sessionfailure' ) );
6967 if ( !$wgUser->isAllowed( 'block' ) )
——@@ -163,13 +161,16 @@
164162 public function getPossibleErrors() {
165163 return array_merge( parent::getPossibleErrors(), array(
166164 array( 'missingparam', 'user' ),
167 - array( 'missingparam', 'token' ),
168165 array( 'sessionfailure' ),
169166 array( 'cantblock' ),
170167 array( 'canthide' ),
171168 array( 'cantblock-email' ),
172169 ) );
173170 }
 171+
 172+ public function requiresToken() {
 173+ return true;
 174+ }
174175
175176 protected function getExamples() {
176177 return array (
Index: trunk/phase3/includes/api/ApiPatrol.php
——@@ -44,8 +44,6 @@
4545 global $wgUser;
4646 $params = $this->extractRequestParams();
4747
48 - if ( !isset( $params['token'] ) )
49 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5048 if ( !isset( $params['rcid'] ) )
5149 $this->dieUsageMsg( array( 'missingparam', 'rcid' ) );
5250 if ( !$wgUser->matchEditToken( $params['token'] ) )
——@@ -92,12 +90,15 @@
9391
9492 public function getPossibleErrors() {
9593 return array_merge( parent::getPossibleErrors(), array(
96 - array( 'missingparam', 'token' ),
9794 array( 'missingparam', 'rcid' ),
9895 array( 'sessionfailure' ),
9996 array( 'nosuchrcid', 'rcid' ),
10097 ) );
10198 }
 99+
 100+ public function requiresToken() {
 101+ return true;
 102+ }
102103
103104 protected function getExamples() {
104105 return array(
Index: trunk/phase3/includes/api/ApiUndelete.php
——@@ -43,8 +43,6 @@
4444 $titleObj = null;
4545 if ( !isset( $params['title'] ) )
4646 $this->dieUsageMsg( array( 'missingparam', 'title' ) );
47 - if ( !isset( $params['token'] ) )
48 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
4947
5048 if ( !$wgUser->isAllowed( 'undelete' ) )
5149 $this->dieUsageMsg( array( 'permdenied-undelete' ) );
——@@ -123,7 +121,6 @@
124122 public function getPossibleErrors() {
125123 return array_merge( parent::getPossibleErrors(), array(
126124 array( 'missingparam', 'title' ),
127 - array( 'missingparam', 'token' ),
128125 array( 'permdenied-undelete' ),
129126 array( 'blockedtext' ),
130127 array( 'sessionfailure' ),
——@@ -131,6 +128,10 @@
132129 array( 'cannotundelete' ),
133130 ) );
134131 }
 132+
 133+ public function requiresToken() {
 134+ return true;
 135+ }
135136
136137 protected function getExamples() {
137138 return array (
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -965,6 +965,14 @@
966966 public function mustBePosted() {
967967 return false;
968968 }
 969+
 970+ /**
 971+ * Indicates whether this module needs a token to preform the request
 972+ * @returns bool
 973+ */
 974+ public function requiresToken() {
 975+ return false;
 976+ }
969977
970978 /**
971979 * Returns a list of all possible errors returned by the module
——@@ -985,6 +993,10 @@
986994 $ret[] = array ( 'writerequired' );
987995 $ret[] = array ( 'writedisabled' );
988996 }
 997+
 998+ if ( $this->requiresToken() ) {
 999+ $ret[] = array( 'missingparam', 'token' );
 1000+ }
9891001
9901002 return $ret;
9911003 }
Index: trunk/phase3/includes/api/ApiUpload.php
——@@ -48,8 +48,6 @@
4949 $request = $this->getMain()->getRequest();
5050
5151 // Do token checks:
52 - if ( is_null( $this->mParams['token'] ) )
53 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
5452 if ( !$wgUser->matchEditToken( $this->mParams['token'] ) )
5553 $this->dieUsageMsg( array( 'sessionfailure' ) );
5654
——@@ -330,7 +328,6 @@
331329 public function getPossibleErrors() {
332330 return array_merge( parent::getPossibleErrors(), array(
333331 array( 'uploaddisabled' ),
334 - array( 'missingparam', 'token' ),
335332 array( 'sessionfailure' ),
336333 array( 'invalid-session-key' ),
337334 array( 'uploaddisabled' ),
——@@ -349,6 +346,10 @@
350347 array( 'code' => 'internal-error', 'info' => 'An internal error occurred' ),
351348 ) );
352349 }
 350+
 351+ public function requiresToken() {
 352+ return true;
 353+ }
353354
354355 protected function getExamples() {
355356 return array(

Follow-up revisions

RevisionCommit summaryAuthorDate
r62504Fix fir r62482: PHP Notice: Undefined index: token in /www/w/includes/api/Api...raymond09:22, 15 February 2010
r62557Refactor requiresToken to getTokenSalt - Returns salt if exists, null if no s...reedy23:53, 15 February 2010
r64826Refactor code in ApiRollback to fix bug 23117...reedy20:05, 9 April 2010

Comments

#Comment by MZMcBride (talk | contribs)   22:50, 14 February 2010 
&& !is_null( $params['gettoken'] )

:D

#Comment by Reedy (talk | contribs)   22:54, 14 February 2010

Indeed!

#Comment by OverlordQ (talk | contribs)   23:48, 14 February 2010

Notice: Undefined index: token in [...] includes/api/ApiMain.php on line 387

#Comment by Raymond (talk | contribs)   07:34, 15 February 2010

Another notice, seen on translatewiki:

PHP Notice: Undefined index: token in /www/w/includes/api/ApiMain.php on line 415

#Comment by Raymond (talk | contribs)   09:24, 15 February 2010

Fixed in r62504. Hopefully correct.

#Comment by Catrope (talk | contribs)   12:09, 15 February 2010

With the presence check of the token broken out, it'd make a lot of sense to break out token validation as well: change requiresToken() to getTokenSalt() (return null if no token, or a salt (string, usually ) to check the token against) and break out the sessionfailure error as well.

#Comment by Reedy (talk | contribs)   13:03, 15 February 2010

https://bugzilla.wikimedia.org/show_bug.cgi?id=21991

I'd repurposed the bug for that reason (to cater for the 2 extra similar improvements)

Had commented about how to do this, but no reply as of yet ;).

I was just doing it one part at a time :)

#Comment by Catrope (talk | contribs)   14:56, 15 February 2010

Sure, take your time :) I read commit notifications before bug notifications this morning, so I missed that.

Status & tagging log