r70135 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70134‎ | r70135 | r70136 >
Date:13:04, 29 July 2010
Author:btongminh
Status:ok (Comments)
Tags:
Comment:
Uploading to a protected title will allow the user to choose a new name instead of showing an error page
* Made validateNameAndOverwrite protected and moved it to validateName since overwriting is now checked in verifyPermissions()
* Fixed mime verification in case getTitle was not yet called
* Checking for overwrites no longer uses $wgUser

Other changes:
* convertVerifyErrorToStatus now works
* Allow passing the session key to stashSession in UploadFromStash as well
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadBase.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromStash.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadFromStash.php
——@@ -65,7 +65,7 @@
6666 /**
6767 * There is no need to stash the image twice
6868 */
69 - public function stashSession() {
 69+ public function stashSession( $key = null ) {
7070 if ( !empty( $this->mSessionKey ) )
7171 return $this->mSessionKey;
7272 return parent::stashSession();
Index: trunk/phase3/includes/upload/UploadBase.php
——@@ -25,7 +25,7 @@
2626 const EMPTY_FILE = 3;
2727 const MIN_LENGTH_PARTNAME = 4;
2828 const ILLEGAL_FILENAME = 5;
29 - const OVERWRITE_EXISTING_FILE = 7;
 29+ const OVERWRITE_EXISTING_FILE = 7; # Not used anymore; handled by verifyPermissions()
3030 const FILETYPE_MISSING = 8;
3131 const FILETYPE_BADTYPE = 9;
3232 const VERIFICATION_ERROR = 10;
——@@ -223,7 +223,7 @@
224224 * Verify whether the upload is sane.
225225 * @return mixed self::OK or else an array with error information
226226 */
227 - public function verifyUpload( ) {
 227+ public function verifyUpload() {
228228 /**
229229 * If there was no filename or a zero size given, give up quick.
230230 */
——@@ -258,7 +258,7 @@
259259 /**
260260 * Make sure this file can be created
261261 */
262 - $result = $this->validateNameAndOverwrite();
 262+ $result = $this->validateName();
263263 if( $result !== true ) {
264264 return $result;
265265 }
——@@ -279,7 +279,7 @@
280280 * @return mixed true if valid, otherwise and array with 'status'
281281 * and other keys
282282 **/
283 - public function validateNameAndOverwrite() {
 283+ protected function validateName() {
284284 $nt = $this->getTitle();
285285 if( is_null( $nt ) ) {
286286 $result = array( 'status' => $this->mTitleError );
——@@ -293,16 +293,6 @@
294294 }
295295 $this->mDestName = $this->getLocalFile()->getName();
296296
297 - /**
298 - * In some cases we may forbid overwriting of existing files.
299 - */
300 - $overwrite = $this->checkOverwrite();
301 - if( $overwrite !== true ) {
302 - return array(
303 - 'status' => self::OVERWRITE_EXISTING_FILE,
304 - 'overwrite' => $overwrite
305 - );
306 - }
307297 return true;
308298 }
309299
——@@ -347,6 +337,10 @@
348338 * @return mixed true of the file is verified, array otherwise.
349339 */
350340 protected function verifyFile() {
 341+ # get the title, even though we are doing nothing with it, because
 342+ # we need to populate mFinalExtension
 343+ $nt = $this->getTitle();
 344+
351345 $this->mFileProps = File::getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
352346 $this->checkMacBinary();
353347
——@@ -382,7 +376,11 @@
383377 }
384378
385379 /**
386 - * Check whether the user can edit, upload and create the image.
 380+ * Check whether the user can edit, upload and create the image. This
 381+ * checks only against the current title; if it returns errors, it may
 382+ * very well be that another title will not give errors. Therefore
 383+ * isAllowed() should be called as well for generic is-user-blocked or
 384+ * can-user-upload checking.
387385 *
388386 * @param $user the User object to verify the permissions against
389387 * @return mixed An array as returned by getUserPermissionsErrors or true
——@@ -409,6 +407,12 @@
410408 $permErrors = array_merge( $permErrors, wfArrayDiff2( $permErrorsCreate, $permErrors ) );
411409 return $permErrors;
412410 }
 411+
 412+ $overwriteError = $this->checkOverwrite( $user );
 413+ if ( $overwriteError !== true ) {
 414+ return array( array( $overwriteError ) );
 415+ }
 416+
413417 return true;
414418 }
415419
——@@ -1007,12 +1011,11 @@
10081012 *
10091013 * @return mixed true on success, error string on failure
10101014 */
1011 - private function checkOverwrite() {
1012 - global $wgUser;
 1015+ private function checkOverwrite( $user ) {
10131016 // First check whether the local file can be overwritten
10141017 $file = $this->getLocalFile();
10151018 if( $file->exists() ) {
1016 - if( !self::userCanReUpload( $wgUser, $file ) ) {
 1019+ if( !self::userCanReUpload( $user, $file ) ) {
10171020 return 'fileexists-forbidden';
10181021 } else {
10191022 return true;
——@@ -1023,7 +1026,7 @@
10241027 * wfFindFile finds a file, it exists in a shared repository.
10251028 */
10261029 $file = wfFindFile( $this->getTitle() );
1027 - if ( $file && !$wgUser->isAllowed( 'reupload-shared' ) ) {
 1030+ if ( $file && !$user->isAllowed( 'reupload-shared' ) ) {
10281031 return 'fileexists-shared-forbidden';
10291032 }
10301033
——@@ -1187,8 +1190,8 @@
11881191 }
11891192
11901193 public function convertVerifyErrorToStatus( $error ) {
1191 - $args = func_get_args();
1192 - array_shift($args);
1193 - return Status::newFatal( $this->getVerificationErrorCode( $error ), $args );
 1194+ $code = $error['status'];
 1195+ unset( $code['status'] );
 1196+ return Status::newFatal( $this->getVerificationErrorCode( $code ), $error );
11941197 }
11951198 }
Index: trunk/phase3/includes/specials/SpecialUpload.php
——@@ -193,6 +193,7 @@
194194 wfDebug( "Hook 'UploadForm:initial' broke output of the upload form" );
195195 return;
196196 }
 197+
197198
198199 $this->showUploadForm( $this->getUploadForm() );
199200 }
——@@ -415,13 +416,6 @@
416417 protected function processUpload() {
417418 global $wgUser, $wgOut;
418419
419 - // Verify permissions
420 - $permErrors = $this->mUpload->verifyPermissions( $wgUser );
421 - if( $permErrors !== true ) {
422 - $wgOut->showPermissionsErrorPage( $permErrors );
423 - return;
424 - }
425 -
426420 // Fetch the file if required
427421 $status = $this->mUpload->fetchFile();
428422 if( !$status->isOK() ) {
——@@ -445,6 +439,15 @@
446440 $this->processVerificationError( $details );
447441 return;
448442 }
 443+
 444+ // Verify permissions for this title
 445+ $permErrors = $this->mUpload->verifyPermissions( $wgUser );
 446+ if( $permErrors !== true ) {
 447+ $code = array_shift( $permErrors[0] );
 448+ $this->showRecoverableUploadError( wfMsgExt( $code,
 449+ 'parseinline', $permErrors[0] ) );
 450+ return;
 451+ }
449452
450453 $this->mLocalFile = $this->mUpload->getLocalFile();
451454
——@@ -549,10 +552,6 @@
550553 $this->showRecoverableUploadError( wfMsgExt( 'illegalfilename',
551554 'parseinline', $details['filtered'] ) );
552555 break;
553 - case UploadBase::OVERWRITE_EXISTING_FILE:
554 - $this->showRecoverableUploadError( wfMsgExt( $details['overwrite'],
555 - 'parseinline' ) );
556 - break;
557556 case UploadBase::FILETYPE_MISSING:
558557 $this->showRecoverableUploadError( wfMsgExt( 'filetype-missing',
559558 'parseinline' ) );
Index: trunk/phase3/RELEASE-NOTES
——@@ -255,6 +255,8 @@
256256 throw fatal errors
257257 * (bug 23380) Uploaded files that are larger than allowed by PHP now show a
258258 useful error message.
 259+* Uploading to a protected title will allow the user to choose a new name
 260+ instead of showing an error page
259261
260262 === API changes in 1.17 ===
261263 * (bug 22738) Allow filtering by action type on query=logevent.

Follow-up revisions

RevisionCommit summaryAuthorDate
r70137Made asynchronous upload by URL working, partly. Hid it behind $wgAllowAsyncC...btongminh13:53, 29 July 2010
r76103Add MIME type validation fix from r70135. Not merging r70135 itself, has a lo...catrope15:08, 5 November 2010

Comments

#Comment by Reedy (talk | contribs)   20:53, 14 March 2011

bug 28034 seems to have maybe come from here...

Status & tagging log