Gergő Tisza has uploaded a new change for review. https://gerrit.wikimedia.org/r/265404
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 (cherry picked from commit 4f5057b84b36eccd16627a6b29831dfdb4483b02) --- 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/04/265404/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/265404 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: wmf/1.27.0-wmf.11 Gerrit-Owner: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Anomie <bjor...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits