[FileBackend] Refactored backend access control functions.
authorAaron <aschulz@wikimedia.org>
Mon, 21 May 2012 22:19:06 +0000 (15:19 -0700)
committerAaron <aschulz@wikimedia.org>
Tue, 10 Jul 2012 23:52:01 +0000 (16:52 -0700)
* Made secure() no longer create the container/directories.
* Added a new publish() function, which is the reverse of secure().
  It's seems reasonable to be able to reverse secure() if needed.
* Added the ability to call prepare() with the secure() parameters.
  The "securing" will only be done if the container had to be created.
  This kills a bunch of slow RTTs with setContainerAccess() in Swift.
* Also made the Swift doSecureInternal() function respect the arguments
  and set 'r:*' to properly make containers public.
* Consolidated FileRepo directory creation into an initDirectory() function.
  This uses the new prepare() arguments.

Change-Id: Ie16331ebf26c99295f60b266e07a4727228f53f2

includes/filerepo/FileRepo.php
includes/filerepo/backend/FSFileBackend.php
includes/filerepo/backend/FileBackend.php
includes/filerepo/backend/FileBackendMultiWrite.php
includes/filerepo/backend/FileBackendStore.php
includes/filerepo/backend/SwiftFileBackend.php

index d4eef87..3d2ccf4 100644 (file)
@@ -164,18 +164,6 @@ class FileRepo {
                return $status;
        }
 
