From f013c5fec3c274b063d7b875aec415e3ffe9ad17 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Fri, 13 May 2016 00:03:20 +0000 Subject: [PATCH] Add SessionProvider::getRememberUserDuration(), fix some durations - handle $wgExtendedLoginCookieExpiration = 0, $wgCookieExpiration >0 correctly (as nonsensical as it is) - honor $wgExtendedLoginCookies for forceHTTPS - consistently ignore shouldRememberUser in ImmutableSessionProviderWithCookie Change-Id: I1e8fc632b52694aa6eb34ca1e9eae6d0b57df920 --- includes/session/CookieSessionProvider.php | 40 +++++++++++++----- .../ImmutableSessionProviderWithCookie.php | 2 +- includes/session/SessionProvider.php | 10 +++++ .../session/CookieSessionProviderTest.php | 42 ++++++++++++++++--- ...ImmutableSessionProviderWithCookieTest.php | 2 +- .../includes/session/SessionProviderTest.php | 2 + 6 files changed, 81 insertions(+), 17 deletions(-) diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php index 8ce3174dc0..3df0daefde 100644 --- a/includes/session/CookieSessionProvider.php +++ b/includes/session/CookieSessionProvider.php @@ -217,19 +217,13 @@ class CookieSessionProvider extends SessionProvider { [ 'prefix' => '' ] + $options ); - $extendedCookies = $this->config->get( 'ExtendedLoginCookies' ); - $extendedExpiry = $this->config->get( 'ExtendedLoginCookieExpiration' ); - foreach ( $cookies as $key => $value ) { if ( $value === false ) { $response->clearCookie( $key, $options ); } else { - if ( $extendedExpiry !== null && in_array( $key, $extendedCookies ) ) { - $expiry = time() + (int)$extendedExpiry; - } else { - $expiry = 0; // Default cookie expiration - } - $response->setCookie( $key, (string)$value, $expiry, $options ); + $expirationDuration = $this->getLoginCookieExpiration( $key ); + $expiration = $expirationDuration ? $expirationDuration + time() : null; + $response->setCookie( $key, (string)$value, $expiration, $options ); } } @@ -276,7 +270,13 @@ class CookieSessionProvider extends SessionProvider { ) { $response = $request->response(); if ( $set ) { - $response->setCookie( 'forceHTTPS', 'true', $backend->shouldRememberUser() ? 0 : null, + if ( $backend->shouldRememberUser() ) { + $expirationDuration = $this->getLoginCookieExpiration( 'forceHTTPS' ); + $expiration = $expirationDuration ? $expirationDuration + time() : null; + } else { + $expiration = null; + } + $response->setCookie( 'forceHTTPS', 'true', $expiration, [ 'prefix' => '', 'secure' => false ] + $this->cookieOptions ); } else { $response->clearCookie( 'forceHTTPS', @@ -396,4 +396,24 @@ class CookieSessionProvider extends SessionProvider { return wfMessage( 'sessionprovider-nocookies' ); } + public function getRememberUserDuration() { + return min( $this->getLoginCookieExpiration( 'UserID' ), + $this->getLoginCookieExpiration( 'Token' ) ) ?: null; + } + + /** + * Returns the lifespan of the login cookies, in seconds. 0 means until the end of the session. + * @param string $cookieName + * @return int Cookie expiration time in seconds; 0 for session cookies + */ + protected function getLoginCookieExpiration( $cookieName ) { + $normalExpiration = $this->config->get( 'CookieExpiration' ); + $extendedExpiration = $this->config->get( 'ExtendedLoginCookieExpiration' ); + $extendedCookies = $this->config->get( 'ExtendedLoginCookies' ); + + if ( !in_array( $cookieName, $extendedCookies, true ) ) { + return (int)$normalExpiration; + } + return ( $extendedExpiration !== null ) ? (int)$extendedExpiration : (int)$normalExpiration; + } } diff --git a/includes/session/ImmutableSessionProviderWithCookie.php b/includes/session/ImmutableSessionProviderWithCookie.php index 2f6e1330a2..1cab3d3c0a 100644 --- a/includes/session/ImmutableSessionProviderWithCookie.php +++ b/includes/session/ImmutableSessionProviderWithCookie.php @@ -113,7 +113,7 @@ abstract class ImmutableSessionProviderWithCookie extends SessionProvider { $options = $this->sessionCookieOptions; if ( $session->shouldForceHTTPS() || $session->getUser()->requiresHTTPS() ) { - $response->setCookie( 'forceHTTPS', 'true', $session->shouldRememberUser() ? 0 : null, + $response->setCookie( 'forceHTTPS', 'true', null, [ 'prefix' => '', 'secure' => false ] + $options ); $options['secure'] = true; } diff --git a/includes/session/SessionProvider.php b/includes/session/SessionProvider.php index ed113b7ad8..50794d0104 100644 --- a/includes/session/SessionProvider.php +++ b/includes/session/SessionProvider.php @@ -275,6 +275,16 @@ abstract class SessionProvider implements SessionProviderInterface, LoggerAwareI */ abstract public function canChangeUser(); + /** + * Returns the duration (in seconds) for which users will be remembered when + * Session::setRememberUser() is set. Null means setting the remember flag will + * have no effect (and endpoints should not offer that option). + * @return int|null + */ + public function getRememberUserDuration() { + return null; + } + /** * Notification that the session ID was reset * diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php index 70e89d4b0f..9600184d0f 100644 --- a/tests/phpunit/includes/session/CookieSessionProviderTest.php +++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php @@ -14,7 +14,6 @@ use Psr\Log\LogLevel; class CookieSessionProviderTest extends MediaWikiTestCase { private function getConfig() { - global $wgCookieExpiration; return new \HashConfig( [ 'CookiePrefix' => 'CookiePrefix', 'CookiePath' => 'CookiePath', @@ -22,8 +21,9 @@ class CookieSessionProviderTest extends MediaWikiTestCase { 'CookieSecure' => true, 'CookieHttpOnly' => true, 'SessionName' => false, + 'CookieExpiration' => 100, 'ExtendedLoginCookies' => [ 'UserID', 'Token' ], - 'ExtendedLoginCookieExpiration' => $wgCookieExpiration * 2, + 'ExtendedLoginCookieExpiration' => 200, ] ); } @@ -377,8 +377,6 @@ class CookieSessionProviderTest extends MediaWikiTestCase { } public function testPersistSession() { - $this->setMwGlobals( [ 'wgCookieExpiration' => 100 ] ); - $provider = new CookieSessionProvider( [ 'priority' => 1, 'sessionName' => 'MySessionName', @@ -461,7 +459,6 @@ class CookieSessionProviderTest extends MediaWikiTestCase { */ public function testCookieData( $secure, $remember ) { $this->setMwGlobals( [ - 'wgCookieExpiration' => 100, 'wgSecureLogin' => false, ] ); @@ -783,4 +780,39 @@ class CookieSessionProviderTest extends MediaWikiTestCase { $this->assertNull( $provider->getCookie( $request, 'Baz', 'x' ) ); } + public function testGetRememberUserDuration() { + $config = $this->getConfig(); + $provider = new CookieSessionProvider( [ 'priority' => 10 ] ); + $provider->setLogger( new \Psr\Log\NullLogger() ); + $provider->setConfig( $config ); + $provider->setManager( SessionManager::singleton() ); + + $this->assertSame( 200, $provider->getRememberUserDuration() ); + + $config->set( 'ExtendedLoginCookieExpiration', null ); + + $this->assertSame( 100, $provider->getRememberUserDuration() ); + + $config->set( 'ExtendedLoginCookieExpiration', 0 ); + + $this->assertSame( null, $provider->getRememberUserDuration() ); + } + + public function testGetLoginCookieExpiration() { + $config = $this->getConfig(); + $provider = \TestingAccessWrapper::newFromObject( new CookieSessionProvider( [ + 'priority' => 10 + ] ) ); + $provider->setLogger( new \Psr\Log\NullLogger() ); + $provider->setConfig( $config ); + $provider->setManager( SessionManager::singleton() ); + + $this->assertSame( 200, $provider->getLoginCookieExpiration( 'Token' ) ); + $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User' ) ); + + $config->set( 'ExtendedLoginCookieExpiration', null ); + + $this->assertSame( 100, $provider->getLoginCookieExpiration( 'Token' ) ); + $this->assertSame( 100, $provider->getLoginCookieExpiration( 'User' ) ); + } } diff --git a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php index d705fc0191..78edb7671e 100644 --- a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php +++ b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php @@ -249,7 +249,7 @@ class ImmutableSessionProviderWithCookieTest extends MediaWikiTestCase { } $this->assertEquals( [ 'value' => 'true', - 'expire' => $remember ? 100 : null, + 'expire' => null, 'path' => 'CookiePath', 'domain' => 'CookieDomain', 'secure' => false, diff --git a/tests/phpunit/includes/session/SessionProviderTest.php b/tests/phpunit/includes/session/SessionProviderTest.php index f80baf2fa6..4cbeeb954c 100644 --- a/tests/phpunit/includes/session/SessionProviderTest.php +++ b/tests/phpunit/includes/session/SessionProviderTest.php @@ -35,6 +35,8 @@ class SessionProviderTest extends MediaWikiTestCase { $this->assertSame( get_class( $provider ), (string)$provider ); + $this->assertNull( $provider->getRememberUserDuration() ); + $this->assertNull( $provider->whyNoSession() ); $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [ -- 2.20.1