From 2ee27d9a4de163dc3a2b752823a04f5338005627 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Wed, 3 Aug 2016 02:13:01 +0200 Subject: [PATCH] Introduce UploadStashFile hook, improve API handling of stash errors UploadBase: * Introduce a new method, tryStashFile(), as a replacement for the now-soft-deprecated stashFile(). The method runs the new hook and returns a Status object, with an error (if the hook returned an error) or a value (if it didn't). * Introduce a new hook, UploadStashFile, allowing extensions to prevent a file from being stashed. Note that code in extensions which has not been updated for MediaWiki 1.28 may still call stashFile() directly, and therefore not call this hook. For important checks (not just for UI), extension authors should use UploadVerifyFile or UploadVerifyUpload hooks. * Extract common code of tryStashFile() and stashFile() to a new protected method doStashFile(). SpecialUpload: * Use tryStashFile() when stashing a file after a warning or "recoverable error" was encountered. ApiUpload: * Refactor stashing code so that error handling only happens in one place, not four different ones. Use Status objects rather than exception throwing/catching for control flow. * Simplify the error messages slightly (error codes are unchanged). Produce better ones by always using handleStashException(). 'stashfailed' is now always at root (not nested inside 'warnings'), behaving the same as 'filekey' does on success. * Use tryStashFile() when stashing. Handle errors so as to allow custom API results passed via ApiMessage to be preserved. Some API result changes for different requests are shown below. api.php?action=upload&format=json&filename=good.png&file=...&stash=1 Before: { "error": { "code": "stashfilestorage", "info": "Could not store upload in the stash: Stashing temporary file failed: UploadStashFileException Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\".", "*": "See http://localhost:3080/w/api.php for API usage" } } After: { "error": { "code": "stashfilestorage", "info": "Could not store upload in the stash: Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\".", "*": "See http://localhost:3080/w/api.php for API usage" } } api.php?action=upload&format=json&filename=[bad].png&file=... Before: { "upload": { "result": "Warning", "warnings": { "badfilename": "-bad-.png", "stashfailed": "Stashing temporary file failed: UploadStashFileException Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\"." } } } After: { "upload": { "result": "Warning", "stashfailed": "Could not store upload in the stash: Error storing file in '/tmp/phpB32SRT': Could not create directory \"mwstore://local-backend/local-temp/3/3a\"." "warnings": { "badfilename": "-bad-.png", } } } Bug: T140521 Change-Id: I2f574b355cd33b2e9fa7ff8e1793503b257cce65 --- docs/hooks.txt | 12 +++ includes/api/ApiUpload.php | 152 +++++++++++++-------------- includes/specials/SpecialUpload.php | 16 ++- includes/upload/UploadBase.php | 42 +++++++- includes/upload/UploadFromChunks.php | 8 +- includes/upload/UploadFromStash.php | 4 +- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + 8 files changed, 150 insertions(+), 86 deletions(-) diff --git a/docs/hooks.txt b/docs/hooks.txt index 8fa3793d8f..5cf8ffeea6 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -3373,6 +3373,18 @@ added to the descriptor &$radio: Boolean, if source type should be shown as radio button $selectedSourceType: The selected source type +'UploadStashFile': Before a file is stashed (uploaded to stash). +Note that code which has not been updated for MediaWiki 1.28 may not call this +hook. If your extension absolutely, positively must prevent some files from +being uploaded, use UploadVerifyFile or UploadVerifyUpload. +$upload: (object) An instance of UploadBase, with all info about the upload +$user: (object) An instance of User, the user uploading this file +$props: (array) File properties, as returned by FSFile::getPropsFromPath() +&$error: output: If the file stashing should be prevented, set this to the reason + in the form of array( messagename, param1, param2, ... ) or a MessageSpecifier + instance (you might want to use ApiMessage to provide machine-readable details + for the API). + 'UploadVerification': DEPRECATED! Use UploadVerifyFile instead. Additional chances to reject an uploaded file. $saveName: (string) destination file name diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 95659e59c7..89d9af8ee8 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -64,7 +64,8 @@ class ApiUpload extends ApiBase { $this->dieUsage( 'No upload module set', 'nomodule' ); } } catch ( UploadStashException $e ) { // XXX: don't spam exception log - $this->handleStashException( $e ); + list( $msg, $code ) = $this->handleStashException( get_class( $e ), $e->getMessage() ); + $this->dieUsage( $msg, $code ); } // First check permission to upload @@ -112,7 +113,8 @@ class ApiUpload extends ApiBase { $result['imageinfo'] = $this->mUpload->getImageInfo( $this->getResult() ); } } catch ( UploadStashException $e ) { // XXX: don't spam exception log - $this->handleStashException( $e ); + list( $msg, $code ) = $this->handleStashException( get_class( $e ), $e->getMessage() ); + $this->dieUsage( $msg, $code ); } $this->getResult()->addValue( null, $this->getModuleName(), $result ); @@ -156,20 +158,13 @@ class ApiUpload extends ApiBase { */ private function getStashResult( $warnings ) { $result = []; + $result['result'] = 'Success'; + if ( $warnings && count( $warnings ) > 0 ) { + $result['warnings'] = $warnings; + } // Some uploads can request they be stashed, so as not to publish them immediately. // In this case, a failure to stash ought to be fatal - try { - $result['result'] = 'Success'; - $result['filekey'] = $this->performStash(); - $result['sessionkey'] = $result['filekey']; // backwards compatibility - if ( $warnings && count( $warnings ) > 0 ) { - $result['warnings'] = $warnings; - } - } catch ( UploadStashException $e ) { - $this->handleStashException( $e ); - } catch ( Exception $e ) { - $this->dieUsage( $e->getMessage(), 'stashfailed' ); - } + $this->performStash( 'critical', $result ); return $result; } @@ -185,12 +180,7 @@ class ApiUpload extends ApiBase { $result['warnings'] = $warnings; // in case the warnings can be fixed with some further user action, let's stash this upload // and return a key they can use to restart it - try { - $result['filekey'] = $this->performStash(); - $result['sessionkey'] = $result['filekey']; // backwards compatibility - } catch ( Exception $e ) { - $result['warnings']['stashfailed'] = $e->getMessage(); - } + $this->performStash( 'optional', $result ); return $result; } @@ -228,14 +218,7 @@ class ApiUpload extends ApiBase { } if ( $this->mParams['offset'] == 0 ) { - try { - $filekey = $this->performStash(); - } catch ( UploadStashException $e ) { - $this->handleStashException( $e ); - } catch ( Exception $e ) { - // FIXME: Error handling here is wrong/different from rest of this - $this->dieUsage( $e->getMessage(), 'stashfailed' ); - } + $filekey = $this->performStash( 'critical' ); } else { $filekey = $this->mParams['filekey']; @@ -320,27 +303,57 @@ class ApiUpload extends ApiBase { } /** - * Stash the file and return the file key - * Also re-raises exceptions with slightly more informative message strings (useful for API) - * @throws MWException - * @return string File key + * Stash the file and add the file key, or error information if it fails, to the data. + * + * @param string $failureMode What to do on failure to stash: + * - When 'critical', use dieStatus() to produce an error response and throw an exception. + * Use this when stashing the file was the primary purpose of the API request. + * - When 'optional', only add a 'stashfailed' key to the data and return null. + * Use this when some error happened for a non-stash upload and we're stashing the file + * only to save the client the trouble of re-uploading it. + * @param array &$data API result to which to add the information + * @return string|null File key */ - private function performStash() { + private function performStash( $failureMode, &$data = null ) { try { - $stashFile = $this->mUpload->stashFile( $this->getUser() ); + $status = $this->mUpload->tryStashFile( $this->getUser() ); - if ( !$stashFile ) { - throw new MWException( 'Invalid stashed file' ); + if ( $status->isGood() && !$status->getValue() ) { + // Not actually a 'good' status... + $status->fatal( new ApiRawMessage( 'Invalid stashed file', 'stashfailed' ) ); } - $fileKey = $stashFile->getFileKey(); } catch ( Exception $e ) { - $message = 'Stashing temporary file failed: ' . get_class( $e ) . ' ' . $e->getMessage(); - wfDebug( __METHOD__ . ' ' . $message . "\n" ); - $className = get_class( $e ); - throw new $className( $message ); + $debugMessage = 'Stashing temporary file failed: ' . get_class( $e ) . ' ' . $e->getMessage(); + wfDebug( __METHOD__ . ' ' . $debugMessage . "\n" ); + $status = Status::newFatal( new ApiRawMessage( $e->getMessage(), 'stashfailed' ) ); + } + + if ( $status->isGood() ) { + $stashFile = $status->getValue(); + $data['filekey'] = $stashFile->getFileKey(); + // Backwards compatibility + $data['sessionkey'] = $data['filekey']; + return $data['filekey']; + } + + if ( $status->getMessage()->getKey() === 'uploadstash-exception' ) { + // The exceptions thrown by upload stash code and pretty silly and UploadBase returns poor + // Statuses for it. Just extract the exception details and parse them ourselves. + list( $exceptionType, $message ) = $status->getMessage()->getParams(); + $debugMessage = 'Stashing temporary file failed: ' . $exceptionType . ' ' . $message; + wfDebug( __METHOD__ . ' ' . $debugMessage . "\n" ); + list( $msg, $code ) = $this->handleStashException( $exceptionType, $message ); + $status = Status::newFatal( new ApiRawMessage( $msg, $code ) ); } - return $fileKey; + // Bad status + if ( $failureMode !== 'optional' ) { + $this->dieStatus( $status ); + } else { + list( $code, $msg ) = $this->getErrorFromStatus( $status ); + $data['stashfailed'] = $msg; + return null; + } } /** @@ -354,12 +367,7 @@ class ApiUpload extends ApiBase { * @throws UsageException */ private function dieRecoverableError( $error, $parameter, $data = [], $code = 'unknownerror' ) { - try { - $data['filekey'] = $this->performStash(); - $data['sessionkey'] = $data['filekey']; - } catch ( Exception $e ) { - $data['stashfailed'] = $e->getMessage(); - } + $this->performStash( 'optional', $data ); $data['invalidparameter'] = $parameter; $parsed = $this->parseMsg( $error ); @@ -653,49 +661,41 @@ class ApiUpload extends ApiBase { /** * Handles a stash exception, giving a useful error to the user. - * @param Exception $e The exception we encountered. + * @param string $exceptionType Class name of the exception we encountered. + * @param string $message Message of the exception we encountered. + * @return array Array of message and code, suitable for passing to dieUsage() */ - protected function handleStashException( $e ) { - $exceptionType = get_class( $e ); - + protected function handleStashException( $exceptionType, $message ) { switch ( $exceptionType ) { case 'UploadStashFileNotFoundException': - $this->dieUsage( - 'Could not find the file in the stash: ' . $e->getMessage(), + return [ + 'Could not find the file in the stash: ' . $message, 'stashedfilenotfound' - ); - break; + ]; case 'UploadStashBadPathException': - $this->dieUsage( - 'File key of improper format or otherwise invalid: ' . $e->getMessage(), + return [ + 'File key of improper format or otherwise invalid: ' . $message, 'stashpathinvalid' - ); - break; + ]; case 'UploadStashFileException': - $this->dieUsage( - 'Could not store upload in the stash: ' . $e->getMessage(), + return [ + 'Could not store upload in the stash: ' . $message, 'stashfilestorage' - ); - break; + ]; case 'UploadStashZeroLengthFileException': - $this->dieUsage( + return [ 'File is of zero length, and could not be stored in the stash: ' . - $e->getMessage(), + $message, 'stashzerolength' - ); - break; + ]; case 'UploadStashNotLoggedInException': - $this->dieUsage( 'Not logged in: ' . $e->getMessage(), 'stashnotloggedin' ); - break; + return [ 'Not logged in: ' . $message, 'stashnotloggedin' ]; case 'UploadStashWrongOwnerException': - $this->dieUsage( 'Wrong owner: ' . $e->getMessage(), 'stashwrongowner' ); - break; + return [ 'Wrong owner: ' . $message, 'stashwrongowner' ]; case 'UploadStashNoSuchKeyException': - $this->dieUsage( 'No such filekey: ' . $e->getMessage(), 'stashnosuchfilekey' ); - break; + return [ 'No such filekey: ' . $message, 'stashnosuchfilekey' ]; default: - $this->dieUsage( $exceptionType . ': ' . $e->getMessage(), 'stasherror' ); - break; + return [ $exceptionType . ': ' . $message, 'stasherror' ]; } } diff --git a/includes/specials/SpecialUpload.php b/includes/specials/SpecialUpload.php index bfb52e8063..6ca1100a75 100644 --- a/includes/specials/SpecialUpload.php +++ b/includes/specials/SpecialUpload.php @@ -339,7 +339,13 @@ class SpecialUpload extends SpecialPage { * @param string $message HTML message to be passed to mainUploadForm */ protected function showRecoverableUploadError( $message ) { - $sessionKey = $this->mUpload->stashFile()->getFileKey(); + $stashStatus = $this->mUpload->tryStashFile( $this->getUser() ); + if ( $stashStatus->isGood() ) { + $sessionKey = $stashStatus->getValue()->getFileKey(); + } else { + $sessionKey = null; + // TODO Add a warning message about the failure to stash here? + } $message = '

