Introduce User::INVALID_TOKEN
authorBrad Jorsch <bjorsch@wikimedia.org>
Mon, 1 Feb 2016 21:59:27 +0000 (16:59 -0500)
committerAnomie <bjorsch@wikimedia.org>
Wed, 3 Feb 2016 21:45:45 +0000 (21:45 +0000)
To avoid having to have SessionManager try to reset sessions on every
request, we set the user_token to a special value. When that value is
present, User::getToken() returns a different value every time (so
existing checks will fail) and User::setToken() refuses to alter it.

Bug: T124414
Change-Id: Ie4c84ce993e40a081288cf5a543f8ba99f98806a

includes/session/SessionManager.php
includes/user/User.php
tests/phpunit/includes/session/SessionManagerTest.php

index 4b38a5c..c1bbc8f 100644 (file)
@@ -536,13 +536,6 @@ final class SessionManager implements SessionManagerInterface {
        public function preventSessionsForUser( $username ) {
                $this->preventUsers[$username] = true;
 
-               // Reset the user's token to kill existing sessions
-               $user = User::newFromName( $username );
-               if ( $user && $user->getToken( false ) ) {
-                       $user->setToken();
-                       $user->saveSettings();
-               }
-
                // Instruct the session providers to kill any other sessions too.
                foreach ( $this->getProviders() as $provider ) {
                        $provider->preventSessionsForUser( $username );
index 0fa7d59..be528b6 100644 (file)
@@ -45,6 +45,11 @@ class User implements IDBAccessObject {
         */
        const TOKEN_LENGTH = 32;
 
+       /**
+        * @const string An invalid value for user_token
+        */
+       const INVALID_TOKEN = '*** INVALID ***';
+
        /**
         * Global constant made accessible as class constants so that autoloader
         * magic can be used.
@@ -641,7 +646,8 @@ class User implements IDBAccessObject {
                $user = self::newFromRow( $row );
 
                // A user is considered to exist as a non-system user if it has a
-               // password set, or a temporary password set, or an email set.
+               // password set, or a temporary password set, or an email set, or a
+               // non-invalid token.
                $passwordFactory = new PasswordFactory();
                $passwordFactory->init( RequestContext::getMain()->getConfig() );
                try {
@@ -657,7 +663,7 @@ class User implements IDBAccessObject {
                        $newpassword = PasswordFactory::newInvalidPassword();
                }
                if ( !$password instanceof InvalidPassword || !$newpassword instanceof InvalidPassword
-                       || $user->mEmail
+                       || $user->mEmail || $user->mToken !== self::INVALID_TOKEN
                ) {
                        // User exists. Steal it?
                        if ( !$options['steal'] ) {
@@ -677,11 +683,11 @@ class User implements IDBAccessObject {
                                __METHOD__
                        );
                        $user->invalidateEmail();
+                       $user->mToken = self::INVALID_TOKEN;
                        $user->saveSettings();
+                       SessionManager::singleton()->preventSessionsForUser( $user->getName() );
                }
 
-               SessionManager::singleton()->preventSessionsForUser( $user->getName() );
-
                return $user;
        }
 
@@ -2439,13 +2445,18 @@ class User implements IDBAccessObject {
                        $this->setToken();
                }
 
-               // If the user doesn't have a token, return null to indicate that.
-               // Otherwise, hmac the version with the secret if we have a version.
                if ( !$this->mToken ) {
+                       // The user doesn't have a token, return null to indicate that.
                        return null;
+               } elseif ( $this->mToken === self::INVALID_TOKEN ) {
+                       // We return a random value here so existing token checks are very
+                       // likely to fail.
+                       return MWCryptRand::generateHex( self::TOKEN_LENGTH );
                } elseif ( $wgAuthenticationTokenVersion === null ) {
+                       // $wgAuthenticationTokenVersion not in use, so return the raw secret
                        return $this->mToken;
                } else {
+                       // $wgAuthenticationTokenVersion in use, so hmac it.
                        $ret = MWCryptHash::hmac( $wgAuthenticationTokenVersion, $this->mToken, false );
 
                        // The raw hash can be overly long. Shorten it up.
@@ -2466,7 +2477,10 @@ class User implements IDBAccessObject {
         */
        public function setToken( $token = false ) {
                $this->load();
-               if ( !$token ) {
+               if ( $this->mToken === self::INVALID_TOKEN ) {
+                       \MediaWiki\Logger\LoggerFactory::getInstance( 'session' )
+                               ->debug( __METHOD__ . ": Ignoring attempt to set token for system user \"$this\"" );
+               } elseif ( !$token ) {
                        $this->mToken = MWCryptRand::generateHex( self::TOKEN_LENGTH );
                } else {
                        $this->mToken = $token;
index 4fde341..7277a1c 100644 (file)
@@ -1067,12 +1067,8 @@ class SessionManagerTest extends MediaWikiTestCase {
                        $this->objectCacheDef( $provider1 ),
                ) );
 
-               $user = User::newFromName( 'UTSysop' );
-               $token = $user->getToken( true );
-
                $this->assertFalse( $manager->isUserSessionPrevented( 'UTSysop' ) );
                $manager->preventSessionsForUser( 'UTSysop' );
-               $this->assertNotEquals( $token, User::newFromName( 'UTSysop' )->getToken() );
                $this->assertTrue( $manager->isUserSessionPrevented( 'UTSysop' ) );
        }