From: Matthew Flaschen <mflaschen@wikimedia.org>
Date: Wed, 22 Jun 2016 16:36:16 +0000 (+0200)
Subject: Extended login: Don't use a $wg config variable, add UserName
X-Git-Tag: 1.31.0-rc.0~6526^2
X-Git-Url: https://git.cyclocoop.org/%7B%24admin_url%7Dmembres/modifier.php?a=commitdiff_plain;h=04993acecbe27c5df6d1191d2b694d6ce450c3b7;p=lhc%2Fweb%2Fwiklou.git

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 6beeac9344..c6251435eb 100644
--- 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 f176556c48..39e22a0277 100644
--- 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 3df0daefde..79fc720d1e 100644
--- 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 b35b6850df..da4b06ec83 100644
--- 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 ) );
 	}
 }