Merge "API: Insist authn parameters be in the POST body"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Fri, 19 Aug 2016 01:43:11 +0000 (01:43 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Fri, 19 Aug 2016 01:43:11 +0000 (01:43 +0000)
includes/api/ApiUpload.php
includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php
includes/jobqueue/jobs/AssembleUploadChunksJob.php
includes/resourceloader/ResourceLoaderImageModule.php
includes/specials/SpecialSearch.php
includes/upload/UploadBase.php
includes/upload/UploadFromChunks.php
tests/phpunit/specials/SpecialSearchTest.php [new file with mode: 0644]

index fc2fd59..227278c 100644 (file)
@@ -278,7 +278,7 @@ class ApiUpload extends ApiBase {
                                // the old filekey and fetch the new one.
                                UploadBase::setSessionStatus( $this->getUser(), $filekey, false );
                                $this->mUpload->stash->removeFile( $filekey );
-                               $filekey = $this->mUpload->getLocalFile()->getFileKey();
+                               $filekey = $this->mUpload->getStashFile()->getFileKey();
 
                                $result['result'] = 'Success';
                        }
@@ -735,7 +735,7 @@ class ApiUpload extends ApiBase {
                        $this->mParams['text'] = $this->mParams['comment'];
                }
 
-               /** @var $file File */
+               /** @var $file LocalFile */
                $file = $this->mUpload->getLocalFile();
 
                // For preferences mode, we want to watch if 'watchdefault' is set,
index 46cbab5..ed94c1a 100644 (file)
@@ -303,7 +303,11 @@ class TemporaryPasswordPrimaryAuthenticationProvider
                );
 
                if ( $sendMail ) {
-                       $this->sendPasswordResetEmail( $req );
+                       // Send email after DB commit
+                       $dbw->onTransactionIdle( function () use ( $req ) {
+                               /** @var TemporaryPasswordAuthenticationRequest $req */
+                               $this->sendPasswordResetEmail( $req );
+                       } );
                }
        }
 
@@ -370,7 +374,10 @@ class TemporaryPasswordPrimaryAuthenticationProvider
                $this->providerChangeAuthenticationData( $req );
 
                if ( $mailpassword ) {
-                       $this->sendNewAccountEmail( $user, $creator, $req->password );
+                       // Send email after DB commit
+                       wfGetDB( DB_MASTER )->onTransactionIdle( function () use ( $user, $creator, $req ) {
+                               $this->sendNewAccountEmail( $user, $creator, $req->password );
+                       } );
                }
 
                return $mailpassword ? 'byemail' : null;
index 1e804c4..f3fcadf 100644 (file)
@@ -74,7 +74,7 @@ class AssembleUploadChunksJob extends Job {
                        }
 
                        // We have a new filekey for the fully concatenated file
-                       $newFileKey = $upload->getLocalFile()->getFileKey();
+                       $newFileKey = $upload->getStashFile()->getFileKey();
 
                        // Remove the old stash file row and first chunk file
                        $upload->stash->removeFileNoAuth( $this->params['filekey'] );