' . $this->msg( 'uploaderror' )->escaped() . "

\n" . '
' . $message . "
\n"; @@ -368,7 +374,13 @@ class SpecialUpload extends SpecialPage { return false; } - $sessionKey = $this->mUpload->stashFile()->getFileKey(); + $stashStatus = $this->mUpload->tryStashFile( $this->getUser() ); + if ( $stashStatus->isGood() ) { + $sessionKey = $stashStatus->getValue()->getFileKey(); + } else { + $sessionKey = null; + // TODO Add a warning message about the failure to stash here? + } // Add styles for the warning, reused from the live preview $this->getOutput()->addModuleStyles( 'mediawiki.special.upload.styles' ); diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php index b62013239a..03c864a387 100644 --- a/includes/upload/UploadBase.php +++ b/includes/upload/UploadBase.php @@ -944,6 +944,33 @@ abstract class UploadBase { return $this->mLocalFile; } + /** + * Like stashFile(), but respects extensions' wishes to prevent the stashing. + * + * Upload stash exceptions are also caught and converted to an error status. + * + * @since 1.28 + * @param User $user + * @return Status If successful, value is an UploadStashFile instance + */ + public function tryStashFile( User $user ) { + $props = $this->mFileProps; + $error = null; + Hooks::run( 'UploadStashFile', [ $this, $user, $props, &$error ] ); + if ( $error ) { + if ( !is_array( $error ) ) { + $error = [ $error ]; + } + return call_user_func_array( 'Status::newFatal', $error ); + } + try { + $file = $this->doStashFile( $user ); + return Status::newGood( $file ); + } catch ( UploadStashException $e ) { + return Status::newFatal( 'uploadstash-exception', get_class( $e ), $e->getMessage() ); + } + } + /** * If the user does not supply all necessary information in the first upload * form submission (either by accident or by design) then we may want to @@ -956,6 +983,7 @@ abstract class UploadBase { * which can be passed through a form or API request to find this stashed * file again. * + * @deprecated since 1.28 Use tryStashFile() instead * @param User $user * @return UploadStashFile Stashed file * @throws UploadStashBadPathException @@ -963,6 +991,16 @@ abstract class UploadBase { * @throws UploadStashNotLoggedInException */ public function stashFile( User $user = null ) { + return $this->doStashFile( $user ); + } + + /** + * Implementation for stashFile() and tryStashFile(). + * + * @param User $user + * @return UploadStashFile Stashed file + */ + protected function doStashFile( User $user = null ) { $stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash( $user ); $file = $stash->stashFile( $this->mTempPath, $this->getSourceType() ); $this->mLocalFile = $file; @@ -979,7 +1017,7 @@ abstract class UploadBase { */ public function stashFileGetKey() { wfDeprecated( __METHOD__, '1.28' ); - return $this->stashFile()->getFileKey(); + return $this->doStashFile()->getFileKey(); } /** @@ -990,7 +1028,7 @@ abstract class UploadBase { */ public function stashSession() { wfDeprecated( __METHOD__, '1.28' ); - return $this->stashFile()->getFileKey(); + return $this->doStashFile()->getFileKey(); } /** diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php index 0323b685b8..cc1d69854a 100644 --- a/includes/upload/UploadFromChunks.php +++ b/includes/upload/UploadFromChunks.php @@ -65,19 +65,19 @@ class UploadFromChunks extends UploadFromFile { } /** - * Calls the parent stashFile and updates the uploadsession table to handle "chunks" + * Calls the parent doStashFile and updates the uploadsession table to handle "chunks" * * @param User|null $user * @return UploadStashFile Stashed file */ - public function stashFile( User $user = null ) { + protected function doStashFile( User $user = null ) { // Stash file is the called on creating a new chunk session: $this->mChunkIndex = 0; $this->mOffset = 0; $this->verifyChunk(); // Create a local stash target - $this->mLocalFile = parent::stashFile( $user ); + $this->mLocalFile = parent::doStashFile( $user ); // Update the initial file offset (based on file size) $this->mOffset = $this->mLocalFile->getSize(); $this->mFileKey = $this->mLocalFile->getFileKey(); @@ -162,7 +162,7 @@ class UploadFromChunks extends UploadFromFile { // Update the mTempPath and mLocalFile // (for FileUpload or normal Stash to take over) $tStart = microtime( true ); - $this->mLocalFile = parent::stashFile( $this->user ); + $this->mLocalFile = parent::doStashFile( $this->user ); $tAmount = microtime( true ) - $tStart; $this->mLocalFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo()) wfDebugLog( 'fileconcatenate', "Stashed combined file ($i chunks) in $tAmount seconds." ); diff --git a/includes/upload/UploadFromStash.php b/includes/upload/UploadFromStash.php index 908c8aa6c5..50bcbc4260 100644 --- a/includes/upload/UploadFromStash.php +++ b/includes/upload/UploadFromStash.php @@ -153,10 +153,10 @@ class UploadFromStash extends UploadBase { * @param User $user * @return UploadStashFile */ - public function stashFile( User $user = null ) { + protected function doStashFile( User $user = null ) { // replace mLocalFile with an instance of UploadStashFile, which adds some methods // that are useful for stashed files. - $this->mLocalFile = parent::stashFile( $user ); + $this->mLocalFile = parent::doStashFile( $user ); return $this->mLocalFile; } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index cddc30e492..82fdee75a3 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -1592,6 +1592,7 @@ "uploadstash-errclear": "Clearing the files failed.", "uploadstash-refresh": "Refresh the list of files", "uploadstash-thumbnail": "view thumbnail", + "uploadstash-exception": "Could not store upload in the stash ($1): \"$2\".", "invalid-chunk-offset": "Invalid chunk offset", "img-auth-accessdenied": "Access denied", "img-auth-nopathinfo": "Missing PATH_INFO.\nYour server is not set up to pass this information.\nIt may be CGI-based and cannot support img_auth.\nSee https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:Image_Authorization.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 054ab426c6..1cca279319 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -1774,6 +1774,7 @@ "uploadstash-errclear": "Used as error message in [[Special:UploadStash]].", "uploadstash-refresh": "Used as link text in [[Special:UploadStash]].", "uploadstash-thumbnail": "Used as link text in [[Special:UploadStash]].", + "uploadstash-exception": "Error message shown when an action related to the upload stash fails unexpectedly.\n\nParameters:\n* $1 - exception name, e.g. 'UploadStashFileNotFoundException'\n* $2 - exceptions details (always in English), e.g. 'cannot find path, or not a plain file'", "invalid-chunk-offset": "Error that can happen if chunks get uploaded out of order.\nAs a result of this error, clients can continue from an offset provided or restart the upload.\nUsed on [[Special:UploadWizard]].", "img-auth-accessdenied": "[[mw:Manual:Image Authorization|Manual:Image Authorization]]: Access Denied\n{{Identical|Access denied}}", "img-auth-nopathinfo": "[[mw:Manual:Image Authorization|Manual:Image Authorization]]: Missing PATH_INFO - see english description\n{{Doc-important|This is plain text. Do not use any wiki syntax.}}", -- 2.20.1