From 43f904b51a746d7f71ea2ab9951c5c98d269765b Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 22 Jan 2016 14:47:33 -0500 Subject: [PATCH] SessionManager: Kill getPersistedSessionId() It's not guaranteed that loadSessionFromStore() will succeed after whatever alterations the SessionProvider might have made later in the request. So instead, let's make a new global object that stores the SessionId of the persistent session that was loaded during Setup.php, if any. Then we can check that when we need to know whether the session was persisted. Bug: T124468 Change-Id: I1e8e616c83b16aadd86b0a0a40826d40f6e8abe4 --- includes/Setup.php | 9 +++++++ includes/WebRequest.php | 4 ++- includes/session/SessionManager.php | 9 ------- includes/session/SessionManagerInterface.php | 15 ----------- includes/specials/SpecialUserlogin.php | 8 +++--- .../includes/session/SessionManagerTest.php | 26 ------------------- 6 files changed, 17 insertions(+), 54 deletions(-) diff --git a/includes/Setup.php b/includes/Setup.php index e962a4923e..85ff3f32f6 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -691,6 +691,11 @@ if ( !is_object( $wgAuth ) ) { // Set up the session $ps_session = Profiler::instance()->scopedProfileIn( $fname . '-session' ); +/** + * @var MediaWiki\\Session\\SessionId|null $wgInitialSessionId The persistent + * session ID (if any) loaded at startup + */ +$wgInitialSessionId = null; if ( !defined( 'MW_NO_SESSION' ) && !$wgCommandLineMode ) { // If session.auto_start is there, we can't touch session name if ( $wgPHPSessionHandling !== 'disable' && !wfIniGetBool( 'session.auto_start' ) ) { @@ -723,6 +728,10 @@ if ( !defined( 'MW_NO_SESSION' ) && !$wgCommandLineMode ) { throw $ex; } + if ( $session->isPersistent() ) { + $wgInitialSessionId = $session->getSessionId(); + } + $session->renew(); if ( MediaWiki\Session\PHPSessionHandler::isEnabled() && ( $session->isPersistent() || $session->shouldRememberUser() ) diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 2c146188a1..4c4ca976de 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -686,8 +686,10 @@ class WebRequest { * @return bool */ public function checkSessionCookie() { + global $wgInitialSessionId; wfDeprecated( __METHOD__, '1.27' ); - return SessionManager::singleton()->getPersistedSessionId( $this ) !== null; + return $wgInitialSessionId !== null && + $this->getSession()->getId() === (string)$wgInitialSessionId; } /** diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php index c4f33d05c3..ecc4e54953 100644 --- a/includes/session/SessionManager.php +++ b/includes/session/SessionManager.php @@ -178,15 +178,6 @@ final class SessionManager implements SessionManagerInterface { $this->logger = $logger; } - public function getPersistedSessionId( WebRequest $request ) { - $info = $this->getSessionInfoForRequest( $request ); - if ( $info && $info->wasPersisted() ) { - return $info->getId(); - } else { - return null; - } - } - public function getSessionForRequest( WebRequest $request ) { $info = $this->getSessionInfoForRequest( $request ); diff --git a/includes/session/SessionManagerInterface.php b/includes/session/SessionManagerInterface.php index d58d3b97ed..14af630bc0 100644 --- a/includes/session/SessionManagerInterface.php +++ b/includes/session/SessionManagerInterface.php @@ -34,21 +34,6 @@ use WebRequest; * @since 1.27 */ interface SessionManagerInterface extends LoggerAwareInterface { - /** - * Fetch the persisted session ID in a request. - * - * Note this is not the same thing as whether the session associated with - * the request is currently persistent, as the session might have been - * first made persistent during this request. - * - * @param WebRequest $request - * @return string|null - * @throws \\OverflowException if there are multiple sessions tied for top - * priority in the request. Exception has a property "sessionInfos" - * holding the SessionInfo objects for the sessions involved. - */ - public function getPersistedSessionId( WebRequest $request ); - /** * Fetch the session for a request * diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 24e167599f..9473dff101 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -1566,10 +1566,12 @@ class LoginForm extends SpecialPage { * @return bool */ function hasSessionCookie() { - global $wgDisableCookieCheck; + global $wgDisableCookieCheck, $wgInitialSessionId; - return $wgDisableCookieCheck || - SessionManager::singleton()->getPersistedSessionId( $this->getRequest() ) !== null; + return $wgDisableCookieCheck || ( + $wgInitialSessionId && + $this->getRequest()->getSession()->getId() === (string)$wgInitialSessionId + ); } /** diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php index b4687ba2a7..083223ebed 100644 --- a/tests/phpunit/includes/session/SessionManagerTest.php +++ b/tests/phpunit/includes/session/SessionManagerTest.php @@ -182,7 +182,6 @@ class SessionManagerTest extends MediaWikiTestCase { $session = $manager->getSessionForRequest( $request ); $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session ); $this->assertSame( $idEmpty, $session->getId() ); - $this->assertNull( $manager->getPersistedSessionId( $request ) ); // Both providers return info, picks best one $request->info1 = new SessionInfo( SessionInfo::MIN_PRIORITY + 1, array( @@ -200,7 +199,6 @@ class SessionManagerTest extends MediaWikiTestCase { $session = $manager->getSessionForRequest( $request ); $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session ); $this->assertSame( $id2, $session->getId() ); - $this->assertSame( $id2, $manager->getPersistedSessionId( $request ) ); $request->info1 = new SessionInfo( SessionInfo::MIN_PRIORITY + 2, array( 'provider' => $provider1, @@ -217,7 +215,6 @@ class SessionManagerTest extends MediaWikiTestCase { $session = $manager->getSessionForRequest( $request ); $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session ); $this->assertSame( $id1, $session->getId() ); - $this->assertSame( $id1, $manager->getPersistedSessionId( $request ) ); // Tied priorities $request->info1 = new SessionInfo( SessionInfo::MAX_PRIORITY, array( @@ -246,18 +243,6 @@ class SessionManagerTest extends MediaWikiTestCase { $this->assertContains( $request->info1, $ex->sessionInfos ); $this->assertContains( $request->info2, $ex->sessionInfos ); } - try { - $manager->getPersistedSessionId( $request ); - $this->fail( 'Expcected exception not thrown' ); - } catch ( \OverFlowException $ex ) { - $this->assertStringStartsWith( - 'Multiple sessions for this request tied for top priority: ', - $ex->getMessage() - ); - $this->assertCount( 2, $ex->sessionInfos ); - $this->assertContains( $request->info1, $ex->sessionInfos ); - $this->assertContains( $request->info2, $ex->sessionInfos ); - } // Bad provider $request->info1 = new SessionInfo( SessionInfo::MAX_PRIORITY, array( @@ -276,15 +261,6 @@ class SessionManagerTest extends MediaWikiTestCase { $ex->getMessage() ); } - try { - $manager->getPersistedSessionId( $request ); - $this->fail( 'Expcected exception not thrown' ); - } catch ( \UnexpectedValueException $ex ) { - $this->assertSame( - 'Provider1 returned session info for a different provider: ' . $request->info1, - $ex->getMessage() - ); - } // Unusable session info $this->logger->setCollect( true ); @@ -304,7 +280,6 @@ class SessionManagerTest extends MediaWikiTestCase { $session = $manager->getSessionForRequest( $request ); $this->assertInstanceOf( 'MediaWiki\\Session\\Session', $session ); $this->assertSame( $id2, $session->getId() ); - $this->assertSame( $id2, $manager->getPersistedSessionId( $request ) ); $this->logger->setCollect( false ); // Unpersisted session ID @@ -321,7 +296,6 @@ class SessionManagerTest extends MediaWikiTestCase { $this->assertSame( $id1, $session->getId() ); $session->persist(); $this->assertTrue( $session->isPersistent(), 'sanity check' ); - $this->assertNull( $manager->getPersistedSessionId( $request ) ); } public function testGetSessionById() { -- 2.20.1