index 6cdab1b..43327c9 100644 (file)
@@ -393,6 +393,8 @@ class ResourceLoaderImageModule extends ResourceLoaderModule {
        public function getDefinitionSummary( ResourceLoaderContext $context ) {
                $this->loadFromDefinition();
                $summary = parent::getDefinitionSummary( $context );
+
+               $options = [];
                foreach ( [
                        'localBasePath',
                        'images',
@@ -401,29 +403,27 @@ class ResourceLoaderImageModule extends ResourceLoaderModule {
                        'selectorWithoutVariant',
                        'selectorWithVariant',
                ] as $member ) {
-                       $summary[$member] = $this->{$member};
+                       $options[$member] = $this->{$member};
                };
+
+               $summary[] = [
+                       'options' => $options,
+                       'fileHashes' => $this->getFileHashes( $context ),
+               ];
                return $summary;
        }
 
        /**
-        * Get the last modified timestamp of this module.
-        *
-        * @param ResourceLoaderContext $context Context in which to calculate
-        *     the modified time
-        * @return int UNIX timestamp
+        * Helper method for getDefinitionSummary.
         */
-       public function getModifiedTime( ResourceLoaderContext $context ) {
+       protected function getFileHashes( ResourceLoaderContext $context ) {
                $this->loadFromDefinition();
                $files = [];
                foreach ( $this->getImages( $context ) as $name => $image ) {
                        $files[] = $image->getPath( $context );
                }
-
                $files = array_values( array_unique( $files ) );
-               $filesMtime = max( array_map( [ __CLASS__, 'safeFilemtime' ], $files ) );
-
-               return $filesMtime;
+               return array_map( [ __CLASS__, 'safeFileHash' ], $files );
        }
 
        /**
index 9690d45..26b86f9 100644 (file)
@@ -100,6 +100,25 @@ class SpecialSearch extends SpecialPage {
         * @param string $par
         */
        public function execute( $par ) {
+               $request = $this->getRequest();
+
+               // Fetch the search term
+               $search = str_replace( "\n", " ", $request->getText( 'search' ) );
+
+               // Historically search terms have been accepted not only in the search query
+               // parameter, but also as part of the primary url. This can have PII implications
+               // in releasing page view data. As such issue a 301 redirect to the correct
+               // URL.
+               if ( strlen( $par ) && !strlen( $search ) ) {
+                       $query = $request->getValues();
+                       unset( $query['title'] );
+                       // Strip underscores from title parameter; most of the time we'll want
+                       // text form here. But don't strip underscores from actual text params!
+                       $query['search'] = str_replace( '_', ' ', $par );
+                       $this->getOutput()->redirect( $this->getPageTitle()->getFullURL( $query ), 301 );
+                       return;
+               }
+
                $this->setHeaders();
                $this->outputHeader();
                $out = $this->getOutput();
@@ -110,15 +129,6 @@ class SpecialSearch extends SpecialPage {
                ] );
                $this->addHelpLink( 'Help:Searching' );
 
-               // Strip underscores from title parameter; most of the time we'll want
-               // text form here. But don't strip underscores from actual text params!
-               $titleParam = str_replace( '_', ' ', $par );
-
-               $request = $this->getRequest();
-
-               // Fetch the search term
-               $search = str_replace( "\n", " ", $request->getText( 'search', $titleParam ) );
-
                $this->load();
                if ( !is_null( $request->getVal( 'nsRemember' ) ) ) {
                        $this->saveNamespaces();
index ae16f70..e2f7763 100644 (file)
@@ -44,7 +44,7 @@ abstract class UploadBase {
        protected $mDesiredDestName, $mDestName, $mRemoveTempFile, $mSourceType;
        protected $mTitle = false, $mTitleError = 0;
        protected $mFilteredName, $mFinalExtension;
-       protected $mLocalFile, $mFileSize, $mFileProps;
+       protected $mLocalFile, $mStashFile, $mFileSize, $mFileProps;
        protected $mBlackListedExtensions;
        protected $mJavaDetected, $mSVGNSError;
 
@@ -912,7 +912,7 @@ abstract class UploadBase {
        /**
         * Return the local file and initializes if necessary.
         *
-        * @return LocalFile|UploadStashFile|null
+        * @return LocalFile|null
         */
        public function getLocalFile() {
                if ( is_null( $this->mLocalFile ) ) {
@@ -923,6 +923,13 @@ abstract class UploadBase {
                return $this->mLocalFile;
        }
 
+       /**
+        * @return UploadStashFile|null
+        */
+       public function getStashFile() {
+               return $this->mStashFile;
+       }
+
        /**
         * Like stashFile(), but respects extensions' wishes to prevent the stashing. verifyUpload() must
         * be called before calling this method (unless $isPartial is true).
@@ -997,7 +1004,7 @@ abstract class UploadBase {
        protected function doStashFile( User $user = null ) {
                $stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash( $user );
                $file = $stash->stashFile( $this->mTempPath, $this->getSourceType() );
-               $this->mLocalFile = $file;
+               $this->mStashFile = $file;
 
                return $file;
        }
@@ -1975,18 +1982,16 @@ abstract class UploadBase {
         * @return array Image info
         */
        public function getImageInfo( $result ) {
-               $file = $this->getLocalFile();
-               /** @todo This cries out for refactoring.
-                *  We really want to say $file->getAllInfo(); here.
-                * Perhaps "info" methods should be moved into files, and the API should
-                * just wrap them in queries.
-                */
-               if ( $file instanceof UploadStashFile ) {
+               $localFile = $this->getLocalFile();
+               $stashFile = $this->getStashFile();
+               // Calling a different API module depending on whether the file was stashed is less than optimal.
+               // In fact, calling API modules here at all is less than optimal. Maybe it should be refactored.
+               if ( $stashFile ) {
                        $imParam = ApiQueryStashImageInfo::getPropertyNames();
-                       $info = ApiQueryStashImageInfo::getInfo( $file, array_flip( $imParam ), $result );
+                       $info = ApiQueryStashImageInfo::getInfo( $stashFile, array_flip( $imParam ), $result );
                } else {
                        $imParam = ApiQueryImageInfo::getPropertyNames();
-                       $info = ApiQueryImageInfo::getInfo( $file, array_flip( $imParam ), $result );
+                       $info = ApiQueryImageInfo::getInfo( $localFile, array_flip( $imParam ), $result );
                }
 
                return $info;
index 6368db8..9145a85 100644 (file)
@@ -76,18 +76,18 @@ class UploadFromChunks extends UploadFromFile {
 
                $this->verifyChunk();
                // Create a local stash target
-               $this->mLocalFile = parent::doStashFile( $user );
+               $this->mStashFile = parent::doStashFile( $user );
                // Update the initial file offset (based on file size)
-               $this->mOffset = $this->mLocalFile->getSize();
-               $this->mFileKey = $this->mLocalFile->getFileKey();
+               $this->mOffset = $this->mStashFile->getSize();
+               $this->mFileKey = $this->mStashFile->getFileKey();
 
                // Output a copy of this first to chunk 0 location:
-               $this->outputChunk( $this->mLocalFile->getPath() );
+               $this->outputChunk( $this->mStashFile->getPath() );
 
                // Update db table to reflect initial "chunk" state
                $this->updateChunkStatus();
 
-               return $this->mLocalFile;
+               return $this->mStashFile;
        }
 
        /**
@@ -158,7 +158,7 @@ class UploadFromChunks extends UploadFromFile {
                        return $status;
                }
 
-               // Update the mTempPath and mLocalFile
+               // Update the mTempPath and mStashFile
                // (for FileUpload or normal Stash to take over)
                $tStart = microtime( true );
                // This is a re-implementation of UploadBase::tryStashFile(), we can't call it because we
@@ -169,14 +169,14 @@ class UploadFromChunks extends UploadFromFile {
                        return $status;
                }
                try {
-                       $this->mLocalFile = parent::doStashFile( $this->user );
+                       $this->mStashFile = parent::doStashFile( $this->user );
                } catch ( UploadStashException $e ) {
                        $status->fatal( 'uploadstash-exception', get_class( $e ), $e->getMessage() );
                        return $status;
                }
 
                $tAmount = microtime( true ) - $tStart;
-               $this->mLocalFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo())
+               $this->mStashFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo())
                wfDebugLog( 'fileconcatenate', "Stashed combined file ($i chunks) in $tAmount seconds." );
 
                return $status;
diff --git a/tests/phpunit/specials/SpecialSearchTest.php b/tests/phpunit/specials/SpecialSearchTest.php
new file mode 100644 (file)
index 0000000..20e88f5
--- /dev/null
@@ -0,0 +1,23 @@
+<?php
+
+class SpecialSearchText extends \PHPUnit_Framework_TestCase {
+       public function testSubPageRedirect() {
+               $ctx = new RequestContext;
+
+               SpecialPageFactory::executePath(
+                       Title::newFromText( 'Special:Search/foo_bar' ),
+                       $ctx
+               );
+               $url = $ctx->getOutput()->getRedirect();
+               // some older versions of hhvm have a bug that doesn't parse relative
+               // urls with a port, so help it out a little bit.
+               // https://github.com/facebook/hhvm/issues/7136
+               $url = wfExpandUrl( $url, PROTO_CURRENT );
+
+               $parts = parse_url( $url );
+               $this->assertEquals( '/w/index.php', $parts['path'] );
+               parse_str( $parts['query'], $query );
+               $this->assertEquals( 'Special:Search', $query['title'] );
+               $this->assertEquals( 'foo bar', $query['search'] );
+       }
+}