Merge "Add SessionInfo force-use flag"

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 4320e20..777d3d6 100644 (file)
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -717,6 +717,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 ) {
@@ -726,7 +740,7 @@ final class SessionManager implements SessionManagerInterface {
                                        'session' => $info,
                                ] );
                                $this->store->delete( $key );
-                               return false;
+                               return $failHandler();
                        }
 
                        // Sanity check: blob has data and metadata arrays
@@ -737,7 +751,7 @@ final class SessionManager implements SessionManagerInterface {
                                        'session' => $info,
                                ] );
                                $this->store->delete( $key );
-                               return false;
+                               return $failHandler();
                        }
 
                        $data = $blob['data'];
@@ -754,7 +768,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.
@@ -769,7 +783,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 ' .
@@ -777,7 +791,7 @@ final class SessionManager implements SessionManagerInterface {
                                        [
                                                'session' => $info,
                                ] );
-                               return false;
+                               return $failHandler();
                        }
 
                        // Load provider metadata from metadata, or validate it against the metadata
@@ -801,7 +815,7 @@ final class SessionManager implements SessionManagerInterface {
                                                                'exception' => $ex,
                                                        ] + $ex->getContext()
                                                );
-                                               return false;
+                                               return $failHandler();
                                        }
                                }
                        }
@@ -823,7 +837,7 @@ final class SessionManager implements SessionManagerInterface {
                                                'session' => $info,
                                                'exception' => $ex,
                                        ] );
-                                       return false;
+                                       return $failHandler();
                                }
                                $newParams['userInfo'] = $userInfo;
                        } else {
@@ -838,7 +852,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.
@@ -852,7 +866,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
@@ -864,7 +878,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
@@ -874,7 +888,7 @@ final class SessionManager implements SessionManagerInterface {
                                                [
                                                        'session' => $info,
                                        ] );
-                                       return false;
+                                       return $failHandler();
                                }
                        }
 
@@ -885,7 +899,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();
@@ -912,7 +926,7 @@ final class SessionManager implements SessionManagerInterface {
                                        [
                                                'session' => $info,
                                ] );
-                               return false;
+                               return $failHandler();
                        }
 
                        // If no user was provided and no metadata, it must be anon.
@@ -925,7 +939,7 @@ final class SessionManager implements SessionManagerInterface {
                                                [
                                                        'session' => $info,
                                        ] );
-                                       return false;
+                                       return $failHandler();
                                }
                        } elseif ( !$info->getUserInfo()->isVerified() ) {
                                $this->logger->warning(
@@ -933,7 +947,7 @@ final class SessionManager implements SessionManagerInterface {
                                        [
                                                'session' => $info,
                                ] );
-                               return false;
+                               return $failHandler();
                        }
 
                        $data = false;
@@ -955,7 +969,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(), [
@@ -975,7 +989,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 995af24..ed113b7 100644 (file)
--- a/includes/session/SessionProvider.php
+++ b/includes/session/SessionProvider.php
@@ -472,7 +472,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 2dee8bc..5f387ea 100644 (file)
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -1785,5 +1785,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();
        }
 }