From 2c34aeea72471f9a598e67bdbf34bc5f9fb3f0c5 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Sat, 23 Jan 2016 12:34:27 -0500 Subject: [PATCH] SessionManager: Abstract forceHTTPS cookie setting This allows CentralAuthSessionProvider to avoid doing craziness like this all the time: Set-Cookie: forceHTTPS=true; path=/; httponly Set-Cookie: forceHTTPS=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; httponly Set-Cookie: forceHTTPS=true; path=/; domain=.wikipedia.org; httponly Set-Cookie: forceHTTPS=true; path=/; httponly Set-Cookie: forceHTTPS=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; httponly Set-Cookie: forceHTTPS=true; path=/; domain=.wikipedia.org; httponly Bug: T124421 Change-Id: I7e02afd032a246df6850208c26d3447798bc0fc2 --- includes/session/CookieSessionProvider.php | 29 +++++++++++++++---- .../session/CookieSessionProviderTest.php | 14 ++++----- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php index 7854416fa1..915127ff3c 100644 --- a/includes/session/CookieSessionProvider.php +++ b/includes/session/CookieSessionProvider.php @@ -173,9 +173,9 @@ class CookieSessionProvider extends SessionProvider { } $options = $this->cookieOptions; - if ( $session->shouldForceHTTPS() || $user->requiresHTTPS() ) { - $response->setCookie( 'forceHTTPS', 'true', $session->shouldRememberUser() ? 0 : null, - array( 'prefix' => '', 'secure' => false ) + $options ); + + $forceHTTPS = $session->shouldForceHTTPS() || $user->requiresHTTPS(); + if ( $forceHTTPS ) { $options['secure'] = true; } @@ -199,6 +199,7 @@ class CookieSessionProvider extends SessionProvider { } } + $this->setForceHTTPSCookie( $forceHTTPS, $session, $request ); $this->setLoggedOutCookie( $session->getLoggedOutTimestamp(), $request ); if ( $sessionData ) { @@ -227,8 +228,26 @@ class CookieSessionProvider extends SessionProvider { $response->clearCookie( $key, $this->cookieOptions ); } - $response->clearCookie( 'forceHTTPS', - array( 'prefix' => '', 'secure' => false ) + $this->cookieOptions ); + $this->setForceHTTPSCookie( false, null, $request ); + } + + /** + * Set the "forceHTTPS" cookie + * @param bool $set Whether the cookie should be set or not + * @param SessionBackend|null $backend + * @param WebRequest $request + */ + protected function setForceHTTPSCookie( + $set, SessionBackend $backend = null, WebRequest $request + ) { + $response = $request->response(); + if ( $set ) { + $response->setCookie( 'forceHTTPS', 'true', $backend->shouldRememberUser() ? 0 : null, + array( 'prefix' => '', 'secure' => false ) + $this->cookieOptions ); + } else { + $response->clearCookie( 'forceHTTPS', + array( 'prefix' => '', 'secure' => false ) + $this->cookieOptions ); + } } /** diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php index a73bf7c098..ccf45f68fd 100644 --- a/tests/phpunit/includes/session/CookieSessionProviderTest.php +++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php @@ -382,7 +382,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { $this->assertSame( '', $request->response()->getCookie( 'xUserID' ) ); $this->assertSame( null, $request->response()->getCookie( 'xUserName' ) ); $this->assertSame( '', $request->response()->getCookie( 'xToken' ) ); - $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) ); + $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) ); $this->assertSame( array(), $backend->getData() ); // Logged-in user, no remember @@ -395,7 +395,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { $this->assertSame( (string)$user->getId(), $request->response()->getCookie( 'xUserID' ) ); $this->assertSame( $user->getName(), $request->response()->getCookie( 'xUserName' ) ); $this->assertSame( '', $request->response()->getCookie( 'xToken' ) ); - $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) ); + $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) ); $this->assertSame( array(), $backend->getData() ); // Logged-in user, remember @@ -488,10 +488,10 @@ class CookieSessionProviderTest extends MediaWikiTestCase { 'value' => $remember ? $user->getToken() : '', 'expire' => $remember ? $extendedExpiry : -31536000, ) + $defaults, - 'forceHTTPS' => !$secure ? null : array( - 'value' => 'true', + 'forceHTTPS' => array( + 'value' => $secure ? 'true' : '', 'secure' => false, - 'expire' => $remember ? $defaults['expire'] : null, + 'expire' => $secure ? $remember ? $defaults['expire'] : 0 : -31536000, ) + $defaults, ); foreach ( $expect as $key => $value ) { @@ -571,7 +571,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { $this->assertSame( '', $request->response()->getCookie( 'xUserID' ) ); $this->assertSame( null, $request->response()->getCookie( 'xUserName' ) ); $this->assertSame( '', $request->response()->getCookie( 'xToken' ) ); - $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) ); + $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) ); $this->assertSame( array(), $backend->getData() ); $provider->persistSession( $backend, $this->getSentRequest() ); @@ -607,7 +607,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase { $this->assertSame( (string)$user->getId(), $request->response()->getCookie( 'xUserID' ) ); $this->assertSame( $user->getName(), $request->response()->getCookie( 'xUserName' ) ); $this->assertSame( '', $request->response()->getCookie( 'xToken' ) ); - $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) ); + $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) ); $this->assertSame( 'bar!', $request->response()->getCookie( 'xbar' ) ); $this->assertSame( (string)$loggedOut, $request->response()->getCookie( 'xLoggedOut' ) ); $this->assertEquals( array( -- 2.20.1