From 36c143110a88965c5a6aa8db8b62c04f0acc214b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 13 Jan 2012 23:30:46 +0000 Subject: [PATCH] In FileBackend: * Made secure() call doPrepare() for caller dummy proofing (especially those that don't check the status). In FSFileBackend: * Removed redundant wfMkdirParents() calls in FSFileBackend, we already have prepare() for this purpose. This also keeps it's behavior more consistent with the other backends. * Made use of 'backend-fail-store' message in doStoreInternal(). Other: * Updated unit tests and renamed $src => $source in some functions for consistency. * Added some documentation comments and @since tags. --- includes/filerepo/backend/FSFileBackend.php | 30 ++-------- includes/filerepo/backend/FileBackend.php | 6 +- .../filerepo/backend/FileBackendGroup.php | 1 + .../backend/FileBackendMultiWrite.php | 1 + includes/filerepo/backend/FileOp.php | 3 +- .../backend/lockmanager/DBLockManager.php | 1 + .../backend/lockmanager/FSLockManager.php | 1 + .../backend/lockmanager/LSLockManager.php | 1 + .../backend/lockmanager/LockManager.php | 1 + .../backend/lockmanager/LockManagerGroup.php | 1 + .../includes/filerepo/FileBackendTest.php | 56 +++++++++++++------ 11 files changed, 56 insertions(+), 46 deletions(-) diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php index fbf101757a..007d15542e 100644 --- a/includes/filerepo/backend/FSFileBackend.php +++ b/includes/filerepo/backend/FSFileBackend.php @@ -19,6 +19,7 @@ * Likewise, error suppression should be used to avoid path disclosure. * * @ingroup FileBackend + * @since 1.19 */ class FSFileBackend extends FileBackend { protected $basePath; // string; directory holding the container directories @@ -125,18 +126,13 @@ class FSFileBackend extends FileBackend { $status->fatal( 'backend-fail-alreadyexists', $params['dst'] ); return $status; } - } else { - if ( !wfMkdirParents( dirname( $dest ) ) ) { - $status->fatal( 'directorycreateerror', $params['dst'] ); - return $status; - } } wfSuppressWarnings(); $ok = copy( $params['src'], $dest ); wfRestoreWarnings(); if ( !$ok ) { - $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] ); + $status->fatal( 'backend-fail-store', $params['src'], $params['dst'] ); return $status; } @@ -176,11 +172,6 @@ class FSFileBackend extends FileBackend { $status->fatal( 'backend-fail-alreadyexists', $params['dst'] ); return $status; } - } else { - if ( !wfMkdirParents( dirname( $dest ) ) ) { - $status->fatal( 'directorycreateerror', $params['dst'] ); - return $status; - } } wfSuppressWarnings(); @@ -230,11 +221,6 @@ class FSFileBackend extends FileBackend { $status->fatal( 'backend-fail-alreadyexists', $params['dst'] ); return $status; } - } else { - if ( !wfMkdirParents( dirname( $dest ) ) ) { - $status->fatal( 'directorycreateerror', $params['dst'] ); - return $status; - } } wfSuppressWarnings(); @@ -304,11 +290,6 @@ class FSFileBackend extends FileBackend { $status->fatal( 'backend-fail-alreadyexists', $params['dst'] ); return $status; } - } else { - if ( !wfMkdirParents( dirname( $dest ) ) ) { - $status->fatal( 'directorycreateerror', $params['dst'] ); - return $status; - } } wfSuppressWarnings(); @@ -332,7 +313,7 @@ class FSFileBackend extends FileBackend { list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] ); $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; - if ( !wfMkdirParents( $dir ) ) { + if ( !wfMkdirParents( $dir ) ) { // make directory and its parents $status->fatal( 'directorycreateerror', $params['dir'] ); } elseif ( !is_writable( $dir ) ) { $status->fatal( 'directoryreadonlyerror', $params['dir'] ); @@ -350,10 +331,6 @@ class FSFileBackend extends FileBackend { list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] ); $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; - if ( !wfMkdirParents( $dir ) ) { - $status->fatal( 'directorycreateerror', $params['dir'] ); - return $status; - } // Seed new directories with a blank index.html, to prevent crawling... if ( !empty( $params['noListing'] ) && !file_exists( "{$dir}/index.html" ) ) { wfSuppressWarnings(); @@ -527,6 +504,7 @@ class FSFileBackend extends FileBackend { /** * Wrapper around RecursiveDirectoryIterator that catches * exception or does any custom behavoir that we may want. + * Do not use this class from places outside FSFileBackend. * * @ingroup FileBackend */ diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 7af34efb02..267bc5ebf7 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -323,7 +323,11 @@ abstract class FileBackendBase { if ( $this->readOnly != '' ) { return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly ); } - return $this->doSecure( $params ); + $status = $this->doPrepare( $params ); // dir must exist to restrict it + if ( $status->isOK() ) { + $status->merge( $this->doSecure( $params ) ); + } + return $status; } /** diff --git a/includes/filerepo/backend/FileBackendGroup.php b/includes/filerepo/backend/FileBackendGroup.php index 24277b114e..cb4ef3106f 100644 --- a/includes/filerepo/backend/FileBackendGroup.php +++ b/includes/filerepo/backend/FileBackendGroup.php @@ -9,6 +9,7 @@ * Class to handle file backend registration * * @ingroup FileBackend + * @since 1.19 */ class FileBackendGroup { protected static $instance = null; diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index 4258d13543..8685774215 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -18,6 +18,7 @@ * If an operation fails on one backend it will be rolled back from the others. * * @ingroup FileBackend + * @since 1.19 */ class FileBackendMultiWrite extends FileBackendBase { /** @var Array Prioritized list of FileBackend objects */ diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php index 11fe865b56..492a0e71a2 100644 --- a/includes/filerepo/backend/FileOp.php +++ b/includes/filerepo/backend/FileOp.php @@ -8,8 +8,9 @@ /** * Helper class for representing operations with transaction support. * FileBackend::doOperations() will require these classes for supported operations. + * Do not use this class from places outside FileBackend. * - * Use of large fields should be avoided as we want to be able to support + * Use of large fields should be avoided as we want to support * potentially many FileOp classes in large arrays in memory. * * @ingroup FileBackend diff --git a/includes/filerepo/backend/lockmanager/DBLockManager.php b/includes/filerepo/backend/lockmanager/DBLockManager.php index ac9c0791d6..900a6396b9 100644 --- a/includes/filerepo/backend/lockmanager/DBLockManager.php +++ b/includes/filerepo/backend/lockmanager/DBLockManager.php @@ -14,6 +14,7 @@ * Caching is used to avoid hitting servers that are down. * * @ingroup LockManager + * @since 1.19 */ class DBLockManager extends LockManager { /** @var Array Map of DB names to server config */ diff --git a/includes/filerepo/backend/lockmanager/FSLockManager.php b/includes/filerepo/backend/lockmanager/FSLockManager.php index be5fda9b62..33aceb447a 100644 --- a/includes/filerepo/backend/lockmanager/FSLockManager.php +++ b/includes/filerepo/backend/lockmanager/FSLockManager.php @@ -10,6 +10,7 @@ * locks will be ignored; see http://nfs.sourceforge.net/#section_d. * * @ingroup LockManager + * @since 1.19 */ class FSLockManager extends LockManager { /** @var Array Mapping of lock types to the type actually used */ diff --git a/includes/filerepo/backend/lockmanager/LSLockManager.php b/includes/filerepo/backend/lockmanager/LSLockManager.php index 383c77c53a..58fb8f3f2d 100644 --- a/includes/filerepo/backend/lockmanager/LSLockManager.php +++ b/includes/filerepo/backend/lockmanager/LSLockManager.php @@ -11,6 +11,7 @@ * A majority of peers must agree for a lock to be acquired. * * @ingroup LockManager + * @since 1.19 */ class LSLockManager extends LockManager { /** @var Array Mapping of lock types to the type actually used */ diff --git a/includes/filerepo/backend/lockmanager/LockManager.php b/includes/filerepo/backend/lockmanager/LockManager.php index eb4762337e..c197976134 100644 --- a/includes/filerepo/backend/lockmanager/LockManager.php +++ b/includes/filerepo/backend/lockmanager/LockManager.php @@ -161,6 +161,7 @@ class ScopedLock { /** * Simple version of LockManager that does nothing + * @since 1.19 */ class NullLockManager extends LockManager { protected function doLock( array $paths, $type ) { diff --git a/includes/filerepo/backend/lockmanager/LockManagerGroup.php b/includes/filerepo/backend/lockmanager/LockManagerGroup.php index e501a0f616..3fa91d581d 100644 --- a/includes/filerepo/backend/lockmanager/LockManagerGroup.php +++ b/includes/filerepo/backend/lockmanager/LockManagerGroup.php @@ -4,6 +4,7 @@ * * @ingroup LockManager * @author Aaron Schulz + * @since 1.19 */ class LockManagerGroup { protected static $instance = null; diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index 59fa1ab013..cffa7e3dd5 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -72,6 +72,8 @@ class FileBackendTest extends MediaWikiTestCase { function doTestStore( $op, $source, $dest ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $dest ) ) ); + file_put_contents( $source, "Unit test file" ); $status = $this->backend->doOperation( $op ); @@ -137,6 +139,9 @@ class FileBackendTest extends MediaWikiTestCase { function doTestCopy( $op, $source, $dest ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + $this->backend->prepare( array( 'dir' => dirname( $dest ) ) ); + $status = $this->backend->doOperation( array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); $this->assertEquals( true, $status->isOK(), @@ -207,6 +212,9 @@ class FileBackendTest extends MediaWikiTestCase { public function doTestMove( $op, $source, $dest ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + $this->backend->prepare( array( 'dir' => dirname( $dest ) ) ); + $status = $this->backend->doOperation( array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); $this->assertEquals( true, $status->isOK(), @@ -278,6 +286,8 @@ class FileBackendTest extends MediaWikiTestCase { public function doTestDelete( $op, $source, $withSource, $okStatus ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + if ( $withSource ) { $status = $this->backend->doOperation( array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); @@ -359,6 +369,8 @@ class FileBackendTest extends MediaWikiTestCase { public function doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $dest ) ) ); + $oldText = 'blah...blah...waahwaah'; if ( $alreadyExists ) { $status = $this->backend->doOperation( @@ -462,6 +474,7 @@ class FileBackendTest extends MediaWikiTestCase { // Create sources $ops = array(); foreach ( $srcs as $i => $source ) { + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); $ops[] = array( 'op' => 'create', // operation 'dst' => $source, // source @@ -585,20 +598,22 @@ class FileBackendTest extends MediaWikiTestCase { /** * @dataProvider provider_testGetFileContents */ - public function doTestGetFileContents( $src, $content ) { + public function doTestGetFileContents( $source, $content ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content, 'dst' => $source ) ); $this->assertEquals( true, $status->isOK(), - "Creation of file at $src succeeded ($backendName)." ); + "Creation of file at $source succeeded ($backendName)." ); - $newContents = $this->backend->getFileContents( array( 'src' => $src ) ); + $newContents = $this->backend->getFileContents( array( 'src' => $source ) ); $this->assertNotEquals( false, $newContents, - "Read of file at $src succeeded ($backendName)." ); + "Read of file at $source succeeded ($backendName)." ); $this->assertEquals( $content, $newContents, - "Contents read match data at $src ($backendName)." ); + "Contents read match data at $source ($backendName)." ); } function provider_testGetFileContents() { @@ -626,20 +641,22 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - public function doTestGetLocalCopy( $src, $content ) { + public function doTestGetLocalCopy( $source, $content ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content, 'dst' => $source ) ); $this->assertEquals( true, $status->isOK(), - "Creation of file at $src succeeded ($backendName)." ); + "Creation of file at $source succeeded ($backendName)." ); - $tmpFile = $this->backend->getLocalCopy( array( 'src' => $src ) ); + $tmpFile = $this->backend->getLocalCopy( array( 'src' => $source ) ); $this->assertNotNull( $tmpFile, - "Creation of local copy of $src succeeded ($backendName)." ); + "Creation of local copy of $source succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); - $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." ); + $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." ); } function provider_testGetLocalCopy() { @@ -667,20 +684,22 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - public function doTestGetLocalReference( $src, $content ) { + public function doTestGetLocalReference( $source, $content ) { $backendName = $this->backendClass(); + $this->backend->prepare( array( 'dir' => dirname( $source ) ) ); + $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content, 'dst' => $source ) ); $this->assertEquals( true, $status->isOK(), - "Creation of file at $src succeeded ($backendName)." ); + "Creation of file at $source succeeded ($backendName)." ); - $tmpFile = $this->backend->getLocalReference( array( 'src' => $src ) ); + $tmpFile = $this->backend->getLocalReference( array( 'src' => $source ) ); $this->assertNotNull( $tmpFile, - "Creation of local copy of $src succeeded ($backendName)." ); + "Creation of local copy of $source succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); - $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." ); + $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." ); } function provider_testGetLocalReference() { @@ -779,6 +798,7 @@ class FileBackendTest extends MediaWikiTestCase { $ops = array(); foreach ( $files as $file ) { $ops[] = array( 'op' => 'create', 'content' => 'xxy', 'dst' => $file ); + $this->backend->prepare( array( 'dir' => dirname( $file ) ) ); } $status = $this->backend->doOperations( $ops ); $this->assertEquals( true, $status->isOK(), -- 2.20.1