From deb46ebfd7542ff820d4028c3644f13ef167f495 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 22 Mar 2016 17:50:32 -0400 Subject: [PATCH] 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 --- includes/session/SessionManager.php | 12 ++-- .../session/PHPSessionHandlerTest.php | 9 ++- .../includes/session/SessionBackendTest.php | 62 +++++++++++++++---- .../includes/session/SessionManagerTest.php | 34 ++++++++++ 4 files changed, 98 insertions(+), 19 deletions(-) diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php index 0a304a99b2..da7bc57334 100644 --- 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 64b16db844..05773a9a5c 100644 --- 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 61be8e07c1..7459ed2528 100644 --- 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 cf59ced36b..b0f84fcd65 100644 --- 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() { -- 2.20.1