r62557 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62556‎ | r62557 | r62558 >
Date:23:53, 15 February 2010
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Refactor requiresToken to getTokenSalt - Returns salt if exists, null if no salt, else false if no token required

Move sessionfailure (token validation checking) up a couple of levels

Part of bug 21991

Followup to r62482 and r62504
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/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 ( !$wgUser->matchEditToken( $params['token'] ) )
51 - $this->dieUsageMsg( array( 'sessionfailure' ) );
5250
5351 if ( isset( $params['from'] ) )
5452 {
——@@ -213,7 +211,6 @@
214212 public function getPossibleErrors() {
215213 return array_merge( parent::getPossibleErrors(), array(
216214 array( 'missingparam', 'to' ),
217 - array( 'sessionfailure' ),
218215 array( 'invalidtitle', 'from' ),
219216 array( 'nosuchpageid', 'fromid' ),
220217 array( 'notanarticle' ),
——@@ -222,8 +219,8 @@
223220 ) );
224221 }
225222
226 - public function requiresToken() {
227 - return true;
 223+ public function getTokenSalt() {
 224+ return null;
228225 }
229226
230227 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiProtect.php
——@@ -46,9 +46,6 @@
4747 if ( empty( $params['protections'] ) )
4848 $this->dieUsageMsg( array( 'missingparam', 'protections' ) );
4949
50 - if ( !$wgUser->matchEditToken( $params['token'] ) )
51 - $this->dieUsageMsg( array( 'sessionfailure' ) );
52 -
5350 $titleObj = Title::newFromText( $params['title'] );
5451 if ( !$titleObj )
5552 $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) );
——@@ -176,7 +173,6 @@
177174 return array_merge( parent::getPossibleErrors(), array(
178175 array( 'missingparam', 'title' ),
179176 array( 'missingparam', 'protections' ),
180 - array( 'sessionfailure' ),
181177 array( 'invalidtitle', 'title' ),
182178 array( 'toofewexpiries', 'noofexpiries', 'noofprotections' ),
183179 array( 'create-titleexists' ),
——@@ -188,8 +184,8 @@
189185 ) );
190186 }
191187
192 - public function requiresToken() {
193 - return true;
 188+ public function getTokenSalt() {
 189+ return null;
194190 }
195191
196192 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiRollback.php
——@@ -122,8 +122,8 @@
123123 ) );
124124 }
125125
126 - public function requiresToken() {
127 - return true;
 126+ public function getTokenSalt() {
 127+ return null;
128128 }
129129
130130 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiUserrights.php
——@@ -37,19 +37,11 @@
3838 }
3939
4040 public function execute() {
41 - global $wgUser;
4241 $params = $this->extractRequestParams();
43 - if ( is_null( $params['user'] ) )
44 - $this->dieUsageMsg( array( 'missingparam', 'user' ) );
45 -
 42+
 43+ //User already validated in call to getTokenSalt from Main
4644 $form = new UserrightsPage;
4745 $user = $form->fetchUser( $params['user'] );
48 - if ( $user instanceof WikiErrorMsg )
49 - $this->dieUsageMsg( array_merge(
50 - (array)$user->getMessageKey(), $user->getMessageArgs() ) );
51 -
52 - if ( !$wgUser->matchEditToken( $params['token'], $user->getName() ) )
53 - $this->dieUsageMsg( array( 'sessionfailure' ) );
5446
5547 $r['user'] = $user->getName();
5648 list( $r['added'], $r['removed'] ) =
——@@ -107,12 +99,21 @@
108100 public function getPossibleErrors() {
109101 return array_merge( parent::getPossibleErrors(), array(
110102 array( 'missingparam', 'user' ),
111 - array( 'sessionfailure' ),
112103 ) );
113104 }
114105
115 - public function requiresToken() {
116 - return true;
 106+ public function getTokenSalt() {
 107+ $params = $this->extractRequestParams();
 108+ if ( is_null( $params['user'] ) )
 109+ $this->dieUsageMsg( array( 'missingparam', 'user' ) );
 110+
 111+ $form = new UserrightsPage;
 112+ $user = $form->fetchUser( $params['user'] );
 113+ if ( $user instanceof WikiErrorMsg )
 114+ $this->dieUsageMsg( array_merge(
 115+ (array)$user->getMessageKey(), $user->getMessageArgs() ) );
 116+
 117+ return $user->getName();
117118 }
118119
119120 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiDelete.php
——@@ -47,7 +47,8 @@
4848 * result object.
4949 */
5050 public function execute() {
51 - global $wgUser;
 51+ global $wgUser;
 52+
5253 $params = $this->extractRequestParams();
5354
5455 $this->requireOnlyOneParameter( $params, 'title', 'pageid' );
——@@ -78,7 +79,7 @@
7980
8081 if ( count( $retval ) )
8182 $this->dieUsageMsg( reset( $retval ) ); // We don't care about multiple errors, just report one of them
82 -
 83+
8384 if ( $params['watch'] || $wgUser->getOption( 'watchdeletion' ) )
8485 $articleObj->doWatch();
8586 else if ( $params['unwatch'] )
——@@ -95,10 +96,7 @@
9697 // Check permissions
9798 $errors = $title->getUserPermissionsErrors( 'delete', $wgUser );
9899 if ( count( $errors ) > 0 ) return $errors;
99 -
100 - // Check token
101 - if ( !$wgUser->matchEditToken( $token ) )
102 - return array( array( 'sessionfailure' ) );
 100+
103101 return array();
104102 }
105103
——@@ -219,8 +217,8 @@
220218 ) );
221219 }
222220
223 - public function requiresToken() {
224 - return true;
 221+ public function getTokenSalt() {
 222+ return null;
225223 }
226224
227225 protected function getExamples() {
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 ( !$wgUser->matchEditToken( $params['token'] ) )
49 - $this->dieUsageMsg( array( 'sessionfailure' ) );
5048
5149 $source = null;
5250 $isUpload = false;
——@@ -144,7 +142,6 @@
145143 public function getPossibleErrors() {
146144 return array_merge( parent::getPossibleErrors(), array(
147145 array( 'cantimport' ),
148 - array( 'sessionfailure' ),
149146 array( 'missingparam', 'interwikipage' ),
150147 array( 'cantimport-upload' ),
151148 array( 'import-unknownerror', 'source' ),
——@@ -152,8 +149,8 @@
153150 ) );
154151 }
155152
156 - public function requiresToken() {
157 - return true;
 153+ public function getTokenSalt() {
 154+ return null;
158155 }
159156
160157 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -53,9 +53,6 @@
5454 $params['undo'] == 0 )
5555 $this->dieUsageMsg( array( 'missingtext' ) );
5656
57 - if ( !$wgUser->matchEditToken( $params['token'] ) )
58 - $this->dieUsageMsg( array( 'sessionfailure' ) );
59 -
6057 $titleObj = Title::newFromText( $params['title'] );
6158 if ( !$titleObj || $titleObj->isExternal() )
6259 $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) );
——@@ -347,7 +344,6 @@
348345 return array_merge( parent::getPossibleErrors(), array(
349346 array( 'missingparam', 'title' ),
350347 array( 'missingtext' ),
351 - array( 'sessionfailure' ),
352348 array( 'invalidtitle', 'title' ),
353349 array( 'createonly-exists' ),
354350 array( 'nocreate-missing' ),
——@@ -463,8 +459,8 @@
464460 );
465461 }
466462
467 - public function requiresToken() {
468 - return true;
 463+ public function getTokenSalt() {
 464+ return null;
469465 }
470466
471467 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiUnblock.php
——@@ -57,8 +57,7 @@
5858 $this->dieUsageMsg( array( 'unblock-notarget' ) );
5959 if ( !is_null( $params['id'] ) && !is_null( $params['user'] ) )
6060 $this->dieUsageMsg( array( 'unblock-idanduser' ) );
61 - if ( !$wgUser->matchEditToken( $params['token'] ) )
62 - $this->dieUsageMsg( array( 'sessionfailure' ) );
 61+
6362 if ( !$wgUser->isAllowed( 'block' ) )
6463 $this->dieUsageMsg( array( 'cantunblock' ) );
6564
——@@ -113,13 +112,12 @@
114113 return array_merge( parent::getPossibleErrors(), array(
115114 array( 'unblock-notarget' ),
116115 array( 'unblock-idanduser' ),
117 - array( 'sessionfailure' ),
118116 array( 'cantunblock' ),
119117 ) );
120118 }
121119
122 - public function requiresToken() {
123 - return true;
 120+ public function getTokenSalt() {
 121+ return null;
124122 }
125123
126124 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiMain.php
——@@ -400,7 +400,7 @@
401401 $this->getResult()->addValue( null, 'requestid', $requestid );
402402
403403 $params = $this->extractRequestParams();
404 -
 404+
405405 $this->mShowVersions = $params['version'];
406406 $this->mAction = $params['action'];
407407
——@@ -412,9 +412,22 @@
413413 $module = new $this->mModules[$this->mAction] ( $this, $this->mAction );
414414 $this->mModule = $module;
415415
 416+ $moduleParams = $module->extractRequestParams();
 417+
416418 //Die if token required, but not provided (unless there is a gettoken parameter)
417 - if ( $module->requiresToken() && !isset( $params['token'] ) && isset( $params['gettoken'] ) )
418 - $this->dieUsageMsg( array( 'missingparam', 'token' ) );
 419+ $salt = $module->getTokenSalt();
 420+ if ( $salt != false )
 421+ {
 422+ if ( !isset( $moduleParams['token'] ) && !isset( $moduleParams['gettoken'] ) ) {
 423+ $this->dieUsageMsg( array( 'missingparam', 'token' ) );
 424+ } else {
 425+ global $wgUser;
 426+ if ( ( $salt != null /*&& !$wgUser->matchEditToken( $moduleParams['token'], $salt )*/ )
 427+ /*|| !$wgUser->matchEditToken( $moduleParams['token'] )*/ ) {
 428+ $this->dieUsageMsg( array( 'sessionfailure' ) );
 429+ }
 430+ }
 431+ }
419432
420433 if ( $module->shouldCheckMaxlag() && isset( $params['maxlag'] ) ) {
421434 // Check for maxlag
Index: trunk/phase3/includes/api/ApiEmailUser.php
——@@ -112,8 +112,8 @@
113113 ) );
114114 }
115115
116 - public function requiresToken() {
117 - return true;
 116+ public function getTokenSalt() {
 117+ return null;
118118 }
119119
120120 protected function getExamples() {
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 ( !$wgUser->matchEditToken( $params['token'] ) )
66 - $this->dieUsageMsg( array( 'sessionfailure' ) );
6765 if ( !$wgUser->isAllowed( 'block' ) )
6866 $this->dieUsageMsg( array( 'cantblock' ) );
6967 if ( $params['hidename'] && !$wgUser->isAllowed( 'hideuser' ) )
——@@ -161,15 +159,14 @@
162160 public function getPossibleErrors() {
163161 return array_merge( parent::getPossibleErrors(), array(
164162 array( 'missingparam', 'user' ),
165 - array( 'sessionfailure' ),
166163 array( 'cantblock' ),
167164 array( 'canthide' ),
168165 array( 'cantblock-email' ),
169166 ) );
170167 }
171168
172 - public function requiresToken() {
173 - return true;
 169+ public function getTokenSalt() {
 170+ return null;
174171 }
175172
176173 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiPatrol.php
——@@ -41,13 +41,10 @@
4242 * Patrols the article or provides the reason the patrol failed.
4343 */
4444 public function execute() {
45 - global $wgUser;
4645 $params = $this->extractRequestParams();
4746
4847 if ( !isset( $params['rcid'] ) )
4948 $this->dieUsageMsg( array( 'missingparam', 'rcid' ) );
50 - if ( !$wgUser->matchEditToken( $params['token'] ) )
51 - $this->dieUsageMsg( array( 'sessionfailure' ) );
5249
5350 $rc = RecentChange::newFromID( $params['rcid'] );
5451 if ( !$rc instanceof RecentChange )
——@@ -91,13 +88,12 @@
9289 public function getPossibleErrors() {
9390 return array_merge( parent::getPossibleErrors(), array(
9491 array( 'missingparam', 'rcid' ),
95 - array( 'sessionfailure' ),
9692 array( 'nosuchrcid', 'rcid' ),
9793 ) );
9894 }
9995
100 - public function requiresToken() {
101 - return true;
 96+ public function getTokenSalt() {
 97+ return null;
10298 }
10399
104100 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiUndelete.php
——@@ -50,9 +50,6 @@
5151 if ( $wgUser->isBlocked() )
5252 $this->dieUsageMsg( array( 'blockedtext' ) );
5353
54 - if ( !$wgUser->matchEditToken( $params['token'] ) )
55 - $this->dieUsageMsg( array( 'sessionfailure' ) );
56 -
5754 $titleObj = Title::newFromText( $params['title'] );
5855 if ( !$titleObj )
5956 $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) );
——@@ -123,14 +120,13 @@
124121 array( 'missingparam', 'title' ),
125122 array( 'permdenied-undelete' ),
126123 array( 'blockedtext' ),
127 - array( 'sessionfailure' ),
128124 array( 'invalidtitle', 'title' ),
129125 array( 'cannotundelete' ),
130126 ) );
131127 }
132128
133 - public function requiresToken() {
134 - return true;
 129+ public function getTokenSalt() {
 130+ return null;
135131 }
136132
137133 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -970,10 +970,10 @@
971971 }
972972
973973 /**
974 - * Indicates whether this module needs a token to preform the request
 974+ * Returns the token salt if there is one, null if the module doesn't require a salt, else false if the module doesn't need a token
975975 * @returns bool
976976 */
977 - public function requiresToken() {
 977+ public function getTokenSalt() {
978978 return false;
979979 }
980980
——@@ -997,7 +997,7 @@
998998 $ret[] = array ( 'writedisabled' );
999999 }
10001000
1001 - if ( $this->requiresToken() ) {
 1001+ if ( $this->getTokenSalt() != false ) {
10021002 $ret[] = array( 'missingparam', 'token' );
10031003 }
10041004
Index: trunk/phase3/includes/api/ApiUpload.php
——@@ -47,10 +47,6 @@
4848 $this->mParams = $this->extractRequestParams();
4949 $request = $this->getMain()->getRequest();
5050
51 - // Do token checks:
52 - if ( !$wgUser->matchEditToken( $this->mParams['token'] ) )
53 - $this->dieUsageMsg( array( 'sessionfailure' ) );
54 -
5551 // Add the uploaded file to the params array
5652 $this->mParams['file'] = $request->getFileName( 'file' );
5753
——@@ -328,7 +324,6 @@
329325 public function getPossibleErrors() {
330326 return array_merge( parent::getPossibleErrors(), array(
331327 array( 'uploaddisabled' ),
332 - array( 'sessionfailure' ),
333328 array( 'invalid-session-key' ),
334329 array( 'uploaddisabled' ),
335330 array( 'badaccess-groups' ),
——@@ -347,8 +342,8 @@
348343 ) );
349344 }
350345
351 - public function requiresToken() {
352 - return true;
 346+ public function getTokenSalt() {
 347+ return null;
353348 }
354349
355350 protected function getExamples() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r62565Followup to r62557...reedy01:27, 16 February 2010
r62566Decomment parts accidentally commited commented in r62557reedy01:29, 16 February 2010
r62599Followup to r62557 as per Roans commentreedy21:59, 16 February 2010
r64826Refactor code in ApiRollback to fix bug 23117...reedy20:05, 9 April 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r62482Start of "Bug 21991 - Move common query parameter (uc, rc) validation, token...reedy22:20, 14 February 2010
r62504Fix fir r62482: PHP Notice: Undefined index: token in /www/w/includes/api/Api...raymond09:22, 15 February 2010

Comments

#Comment by Reedy (talk | contribs)   01:25, 16 February 2010

+ if ( ( $salt != null /*&& !$wgUser->matchEditToken( $moduleParams['token'], $salt )*/ ) + /*|| !$wgUser->matchEditToken( $moduleParams['token'] )*/ ) {


That isn't right.. Heh

#Comment by Reedy (talk | contribs)   02:06, 16 February 2010

Fixed in r62566

#Comment by Catrope (talk | contribs)   15:36, 16 February 2010

Please use the empty string for the return value of getTokenSalt() rather than null, because that's what you'll actually be using. Also use strict comparisons when dealing with false, null and the empty string.

#Comment by Reedy (talk | contribs)   15:46, 16 February 2010

You mean something more like?

+ if ( $salt !== false ) + { + if ( !isset( $moduleParams['token'] ) && !isset( $moduleParams['gettoken'] ) ) { + $this->dieUsageMsg( array( 'missingparam', 'token' ) ); + } else { + global $wgUser; + if ( !$wgUser->matchEditToken( $moduleParams['token'], $salt ) ) { + $this->dieUsageMsg( array( 'sessionfailure' ) ); + } + } + }

#Comment by Catrope (talk | contribs)   16:02, 16 February 2010

Exactly. Also use strict comparisons for other calls to getTokenSalt(), and return ''; from the overridden getTokenSalt() functions rather than null.

#Comment by Reedy (talk | contribs)   16:09, 16 February 2010

Alright, will do that when i'm back home later :)

#Comment by Reedy (talk | contribs)   21:59, 16 February 2010

r62599 does that!

Status & tagging log