Anomie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/265297
Change subject: SessionManager: Change behavior of getSessionById()
......................................................................
SessionManager: Change behavior of getSessionById()
It's easily possible for SessionManager::getSessionById() to not be
able to load the specified session and to not be able to create an empty
one by that ID, for example if the user's token changed. So change this
from an exceptional condition to an expected one, and adjust callers to
deal with it appropriately.
Let's also make the checks for invalid data structure when loading the
session from the store delete the bogus data entirely.
At the same time, let's change the silly "$noEmpty" parameter to
"$create" and make the default behavior be not to create an empty
session.
Bug: T124126
Change-Id: I085d2026d1b366b1af9fd0e8ca3d815fd8288030
---
M includes/WebRequest.php
M includes/context/RequestContext.php
M includes/jobqueue/jobs/UploadFromUrlJob.php
M includes/session/PHPSessionHandler.php
M includes/session/SessionManager.php
M includes/session/SessionManagerInterface.php
M tests/phpunit/includes/session/PHPSessionHandlerTest.php
M tests/phpunit/includes/session/SessionManagerTest.php
8 files changed, 67 insertions(+), 44 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/97/265297/1
diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 7306105..2c14618 100644
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -655,7 +655,10 @@
*/
public function getSession() {
if ( $this->sessionId !== null ) {
- return SessionManager::singleton()->getSessionById(
(string)$this->sessionId, false, $this );
+ $session = SessionManager::singleton()->getSessionById(
(string)$this->sessionId, true, $this );
+ if ( $session ) {
+ return $session;
+ }
}
$session = SessionManager::singleton()->getSessionForRequest(
$this );
diff --git a/includes/context/RequestContext.php
b/includes/context/RequestContext.php
index 16f11ee..afb5704 100644
--- a/includes/context/RequestContext.php
+++ b/includes/context/RequestContext.php
@@ -576,8 +576,9 @@
// Get new session, if applicable
$session = null;
if ( strlen( $params['sessionId'] ) ) { // don't make a
new random ID
- $session =
MediaWiki\Session\SessionManager::singleton()
- ->getSessionById( $params['sessionId']
);
+ $manager =
MediaWiki\Session\SessionManager::singleton();
+ $session = $manager->getSessionById(
$params['sessionId'], true )
+ ?: $manager->getEmptySession();
}
// Remove any user IP or agent information, and attach
the request
diff --git a/includes/jobqueue/jobs/UploadFromUrlJob.php
b/includes/jobqueue/jobs/UploadFromUrlJob.php
index 28e3c40..0491e64 100644
--- a/includes/jobqueue/jobs/UploadFromUrlJob.php
+++ b/includes/jobqueue/jobs/UploadFromUrlJob.php
@@ -155,18 +155,22 @@
* Store a result in the session data. Note that the caller is
responsible
* for appropriate session_start and session_write_close calls.
*
- * @param MediaWiki\\Session\\Session $session Session to store result
into
+ * @param MediaWiki\\Session\\Session|null $session Session to store
result into
* @param string $result The result (Success|Warning|Failure)
* @param string $dataKey The key of the extra data
* @param mixed $dataValue The extra data itself
*/
protected function storeResultInSession(
- MediaWiki\Session\Session $session, $result, $dataKey,
$dataValue
+ MediaWiki\Session\Session $session = null, $result, $dataKey,
$dataValue
) {
- $data = self::getSessionData( $session,
$this->params['sessionKey'] );
- $data['result'] = $result;
- $data[$dataKey] = $dataValue;
- self::setSessionData( $session, $this->params['sessionKey'],
$data );
+ if ( $session ) {
+ $data = self::getSessionData( $session,
$this->params['sessionKey'] );
+ $data['result'] = $result;
+ $data[$dataKey] = $dataValue;
+ self::setSessionData( $session,
$this->params['sessionKey'], $data );
+ } else {
+ wfDebug( __METHOD__ . ': Cannot store result in
session, session does not exist' );
+ }
}
/**
diff --git a/includes/session/PHPSessionHandler.php
b/includes/session/PHPSessionHandler.php
index c59cc96..44d14cd 100644
--- a/includes/session/PHPSessionHandler.php
+++ b/includes/session/PHPSessionHandler.php
@@ -208,7 +208,7 @@
throw new \BadMethodCallException( 'Attempt to use PHP
session management' );
}
- $session = $this->manager->getSessionById( $id, true );
+ $session = $this->manager->getSessionById( $id, false );
if ( !$session ) {
return '';
}
@@ -236,7 +236,13 @@
throw new \BadMethodCallException( 'Attempt to use PHP
session management' );
}
- $session = $this->manager->getSessionById( $id );
+ $session = $this->manager->getSessionById( $id, true );
+ if ( !$session ) {
+ $this->logger->warning(
+ __METHOD__ . ": Session \"$id\" cannot be
loaded, skipping write."
+ );
+ return false;
+ }
// First, decode the string PHP handed us
$data = \Wikimedia\PhpSessionSerializer::decode( $dataStr );
@@ -331,7 +337,7 @@
if ( !$this->enable ) {
throw new \BadMethodCallException( 'Attempt to use PHP
session management' );
}
- $session = $this->manager->getSessionById( $id, true );
+ $session = $this->manager->getSessionById( $id, false );
if ( $session ) {
$session->clear();
}
diff --git a/includes/session/SessionManager.php
b/includes/session/SessionManager.php
index 1c8686c..0e45468 100644
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -123,7 +123,8 @@
// Someone used session_id(), so we need to
follow suit.
// Note this overwrites whatever session might
already be
// associated with $request with the one for
$id.
- self::$globalSession =
self::singleton()->getSessionById( $id, false, $request );
+ self::$globalSession =
self::singleton()->getSessionById( $id, true, $request )
+ ?: $request->getSession();
}
}
return self::$globalSession;
@@ -197,7 +198,7 @@
return $session;
}
- public function getSessionById( $id, $noEmpty = false, WebRequest
$request = null ) {
+ public function getSessionById( $id, $create = false, WebRequest
$request = null ) {
if ( !self::validateSessionId( $id ) ) {
throw new \InvalidArgumentException( 'Invalid session
ID' );
}
@@ -217,7 +218,7 @@
}
}
- if ( !$noEmpty && $session === null ) {
+ if ( $create && $session === null ) {
$ex = null;
try {
$session = $this->getEmptySessionInternal(
$request, $id );
@@ -225,11 +226,6 @@
$this->logger->error( __METHOD__ . ': failed to
create empty session: ' .
$ex->getMessage() );
$session = null;
- }
- if ( $session === null ) {
- throw new \UnexpectedValueException(
- 'Can neither load the session nor
create an empty session', 0, $ex
- );
}
}
@@ -662,7 +658,8 @@
* @return bool Whether the session info matches the stored data (if
any)
*/
private function loadSessionInfoFromStore( SessionInfo &$info,
WebRequest $request ) {
- $blob = $this->store->get( wfMemcKey( 'MWSession',
$info->getId() ) );
+ $key = wfMemcKey( 'MWSession', $info->getId() );
+ $blob = $this->store->get( $key );
$newParams = array();
@@ -670,6 +667,7 @@
// Sanity check: blob must be an array, if it's saved
at all
if ( !is_array( $blob ) ) {
$this->logger->warning( "Session $info: Bad
data" );
+ $this->store->delete( $key );
return false;
}
@@ -678,6 +676,7 @@
!isset( $blob['metadata'] ) || !is_array(
$blob['metadata'] )
) {
$this->logger->warning( "Session $info: Bad
data structure" );
+ $this->store->delete( $key );
return false;
}
@@ -692,6 +691,7 @@
!array_key_exists( 'provider', $metadata )
) {
$this->logger->warning( "Session $info: Bad
metadata" );
+ $this->store->delete( $key );
return false;
}
@@ -701,6 +701,7 @@
$newParams['provider'] = $provider =
$this->getProvider( $metadata['provider'] );
if ( !$provider ) {
$this->logger->warning( "Session $info:
Unknown provider, " . $metadata['provider'] );
+ $this->store->delete( $key );
return false;
}
} elseif ( $metadata['provider'] !== (string)$provider
) {
diff --git a/includes/session/SessionManagerInterface.php
b/includes/session/SessionManagerInterface.php
index 67d6f5d..d58d3b9 100644
--- a/includes/session/SessionManagerInterface.php
+++ b/includes/session/SessionManagerInterface.php
@@ -67,12 +67,13 @@
/**
* Fetch a session by ID
* @param string $id
- * @param bool $noEmpty Don't return an empty session
+ * @param bool $create If no session exists for $id, try to create a
new one.
+ * May still return null if a session for $id exists but cannot be
loaded.
* @param WebRequest|null $request Corresponding request. Any existing
* session associated with this WebRequest object will be overwritten.
* @return Session|null
*/
- public function getSessionById( $id, $noEmpty = false, WebRequest
$request = null );
+ public function getSessionById( $id, $create = false, WebRequest
$request = null );
/**
* Fetch a new, empty session
diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
index c18b821..5a5df6f 100644
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -285,6 +285,24 @@
42 => 'forty-two',
'forty-two' => 42,
), iterator_to_array( $session ) );
+
+ // Test that write doesn't break if the session is invalid
+ $session = $manager->getEmptySession();
+ $session->persist();
+ session_id( $session->getId() );
+ session_start();
+ $this->mergeMwGlobalArrayValue( 'wgHooks', array(
+ 'SessionCheckInfo' => array( function ( &$reason ) {
+ $reason = 'Testing';
+ return false;
+ } ),
+ ) );
+ $this->assertNull( $manager->getSessionById( $session->getId(),
true ), 'sanity check' );
+ session_write_close();
+ $this->mergeMwGlobalArrayValue( 'wgHooks', array(
+ 'SessionCheckInfo' => array(),
+ ) );
+ $this->assertNotNull( $manager->getSessionById(
$session->getId(), true ), 'sanity check' );
}
public static function provideHandlers() {
diff --git a/tests/phpunit/includes/session/SessionManagerTest.php
b/tests/phpunit/includes/session/SessionManagerTest.php
index dc217cd..b4687ba 100644
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -336,39 +336,28 @@
// Unknown session ID
$id = $manager->generateSessionId();
- $session = $manager->getSessionById( $id );
+ $session = $manager->getSessionById( $id, true );
$this->assertInstanceOf( 'MediaWiki\\Session\\Session',
$session );
$this->assertSame( $id, $session->getId() );
$id = $manager->generateSessionId();
- $this->assertNull( $manager->getSessionById( $id, true ) );
+ $this->assertNull( $manager->getSessionById( $id, false ) );
// Known but unloadable session ID
$this->logger->setCollect( true );
$id = $manager->generateSessionId();
- $this->store->setRawSession( $id, array( 'metadata' => array(
- 'provider' => 'DummySessionProvider',
- 'userId' => 0,
- 'userName' => null,
- 'userToken' => null,
+ $this->store->setSession( $id, array( 'metadata' => array(
+ 'userId' => User::idFromName( 'UTSysop' ),
+ 'userToken' => 'bad',
) ) );
- try {
- $manager->getSessionById( $id );
- $this->fail( 'Expected exception not thrown' );
- } catch ( \UnexpectedValueException $ex ) {
- $this->assertSame(
- 'Can neither load the session nor create an
empty session',
- $ex->getMessage()
- );
- }
-
$this->assertNull( $manager->getSessionById( $id, true ) );
+ $this->assertNull( $manager->getSessionById( $id, false ) );
$this->logger->setCollect( false );
// Known session ID
$this->store->setSession( $id, array() );
- $session = $manager->getSessionById( $id );
+ $session = $manager->getSessionById( $id, false );
$this->assertInstanceOf( 'MediaWiki\\Session\\Session',
$session );
$this->assertSame( $id, $session->getId() );
}
@@ -754,14 +743,14 @@
$sessionId = $session->getSessionId();
$id = (string)$sessionId;
- $this->assertSame( $sessionId, $manager->getSessionById( $id
)->getSessionId() );
+ $this->assertSame( $sessionId, $manager->getSessionById( $id,
true )->getSessionId() );
$manager->changeBackendId( $backend );
$this->assertSame( $sessionId, $session->getSessionId() );
$this->assertNotEquals( $id, (string)$sessionId );
$id = (string)$sessionId;
- $this->assertSame( $sessionId, $manager->getSessionById( $id
)->getSessionId() );
+ $this->assertSame( $sessionId, $manager->getSessionById( $id,
true )->getSessionId() );
// Destruction of the session here causes the backend to be
deregistered
$session = null;
@@ -784,7 +773,7 @@
);
}
- $session = $manager->getSessionById( $id );
+ $session = $manager->getSessionById( $id, true );
$this->assertSame( $sessionId, $session->getSessionId() );
}
--
To view, visit https://gerrit.wikimedia.org/r/265297
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I085d2026d1b366b1af9fd0e8ca3d815fd8288030
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <bjor...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