-       /**
-        * Take all available measures to prevent web accessibility of new deleted
-        * directories, in case the user has not configured offline storage
-        *
-        * @param $dir string
-        * @return void
-        */
-       protected function initDeletedDir( $dir ) {
-               $this->backend->secure( // prevent web access & dir listings
-                       array( 'dir' => $dir, 'noAccess' => true, 'noListing' => true ) );
-       }
-
        /**
         * Determine if a string is an mwrepo:// URL
         *
@@ -726,14 +714,10 @@ class FileRepo {
                        $dstPath = "$root/$dstRel";
                        $dstDir  = dirname( $dstPath );
                        // Create destination directories for this triplet
-                       if ( !$backend->prepare( array( 'dir' => $dstDir ) )->isOK() ) {
+                       if ( !$this->initDirectory( $dstDir )->isOK() ) {
                                return $this->newFatal( 'directorycreateerror', $dstDir );
                        }
 
-                       if ( $dstZone == 'deleted' ) {
-                               $this->initDeletedDir( $dstDir );
-                       }
-
                        // Resolve source to a storage path if virtual
                        $srcPath = $this->resolveToStoragePath( $srcPath );
 
@@ -863,12 +847,13 @@ class FileRepo {
                $operations = array();
                foreach ( $pairs as $pair ) {
                        list ( $src, $dst ) = $pair;
+                       $dst = $this->resolveToStoragePath( $dst );
                        $operations[] = array(
                                'op'        => 'store',
                                'src'       => $src,
-                               'dst'       => $this->resolveToStoragePath( $dst )
+                               'dst'       => $dst
                        );
-                       $this->backend->prepare( array( 'dir' => dirname( $dst ) ) );
+                       $status->merge( $this->initDirectory( dirname( $dst ) ) );
                }
                $status->merge( $this->backend->doQuickOperations( $operations ) );
 
@@ -1058,10 +1043,10 @@ class FileRepo {
                        $dstDir = dirname( $dstPath );
                        $archiveDir = dirname( $archivePath );
                        // Abort immediately on directory creation errors since they're likely to be repetitive
-                       if ( !$backend->prepare( array( 'dir' => $dstDir ) )->isOK() ) {
+                       if ( !$this->initDirectory( $dstDir )->isOK() ) {
                                return $this->newFatal( 'directorycreateerror', $dstDir );
                        }
-                       if ( !$backend->prepare( array( 'dir' => $archiveDir ) )->isOK() ) {
+                       if ( !$this->initDirectory($archiveDir )->isOK() ) {
                                return $this->newFatal( 'directorycreateerror', $archiveDir );
                        }
 
@@ -1126,6 +1111,27 @@ class FileRepo {
                return $status;
        }
 
+       /**
+        * Creates a directory with the appropriate zone permissions.
+        * Callers are responsible for doing read-only and "writable repo" checks.
+        *
+        * @param $dir string Virtual URL (or storage path) of directory to clean
+        * @return Status
+        */
+       protected function initDirectory( $dir ) {
+               $path = $this->resolveToStoragePath( $dir );
+               list( $b, $container, $r ) = FileBackend::splitStoragePath( $path );
+
+               $params = array( 'dir' => $path );
+               if ( $container === $this->zones['deleted']['container'] ) {
+                       # Take all available measures to prevent web accessibility of new deleted
+                       # directories, in case the user has not configured offline storage
+                       $params = array( 'noAccess' => true, 'noListing' => true ) + $params;
+               }
+
+               return $this->backend->prepare( $params );
+       }
+
        /**
         * Deletes a directory if empty.
         *
@@ -1231,10 +1237,9 @@ class FileRepo {
                        $archiveDir = dirname( $archivePath ); // does not touch FS
 
                        // Create destination directories
-                       if ( !$backend->prepare( array( 'dir' => $archiveDir ) )->isOK() ) {
+                       if ( !$this->initDirectory( $archiveDir )->isOK() ) {
                                return $this->newFatal( 'directorycreateerror', $archiveDir );
                        }
-                       $this->initDeletedDir( $archiveDir );
 
                        $operations[] = array(
                                'op'            => 'move',
index 8157916..4c861e4 100644 (file)
@@ -474,13 +474,18 @@ class FSFileBackend extends FileBackendStore {
                list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
                $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
                $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
+               $existed = is_dir( $dir ); // already there?
                if ( !wfMkdirParents( $dir ) ) { // make directory and its parents
-                       $status->fatal( 'directorycreateerror', $params['dir'] );
+                       $status->fatal( 'directorycreateerror', $params['dir'] ); // fails on races
                } elseif ( !is_writable( $dir ) ) {
                        $status->fatal( 'directoryreadonlyerror', $params['dir'] );
                } elseif ( !is_readable( $dir ) ) {
                        $status->fatal( 'directorynotreadableerror', $params['dir'] );
                }
+               if ( is_dir( $dir ) && !$existed ) {
+                       // Respect any 'noAccess' or 'noListing' flags...
+                       $status->merge( $this->doSecureInternal( $fullCont, $dirRel, $params ) );
+               }
                return $status;
        }
 
@@ -495,21 +500,48 @@ class FSFileBackend extends FileBackendStore {
                $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
                // Seed new directories with a blank index.html, to prevent crawling...
                if ( !empty( $params['noListing'] ) && !file_exists( "{$dir}/index.html" ) ) {
-                       $bytes = file_put_contents( "{$dir}/index.html", '' );
-                       if ( !$bytes ) {
+                       $bytes = file_put_contents( "{$dir}/index.html", $this->indexHtmlPrivate() );
+                       if ( $bytes === false ) {
                                $status->fatal( 'backend-fail-create', $params['dir'] . '/index.html' );
                                return $status;
                        }
                }
                // Add a .htaccess file to the root of the container...
-               if ( !empty( $params['noAccess'] ) ) {
-                       if ( !file_exists( "{$contRoot}/.htaccess" ) ) {
-                               $bytes = file_put_contents( "{$contRoot}/.htaccess", "Deny from all\n" );
-                               if ( !$bytes ) {
-                                       $storeDir = "mwstore://{$this->name}/{$shortCont}";
-                                       $status->fatal( 'backend-fail-create', "{$storeDir}/.htaccess" );
-                                       return $status;
-                               }
+               if ( !empty( $params['noAccess'] ) && !file_exists( "{$contRoot}/.htaccess" ) ) {
+                       $bytes = file_put_contents( "{$contRoot}/.htaccess", $this->htaccessPrivate() );
+                       if ( $bytes === false ) {
+                               $storeDir = "mwstore://{$this->name}/{$shortCont}";
+                               $status->fatal( 'backend-fail-create', "{$storeDir}/.htaccess" );
+                               return $status;
+                       }
+               }
+               return $status;
+       }
+
+       /**
+        * @see FileBackendStore::doPublishInternal()
+        * @return Status
+        */
+       protected function doPublishInternal( $fullCont, $dirRel, array $params ) {
+               $status = Status::newGood();
+               list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
+               $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
+               $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
+               // Unseed new directories with a blank index.html, to allow crawling...
+               if ( !empty( $params['listing'] ) && is_file( "{$dir}/index.html" ) ) {
+                       $exists = ( file_get_contents( "{$dir}/index.html" ) === $this->indexHtmlPrivate() );
+                       if ( $exists && !unlink( "{$dir}/index.html" ) ) { // reverse secure()
+                               $status->fatal( 'backend-fail-delete', $params['dir'] . '/index.html' );
+                               return $status;
+                       }
+               }
+               // Remove the .htaccess file from the root of the container...
+               if ( !empty( $params['access'] ) && is_file( "{$contRoot}/.htaccess" ) ) {
+                       $exists = ( file_get_contents( "{$contRoot}/.htaccess" ) === $this->htaccessPrivate() );
+                       if ( $exists && !unlink( "{$contRoot}/.htaccess" ) ) { // reverse secure()
+                               $storeDir = "mwstore://{$this->name}/{$shortCont}";
+                               $status->fatal( 'backend-fail-delete', "{$storeDir}/.htaccess" );
+                               return $status;
                        }
                }
                return $status;
@@ -716,6 +748,24 @@ class FSFileBackend extends FileBackendStore {
                return $ok;
        }
 
+       /**
+        * Return the text of an index.html file to hide directory listings
+        *
+        * @return string
+        */
+       protected function indexHtmlPrivate() {
+               return '';
+       }
+
+       /**
+        * Return the text of a .htaccess file to make a directory private
+        *
+        * @return string
+        */
+       protected function htaccessPrivate() {
+               return "Deny from all\n";
+       }
+
        /**
         * Clean up directory separators for the given OS
         *
index 76816b4..9cf8f94 100644 (file)
@@ -568,9 +568,16 @@ abstract class FileBackend {
         * This will create any required containers and parent directories.
         * Backends using key/value stores only need to create the container.
         *
-        * @param $params Array
+        * The 'noAccess' and 'noListing' parameters works the same as in secure(),
+        * except they are only applied *if* the directory/container had to be created.
+        * These flags should always be set for directories that have private files.
+        *
         * $params include:
-        *   - dir : storage directory
+        *     dir       : storage directory
+        *     noAccess  : try to deny file access (@since 1.20)
+        *     noListing : try to deny file listing (@since 1.20)
+        *
+        * @param $params Array
         * @return Status
         */
        final public function prepare( array $params ) {
@@ -588,8 +595,8 @@ abstract class FileBackend {
        /**
         * Take measures to block web access to a storage directory and
         * the container it belongs to. FS backends might add .htaccess
-        * files whereas key/value store backends might restrict container
-        * access to the auth user that represents end-users in web request.
+        * files whereas key/value store backends might revoke container
+        * access to the storage user representing end-users in web requests.
         * This is not guaranteed to actually do anything.
         *
         * @param $params Array
@@ -603,11 +610,7 @@ abstract class FileBackend {
                if ( $this->isReadOnly() ) {
                        return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
                }
-               $status = $this->doPrepare( $params ); // dir must exist to restrict it
-               if ( $status->isOK() ) {
-                       $status->merge( $this->doSecure( $params ) );
-               }
-               return $status;
+               return $this->doSecure( $params );
        }
 
        /**
@@ -615,6 +618,34 @@ abstract class FileBackend {
         */
        abstract protected function doSecure( array $params );
 
+       /**
+        * Remove measures to block web access to a storage directory and
+        * the container it belongs to. FS backends might remove .htaccess
+        * files whereas key/value store backends might grant container
+        * access to the storage user representing end-users in web requests.
+        * This essentially can undo the result of secure() calls.
+        *
+        * $params include:
+        *     dir     : storage directory
+        *     access  : try to allow file access
+        *     listing : try to allow file listing
+        *
+        * @param $params Array
+        * @return Status
+        * @since 1.20
+        */
+       final public function publish( array $params ) {
+               if ( $this->isReadOnly() ) {
+                       return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
+               }
+               return $this->doPublish( $params );
+       }
+
+       /**
+        * @see FileBackend::publish()
+        */
+       abstract protected function doPublish( array $params );
+
        /**
         * Delete a storage directory if it is empty.
         * Backends using key/value stores may do nothing unless the directory
index efc6053..2fc9d8e 100644 (file)
@@ -343,6 +343,20 @@ class FileBackendMultiWrite extends FileBackend {
                return $status;
        }
 
+       /**
+        * @see FileBackend::doPublish()
+        * @param $params array
+        * @return Status
+        */
+       protected function doPublish( array $params ) {
+               $status = Status::newGood();
+               foreach ( $this->backends as $backend ) {
+                       $realParams = $this->substOpPaths( $params, $backend );
+                       $status->merge( $backend->doPublish( $realParams ) );
+               }
+               return $status;
+       }
+
        /**
         * @see FileBackend::doClean()
         * @param $params array
index 5fe33ba..9c713d3 100644 (file)
@@ -426,6 +426,46 @@ abstract class FileBackendStore extends FileBackend {
                return Status::newGood();
        }
 
+       /**
+        * @see FileBackend::doPublish()
+        * @return Status
+        */
+       final protected function doPublish( array $params ) {
+               wfProfileIn( __METHOD__ );
+               wfProfileIn( __METHOD__ . '-' . $this->name );
+               $status = Status::newGood();
+
+               list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
+               if ( $dir === null ) {
+                       $status->fatal( 'backend-fail-invalidpath', $params['dir'] );
+                       wfProfileOut( __METHOD__ . '-' . $this->name );
+                       wfProfileOut( __METHOD__ );
+                       return $status; // invalid storage path
+               }
+
+               if ( $shard !== null ) { // confined to a single container/shard
+                       $status->merge( $this->doPublishInternal( $fullCont, $dir, $params ) );
+               } else { // directory is on several shards
+                       wfDebug( __METHOD__ . ": iterating over all container shards.\n" );
+                       list( $b, $shortCont, $r ) = self::splitStoragePath( $params['dir'] );
+                       foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) {
+                               $status->merge( $this->doPublishInternal( "{$fullCont}{$suffix}", $dir, $params ) );
+                       }
+               }
+
+               wfProfileOut( __METHOD__ . '-' . $this->name );
+               wfProfileOut( __METHOD__ );
+               return $status;
+       }
+
+       /**
+        * @see FileBackendStore::doPublish()
+        * @return Status
+        */
+       protected function doPublishInternal( $container, $dir, array $params ) {
+               return Status::newGood();
+       }
+
        /**
         * @see FileBackend::doClean()
         * @return Status
index 2b10917..6bd8f0c 100644 (file)
@@ -524,13 +524,12 @@ class SwiftFileBackend extends FileBackendStore {
                // (b) Create container as needed
                try {
                        $contObj = $this->createContainer( $fullCont );
-                       // Make container public to end-users...
-                       if ( $this->swiftAnonUser != '' ) {
-                               $status->merge( $this->setContainerAccess(
-                                       $contObj,
-                                       array( $this->auth->username, $this->swiftAnonUser ), // read
-                                       array( $this->auth->username ) // write
-                               ) );
+                       if ( !empty( $params['noAccess'] ) ) {
+                               // Make container private to end-users...
+                               $status->merge( $this->doSecureInternal( $fullCont, $dir, $params ) );
+                       } else {
+                               // Make container public to end-users...
+                               $status->merge( $this->doPublishInternal( $fullCont, $dir, $params ) );
                        }
                        if ( $this->swiftUseCDN ) { // Rackspace style CDN
                                $contObj->make_public( $this->swiftCDNExpiry );
@@ -551,6 +550,9 @@ class SwiftFileBackend extends FileBackendStore {
         */
        protected function doSecureInternal( $fullCont, $dir, array $params ) {
                $status = Status::newGood();
+               if ( empty( $params['noAccess'] ) ) {
+                       return $status; // nothing to do
+               }
 
                // Restrict container from end-users...
                try {
@@ -560,18 +562,53 @@ class SwiftFileBackend extends FileBackendStore {
                        // NoSuchContainerException not thrown: container must exist
 
                        // Make container private to end-users...
-                       if ( $this->swiftAnonUser != '' && !isset( $contObj->mw_wasSecured ) ) {
+                       $status->merge( $this->setContainerAccess(
+                               $contObj,
+                               array( $this->auth->username ), // read
+                               array( $this->auth->username ) // write
+                       ) );
+                       if ( $this->swiftUseCDN && $contObj->is_public() ) { // Rackspace style CDN
+                               $contObj->make_private();
+                       }
+               } catch ( CDNNotEnabledException $e ) {
+                       // CDN not enabled; nothing to see here
+               } catch ( CloudFilesException $e ) { // some other exception?
+                       $this->handleException( $e, $status, __METHOD__, $params );
+               }
+
+               return $status;
+       }
+
+       /**
+        * @see FileBackendStore::doPublishInternal()
+        * @return Status
+        */
+       protected function doPublishInternal( $fullCont, $dir, array $params ) {
+               $status = Status::newGood();
+
+               // Unrestrict container from end-users...
+               try {
+                       // doPrepareInternal() should have been called,
+                       // so the Swift container should already exist...
+                       $contObj = $this->getContainer( $fullCont ); // normally a cache hit
+                       // NoSuchContainerException not thrown: container must exist
+
+                       // Make container public to end-users...
+                       if ( $this->swiftAnonUser != '' ) {
                                $status->merge( $this->setContainerAccess(
                                        $contObj,
-                                       array( $this->auth->username ), // read
+                                       array( $this->auth->username, $this->swiftAnonUser ), // read
+                                       array( $this->auth->username, $this->swiftAnonUser ) // write
+                               ) );
+                       } else {
+                               $status->merge( $this->setContainerAccess(
+                                       $contObj,
+                                       array( $this->auth->username, '.r:*' ), // read
                                        array( $this->auth->username ) // write
                                ) );
-                               // @TODO: when php-cloudfiles supports container
-                               // metadata, we can make use of that to avoid RTTs
-                               $contObj->mw_wasSecured = true; // avoid useless RTTs
                        }
-                       if ( $this->swiftUseCDN && $contObj->is_public() ) { // Rackspace style CDN
-                               $contObj->make_private();
+                       if ( $this->swiftUseCDN && !$contObj->is_public() ) { // Rackspace style CDN
+                               $contObj->make_public();
                        }
                } catch ( CDNNotEnabledException $e ) {
                        // CDN not enabled; nothing to see here
@@ -1008,11 +1045,28 @@ class SwiftFileBackend extends FileBackendStore {
        }
 
        /**
-        * Set read/write permissions for a Swift container
+        * Set read/write permissions for a Swift container.
+        *
+        * $readGrps is a list of the possible criteria for a request to have
+        * access to write to a container. Each item is of the following format:
+        *   account:user       - Grants access if the request is by this user
+        *
+        * $readGrps is a list of the possible criteria for a request to have
+        * access to read a container. Each item is one of the following formats:
+        *   .r:<regex>         - Grants access if the request is from a referrer host that
+        *                        matches the expression and the request is not for a listing.
+        *                        Setting this to '*' effectively makes a container public.
+        *   .rlistings:<regex> - Grants access if the request is from a referrer host that
+        *                        matches the expression and the request for a listing.
+        *
+        * @see http://swift.openstack.org/misc.html#acls
+        *
+        * In general, we don't allow listings to end-users. It's not useful, isn't well-defined
+        * (lists are truncated to 10000 item with no way to page), and is just a performance risk.
         *
         * @param $contObj CF_Container Swift container
-        * @param $readGrps Array Swift users who can read (account:user)
-        * @param $writeGrps Array Swift users who can write (account:user)
+        * @param $readGrps Array List of read access routes
+        * @param $writeGrps Array List of write access routes
         * @return Status
         */
        protected function setContainerAccess(