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 1b5a834..c235861 100644 (file)
--- 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 6c17de5..7ba3bbe 100644 (file)
--- 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 3cd065d..1d693a1 100644 (file)
--- 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 ff22bfa..8f7b2a6 100644 (file)
--- 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 d04d7ec..9c9115d 100644 (file)
--- 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();
        }
 }