SessionManager: Change behavior of getSessionById()

It's easily possible for SessionManager::getSessionById() to not be
able to load the specified session and to not be able to create an empty
one by that ID, for example if the user's token changed. So change this
from an exceptional condition to an expected one, and adjust callers to
deal with it appropriately.

Let's also make the checks for invalid data structure when loading the
session from the store delete the bogus data entirely.

At the same time, let's change the silly "$noEmpty" parameter to
"$create" and make the default behavior be not to create an empty
session.

Bug: T124126
Change-Id: I085d2026d1b366b1af9fd0e8ca3d815fd8288030

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 7306105..2c14618 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -655,7 +655,10 @@ class WebRequest {
         */
        public function getSession() {
                if ( $this->sessionId !== null ) {
-                       return SessionManager::singleton()->getSessionById( (string)$this->sessionId, false, $this );
+                       $session = SessionManager::singleton()->getSessionById( (string)$this->sessionId, true, $this );
+                       if ( $session ) {
+                               return $session;
+                       }
                }
 
                $session = SessionManager::singleton()->getSessionForRequest( $this );

diff --git a/includes/context/RequestContext.php b/includes/context/RequestContext.php
index 16f11ee..afb5704 100644 (file)
--- a/includes/context/RequestContext.php
+++ b/includes/context/RequestContext.php
@@ -576,8 +576,9 @@ class RequestContext implements IContextSource, MutableContext {
                        // Get new session, if applicable
                        $session = null;
                        if ( strlen( $params['sessionId'] ) ) { // don't make a new random ID
-                               $session = MediaWiki\Session\SessionManager::singleton()
-                                       ->getSessionById( $params['sessionId'] );
+                               $manager = MediaWiki\Session\SessionManager::singleton();
+                               $session = $manager->getSessionById( $params['sessionId'], true )
+                                       ?: $manager->getEmptySession();
                        }
 
                        // Remove any user IP or agent information, and attach the request

diff --git a/includes/jobqueue/jobs/UploadFromUrlJob.php b/includes/jobqueue/jobs/UploadFromUrlJob.php
index 28e3c40..0491e64 100644 (file)
--- a/includes/jobqueue/jobs/UploadFromUrlJob.php
+++ b/includes/jobqueue/jobs/UploadFromUrlJob.php
@@ -155,18 +155,22 @@ class UploadFromUrlJob extends Job {
         * Store a result in the session data. Note that the caller is responsible
         * for appropriate session_start and session_write_close calls.
         *
-        * @param MediaWiki\\Session\\Session $session Session to store result into
+        * @param MediaWiki\\Session\\Session|null $session Session to store result into
         * @param string $result The result (Success|Warning|Failure)
         * @param string $dataKey The key of the extra data
         * @param mixed $dataValue The extra data itself
         */
        protected function storeResultInSession(
-               MediaWiki\Session\Session $session, $result, $dataKey, $dataValue
+               MediaWiki\Session\Session $session = null, $result, $dataKey, $dataValue
        ) {
-               $data = self::getSessionData( $session, $this->params['sessionKey'] );
-               $data['result'] = $result;
-               $data[$dataKey] = $dataValue;
-               self::setSessionData( $session, $this->params['sessionKey'], $data );
+               if ( $session ) {
+                       $data = self::getSessionData( $session, $this->params['sessionKey'] );
+                       $data['result'] = $result;
+                       $data[$dataKey] = $dataValue;
+                       self::setSessionData( $session, $this->params['sessionKey'], $data );
+               } else {
+                       wfDebug( __METHOD__ . ': Cannot store result in session, session does not exist' );
+               }
        }
 
        /**

diff --git a/includes/session/PHPSessionHandler.php b/includes/session/PHPSessionHandler.php
index c59cc96..44d14cd 100644 (file)
--- a/includes/session/PHPSessionHandler.php
+++ b/includes/session/PHPSessionHandler.php
@@ -208,7 +208,7 @@ class PHPSessionHandler {
                        throw new \BadMethodCallException( 'Attempt to use PHP session management' );
                }
 
-               $session = $this->manager->getSessionById( $id, true );
+               $session = $this->manager->getSessionById( $id, false );
                if ( !$session ) {
                        return '';
                }
@@ -236,7 +236,13 @@ class PHPSessionHandler {
                        throw new \BadMethodCallException( 'Attempt to use PHP session management' );
                }
 
-               $session = $this->manager->getSessionById( $id );
+               $session = $this->manager->getSessionById( $id, true );
+               if ( !$session ) {
+                       $this->logger->warning(
+                               __METHOD__ . ": Session \"$id\" cannot be loaded, skipping write."
+                       );
+                       return false;
+               }
 
                // First, decode the string PHP handed us
                $data = \Wikimedia\PhpSessionSerializer::decode( $dataStr );
@@ -331,7 +337,7 @@ class PHPSessionHandler {
                if ( !$this->enable ) {
                        throw new \BadMethodCallException( 'Attempt to use PHP session management' );
                }
-               $session = $this->manager->getSessionById( $id, true );
+               $session = $this->manager->getSessionById( $id, false );
                if ( $session ) {
                        $session->clear();
                }

diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php
index 1c8686c..0e45468 100644 (file)
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -123,7 +123,8 @@ final class SessionManager implements SessionManagerInterface {
                                // Someone used session_id(), so we need to follow suit.
                                // Note this overwrites whatever session might already be
                                // associated with $request with the one for $id.
-                               self::$globalSession = self::singleton()->getSessionById( $id, false, $request );
+                               self::$globalSession = self::singleton()->getSessionById( $id, true, $request )
+                                       ?: $request->getSession();
                        }
                }
                return self::$globalSession;
@@ -197,7 +198,7 @@ final class SessionManager implements SessionManagerInterface {
                return $session;
        }
 
-       public function getSessionById( $id, $noEmpty = false, WebRequest $request = null ) {
+       public function getSessionById( $id, $create = false, WebRequest $request = null ) {
                if ( !self::validateSessionId( $id ) ) {
                        throw new \InvalidArgumentException( 'Invalid session ID' );
                }
@@ -217,7 +218,7 @@ final class SessionManager implements SessionManagerInterface {
                        }
                }
 
-               if ( !$noEmpty && $session === null ) {
+               if ( $create && $session === null ) {
                        $ex = null;
                        try {
                                $session = $this->getEmptySessionInternal( $request, $id );
@@ -226,11 +227,6 @@ final class SessionManager implements SessionManagerInterface {
                                        $ex->getMessage() );
                                $session = null;
                        }
-                       if ( $session === null ) {
-                               throw new \UnexpectedValueException(
-                                       'Can neither load the session nor create an empty session', 0, $ex
-                               );
-                       }
                }
 
                return $session;
@@ -662,7 +658,8 @@ final class SessionManager implements SessionManagerInterface {
         * @return bool Whether the session info matches the stored data (if any)
         */
        private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) {
-               $blob = $this->store->get( wfMemcKey( 'MWSession', $info->getId() ) );
+               $key = wfMemcKey( 'MWSession', $info->getId() );
+               $blob = $this->store->get( $key );
 
                $newParams = array();
 
@@ -670,6 +667,7 @@ final class SessionManager implements SessionManagerInterface {
                        // Sanity check: blob must be an array, if it's saved at all
                        if ( !is_array( $blob ) ) {
                                $this->logger->warning( "Session $info: Bad data" );
+                               $this->store->delete( $key );
                                return false;
                        }
 
@@ -678,6 +676,7 @@ final class SessionManager implements SessionManagerInterface {
                                !isset( $blob['metadata'] ) || !is_array( $blob['metadata'] )
                        ) {
                                $this->logger->warning( "Session $info: Bad data structure" );
+                               $this->store->delete( $key );
                                return false;
                        }
 
@@ -692,6 +691,7 @@ final class SessionManager implements SessionManagerInterface {
                                !array_key_exists( 'provider', $metadata )
                        ) {
                                $this->logger->warning( "Session $info: Bad metadata" );
+                               $this->store->delete( $key );
                                return false;
                        }
 
@@ -701,6 +701,7 @@ final class SessionManager implements SessionManagerInterface {
                                $newParams['provider'] = $provider = $this->getProvider( $metadata['provider'] );
                                if ( !$provider ) {
                                        $this->logger->warning( "Session $info: Unknown provider, " . $metadata['provider'] );
+                                       $this->store->delete( $key );
                                        return false;
                                }
                        } elseif ( $metadata['provider'] !== (string)$provider ) {

diff --git a/includes/session/SessionManagerInterface.php b/includes/session/SessionManagerInterface.php
index 67d6f5d..d58d3b9 100644 (file)
--- a/includes/session/SessionManagerInterface.php
+++ b/includes/session/SessionManagerInterface.php
@@ -67,12 +67,13 @@ interface SessionManagerInterface extends LoggerAwareInterface {
        /**
         * Fetch a session by ID
         * @param string $id
-        * @param bool $noEmpty Don't return an empty session
+        * @param bool $create If no session exists for $id, try to create a new one.
+        *  May still return null if a session for $id exists but cannot be loaded.
         * @param WebRequest|null $request Corresponding request. Any existing
         *  session associated with this WebRequest object will be overwritten.
         * @return Session|null
         */
-       public function getSessionById( $id, $noEmpty = false, WebRequest $request = null );
+       public function getSessionById( $id, $create = false, WebRequest $request = null );
 
        /**
         * Fetch a new, empty session

diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
index c18b821..5a5df6f 100644 (file)
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -285,6 +285,24 @@ class PHPSessionHandlerTest extends MediaWikiTestCase {
                        42 => 'forty-two',
                        'forty-two' => 42,
                ), iterator_to_array( $session ) );
+
+               // Test that write doesn't break if the session is invalid
+               $session = $manager->getEmptySession();
+               $session->persist();
+               session_id( $session->getId() );
+               session_start();
+               $this->mergeMwGlobalArrayValue( 'wgHooks', array(
+                       'SessionCheckInfo' => array( function ( &$reason ) {
+                               $reason = 'Testing';
+                               return false;
+                       } ),
+               ) );
+               $this->assertNull( $manager->getSessionById( $session->getId(), true ), 'sanity check' );
+               session_write_close();
+               $this->mergeMwGlobalArrayValue( 'wgHooks', array(
+                       'SessionCheckInfo' => array(),
+               ) );
+               $this->assertNotNull( $manager->getSessionById( $session->getId(), true ), 'sanity check' );
        }
 
        public static function provideHandlers() {

diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php
index dc217cd..b4687ba 100644 (file)
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -336,39 +336,28 @@ class SessionManagerTest extends MediaWikiTestCase {
 
                // Unknown session ID
                $id = $manager->generateSessionId();
-               $session = $manager->getSessionById( $id );
+               $session = $manager->getSessionById( $id, true );
                $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session );
                $this->assertSame( $id, $session->getId() );
 
                $id = $manager->generateSessionId();
-               $this->assertNull( $manager->getSessionById( $id, true ) );
+               $this->assertNull( $manager->getSessionById( $id, false ) );
 
                // Known but unloadable session ID
                $this->logger->setCollect( true );
                $id = $manager->generateSessionId();
-               $this->store->setRawSession( $id, array( 'metadata' => array(
-                       'provider' => 'DummySessionProvider',
-                       'userId' => 0,
-                       'userName' => null,
-                       'userToken' => null,
+               $this->store->setSession( $id, array( 'metadata' => array(
+                       'userId' => User::idFromName( 'UTSysop' ),
+                       'userToken' => 'bad',
                ) ) );
 
-               try {
-                       $manager->getSessionById( $id );
-                       $this->fail( 'Expected exception not thrown' );
-               } catch ( \UnexpectedValueException $ex ) {
-                       $this->assertSame(
-                               'Can neither load the session nor create an empty session',
-                               $ex->getMessage()
-                       );
-               }
-
                $this->assertNull( $manager->getSessionById( $id, true ) );
+               $this->assertNull( $manager->getSessionById( $id, false ) );
                $this->logger->setCollect( false );
 
                // Known session ID
                $this->store->setSession( $id, array() );
-               $session = $manager->getSessionById( $id );
+               $session = $manager->getSessionById( $id, false );
                $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session );
                $this->assertSame( $id, $session->getId() );
        }
@@ -754,14 +743,14 @@ class SessionManagerTest extends MediaWikiTestCase {
                $sessionId = $session->getSessionId();
                $id = (string)$sessionId;
 
-               $this->assertSame( $sessionId, $manager->getSessionById( $id )->getSessionId() );
+               $this->assertSame( $sessionId, $manager->getSessionById( $id, true )->getSessionId() );
 
                $manager->changeBackendId( $backend );
                $this->assertSame( $sessionId, $session->getSessionId() );
                $this->assertNotEquals( $id, (string)$sessionId );
                $id = (string)$sessionId;
 
-               $this->assertSame( $sessionId, $manager->getSessionById( $id )->getSessionId() );
+               $this->assertSame( $sessionId, $manager->getSessionById( $id, true )->getSessionId() );
 
                // Destruction of the session here causes the backend to be deregistered
                $session = null;
@@ -784,7 +773,7 @@ class SessionManagerTest extends MediaWikiTestCase {
                        );
                }
 
-               $session = $manager->getSessionById( $id );
+               $session = $manager->getSessionById( $id, true );
                $this->assertSame( $sessionId, $session->getSessionId() );
        }