From 9e63cdf976309217eecab3513821b37ab96e4837 Mon Sep 17 00:00:00 2001 From: Bryan Tong Minh Date: Thu, 29 Jul 2010 13:04:34 +0000 Subject: [PATCH] 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 --- RELEASE-NOTES | 2 ++ includes/specials/SpecialUpload.php | 21 ++++++------- includes/upload/UploadBase.php | 47 +++++++++++++++-------------- includes/upload/UploadFromStash.php | 2 +- 4 files changed, 38 insertions(+), 34 deletions(-) diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 255afa5eff..460d370d1c 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -255,6 +255,8 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN throw fatal errors * (bug 23380) Uploaded files that are larger than allowed by PHP now show a useful error message. +* Uploading to a protected title will allow the user to choose a new name + instead of showing an error page === API changes in 1.17 === * (bug 22738) Allow filtering by action type on query=logevent. diff --git a/includes/specials/SpecialUpload.php b/includes/specials/SpecialUpload.php index df2585d712..d8ad6f2172 100644 --- a/includes/specials/SpecialUpload.php +++ b/includes/specials/SpecialUpload.php @@ -193,6 +193,7 @@ class SpecialUpload extends SpecialPage { wfDebug( "Hook 'UploadForm:initial' broke output of the upload form" ); return; } + $this->showUploadForm( $this->getUploadForm() ); } @@ -415,13 +416,6 @@ class SpecialUpload extends SpecialPage { protected function processUpload() { global $wgUser, $wgOut; - // Verify permissions - $permErrors = $this->mUpload->verifyPermissions( $wgUser ); - if( $permErrors !== true ) { - $wgOut->showPermissionsErrorPage( $permErrors ); - return; - } - // Fetch the file if required $status = $this->mUpload->fetchFile(); if( !$status->isOK() ) { @@ -445,6 +439,15 @@ class SpecialUpload extends SpecialPage { $this->processVerificationError( $details ); return; } + + // Verify permissions for this title + $permErrors = $this->mUpload->verifyPermissions( $wgUser ); + if( $permErrors !== true ) { + $code = array_shift( $permErrors[0] ); + $this->showRecoverableUploadError( wfMsgExt( $code, + 'parseinline', $permErrors[0] ) ); + return; + } $this->mLocalFile = $this->mUpload->getLocalFile(); @@ -549,10 +552,6 @@ class SpecialUpload extends SpecialPage { $this->showRecoverableUploadError( wfMsgExt( 'illegalfilename', 'parseinline', $details['filtered'] ) ); break; - case UploadBase::OVERWRITE_EXISTING_FILE: - $this->showRecoverableUploadError( wfMsgExt( $details['overwrite'], - 'parseinline' ) ); - break; case UploadBase::FILETYPE_MISSING: $this->showRecoverableUploadError( wfMsgExt( 'filetype-missing', 'parseinline' ) ); diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php index 63082a450c..1e23fbeb5b 100644 --- a/includes/upload/UploadBase.php +++ b/includes/upload/UploadBase.php @@ -25,7 +25,7 @@ abstract class UploadBase { const EMPTY_FILE = 3; const MIN_LENGTH_PARTNAME = 4; const ILLEGAL_FILENAME = 5; - const OVERWRITE_EXISTING_FILE = 7; + const OVERWRITE_EXISTING_FILE = 7; # Not used anymore; handled by verifyPermissions() const FILETYPE_MISSING = 8; const FILETYPE_BADTYPE = 9; const VERIFICATION_ERROR = 10; @@ -223,7 +223,7 @@ abstract class UploadBase { * Verify whether the upload is sane. * @return mixed self::OK or else an array with error information */ - public function verifyUpload( ) { + public function verifyUpload() { /** * If there was no filename or a zero size given, give up quick. */ @@ -258,7 +258,7 @@ abstract class UploadBase { /** * Make sure this file can be created */ - $result = $this->validateNameAndOverwrite(); + $result = $this->validateName(); if( $result !== true ) { return $result; } @@ -279,7 +279,7 @@ abstract class UploadBase { * @return mixed true if valid, otherwise and array with 'status' * and other keys **/ - public function validateNameAndOverwrite() { + protected function validateName() { $nt = $this->getTitle(); if( is_null( $nt ) ) { $result = array( 'status' => $this->mTitleError ); @@ -293,16 +293,6 @@ abstract class UploadBase { } $this->mDestName = $this->getLocalFile()->getName(); - /** - * In some cases we may forbid overwriting of existing files. - */ - $overwrite = $this->checkOverwrite(); - if( $overwrite !== true ) { - return array( - 'status' => self::OVERWRITE_EXISTING_FILE, - 'overwrite' => $overwrite - ); - } return true; } @@ -347,6 +337,10 @@ abstract class UploadBase { * @return mixed true of the file is verified, array otherwise. */ protected function verifyFile() { + # get the title, even though we are doing nothing with it, because + # we need to populate mFinalExtension + $nt = $this->getTitle(); + $this->mFileProps = File::getPropsFromPath( $this->mTempPath, $this->mFinalExtension ); $this->checkMacBinary(); @@ -382,7 +376,11 @@ abstract class UploadBase { } /** - * Check whether the user can edit, upload and create the image. + * Check whether the user can edit, upload and create the image. This + * checks only against the current title; if it returns errors, it may + * very well be that another title will not give errors. Therefore + * isAllowed() should be called as well for generic is-user-blocked or + * can-user-upload checking. * * @param $user the User object to verify the permissions against * @return mixed An array as returned by getUserPermissionsErrors or true @@ -409,6 +407,12 @@ abstract class UploadBase { $permErrors = array_merge( $permErrors, wfArrayDiff2( $permErrorsCreate, $permErrors ) ); return $permErrors; } + + $overwriteError = $this->checkOverwrite( $user ); + if ( $overwriteError !== true ) { + return array( array( $overwriteError ) ); + } + return true; } @@ -1007,12 +1011,11 @@ abstract class UploadBase { * * @return mixed true on success, error string on failure */ - private function checkOverwrite() { - global $wgUser; + private function checkOverwrite( $user ) { // First check whether the local file can be overwritten $file = $this->getLocalFile(); if( $file->exists() ) { - if( !self::userCanReUpload( $wgUser, $file ) ) { + if( !self::userCanReUpload( $user, $file ) ) { return 'fileexists-forbidden'; } else { return true; @@ -1023,7 +1026,7 @@ abstract class UploadBase { * wfFindFile finds a file, it exists in a shared repository. */ $file = wfFindFile( $this->getTitle() ); - if ( $file && !$wgUser->isAllowed( 'reupload-shared' ) ) { + if ( $file && !$user->isAllowed( 'reupload-shared' ) ) { return 'fileexists-shared-forbidden'; } @@ -1187,8 +1190,8 @@ abstract class UploadBase { } public function convertVerifyErrorToStatus( $error ) { - $args = func_get_args(); - array_shift($args); - return Status::newFatal( $this->getVerificationErrorCode( $error ), $args ); + $code = $error['status']; + unset( $code['status'] ); + return Status::newFatal( $this->getVerificationErrorCode( $code ), $error ); } } diff --git a/includes/upload/UploadFromStash.php b/includes/upload/UploadFromStash.php index 6347834303..d4ddf88f14 100644 --- a/includes/upload/UploadFromStash.php +++ b/includes/upload/UploadFromStash.php @@ -65,7 +65,7 @@ class UploadFromStash extends UploadBase { /** * There is no need to stash the image twice */ - public function stashSession() { + public function stashSession( $key = null ) { if ( !empty( $this->mSessionKey ) ) return $this->mSessionKey; return parent::stashSession(); -- 2.20.1