From fcdd643a46d87b677f6cdcc3ba9440e1472d8df7 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 29 Jan 2016 18:02:11 -0500 Subject: [PATCH] SessionManager: Don't save non-persisted sessions to backend storage This introduces an in-process cache (using a HashBagOStuff) for session data, and only saves to the external cache when the session is persisted. Bug: T125267 Change-Id: Ie161e0f7522cd68515b060ad8cf8c151b7198b0b --- includes/session/SessionBackend.php | 37 +++++++++--- includes/session/SessionManager.php | 59 ++++++++++++++----- .../session/CookieSessionProviderTest.php | 3 + ...ImmutableSessionProviderWithCookieTest.php | 1 + .../session/PHPSessionHandlerTest.php | 8 --- .../includes/session/SessionBackendTest.php | 12 ++-- .../includes/session/SessionManagerTest.php | 10 +++- 7 files changed, 93 insertions(+), 37 deletions(-) diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php index f86daaadb9..2a13ed20ef 100644 --- a/includes/session/SessionBackend.php +++ b/includes/session/SessionBackend.php @@ -65,7 +65,9 @@ final class SessionBackend { private $dataHash = null; /** @var BagOStuff */ - private $store; + private $tempStore; + /** @var BagOStuff */ + private $permStore; /** @var LoggerInterface */ private $logger; @@ -97,12 +99,14 @@ final class SessionBackend { /** * @param SessionId $id Session ID object * @param SessionInfo $info Session info to populate from - * @param BagOStuff $store Backend data store + * @param BagOStuff $tempStore In-process data store + * @param BagOStuff $permstore Backend data store for persisted sessions * @param LoggerInterface $logger * @param int $lifetime Session data lifetime in seconds */ public function __construct( - SessionId $id, SessionInfo $info, BagOStuff $store, LoggerInterface $logger, $lifetime + SessionId $id, SessionInfo $info, BagOStuff $tempStore, BagOStuff $permStore, + LoggerInterface $logger, $lifetime ) { $phpSessionHandling = \RequestContext::getMain()->getConfig()->get( 'PHPSessionHandling' ); $this->usePhpSessionHandling = $phpSessionHandling !== 'disable'; @@ -121,7 +125,8 @@ final class SessionBackend { $this->id = $id; $this->user = $info->getUserInfo() ? $info->getUserInfo()->getUser() : new User; - $this->store = $store; + $this->tempStore = $tempStore; + $this->permStore = $permStore; $this->logger = $logger; $this->lifetime = $lifetime; $this->provider = $info->getProvider(); @@ -130,7 +135,14 @@ final class SessionBackend { $this->forceHTTPS = $info->forceHTTPS(); $this->providerMetadata = $info->getProviderMetadata(); - $blob = $store->get( wfMemcKey( 'MWSession', (string)$this->id ) ); + $key = wfMemcKey( 'MWSession', (string)$this->id ); + $blob = $tempStore->get( $key ); + if ( $blob === false ) { + $blob = $permStore->get( $key ); + if ( $blob !== false ) { + $tempStore->set( $key, $blob ); + } + } if ( !is_array( $blob ) || !isset( $blob['metadata'] ) || !is_array( $blob['metadata'] ) || !isset( $blob['data'] ) || !is_array( $blob['data'] ) @@ -229,7 +241,8 @@ final class SessionBackend { $this->autosave(); // Delete the data for the old session ID now - $this->store->delete( wfMemcKey( 'MWSession', $oldId ) ); + $this->tempStore->delete( wfMemcKey( 'MWSession', $oldId ) ); + $this->permStore->delete( wfMemcKey( 'MWSession', $oldId ) ); } } @@ -613,7 +626,7 @@ final class SessionBackend { } } - $this->store->set( + $this->tempStore->set( wfMemcKey( 'MWSession', (string)$this->id ), array( 'data' => $this->data, @@ -621,6 +634,16 @@ final class SessionBackend { ), $metadata['expires'] ); + if ( $this->persist ) { + $this->permStore->set( + wfMemcKey( 'MWSession', (string)$this->id ), + array( + 'data' => $this->data, + 'metadata' => $metadata, + ), + $metadata['expires'] + ); + } $this->metaDirty = false; $this->dataDirty = false; diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php index 06a765ca3a..0441137083 100644 --- a/includes/session/SessionManager.php +++ b/includes/session/SessionManager.php @@ -55,7 +55,10 @@ final class SessionManager implements SessionManagerInterface { private $config; /** @var BagOStuff|null */ - private $store; + private $tempStore; + + /** @var BagOStuff|null */ + private $permStore; /** @var SessionProvider[] */ private $sessionProviders = null; @@ -159,16 +162,17 @@ final class SessionManager implements SessionManagerInterface { $this->setLogger( \MediaWiki\Logger\LoggerFactory::getInstance( 'session' ) ); } + $this->tempStore = new \HashBagOStuff; if ( isset( $options['store'] ) ) { if ( !$options['store'] instanceof BagOStuff ) { throw new \InvalidArgumentException( '$options[\'store\'] must be an instance of BagOStuff' ); } - $this->store = $options['store']; + $this->permStore = $options['store']; } else { - $this->store = \ObjectCache::getInstance( $this->config->get( 'SessionCacheType' ) ); - $this->store->setLogger( $this->logger ); + $this->permStore = \ObjectCache::getInstance( $this->config->get( 'SessionCacheType' ) ); + $this->permStore->setLogger( $this->logger ); } register_shutdown_function( array( $this, 'shutdown' ) ); @@ -202,7 +206,14 @@ final class SessionManager implements SessionManagerInterface { // Test this here to provide a better log message for the common case // of "no such ID" $key = wfMemcKey( 'MWSession', $id ); - if ( is_array( $this->store->get( $key ) ) ) { + $existing = $this->tempStore->get( $key ); + if ( $existing === false ) { + $existing = $this->permStore->get( $key ); + if ( $existing !== false ) { + $this->tempStore->set( $key, $existing ); + } + } + if ( is_array( $existing ) ) { $info = new SessionInfo( SessionInfo::MIN_PRIORITY, array( 'id' => $id, 'idIsSafe' => true ) ); if ( $this->loadSessionInfoFromStore( $info, $request ) ) { $session = $this->getSessionFromInfo( $info, $request ); @@ -240,7 +251,14 @@ final class SessionManager implements SessionManagerInterface { } $key = wfMemcKey( 'MWSession', $id ); - if ( is_array( $this->store->get( $key ) ) ) { + $existing = $this->tempStore->get( $key ); + if ( $existing === false ) { + $existing = $this->permStore->get( $key ); + if ( $existing !== false ) { + $this->tempStore->set( $key, $existing ); + } + } + if ( is_array( $existing ) ) { throw new \InvalidArgumentException( 'Session ID already exists' ); } } @@ -660,7 +678,13 @@ final class SessionManager implements SessionManagerInterface { */ private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) { $key = wfMemcKey( 'MWSession', $info->getId() ); - $blob = $this->store->get( $key ); + $blob = $this->tempStore->get( $key ); + if ( $blob === false ) { + $blob = $this->permStore->get( $key ); + if ( $blob !== false ) { + $this->tempStore->set( $key, $blob ); + } + } $newParams = array(); @@ -668,7 +692,8 @@ 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 ); + $this->tempStore->delete( $key ); + $this->permStore->delete( $key ); return false; } @@ -677,7 +702,8 @@ final class SessionManager implements SessionManagerInterface { !isset( $blob['metadata'] ) || !is_array( $blob['metadata'] ) ) { $this->logger->warning( "Session $info: Bad data structure" ); - $this->store->delete( $key ); + $this->tempStore->delete( $key ); + $this->permStore->delete( $key ); return false; } @@ -692,7 +718,8 @@ final class SessionManager implements SessionManagerInterface { !array_key_exists( 'provider', $metadata ) ) { $this->logger->warning( "Session $info: Bad metadata" ); - $this->store->delete( $key ); + $this->tempStore->delete( $key ); + $this->permStore->delete( $key ); return false; } @@ -702,7 +729,8 @@ 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 ); + $this->tempStore->delete( $key ); + $this->permStore->delete( $key ); return false; } } elseif ( $metadata['provider'] !== (string)$provider ) { @@ -893,7 +921,8 @@ final class SessionManager implements SessionManagerInterface { $backend = new SessionBackend( $this->allSessionIds[$id], $info, - $this->store, + $this->tempStore, + $this->permStore, $this->logger, $this->config->get( 'ObjectCacheSessionExpiry' ) ); @@ -970,7 +999,9 @@ final class SessionManager implements SessionManagerInterface { do { $id = wfBaseConvert( \MWCryptRand::generateHex( 40 ), 16, 32, 32 ); $key = wfMemcKey( 'MWSession', $id ); - } while ( isset( $this->allSessionIds[$id] ) || is_array( $this->store->get( $key ) ) ); + } while ( isset( $this->allSessionIds[$id] ) || + is_array( $this->tempStore->get( $key ) ) || is_array( $this->permStore->get( $key ) ) + ); return $id; } @@ -980,7 +1011,7 @@ final class SessionManager implements SessionManagerInterface { * @param PHPSessionHandler $handler */ public function setupPHPSessionHandler( PHPSessionHandler $handler ) { - $handler->setManager( $this, $this->store, $this->logger ); + $handler->setManager( $this, $this->permStore, $this->logger ); } /** diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php index 702f556311..2b7e0a1372 100644 --- a/tests/phpunit/includes/session/CookieSessionProviderTest.php +++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php @@ -363,6 +363,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ), $store, + $store, new \Psr\Log\NullLogger(), 10 ); @@ -449,6 +450,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ), new \EmptyBagOStuff(), + new \EmptyBagOStuff(), new \Psr\Log\NullLogger(), 10 ); @@ -553,6 +555,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ), $store, + $store, new \Psr\Log\NullLogger(), 10 ); diff --git a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php index e06dfd5555..46f23f3841 100644 --- a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php +++ b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php @@ -205,6 +205,7 @@ class ImmutableSessionProviderWithCookieTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ), new \EmptyBagOStuff(), + new \EmptyBagOStuff(), new \Psr\Log\NullLogger(), 10 ); diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php b/tests/phpunit/includes/session/PHPSessionHandlerTest.php index 125e1b664e..1c54a20af3 100644 --- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php +++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php @@ -172,14 +172,6 @@ class PHPSessionHandlerTest extends MediaWikiTestCase { $this->assertSame( $expect, $_SESSION ); } - // Test expiry - session_write_close(); - ini_set( 'session.gc_divisor', 1 ); - ini_set( 'session.gc_probability', 1 ); - sleep( 3 ); - session_start(); - $this->assertSame( array(), $_SESSION ); - // Re-fill the session, then test that session_destroy() works. $_SESSION['AuthenticationSessionTest'] = $rand; session_write_close(); diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php index d06706bf63..85fa9bdca0 100644 --- a/tests/phpunit/includes/session/SessionBackendTest.php +++ b/tests/phpunit/includes/session/SessionBackendTest.php @@ -59,7 +59,7 @@ class SessionBackendTest extends MediaWikiTestCase { ) ); $id = new SessionId( $info->getId() ); - $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 ); + $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $priv = \TestingAccessWrapper::newFromObject( $backend ); $priv->persist = false; $priv->requests = array( 100 => new \FauxRequest() ); @@ -87,7 +87,7 @@ class SessionBackendTest extends MediaWikiTestCase { $id = new SessionId( $info->getId() ); $logger = new \Psr\Log\NullLogger(); try { - new SessionBackend( $id, $info, $this->store, $logger, 10 ); + new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $this->fail( 'Expected exception not thrown' ); } catch ( \InvalidArgumentException $ex ) { $this->assertSame( @@ -103,7 +103,7 @@ class SessionBackendTest extends MediaWikiTestCase { ) ); $id = new SessionId( $info->getId() ); try { - new SessionBackend( $id, $info, $this->store, $logger, 10 ); + new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $this->fail( 'Expected exception not thrown' ); } catch ( \InvalidArgumentException $ex ) { $this->assertSame( 'Cannot create session without a provider', $ex->getMessage() ); @@ -118,7 +118,7 @@ class SessionBackendTest extends MediaWikiTestCase { ) ); $id = new SessionId( '!' . $info->getId() ); try { - new SessionBackend( $id, $info, $this->store, $logger, 10 ); + new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $this->fail( 'Expected exception not thrown' ); } catch ( \InvalidArgumentException $ex ) { $this->assertSame( @@ -135,7 +135,7 @@ class SessionBackendTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ); $id = new SessionId( $info->getId() ); - $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 ); + $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $this->assertSame( self::SESSIONID, $backend->getId() ); $this->assertSame( $id, $backend->getSessionId() ); $this->assertSame( $this->provider, $backend->getProvider() ); @@ -157,7 +157,7 @@ class SessionBackendTest extends MediaWikiTestCase { 'idIsSafe' => true, ) ); $id = new SessionId( $info->getId() ); - $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 ); + $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 ); $this->assertSame( self::SESSIONID, $backend->getId() ); $this->assertSame( $id, $backend->getSessionId() ); $this->assertSame( $this->provider, $backend->getProvider() ); diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php index f5bb07d1db..4fde3410f9 100644 --- a/tests/phpunit/includes/session/SessionManagerTest.php +++ b/tests/phpunit/includes/session/SessionManagerTest.php @@ -103,7 +103,7 @@ class SessionManagerTest extends MediaWikiTestCase { $manager = \TestingAccessWrapper::newFromObject( $this->getManager() ); $this->assertSame( $this->config, $manager->config ); $this->assertSame( $this->logger, $manager->logger ); - $this->assertSame( $this->store, $manager->store ); + $this->assertSame( $this->store, $manager->permStore ); $manager = \TestingAccessWrapper::newFromObject( new SessionManager() ); $this->assertSame( \RequestContext::getMain()->getConfig(), $manager->config ); @@ -111,7 +111,7 @@ class SessionManagerTest extends MediaWikiTestCase { $manager = \TestingAccessWrapper::newFromObject( new SessionManager( array( 'config' => $this->config, ) ) ); - $this->assertSame( \ObjectCache::$instances['testSessionStore'], $manager->store ); + $this->assertSame( \ObjectCache::$instances['testSessionStore'], $manager->permStore ); foreach ( array( 'config' => '$options[\'config\'] must be an instance of Config', @@ -301,6 +301,9 @@ class SessionManagerTest extends MediaWikiTestCase { public function testGetSessionById() { $manager = $this->getManager(); + // Disable the in-process cache so our $this->store->setSession() takes effect. + \TestingAccessWrapper::newFromObject( $manager )->tempStore = new \EmptyBagOStuff; + try { $manager->getSessionById( 'bad' ); $this->fail( 'Expected exception not thrown' ); @@ -1083,6 +1086,9 @@ class SessionManagerTest extends MediaWikiTestCase { $manager->setLogger( $logger ); $request = new \FauxRequest(); + // Disable the in-process cache so our $this->store->setSession() takes effect. + \TestingAccessWrapper::newFromObject( $manager )->tempStore = new \EmptyBagOStuff; + // TestingAccessWrapper can't handle methods with reference arguments, sigh. $rClass = new \ReflectionClass( $manager ); $rMethod = $rClass->getMethod( 'loadSessionInfoFromStore' ); -- 2.20.1