Extended login: Don't use a $wg config variable, add UserName

CentralAuth needs 'User' as well for this to work.  However, this
shows the exact cookie names are an implementation detail that should
not be exposed as a 'wg'.

Instead, use a function in the CookieSessionProvider.  That way,
CentralAuth can override it properly without requiring users to change
$wg's.

I also added UserName. provideSessionInfo will fail to return
session info if UserID and UserName are both set and don't match.

Also, the UserID<->UserName mapping is public, so there is no
additional privacy issue.  Thus, it seems we should expire them
the same time.

Bug: T68699
Change-Id: Ia3259846433980408f79d44f665e17e15670e8ee

diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index 6beeac9..c625143 100644 (file)
--- a/RELEASE-NOTES-1.28
+++ b/RELEASE-NOTES-1.28
@@ -51,6 +51,8 @@ changes to languages because of Phabricator reports.
 
 === Other changes in 1.28 ===
 * (T128697) Improved handling of large diffs.
+* [BREAKING CHANGE] $wgExtendedLoginCookies has been removed.  You can
+  use or update a custom session provider if needed.
 
 == Compatibility ==
 

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index f176556..39e22a0 100644 (file)
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -5817,14 +5817,6 @@ $wgProxyList = [];
  */
 $wgCookieExpiration = 180 * 86400;
 
-/**
- * The identifiers of the login cookies that can have their lifetimes
- * extended independently of all other login cookies.
- *
- * @var string[]
- */
-$wgExtendedLoginCookies = [ 'UserID', 'Token' ];
-
 /**
  * Default login cookie lifetime, in seconds. Setting
  * $wgExtendLoginCookieExpiration to null will use $wgCookieExpiration to

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 3df0dae..79fc720 100644 (file)
--- a/includes/session/CookieSessionProvider.php
+++ b/includes/session/CookieSessionProvider.php
@@ -221,7 +221,7 @@ class CookieSessionProvider extends SessionProvider {
                        if ( $value === false ) {
                                $response->clearCookie( $key, $options );
                        } else {
-                               $expirationDuration = $this->getLoginCookieExpiration( $key );
+                               $expirationDuration = $this->getLoginCookieExpiration( $key, $session->shouldRememberUser() );
                                $expiration = $expirationDuration ? $expirationDuration + time() : null;
                                $response->setCookie( $key, (string)$value, $expiration, $options );
                        }
@@ -271,7 +271,10 @@ class CookieSessionProvider extends SessionProvider {
                $response = $request->response();
                if ( $set ) {
                        if ( $backend->shouldRememberUser() ) {
-                               $expirationDuration = $this->getLoginCookieExpiration( 'forceHTTPS' );
+                               $expirationDuration = $this->getLoginCookieExpiration(
+                                       'forceHTTPS',
+                                       true
+                               );
                                $expiration = $expirationDuration ? $expirationDuration + time() : null;
                        } else {
                                $expiration = null;
@@ -397,23 +400,40 @@ class CookieSessionProvider extends SessionProvider {
        }
 
        public function getRememberUserDuration() {
-               return min( $this->getLoginCookieExpiration( 'UserID' ),
-                       $this->getLoginCookieExpiration( 'Token' ) ) ?: null;
+               return min( $this->getLoginCookieExpiration( 'UserID', true ),
+                       $this->getLoginCookieExpiration( 'Token', true ) ) ?: null;
+       }
+
+       /**
+        * Gets the list of cookies that must be set to the 'remember me' duration,
+        * if $wgExtendedLoginCookieExpiration is in use.
+        *
+        * @return string[] Array of unprefixed cookie keys
+        */
+       protected function getExtendedLoginCookies() {
+               return [ 'UserID', 'UserName', 'Token' ];
        }
 
        /**
         * Returns the lifespan of the login cookies, in seconds. 0 means until the end of the session.
+        *
+        * Cookies that are session-length do not call this function.
+        *
         * @param string $cookieName
+        * @param boolean $shouldRememberUser Whether the user should be remembered
+        *   long-term
         * @return int Cookie expiration time in seconds; 0 for session cookies
         */
-       protected function getLoginCookieExpiration( $cookieName ) {
+       protected function getLoginCookieExpiration( $cookieName, $shouldRememberUser ) {
+               $extendedCookies = $this->getExtendedLoginCookies();
                $normalExpiration = $this->config->get( 'CookieExpiration' );
-               $extendedExpiration = $this->config->get( 'ExtendedLoginCookieExpiration' );
-               $extendedCookies = $this->config->get( 'ExtendedLoginCookies' );
 
-               if ( !in_array( $cookieName, $extendedCookies, true ) ) {
+               if ( $shouldRememberUser && in_array( $cookieName, $extendedCookies, true ) ) {
+                       $extendedExpiration = $this->config->get( 'ExtendedLoginCookieExpiration' );
+
+                       return ( $extendedExpiration !== null ) ? (int)$extendedExpiration : (int)$normalExpiration;
+               } else {
                        return (int)$normalExpiration;
                }
-               return ( $extendedExpiration !== null ) ? (int)$extendedExpiration : (int)$normalExpiration;
        }
 }

diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php
index b35b685..da4b06e 100644 (file)
--- a/tests/phpunit/includes/session/CookieSessionProviderTest.php
+++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php
@@ -22,7 +22,6 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                        'CookieHttpOnly' => true,
                        'SessionName' => false,
                        'CookieExpiration' => 100,
-                       'ExtendedLoginCookies' => [ 'UserID', 'Token' ],
                        'ExtendedLoginCookieExpiration' => 200,
                ] );
        }
@@ -148,6 +147,14 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $this->assertTrue( $provider->persistsSessionId() );
                $this->assertTrue( $provider->canChangeUser() );
 
+               $extendedCookies = [ 'UserID', 'UserName', 'Token' ];
+
+               $this->assertEquals(
+                       $extendedCookies,
+                       \TestingAccessWrapper::newFromObject( $provider )->getExtendedLoginCookies(),
+                       'List of extended cookies (subclasses can add values, but we\'re calling the core one here)'
+               );
+
                $msg = $provider->whyNoSession();
                $this->assertInstanceOf( 'Message', $msg );
                $this->assertSame( 'sessionprovider-nocookies', $msg->getKey() );
@@ -506,10 +513,10 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                        'httpOnly' => $config->get( 'CookieHttpOnly' ),
                        'raw' => false,
                ];
+
+               $normalExpiry = $config->get( 'CookieExpiration' );
                $extendedExpiry = $config->get( 'ExtendedLoginCookieExpiration' );
                $extendedExpiry = (int)( $extendedExpiry === null ? 0 : $extendedExpiry );
-               $this->assertEquals( [ 'UserID', 'Token' ], $config->get( 'ExtendedLoginCookies' ),
-                       'sanity check' );
                $expect = [
                        'MySessionName' => [
                                'value' => (string)$sessionId,
@@ -517,10 +524,11 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                        ] + $defaults,
                        'xUserID' => [
                                'value' => (string)$user->getId(),
-                               'expire' => $extendedExpiry,
+                               'expire' => $remember ? $extendedExpiry : $normalExpiry,
                        ] + $defaults,
                        'xUserName' => [
                                'value' => $user->getName(),
+                               'expire' => $remember ? $extendedExpiry : $normalExpiry
                        ] + $defaults,
                        'xToken' => [
                                'value' => $remember ? $user->getToken() : '',
@@ -807,12 +815,20 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $provider->setConfig( $config );
                $provider->setManager( SessionManager::singleton() );
 
-               $this->assertSame( 200, $provider->getLoginCookieExpiration( 'Token' ) );
-               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User' ) );
+               // First cookie is an extended cookie, remember me true
+               $this->assertSame( 200, $provider->getLoginCookieExpiration( 'Token', true ) );
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User', true ) );
+
+               // First cookie is an extended cookie, remember me false
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'UserID', false ) );
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User', false ) );
 
                $config->set( 'ExtendedLoginCookieExpiration', null );
 
-               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'Token' ) );
-               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User' ) );
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'Token', true ) );
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User', true ) );
+
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'Token', false ) );
+               $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User', false ) );
        }
 }