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 e962a4923e8..85ff3f32f66 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 2c146188a17..4c4ca976def 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 c4f33d05c35..ecc4e54953a 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 d58d3b97ed7..14af630bc0e 100644 --- a/includes/session/SessionManagerInterface.php +++ b/includes/session/SessionManagerInterface.php @@ -35,21 +35,6 @@ use WebRequest; */ 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 * * @note You probably want to use $request->getSession() instead. It's more diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 24e167599f2..9473dff1016 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 b4687ba2a75..083223ebed1 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.11.4.GIT