Anomie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/279062
Change subject: SessionManager: Use existing backend for the ID if one is loaded
......................................................................
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
---
M includes/session/SessionManager.php
M tests/phpunit/includes/session/PHPSessionHandlerTest.php
M tests/phpunit/includes/session/SessionBackendTest.php
M tests/phpunit/includes/session/SessionManagerTest.php
4 files changed, 98 insertions(+), 19 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/62/279062/1
diff --git a/includes/session/SessionManager.php
b/includes/session/SessionManager.php
index 0a304a9..da7bc57 100644
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -197,13 +197,17 @@
}
$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
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -292,7 +292,9 @@
// 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 @@
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
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -23,8 +23,9 @@
/**
* 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 @@
$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 @@
$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 @@
$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 @@
$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 @@
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 @@
$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
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -381,6 +381,40 @@
$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() {
--
To view, visit https://gerrit.wikimedia.org/r/279062
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I30f159ef1267442a6325aabbbdfaf69defc10ed6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
