From 4f5057b84b36eccd16627a6b29831dfdb4483b02 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 20 Jan 2016 12:40:04 -0500 Subject: [PATCH] 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 --- includes/WebRequest.php | 5 ++- includes/context/RequestContext.php | 5 +-- includes/jobqueue/jobs/UploadFromUrlJob.php | 16 ++++++---- includes/session/PHPSessionHandler.php | 12 +++++-- includes/session/SessionManager.php | 19 ++++++------ includes/session/SessionManagerInterface.php | 5 +-- .../session/PHPSessionHandlerTest.php | 18 +++++++++++ .../includes/session/SessionManagerTest.php | 31 ++++++------------- 8 files changed, 67 insertions(+), 44 deletions(-) diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 730610515e..2c146188a1 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -655,7 +655,10 @@ class WebRequest { */ 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 16f11eeb3e..afb5704074 100644 --- a/includes/context/RequestContext.php +++ b/includes/context/RequestContext.php @@ -576,8 +576,9 @@ class RequestContext implements IContextSource, MutableContext { // 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 28e3c405ef..0491e649d2 100644 --- a/includes/jobqueue/jobs/UploadFromUrlJob.php +++ b/includes/jobqueue/jobs/UploadFromUrlJob.php @@ -155,18 +155,22 @@ class UploadFromUrlJob extends Job { * 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 c59cc9639f..44d14cd009 100644 --- a/includes/session/PHPSessionHandler.php +++ b/includes/session/PHPSessionHandler.php @@ -208,7 +208,7 @@ class PHPSessionHandler { 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 @@ class PHPSessionHandler { 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 @@ class PHPSessionHandler { 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 1c8686c08f..0e4546885a 100644 --- a/includes/session/SessionManager.php +++ b/includes/session/SessionManager.php @@ -123,7 +123,8 @@ final class SessionManager implements SessionManagerInterface { // 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 @@ final class SessionManager implements SessionManagerInterface { 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 @@ final class SessionManager implements SessionManagerInterface { } } - if ( !$noEmpty && $session === null ) { + if ( $create && $session === null ) { $ex = null; try { $session = $this->getEmptySessionInternal( $request, $id ); @@ -226,11 +227,6 @@ final class SessionManager implements SessionManagerInterface { $ex->getMessage() ); $session = null; } - if ( $session === null ) { - throw new \UnexpectedValueException( - 'Can neither load the session nor create an empty session', 0, $ex - ); - } } return $session; @@ -662,7 +658,8 @@ final class SessionManager implements SessionManagerInterface { * @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 @@ final class SessionManager implements SessionManagerInterface { // 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 @@ final class SessionManager implements SessionManagerInterface { !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 @@ final class SessionManager implements SessionManagerInterface { !array_key_exists( 'provider', $metadata ) ) { $this->logger->warning( "Session $info: Bad metadata" ); + $this->store->delete( $key ); return false; } @@ -701,6 +701,7 @@ final class SessionManager implements SessionManagerInterface { $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 67d6f5d7d8..d58d3b97ed 100644 --- a/includes/session/SessionManagerInterface.php +++ b/includes/session/SessionManagerInterface.php @@ -67,12 +67,13 @@ interface SessionManagerInterface extends LoggerAwareInterface { /** * 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 c18b82130a..5a5df6f2f3 100644 --- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php +++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php @@ -285,6 +285,24 @@ class PHPSessionHandlerTest extends MediaWikiTestCase { 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 dc217cda83..b4687ba2a7 100644 --- a/tests/phpunit/includes/session/SessionManagerTest.php +++ b/tests/phpunit/includes/session/SessionManagerTest.php @@ -336,39 +336,28 @@ class SessionManagerTest extends MediaWikiTestCase { // 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 @@ class SessionManagerTest extends MediaWikiTestCase { $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 @@ class SessionManagerTest extends MediaWikiTestCase { ); } - $session = $manager->getSessionById( $id ); + $session = $manager->getSessionById( $id, true ); $this->assertSame( $sessionId, $session->getSessionId() ); } -- 2.20.1