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

diff --git a/includes/Setup.php b/includes/Setup.php
index e962a49..85ff3f3 100644 (file)
--- 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' );
 
 // 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' ) ) {
 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;
        }
 
                throw $ex;
        }
 

+       if ( $session->isPersistent() ) {
+               $wgInitialSessionId = $session->getSessionId();
+       }
+

        $session->renew();
        if ( MediaWiki\Session\PHPSessionHandler::isEnabled() &&
                ( $session->isPersistent() || $session->shouldRememberUser() )
        $session->renew();
        if ( MediaWiki\Session\PHPSessionHandler::isEnabled() &&
                ( $session->isPersistent() || $session->shouldRememberUser() )

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 2c14618..4c4ca97 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -686,8 +686,10 @@ class WebRequest {
         * @return bool
         */
        public function checkSessionCookie() {
         * @return bool
         */
        public function checkSessionCookie() {

+               global $wgInitialSessionId;

                wfDeprecated( __METHOD__, '1.27' );
                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 c4f33d0..ecc4e54 100644 (file)
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -178,15 +178,6 @@ final class SessionManager implements SessionManagerInterface {
                $this->logger = $logger;
        }
 
                $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 );
 
        public function getSessionForRequest( WebRequest $request ) {
                $info = $this->getSessionInfoForRequest( $request );
 

diff --git a/includes/session/SessionManagerInterface.php b/includes/session/SessionManagerInterface.php
index d58d3b9..14af630 100644 (file)
--- a/includes/session/SessionManagerInterface.php
+++ b/includes/session/SessionManagerInterface.php
@@ -34,21 +34,6 @@ use WebRequest;
  * @since 1.27
  */
 interface SessionManagerInterface extends LoggerAwareInterface {
  * @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
         *
        /**
         * Fetch the session for a request
         *

diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php
index 24e1675..9473dff 100644 (file)
--- a/includes/specials/SpecialUserlogin.php
+++ b/includes/specials/SpecialUserlogin.php
@@ -1566,10 +1566,12 @@ class LoginForm extends SpecialPage {
         * @return bool
         */
        function hasSessionCookie() {
         * @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 b4687ba..083223e 100644 (file)
--- 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() );
                $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(
 
                // 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() );
                $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,
 
                $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() );
                $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(
 
                // 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 );
                }
                        $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(
 
                // Bad provider
                $request->info1 = new SessionInfo( SessionInfo::MAX_PRIORITY, array(


@@ -276,15 +261,6 @@ class SessionManagerTest extends MediaWikiTestCase {
                                $ex->getMessage()
                        );
                }
                                $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 );
 
                // 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() );
                $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
                $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->assertSame( $id1, $session->getId() );
                $session->persist();
                $this->assertTrue( $session->isPersistent(), 'sanity check' );

-               $this->assertNull( $manager->getPersistedSessionId( $request ) );

        }
 
        public function testGetSessionById() {
        }
 
        public function testGetSessionById() {