From: Brad Jorsch Date: Tue, 10 May 2016 19:25:39 +0000 (-0400) Subject: Add SessionInfo force-use flag X-Git-Tag: 1.31.0-rc.0~7023^2 X-Git-Url: https://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=afdcd30599d0670900463281025c582e3addba82;p=lhc%2Fweb%2Fwiklou.git Add SessionInfo force-use flag A provider that uses SessionProvider::hashToSessionId() will likely have issues if something such as a call to $user->setToken() causes SessionManager::loadSessionInfoFromStore() to fail, since the provider can't just arbitrarily change the session ID it returns. The two solutions to this problem are: * Somehow include everything that could cause loadSessionInfoFromStore to fail in the data hashed by hashToSessionId. * Flag the SessionInfo so that, if stored data and the SessionInfo conflict, it should delete the stored data instead of discarding the SessionInfo. Since the second is less complexity overall due to the lack of need to define "everything", this patch takes that approach. Change-Id: I8c6fab2ec295e71242bbcb19d0ee5ade6bd655df --- diff --git a/includes/session/SessionInfo.php b/includes/session/SessionInfo.php index 1b5a834c94..c235861f57 100644 --- a/includes/session/SessionInfo.php +++ b/includes/session/SessionInfo.php @@ -54,6 +54,7 @@ class SessionInfo { private $remembered = false; private $forceHTTPS = false; private $idIsSafe = false; + private $forceUse = false; /** @var array|null */ private $providerMetadata = null; @@ -76,6 +77,10 @@ class SessionInfo { * - idIsSafe: (bool) Set true if the 'id' did not come from the user. * Generally you'll use this from SessionProvider::newEmptySession(), * and not from any other method. + * - forceUse: (bool) Set true if the 'id' is from + * SessionProvider::hashToSessionId() to delete conflicting session + * store data instead of discarding this SessionInfo. Ignored unless + * both 'provider' and 'id' are given. * - copyFrom: (SessionInfo) SessionInfo to copy other data items from. */ public function __construct( $priority, array $data ) { @@ -97,6 +102,7 @@ class SessionInfo { 'forceHTTPS' => $from->forceHTTPS, 'metadata' => $from->providerMetadata, 'idIsSafe' => $from->idIsSafe, + 'forceUse' => $from->forceUse, // @codeCoverageIgnoreStart ]; // @codeCoverageIgnoreEnd @@ -110,6 +116,7 @@ class SessionInfo { 'forceHTTPS' => false, 'metadata' => null, 'idIsSafe' => false, + 'forceUse' => false, // @codeCoverageIgnoreStart ]; // @codeCoverageIgnoreEnd @@ -137,9 +144,11 @@ class SessionInfo { if ( $data['id'] !== null ) { $this->id = $data['id']; $this->idIsSafe = $data['idIsSafe']; + $this->forceUse = $data['forceUse'] && $this->provider; } else { $this->id = $this->provider->getManager()->generateSessionId(); $this->idIsSafe = true; + $this->forceUse = false; } $this->priority = (int)$priority; $this->userInfo = $data['userInfo']; @@ -185,6 +194,20 @@ class SessionInfo { return $this->idIsSafe; } + /** + * Force use of this SessionInfo if validation fails + * + * The normal behavior is to discard the SessionInfo if validation against + * the data stored in the session store fails. If this returns true, + * SessionManager will instead delete the session store data so this + * SessionInfo may still be used. + * + * @return bool + */ + final public function forceUse() { + return $this->forceUse; + } + /** * Return the priority * @return int diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php index 6c17de5ef2..7ba3bbee4f 100644 --- a/includes/session/SessionManager.php +++ b/includes/session/SessionManager.php @@ -704,6 +704,20 @@ final class SessionManager implements SessionManagerInterface { $key = wfMemcKey( 'MWSession', $info->getId() ); $blob = $this->store->get( $key ); + // If we got data from the store and the SessionInfo says to force use, + // "fail" means to delete the data from the store and retry. Otherwise, + // "fail" is just return false. + if ( $info->forceUse() && $blob !== false ) { + $failHandler = function () use ( $key, &$info, $request ) { + $this->store->delete( $key ); + return $this->loadSessionInfoFromStore( $info, $request ); + }; + } else { + $failHandler = function () { + return false; + }; + } + $newParams = []; if ( $blob !== false ) { @@ -713,7 +727,7 @@ final class SessionManager implements SessionManagerInterface { 'session' => $info, ] ); $this->store->delete( $key ); - return false; + return $failHandler(); } // Sanity check: blob has data and metadata arrays @@ -724,7 +738,7 @@ final class SessionManager implements SessionManagerInterface { 'session' => $info, ] ); $this->store->delete( $key ); - return false; + return $failHandler(); } $data = $blob['data']; @@ -741,7 +755,7 @@ final class SessionManager implements SessionManagerInterface { 'session' => $info, ] ); $this->store->delete( $key ); - return false; + return $failHandler(); } // First, load the provider from metadata, or validate it against the metadata. @@ -756,7 +770,7 @@ final class SessionManager implements SessionManagerInterface { ] ); $this->store->delete( $key ); - return false; + return $failHandler(); } } elseif ( $metadata['provider'] !== (string)$provider ) { $this->logger->warning( 'Session "{session}": Wrong provider ' . @@ -764,7 +778,7 @@ final class SessionManager implements SessionManagerInterface { [ 'session' => $info, ] ); - return false; + return $failHandler(); } // Load provider metadata from metadata, or validate it against the metadata @@ -788,7 +802,7 @@ final class SessionManager implements SessionManagerInterface { 'exception' => $ex, ] + $ex->getContext() ); - return false; + return $failHandler(); } } } @@ -810,7 +824,7 @@ final class SessionManager implements SessionManagerInterface { 'session' => $info, 'exception' => $ex, ] ); - return false; + return $failHandler(); } $newParams['userInfo'] = $userInfo; } else { @@ -825,7 +839,7 @@ final class SessionManager implements SessionManagerInterface { 'uid_a' => $metadata['userId'], 'uid_b' => $userInfo->getId(), ] ); - return false; + return $failHandler(); } // If the user was renamed, probably best to fail here. @@ -839,7 +853,7 @@ final class SessionManager implements SessionManagerInterface { 'uname_a' => $metadata['userName'], 'uname_b' => $userInfo->getName(), ] ); - return false; + return $failHandler(); } } elseif ( $metadata['userName'] !== null ) { // Shouldn't happen, but just in case @@ -851,7 +865,7 @@ final class SessionManager implements SessionManagerInterface { 'uname_a' => $metadata['userName'], 'uname_b' => $userInfo->getName(), ] ); - return false; + return $failHandler(); } } elseif ( !$userInfo->isAnon() ) { // Metadata specifies an anonymous user, but the passed-in @@ -861,7 +875,7 @@ final class SessionManager implements SessionManagerInterface { [ 'session' => $info, ] ); - return false; + return $failHandler(); } } @@ -872,7 +886,7 @@ final class SessionManager implements SessionManagerInterface { $this->logger->warning( 'Session "{session}": User token mismatch', [ 'session' => $info, ] ); - return false; + return $failHandler(); } if ( !$userInfo->isVerified() ) { $newParams['userInfo'] = $userInfo->verified(); @@ -899,7 +913,7 @@ final class SessionManager implements SessionManagerInterface { [ 'session' => $info, ] ); - return false; + return $failHandler(); } // If no user was provided and no metadata, it must be anon. @@ -912,7 +926,7 @@ final class SessionManager implements SessionManagerInterface { [ 'session' => $info, ] ); - return false; + return $failHandler(); } } elseif ( !$info->getUserInfo()->isVerified() ) { $this->logger->warning( @@ -920,7 +934,7 @@ final class SessionManager implements SessionManagerInterface { [ 'session' => $info, ] ); - return false; + return $failHandler(); } $data = false; @@ -942,7 +956,7 @@ final class SessionManager implements SessionManagerInterface { // Allow the provider to check the loaded SessionInfo $providerMetadata = $info->getProviderMetadata(); if ( !$info->getProvider()->refreshSessionInfo( $info, $request, $providerMetadata ) ) { - return false; + return $failHandler(); } if ( $providerMetadata !== $info->getProviderMetadata() ) { $info = new SessionInfo( $info->getPriority(), [ @@ -962,7 +976,7 @@ final class SessionManager implements SessionManagerInterface { $this->logger->warning( 'Session "{session}": ' . $reason, [ 'session' => $info, ] ); - return false; + return $failHandler(); } return true; diff --git a/includes/session/SessionProvider.php b/includes/session/SessionProvider.php index 3cd065d837..1d693a1cde 100644 --- a/includes/session/SessionProvider.php +++ b/includes/session/SessionProvider.php @@ -458,7 +458,9 @@ abstract class SessionProvider implements SessionProviderInterface, LoggerAwareI * * Generally this will only be used when self::persistsSessionId() is false and * the provider has to base the session ID on the verified user's identity - * or other static data. + * or other static data. The SessionInfo should then typically have the + * 'forceUse' flag set to avoid persistent session failure if validation of + * the stored data fails. * * @param string $data * @param string|null $key Defaults to $this->config->get( 'SecretKey' ) diff --git a/tests/phpunit/includes/session/SessionInfoTest.php b/tests/phpunit/includes/session/SessionInfoTest.php index ff22bfad42..8f7b2a6e70 100644 --- a/tests/phpunit/includes/session/SessionInfoTest.php +++ b/tests/phpunit/includes/session/SessionInfoTest.php @@ -103,6 +103,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $anonInfo, $info->getUserInfo() ); $this->assertTrue( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertFalse( $info->wasPersisted() ); $this->assertFalse( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -118,6 +119,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $unverifiedUserInfo, $info->getUserInfo() ); $this->assertTrue( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertFalse( $info->wasPersisted() ); $this->assertFalse( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -132,6 +134,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $userInfo, $info->getUserInfo() ); $this->assertTrue( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertFalse( $info->wasPersisted() ); $this->assertTrue( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -150,6 +153,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $anonInfo, $info->getUserInfo() ); $this->assertFalse( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertTrue( $info->wasPersisted() ); $this->assertFalse( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -165,6 +169,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $userInfo, $info->getUserInfo() ); $this->assertFalse( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertFalse( $info->wasPersisted() ); $this->assertTrue( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -180,6 +185,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertSame( $userInfo, $info->getUserInfo() ); $this->assertFalse( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertTrue( $info->wasPersisted() ); $this->assertFalse( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); @@ -231,6 +237,25 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( SessionInfo::MIN_PRIORITY + 5, $info->getPriority() ); $this->assertTrue( $info->isIdSafe() ); + $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [ + 'id' => $id, + 'forceUse' => true, + ] ); + $this->assertFalse( $info->forceUse(), 'no provider' ); + + $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [ + 'provider' => $provider, + 'forceUse' => true, + ] ); + $this->assertFalse( $info->forceUse(), 'no id' ); + + $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [ + 'provider' => $provider, + 'id' => $id, + 'forceUse' => true, + ] ); + $this->assertTrue( $info->forceUse(), 'correct use' ); + $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [ 'id' => $id, 'forceHTTPS' => 1, @@ -242,6 +267,7 @@ class SessionInfoTest extends MediaWikiTestCase { 'provider' => $provider, 'userInfo' => $userInfo, 'idIsSafe' => true, + 'forceUse' => true, 'persisted' => true, 'remembered' => true, 'forceHTTPS' => true, @@ -255,6 +281,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( $provider, $info->getProvider() ); $this->assertSame( $userInfo, $info->getUserInfo() ); $this->assertTrue( $info->isIdSafe() ); + $this->assertTrue( $info->forceUse() ); $this->assertTrue( $info->wasPersisted() ); $this->assertTrue( $info->wasRemembered() ); $this->assertTrue( $info->forceHTTPS() ); @@ -265,6 +292,7 @@ class SessionInfoTest extends MediaWikiTestCase { 'provider' => $provider2, 'userInfo' => $unverifiedUserInfo, 'idIsSafe' => false, + 'forceUse' => false, 'persisted' => false, 'remembered' => false, 'forceHTTPS' => false, @@ -276,6 +304,7 @@ class SessionInfoTest extends MediaWikiTestCase { $this->assertSame( $provider2, $info->getProvider() ); $this->assertSame( $unverifiedUserInfo, $info->getUserInfo() ); $this->assertFalse( $info->isIdSafe() ); + $this->assertFalse( $info->forceUse() ); $this->assertFalse( $info->wasPersisted() ); $this->assertFalse( $info->wasRemembered() ); $this->assertFalse( $info->forceHTTPS() ); diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php index d04d7ec46d..9c9115dc13 100644 --- a/tests/phpunit/includes/session/SessionManagerTest.php +++ b/tests/phpunit/includes/session/SessionManagerTest.php @@ -1756,5 +1756,21 @@ class SessionManagerTest extends MediaWikiTestCase { [ LogLevel::WARNING, 'Session "{session}": Hook aborted' ], ], $logger->getBuffer() ); $logger->clearBuffer(); + $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionCheckInfo' => [] ] ); + + // forceUse deletes bad backend data + $this->store->setSessionMeta( $id, [ 'userToken' => 'Bad' ] + $metadata ); + $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [ + 'provider' => $provider, + 'id' => $id, + 'userInfo' => $userInfo, + 'forceUse' => true, + ] ); + $this->assertTrue( $loadSessionInfoFromStore( $info ) ); + $this->assertFalse( $this->store->getSession( $id ) ); + $this->assertSame( [ + [ LogLevel::WARNING, 'Session "{session}": User token mismatch' ], + ], $logger->getBuffer() ); + $logger->clearBuffer(); } }