jenkins-bot has submitted this change and it was merged.
Change subject: Unpersist the session on logout
......................................................................
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
---
M includes/WebRequest.php
M includes/session/Session.php
M includes/session/SessionBackend.php
M includes/user/User.php
M tests/phpunit/includes/session/SessionBackendTest.php
M tests/phpunit/includes/session/SessionTest.php
6 files changed, 168 insertions(+), 12 deletions(-)
Approvals:
Gergő Tisza: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 714ab97..ce5cb96 100644
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -677,6 +677,16 @@
}
/**
+ * 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
--- a/includes/session/Session.php
+++ b/includes/session/Session.php
@@ -125,6 +125,13 @@
}
/**
+ * 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.
* @return bool
diff --git a/includes/session/SessionBackend.php
b/includes/session/SessionBackend.php
index 0424a2d..1e2b476 100644
--- a/includes/session/SessionBackend.php
+++ b/includes/session/SessionBackend.php
@@ -284,6 +284,35 @@
}
/**
+ * 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.
* @return bool
@@ -629,14 +658,22 @@
'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
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3721,6 +3721,7 @@
} 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
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -63,6 +63,7 @@
$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 @@
$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 @@
$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;
@@ -479,6 +503,38 @@
$this->store->setSessionData( self::SESSIONID, $testData );
$backend = $this->getBackend( $user );
$this->store->deleteSession( self::SESSIONID );
+ $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 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;
@@ -520,8 +576,10 @@
$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 @@
$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 @@
$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 @@
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
--- a/tests/phpunit/includes/session/SessionTest.php
+++ b/tests/phpunit/includes/session/SessionTest.php
@@ -75,6 +75,7 @@
[ 'getProvider', [], false, true ],
[ 'isPersistent', [], false, true ],
[ 'persist', [], false, false ],
+ [ 'unpersist', [], false, false ],
[ 'shouldRememberUser', [], false, true ],
[ 'setRememberUser', [ true ], false, false ],
[ 'getRequest', [], true, true ],
--
To view, visit https://gerrit.wikimedia.org/r/273524
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I76897eaac063e5e3c3563398d0f4cb36cf93783b
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: BryanDavis <bda...@wikimedia.org>
Gerrit-Reviewer: CSteipp <cste...@wikimedia.org>
Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
