Fixed importScopedSession() and moved exportUserSession() to RequestContext.
authorAaron Schulz <aschulz@wikimedia.org>
Thu, 14 Mar 2013 22:43:42 +0000 (15:43 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Fri, 15 Mar 2013 19:49:54 +0000 (12:49 -0700)
* Renamed WebRequest::exportUserSession -> RequestContext::exportSession.
  Updated the only callers of this new function.
* Init the user with User::newFromId() instead of relying on the session
  (which breaks when things like CentralAuth are enabled).
* Made RequestContext::exportSession() include the user ID.
* Removed now-redundant user ID checks in upload jobs.
* Added unit tests for the session import function.

Change-Id: I543e6766f7a8a828ea5d270328c3bc7738c6fe94

includes/WebRequest.php
includes/api/ApiUpload.php
includes/context/ContextSource.php
includes/context/IContextSource.php
includes/context/RequestContext.php
includes/job/jobs/AssembleUploadChunksJob.php
includes/job/jobs/PublishStashedFileJob.php
tests/phpunit/includes/RequestContextTest.php

index 9b6d9ce..98007ef 100644 (file)
@@ -1133,21 +1133,6 @@ HTML;
        public function setIP( $ip ) {
                $this->ip = $ip;
        }
-
-       /**
-        * Export the resolved user IP, HTTP headers, and session ID.
-        * The result will be reasonably sized to allow for serialization.
-        *
-        * @return Array
-        * @since 1.21
-        */
-       public function exportUserSession() {
-               return array(
-                       'ip'        => $this->getIP(),
-                       'headers'   => $this->getAllHeaders(),
-                       'sessionId' => session_id()
-               );
-       }
 }
 
 /**
index f2733bd..deeb1c1 100644 (file)
@@ -224,8 +224,7 @@ class ApiUpload extends ApiBase {
                                        array(
                                                'filename'  => $this->mParams['filename'],
                                                'filekey'   => $this->mParams['filekey'],
-                                               'session'   => $this->getRequest()->exportUserSession(),
-                                               'userid'    => $this->getUser()->getId()
+                                               'session'   => $this->getContext()->exportSession()
                                        )
                                ) );
                                if ( $ok ) {
@@ -594,8 +593,7 @@ class ApiUpload extends ApiBase {
                                        'comment'   => $this->mParams['comment'],
                                        'text'      => $this->mParams['text'],
                                        'watch'     => $watch,
-                                       'session'   => $this->getRequest()->exportUserSession(),
-                                       'userid'    => $this->getUser()->getId()
+                                       'session'   => $this->getContext()->exportSession()
                                )
                        ) );
                        if ( $ok ) {
index 4a02f0b..33f51cb 100644 (file)
@@ -164,4 +164,15 @@ abstract class ContextSource implements IContextSource {
                $args = func_get_args();
                return call_user_func_array( array( $this->getContext(), 'msg' ), $args );
        }
+
+       /**
+        * Export the resolved user IP, HTTP headers, user ID, and session ID.
+        * The result will be reasonably sized to allow for serialization.
+        *
+        * @return Array
+        * @since 1.21
+        */
+       public function exportSession() {
+               return $this->getContext()->exportSession();
+       }
 }
index 0399081..c7b221b 100644 (file)
@@ -105,4 +105,13 @@ interface IContextSource {
         * @return Message
         */
        public function msg();
+
+       /**
+        * Export the resolved user IP, HTTP headers, user ID, and session ID.
+        * The result will be reasonably sized to allow for serialization.
+        *
+        * @return Array
+        * @since 1.21
+        */
+       public function exportSession();
 }
