Unpersist the session on logout

Clearing the cookies in this case is probably a good idea.

This also clears cookies when a non-persisted session's metadata is
dirty, for parallelism with what happens to persisted sessions.

Bug: T127436
Change-Id: I76897eaac063e5e3c3563398d0f4cb36cf93783b

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 714ab97..ce5cb96 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -676,6 +676,16 @@ class WebRequest {
                $this->sessionId = $sessionId;
        }
 
+       /**
+        * Get the session id for this request, if any
+        * @since 1.27
+        * @private For use by MediaWiki\\Session classes only
+        * @return MediaWiki\\Session\\SessionId|null
+        */
+       public function getSessionId() {
+               return $this->sessionId;
+       }
+
        /**
         * Returns true if the request has a persistent session.
         * This does not necessarily mean that the user is logged in!

diff --git a/includes/session/Session.php b/includes/session/Session.php
index 96e8d50..21db609 100644 (file)
--- a/includes/session/Session.php
+++ b/includes/session/Session.php
@@ -124,6 +124,13 @@ final class Session implements \Countable, \Iterator, \ArrayAccess {
                $this->backend->persist();
        }
 
+       /**
+        * Make this session not be persisted across requests
+        */
+       public function unpersist() {
+               $this->backend->unpersist();
+       }
+
        /**
         * Indicate whether the user should be remembered independently of the
         * session ID.

diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php
index 0424a2d..1e2b476 100644 (file)
--- a/includes/session/SessionBackend.php
+++ b/includes/session/SessionBackend.php
@@ -283,6 +283,35 @@ final class SessionBackend {
                }
        }
 
+       /**
+        * Make this session not persisted across requests
+        */
+       public function unpersist() {
+               if ( $this->persist ) {
+                       // Close the PHP session, if we're the one that's open
+                       if ( $this->usePhpSessionHandling && PHPSessionHandler::isEnabled() &&
+                               session_id() === (string)$this->id
+                       ) {
+                               $this->logger->debug(
+                                       'SessionBackend "{session}" Closing PHP session for unpersist',
+                                       [ 'session' => $this->id ]
+                               );
+                               session_write_close();
+                               session_id( '' );
+                       }
+
+                       $this->persist = false;
+                       $this->forcePersist = true;
+                       $this->metaDirty = true;
+
+                       // Delete the session data, so the local cache-only write in
+                       // self::save() doesn't get things out of sync with the backend.
+                       $this->store->delete( wfMemcKey( 'MWSession', (string)$this->id ) );
+
+                       $this->autosave();
+               }
+       }
+
        /**
         * Indicate whether the user should be remembered independently of the
         * session ID.
@@ -629,14 +658,22 @@ final class SessionBackend {
                                'forcePersist' => (int)$this->forcePersist,
                ] );
 
-               // Persist to the provider, if flagged
-               if ( $this->persist && ( $this->metaDirty || $this->forcePersist ) ) {
-                       foreach ( $this->requests as $request ) {
-                               $request->setSessionId( $this->getSessionId() );
-                               $this->provider->persistSession( $this, $request );
-                       }
-                       if ( !$closing ) {
-                               $this->checkPHPSession();
+               // Persist or unpersist to the provider, if necessary
+               if ( $this->metaDirty || $this->forcePersist ) {
+                       if ( $this->persist ) {
+                               foreach ( $this->requests as $request ) {
+                                       $request->setSessionId( $this->getSessionId() );
+                                       $this->provider->persistSession( $this, $request );
+                               }
+                               if ( !$closing ) {
+                                       $this->checkPHPSession();
+                               }
+                       } else {
+                               foreach ( $this->requests as $request ) {
+                                       if ( $request->getSessionId() === $this->id ) {
+                                               $this->provider->unpersistSession( $request );
+                                       }
+                               }
                        }
                }
 

diff --git a/includes/user/User.php b/includes/user/User.php
index c92c06b..1f9e882 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3721,6 +3721,7 @@ class User implements IDBAccessObject {
                } else {
                        $this->clearInstanceCache( 'defaults' );
                        $delay = $session->delaySave();
+                       $session->unpersist(); // Clear cookies (T127436)
                        $session->setLoggedOutTimestamp( time() );
                        $session->setUser( new User );
                        $session->set( 'wsUserID', 0 ); // Other code expects this

diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php
index 54ad0f4..61be8e0 100644 (file)
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -63,6 +63,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                $priv = \TestingAccessWrapper::newFromObject( $backend );
                $priv->persist = false;
                $priv->requests = [ 100 => new \FauxRequest() ];
+               $priv->requests[100]->setSessionId( $id );
                $priv->usePhpSessionHandling = false;
 
                $manager = \TestingAccessWrapper::newFromObject( $this->manager );
@@ -309,6 +310,25 @@ class SessionBackendTest extends MediaWikiTestCase {
                $this->assertNotEquals( 0, $wrap->expires );
        }
 
+       public function testUnpersist() {
+               $this->provider = $this->getMock( 'DummySessionProvider', [ 'unpersistSession' ] );
+               $this->provider->expects( $this->once() )->method( 'unpersistSession' );
+               $backend = $this->getBackend();
+               $wrap = \TestingAccessWrapper::newFromObject( $backend );
+               $wrap->store = new \CachedBagOStuff( $this->store );
+               $wrap->persist = true;
+               $wrap->dataDirty = true;
+
+               $backend->save(); // This one shouldn't call $provider->persistSession(), but should save
+               $this->assertTrue( $backend->isPersistent(), 'sanity check' );
+               $this->assertNotFalse( $this->store->getSession( self::SESSIONID ), 'sanity check' );
+
+               $backend->unpersist();
+               $this->assertFalse( $backend->isPersistent() );
+               $this->assertFalse( $this->store->getSession( self::SESSIONID ) );
+               $this->assertNotFalse( $wrap->store->get( wfMemcKey( 'MWSession', self::SESSIONID ) ) );
+       }
+
        public function testRememberUser() {
                $backend = $this->getBackend();
 
@@ -470,8 +490,12 @@ class SessionBackendTest extends MediaWikiTestCase {
                $neverHook = $this->getMock( __CLASS__, [ 'onSessionMetadata' ] );
                $neverHook->expects( $this->never() )->method( 'onSessionMetadata' );
 
-               $neverProvider = $this->getMock( 'DummySessionProvider', [ 'persistSession' ] );
+               $builder = $this->getMockBuilder( 'DummySessionProvider' )
+                       ->setMethods( [ 'persistSession', 'unpersistSession' ] );
+
+               $neverProvider = $builder->getMock();
                $neverProvider->expects( $this->never() )->method( 'persistSession' );
+               $neverProvider->expects( $this->never() )->method( 'unpersistSession' );
 
                // Not persistent or dirty
                $this->provider = $neverProvider;
@@ -485,6 +509,38 @@ class SessionBackendTest extends MediaWikiTestCase {
                $backend->save();
                $this->assertFalse( $this->store->getSession( self::SESSIONID ), 'making sure it didn\'t save' );
 
+               // (but does unpersist if forced)
+               $this->provider = $builder->getMock();
+               $this->provider->expects( $this->never() )->method( 'persistSession' );
+               $this->provider->expects( $this->atLeastOnce() )->method( 'unpersistSession' );
+               $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionMetadata' => [ $neverHook ] ] );
+               $this->store->setSessionData( self::SESSIONID, $testData );
+               $backend = $this->getBackend( $user );
+               $this->store->deleteSession( self::SESSIONID );
+               \TestingAccessWrapper::newFromObject( $backend )->persist = false;
+               \TestingAccessWrapper::newFromObject( $backend )->forcePersist = true;
+               $this->assertFalse( $backend->isPersistent(), 'sanity check' );
+               \TestingAccessWrapper::newFromObject( $backend )->metaDirty = false;
+               \TestingAccessWrapper::newFromObject( $backend )->dataDirty = false;
+               $backend->save();
+               $this->assertFalse( $this->store->getSession( self::SESSIONID ), 'making sure it didn\'t save' );
+
+               // (but not to a WebRequest associated with a different session)
+               $this->provider = $neverProvider;
+               $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionMetadata' => [ $neverHook ] ] );
+               $this->store->setSessionData( self::SESSIONID, $testData );
+               $backend = $this->getBackend( $user );
+               \TestingAccessWrapper::newFromObject( $backend )->requests[100]
+                       ->setSessionId( new SessionId( 'x' ) );
+               $this->store->deleteSession( self::SESSIONID );
+               \TestingAccessWrapper::newFromObject( $backend )->persist = false;
+               \TestingAccessWrapper::newFromObject( $backend )->forcePersist = true;
+               $this->assertFalse( $backend->isPersistent(), 'sanity check' );
+               \TestingAccessWrapper::newFromObject( $backend )->metaDirty = false;
+               \TestingAccessWrapper::newFromObject( $backend )->dataDirty = false;
+               $backend->save();
+               $this->assertFalse( $this->store->getSession( self::SESSIONID ), 'making sure it didn\'t save' );
+
                // Not persistent, but dirty
                $this->provider = $neverProvider;
                $this->onSessionMetadataCalled = false;
@@ -520,8 +576,10 @@ class SessionBackendTest extends MediaWikiTestCase {
                $backend->save();
                $this->assertFalse( $this->store->getSession( self::SESSIONID ), 'making sure it didn\'t save' );
 
-               $this->provider = $this->getMock( 'DummySessionProvider', [ 'persistSession' ] );
+               // (but will persist if forced)
+               $this->provider = $builder->getMock();
                $this->provider->expects( $this->atLeastOnce() )->method( 'persistSession' );
+               $this->provider->expects( $this->never() )->method( 'unpersistSession' );
                $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionMetadata' => [ $neverHook ] ] );
                $this->store->setSessionData( self::SESSIONID, $testData );
                $backend = $this->getBackend( $user );
@@ -557,8 +615,10 @@ class SessionBackendTest extends MediaWikiTestCase {
                $this->assertNotSame( false, $this->store->getSessionFromBackend( self::SESSIONID ),
                        'making sure it did save to backend' );
 
-               $this->provider = $this->getMock( 'DummySessionProvider', [ 'persistSession' ] );
+               // (also persists if forced)
+               $this->provider = $builder->getMock();
                $this->provider->expects( $this->atLeastOnce() )->method( 'persistSession' );
+               $this->provider->expects( $this->never() )->method( 'unpersistSession' );
                $this->onSessionMetadataCalled = false;
                $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionMetadata' => [ $this ] ] );
                $this->store->setSessionData( self::SESSIONID, $testData );
@@ -581,8 +641,10 @@ class SessionBackendTest extends MediaWikiTestCase {
                $this->assertNotSame( false, $this->store->getSessionFromBackend( self::SESSIONID ),
                        'making sure it did save to backend' );
 
-               $this->provider = $this->getMock( 'DummySessionProvider', [ 'persistSession' ] );
+               // (also persists if metadata dirty)
+               $this->provider = $builder->getMock();
                $this->provider->expects( $this->atLeastOnce() )->method( 'persistSession' );
+               $this->provider->expects( $this->never() )->method( 'unpersistSession' );
                $this->onSessionMetadataCalled = false;
                $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionMetadata' => [ $this ] ] );
                $this->store->setSessionData( self::SESSIONID, $testData );
@@ -790,6 +852,44 @@ class SessionBackendTest extends MediaWikiTestCase {
                session_write_close();
        }
 
+       public function testUnpersistOfGlobalSession() {
+               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' ) );
+               $wrap = \TestingAccessWrapper::newFromObject( $backend );
+               $wrap->usePhpSessionHandling = true;
+               $wrap->persist = true;
+
+               TestUtils::setSessionManagerSingleton( $this->manager );
+
+               $manager = \TestingAccessWrapper::newFromObject( $this->manager );
+               $request = \RequestContext::getMain()->getRequest();
+               $manager->globalSession = $backend->getSession( $request );
+               $manager->globalSessionRequest = $request;
+
+               session_id( self::SESSIONID . 'x' );
+               \MediaWiki\quietCall( 'session_start' );
+               $backend->unpersist();
+               $this->assertSame( self::SESSIONID . 'x', session_id() );
+
+               session_id( self::SESSIONID );
+               $wrap->persist = true;
+               $backend->unpersist();
+               $this->assertSame( '', session_id() );
+       }
+
        public function testGetAllowedUserRights() {
                $this->provider = $this->getMockBuilder( 'DummySessionProvider' )
                        ->setMethods( [ 'getAllowedUserRights' ] )

diff --git a/tests/phpunit/includes/session/SessionTest.php b/tests/phpunit/includes/session/SessionTest.php
index d0ebc9c..a4727c4 100644 (file)
--- a/tests/phpunit/includes/session/SessionTest.php
+++ b/tests/phpunit/includes/session/SessionTest.php
@@ -75,6 +75,7 @@ class SessionTest extends MediaWikiTestCase {
                        [ 'getProvider', [], false, true ],
                        [ 'isPersistent', [], false, true ],
                        [ 'persist', [], false, false ],
+                       [ 'unpersist', [], false, false ],
                        [ 'shouldRememberUser', [], false, true ],
                        [ 'setRememberUser', [ true ], false, false ],
                        [ 'getRequest', [], true, true ],