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

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 8ce3174..3df0dae 100644 (file)
--- 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 2f6e133..1cab3d3 100644 (file)
--- 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 ed113b7..50794d0 100644 (file)
--- 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 70e89d4..9600184 100644 (file)
--- 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 d705fc0..78edb76 100644 (file)
--- 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 f80baf2..4cbeeb9 100644 (file)
--- 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, [