index 60c8cd3..6aefc98 100644 (file)
@@ -393,59 +393,92 @@ class RequestContext implements IContextSource {
        }
 
        /**
-        * Import the resolved user IP, HTTP headers, and session ID.
+        * Export the resolved user IP, HTTP headers, user ID, and session ID.
+        * The result will be reasonably sized to allow for serialization.
+        *
+        * @return Array
+        * @since 1.21
+        */
+       public function exportSession() {
+               return array(
+                       'ip'        => $this->getRequest()->getIP(),
+                       'headers'   => $this->getRequest()->getAllHeaders(),
+                       'sessionId' => session_id(),
+                       'userId'    => $this->getUser()->getId()
+               );
+       }
+
+       /**
+        * Import the resolved user IP, HTTP headers, user ID, and session ID.
         * This sets the current session and sets $wgUser and $wgRequest.
         * Once the return value falls out of scope, the old context is restored.
         * This function can only be called within CLI mode scripts.
         *
         * This will setup the session from the given ID. This is useful when
-        * background scripts inherit some context when acting on behalf of a user.
+        * background scripts inherit context when acting on behalf of a user.
         *
-        * $param array $params Result of WebRequest::exportUserSession()
+        * $param array $params Result of RequestContext::exportSession()
         * @return ScopedCallback
         * @throws MWException
         * @since 1.21
         */
        public static function importScopedSession( array $params ) {
                if ( PHP_SAPI !== 'cli' ) {
-                       // Don't send random private cookie headers to other random users
+                       // Don't send random private cookies or turn $wgRequest into FauxRequest
                        throw new MWException( "Sessions can only be imported in cli mode." );
+               } elseif ( !strlen( $params['sessionId'] ) ) {
+                       throw new MWException( "No session ID was specified." );
                }
 
-               $importSessionFunction = function( array $params ) {
+               if ( $params['userId'] ) { // logged-in user
+                       $user = User::newFromId( $params['userId'] );
+                       if ( !$user ) {
+                               throw new MWException( "No user with ID '{$params['userId']}'." );
+                       }
+               } elseif ( !IP::isValid( $params['ip'] ) ) {
+                       throw new MWException( "Could not load user '{$params['ip']}'." );
+               } else { // anon user
+                       $user = User::newFromName( $params['ip'], false );
+               }
+
+               $importSessionFunction = function( User $user, array $params ) {
                        global $wgRequest, $wgUser;
 
-                       // Write and close any current session
+                       $context = RequestContext::getMain();
+                       // Commit and close any current session
                        session_write_close(); // persist
                        session_id( '' ); // detach
                        $_SESSION = array(); // clear in-memory array
-                       // Load the new session from the session ID
-                       if ( strlen( $params['sessionId'] ) ) {
+                       // Remove any user IP or agent information
+                       $context->setRequest( new FauxRequest() );
+                       $wgRequest = $context->getRequest(); // b/c
+                       // Now that all private information is detached from the user, it should
+                       // be safe to load the new user. If errors occur or an exception is thrown
+                       // and caught (leaving the main context in a mixed state), there is no risk
+                       // of the User object being attached to the wrong IP, headers, or session.
+                       $context->setUser( $user );
+                       $wgUser = $context->getUser(); // b/c
+                       if ( strlen( $params['sessionId'] ) ) { // don't make a new random ID
                                wfSetupSession( $params['sessionId'] ); // sets $_SESSION
                        }
-                       // Build the new WebRequest object
                        $request = new FauxRequest( array(), false, $_SESSION );
                        $request->setIP( $params['ip'] );
                        foreach ( $params['headers'] as $name => $value ) {
                                $request->setHeader( $name, $value );
                        }
-
-                       $context = RequestContext::getMain();
                        // Set the current context to use the new WebRequest
                        $context->setRequest( $request );
                        $wgRequest = $context->getRequest(); // b/c
-                       // Set the current user based on the new session and WebRequest
-                       $context->setUser( User::newFromSession( $request ) ); // uses $_SESSION
-                       $wgUser = $context->getUser(); // b/c
                };
 
                // Stash the old session and load in the new one
-               $oldParams = self::getMain()->getRequest()->exportUserSession();
-               $importSessionFunction( $params );
+               $oUser = self::getMain()->getUser();
+               $oParams = self::getMain()->exportSession();
+               $importSessionFunction( $user, $params );
 
                // Set callback to save and close the new session and reload the old one
-               return new ScopedCallback( function() use ( $importSessionFunction, $oldParams ) {
-                       $importSessionFunction( $oldParams );
+               return new ScopedCallback( function() use ( $importSessionFunction, $oUser, $oParams ) {
+                       $importSessionFunction( $oUser, $oParams );
                } );
        }
 
index f243b0c..c5dd9ea 100644 (file)
@@ -37,7 +37,7 @@ class AssembleUploadChunksJob extends Job {
                $context = RequestContext::getMain();
                try {
                        $user = $context->getUser();
-                       if ( !$user->isLoggedIn() || $user->getId() != $this->params['userid'] ) {
+                       if ( !$user->isLoggedIn() ) {
                                $this->setLastError( "Could not load the author user from session." );
                                return false;
                        }
index 87dffc9..d3feda2 100644 (file)
@@ -37,7 +37,7 @@ class PublishStashedFileJob extends Job {
                $context = RequestContext::getMain();
                try {
                        $user = $context->getUser();
-                       if ( !$user->isLoggedIn() || $user->getId() != $this->params['userid'] ) {
+                       if ( !$user->isLoggedIn() ) {
                                $this->setLastError( "Could not load the author user from session." );
                                return false;
                        }
index 48cf6dc..f587171 100644 (file)
@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group Database
+ */
 class RequestContextTest extends MediaWikiTestCase {
 
        /**
@@ -25,4 +28,42 @@ class RequestContextTest extends MediaWikiTestCase {
 
        }
 
+       public function testImportScopedSession() {
+               $context = RequestContext::getMain();
+
+               $oInfo = $context->exportSession();
+               $this->assertEquals( '127.0.0.1', $oInfo['ip'], "Correct initial IP address." );
+               $this->assertEquals( 0, $oInfo['userId'], "Correct initial user ID." );
+
+               $user = User::newFromName( 'UnitTestContextUser' );
+               $user->addToDatabase();
+
+               $sinfo = array(
+                       'sessionId' => 'd612ee607c87e749ef14da4983a702cd',
+                       'userId' => $user->getId(),
+                       'ip' => '192.0.2.0',
+                       'headers' => array( 'USER-AGENT' => 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0' )
+               );
+               $sc = RequestContext::importScopedSession( $sinfo ); // load new context
+
+               $info = $context->exportSession();
+               $this->assertEquals( $sinfo['ip'], $info['ip'], "Correct IP address." );
+               $this->assertEquals( $sinfo['headers'], $info['headers'], "Correct headers." );
+               $this->assertEquals( $sinfo['sessionId'], $info['sessionId'], "Correct session ID." );
+               $this->assertEquals( $sinfo['userId'], $info['userId'], "Correct user ID." );
+               $this->assertEquals( $sinfo['ip'], $context->getRequest()->getIP(), "Correct context IP address." );
+               $this->assertEquals( $sinfo['headers'], $context->getRequest()->getAllHeaders(), "Correct context headers." );
+               $this->assertEquals( $sinfo['sessionId'], session_id(), "Correct context session ID." );
+               $this->assertEquals( true, $context->getUser()->isLoggedIn(), "Correct context user." );
+               $this->assertEquals( $sinfo['userId'], $context->getUser()->getId(), "Correct context user ID." );
+               $this->assertEquals( 'UnitTestContextUser', $context->getUser()->getName(), "Correct context user name." );
+
+               unset ( $sc ); // restore previous context
+
+               $info = $context->exportSession();
+               $this->assertEquals( $oInfo['ip'], $info['ip'], "Correct initial IP address." );
+               $this->assertEquals( $oInfo['headers'], $info['headers'], "Correct initial headers." );
+               $this->assertEquals( $oInfo['sessionId'], $info['sessionId'], "Correct initial session ID." );
+               $this->assertEquals( $oInfo['userId'], $info['userId'], "Correct initial user ID." );
+       }
 }