From 6d4436c9156acba552db4c70d6cc129f6d0ce6ef Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 26 Feb 2016 16:17:37 -0500 Subject: [PATCH] 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 --- includes/WebRequest.php | 10 ++ includes/session/Session.php | 7 ++ includes/session/SessionBackend.php | 53 +++++++-- includes/user/User.php | 1 + .../includes/session/SessionBackendTest.php | 108 +++++++++++++++++- .../phpunit/includes/session/SessionTest.php | 1 + 6 files changed, 168 insertions(+), 12 deletions(-) diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 714ab97b03..ce5cb96190 100644 --- 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 96e8d50445..21db609090 100644 --- 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 0424a2da07..1e2b476baf 100644 --- 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 c92c06b0b1..1f9e882046 100644 --- 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 54ad0f4f8b..61be8e07c1 100644 --- 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 d0ebc9c76e..a4727c4364 100644 --- 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 ], -- 2.20.1