SessionManager: Use existing backend for the ID if one is loaded

This fixes a bug where SessionBackend::resetId() of the PHP session will
fail to properly load $_SESSION because the new session ID hasn't been
saved to the store yet. It's also a reasonable performance improvement,
no need to call loadSessionInfoFromStore() when we already have the
session loaded.

Change-Id: I30f159ef1267442a6325aabbbdfaf69defc10ed6

diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php
index 0a304a9..da7bc57 100644 (file)
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -197,13 +197,17 @@ final class SessionManager implements SessionManagerInterface {
                }
 
                $session = null;
+               $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [ 'id' => $id, 'idIsSafe' => true ] );
 
-               // Test this here to provide a better log message for the common case
-               // of "no such ID"
+               // If we already have the backend loaded, use it directly
+               if ( isset( $this->allSessionBackends[$id] ) ) {
+                       return $this->getSessionFromInfo( $info, $request );
+               }
+
+               // Test if the session is in storage, and if so try to load it.
                $key = wfMemcKey( 'MWSession', $id );
                if ( is_array( $this->store->get( $key ) ) ) {
-                       $create = false;
-                       $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [ 'id' => $id, 'idIsSafe' => true ] );
+                       $create = false; // If loading fails, don't bother creating because it probably will fail too.
                        if ( $this->loadSessionInfoFromStore( $info, $request ) ) {
                                $session = $this->getSessionFromInfo( $info, $request );
                        }

diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
index 64b16db..05773a9 100644 (file)
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -292,7 +292,9 @@ class PHPSessionHandlerTest extends MediaWikiTestCase {
                // Test that write doesn't break if the session is invalid
                $session = $manager->getEmptySession();
                $session->persist();
-               session_id( $session->getId() );
+               $id = $session->getId();
+               unset( $session );
+               session_id( $id );
                session_start();
                $this->mergeMwGlobalArrayValue( 'wgHooks', [
                        'SessionCheckInfo' => [ function ( &$reason ) {
@@ -300,12 +302,13 @@ class PHPSessionHandlerTest extends MediaWikiTestCase {
                                return false;
                        } ],
                ] );
-               $this->assertNull( $manager->getSessionById( $session->getId(), true ), 'sanity check' );
+               $this->assertNull( $manager->getSessionById( $id, true ), 'sanity check' );
                session_write_close();
+
                $this->mergeMwGlobalArrayValue( 'wgHooks', [
                        'SessionCheckInfo' => [],
                ] );
-               $this->assertNotNull( $manager->getSessionById( $session->getId(), true ), 'sanity check' );
+               $this->assertNotNull( $manager->getSessionById( $id, true ), 'sanity check' );
        }
 
        public static function provideHandlers() {

diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php
index 61be8e0..7459ed2 100644 (file)
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -23,8 +23,9 @@ class SessionBackendTest extends MediaWikiTestCase {
        /**
         * Returns a non-persistent backend that thinks it has at least one session active
         * @param User|null $user
+        * @param string $id
         */
-       protected function getBackend( User $user = null ) {
+       protected function getBackend( User $user = null, $id = null ) {
                if ( !$this->config ) {
                        $this->config = new \HashConfig();
                        $this->manager = null;
@@ -52,7 +53,7 @@ class SessionBackendTest extends MediaWikiTestCase {
 
                $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [
                        'provider' => $this->provider,
-                       'id' => self::SESSIONID,
+                       'id' => $id ?: self::SESSIONID,
                        'persisted' => true,
                        'userInfo' => UserInfo::newFromUser( $user ?: new User, true ),
                        'idIsSafe' => true,
@@ -67,8 +68,8 @@ class SessionBackendTest extends MediaWikiTestCase {
                $priv->usePhpSessionHandling = false;
 
                $manager = \TestingAccessWrapper::newFromObject( $this->manager );
-               $manager->allSessionBackends = [ $backend->getId() => $backend ];
-               $manager->allSessionIds = [ $backend->getId() => $id ];
+               $manager->allSessionBackends = [ $backend->getId() => $backend ] + $manager->allSessionBackends;
+               $manager->allSessionIds = [ $backend->getId() => $id ] + $manager->allSessionIds;
                $manager->sessionProviders = [ (string)$this->provider => $this->provider ];
 
                return $backend;
@@ -813,6 +814,46 @@ class SessionBackendTest extends MediaWikiTestCase {
                $metadata['???'] = '!!!';
        }
 
+       public function testTakeOverGlobalSession() {
+               if ( !PHPSessionHandler::isInstalled() ) {
+                       PHPSessionHandler::install( SessionManager::singleton() );
+               }
+               if ( !PHPSessionHandler::isEnabled() ) {
+                       $rProp = new \ReflectionProperty( 'MediaWiki\\Session\\PHPSessionHandler', 'instance' );
+                       $rProp->setAccessible( true );
+                       $handler = \TestingAccessWrapper::newFromObject( $rProp->getValue() );
+                       $resetHandler = new \ScopedCallback( function () use ( $handler ) {
+                               session_write_close();
+                               $handler->enable = false;
+                       } );
+                       $handler->enable = true;
+               }
+
+               $backend = $this->getBackend( User::newFromName( 'UTSysop' ) );
+               \TestingAccessWrapper::newFromObject( $backend )->usePhpSessionHandling = true;
+
+               $resetSingleton = TestUtils::setSessionManagerSingleton( $this->manager );
+
+               $manager = \TestingAccessWrapper::newFromObject( $this->manager );
+               $request = \RequestContext::getMain()->getRequest();
+               $manager->globalSession = $backend->getSession( $request );
+               $manager->globalSessionRequest = $request;
+
+               session_id( '' );
+               \TestingAccessWrapper::newFromObject( $backend )->checkPHPSession();
+               $this->assertSame( $backend->getId(), session_id() );
+               session_write_close();
+
+               $backend2 = $this->getBackend(
+                       User::newFromName( 'UTSysop' ), 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
+               );
+               \TestingAccessWrapper::newFromObject( $backend2 )->usePhpSessionHandling = true;
+
+               session_id( '' );
+               \TestingAccessWrapper::newFromObject( $backend2 )->checkPHPSession();
+               $this->assertSame( '', session_id() );
+       }
+
        public function testResetIdOfGlobalSession() {
                if ( !PHPSessionHandler::isInstalled() ) {
                        PHPSessionHandler::install( SessionManager::singleton() );
@@ -831,7 +872,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                $backend = $this->getBackend( User::newFromName( 'UTSysop' ) );
                \TestingAccessWrapper::newFromObject( $backend )->usePhpSessionHandling = true;
 
-               TestUtils::setSessionManagerSingleton( $this->manager );
+               $resetSingleton = TestUtils::setSessionManagerSingleton( $this->manager );
 
                $manager = \TestingAccessWrapper::newFromObject( $this->manager );
                $request = \RequestContext::getMain()->getRequest();
@@ -840,15 +881,12 @@ class SessionBackendTest extends MediaWikiTestCase {
 
                session_id( self::SESSIONID );
                \MediaWiki\quietCall( 'session_start' );
+               $_SESSION['foo'] = __METHOD__;
                $backend->resetId();
                $this->assertNotEquals( self::SESSIONID, $backend->getId() );
                $this->assertSame( $backend->getId(), session_id() );
-               session_write_close();
-
-               session_id( '' );
-               $this->assertNotSame( $backend->getId(), session_id(), 'sanity check' );
-               $backend->persist();
-               $this->assertSame( $backend->getId(), session_id() );
+               $this->assertArrayHasKey( 'foo', $_SESSION );
+               $this->assertSame( __METHOD__, $_SESSION['foo'] );
                session_write_close();
        }
 
@@ -872,7 +910,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                $wrap->usePhpSessionHandling = true;
                $wrap->persist = true;
 
-               TestUtils::setSessionManagerSingleton( $this->manager );
+               $resetSingleton = TestUtils::setSessionManagerSingleton( $this->manager );
 
                $manager = \TestingAccessWrapper::newFromObject( $this->manager );
                $request = \RequestContext::getMain()->getRequest();

diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php
index cf59ced..b0f84fc 100644 (file)
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -381,6 +381,40 @@ class SessionManagerTest extends MediaWikiTestCase {
                $session = $manager->getSessionById( $id, false );
                $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session );
                $this->assertSame( $id, $session->getId() );
+
+               // Store isn't checked if the session is already loaded
+               $this->store->setSession( $id, [ 'metadata' => [
+                       'userId' => User::idFromName( 'UTSysop' ),
+                       'userToken' => 'bad',
+               ] ] );
+               $session2 = $manager->getSessionById( $id, false );
+               $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session2 );
+               $this->assertSame( $id, $session2->getId() );
+               unset( $session, $session2 );
+               $this->logger->setCollect( true );
+               $this->assertNull( $manager->getSessionById( $id, true ) );
+               $this->logger->setCollect( false );
+
+               // Failure to create an empty session
+               $manager = $this->getManager();
+               $provider = $this->getMockBuilder( 'DummySessionProvider' )
+                       ->setMethods( [ 'provideSessionInfo', 'newSessionInfo', '__toString' ] )
+                       ->getMock();
+               $provider->expects( $this->any() )->method( 'provideSessionInfo' )
+                       ->will( $this->returnValue( null ) );
+               $provider->expects( $this->any() )->method( 'newSessionInfo' )
+                       ->will( $this->returnValue( null ) );
+               $provider->expects( $this->any() )->method( '__toString' )
+                       ->will( $this->returnValue( 'MockProvider' ) );
+               $this->config->set( 'SessionProviders', [
+                       $this->objectCacheDef( $provider ),
+               ] );
+               $this->logger->setCollect( true );
+               $this->assertNull( $manager->getSessionById( $id, true ) );
+               $this->logger->setCollect( false );
+               $this->assertSame( [
+                       [ LogLevel::ERROR, 'Failed to create empty session: {exception}' ]
+               ], $this->logger->getBuffer() );
        }
 
        public function testGetEmptySession() {