From 37af5099ccfaffe9c62a0357ad74a93ee6d8bb53 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Fri, 9 May 2014 16:43:54 +0100 Subject: [PATCH] Issue #2238087 by znerol, YesCT: Rebase SessionManager onto Symfony NativeSessionStorage. --- core/core.services.yml | 5 +- core/lib/Drupal/Core/Session/MetadataBag.php | 29 +++ .../Drupal/Core/Session/SessionHandler.php | 95 ++++------ .../Drupal/Core/Session/SessionManager.php | 174 +++++++++++++----- .../Core/Session/SessionManagerInterface.php | 50 ++--- 5 files changed, 228 insertions(+), 125 deletions(-) create mode 100644 core/lib/Drupal/Core/Session/MetadataBag.php diff --git a/core/core.services.yml b/core/core.services.yml index 4006ceb829..8e203ed8a3 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -767,9 +767,12 @@ services: arguments: ['@authentication', '@request'] session_manager: class: Drupal\Core\Session\SessionManager - arguments: ['@request_stack', '@database'] + arguments: ['@request_stack', '@database', '@session_manager.metadata_bag', '@settings'] tags: - { name: persist } + session_manager.metadata_bag: + class: Drupal\Core\Session\MetadataBag + arguments: ['@settings'] asset.css.collection_renderer: class: Drupal\Core\Asset\CssCollectionRenderer arguments: [ '@state' ] diff --git a/core/lib/Drupal/Core/Session/MetadataBag.php b/core/lib/Drupal/Core/Session/MetadataBag.php new file mode 100644 index 0000000000..164062b1f6 --- /dev/null +++ b/core/lib/Drupal/Core/Session/MetadataBag.php @@ -0,0 +1,29 @@ +get('session_write_interval', 180); + parent::__construct('_sf2_meta', $update_threshold); + } + +} diff --git a/core/lib/Drupal/Core/Session/SessionHandler.php b/core/lib/Drupal/Core/Session/SessionHandler.php index e93e840696..40d3c45afa 100644 --- a/core/lib/Drupal/Core/Session/SessionHandler.php +++ b/core/lib/Drupal/Core/Session/SessionHandler.php @@ -12,11 +12,12 @@ use Drupal\Core\Site\Settings; use Drupal\Core\Utility\Error; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy; /** * Default session handler. */ -class SessionHandler implements \SessionHandlerInterface { +class SessionHandler extends AbstractProxy implements \SessionHandlerInterface { /** * The session manager. @@ -39,13 +40,6 @@ class SessionHandler implements \SessionHandlerInterface { */ protected $connection; - /** - * An array containing the sid and data from last read. - * - * @var array - */ - protected $lastRead; - /** * Constructs a new SessionHandler instance. * @@ -77,9 +71,9 @@ public function read($sid) { // Handle the case of first time visitors and clients that don't store // cookies (eg. web crawlers). - $insecure_session_name = substr(session_name(), 1); + $insecure_session_name = $this->sessionManager->getInsecureName(); $cookies = $this->requestStack->getCurrentRequest()->cookies; - if (!$cookies->has(session_name()) && !$cookies->has($insecure_session_name)) { + if (!$cookies->has($this->getName()) && !$cookies->has($insecure_session_name)) { $user = new UserSession(); return ''; } @@ -136,11 +130,6 @@ public function read($sid) { $user = new UserSession(); } - // Store the session that was read for comparison in self::write(). - $this->lastRead = array( - 'sid' => $sid, - 'value' => $user->session, - ); return $user->session; } @@ -158,48 +147,40 @@ public function write($sid, $value) { // session. return TRUE; } - // Check whether $_SESSION has been changed in this request. - $is_changed = empty($this->lastRead) || $this->lastRead['sid'] != $sid || $this->lastRead['value'] !== $value; - - // For performance reasons, do not update the sessions table, unless - // $_SESSION has changed or more than 180 has passed since the last - // update. - $needs_update = !$user->getLastAccessedTime() || REQUEST_TIME - $user->getLastAccessedTime() > Settings::get('session_write_interval', 180); - if ($is_changed || $needs_update) { - // Either ssid or sid or both will be added from $key below. - $fields = array( - 'uid' => $user->id(), - 'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(), - 'session' => $value, - 'timestamp' => REQUEST_TIME, - ); - // Use the session ID as 'sid' and an empty string as 'ssid' by default. - // read() does not allow empty strings so that's a safe default. - $key = array('sid' => Crypt::hashBase64($sid), 'ssid' => ''); - // On HTTPS connections, use the session ID as both 'sid' and 'ssid'. - if ($this->requestStack->getCurrentRequest()->isSecure()) { - $key['ssid'] = Crypt::hashBase64($sid); - $cookies = $this->requestStack->getCurrentRequest()->cookies; - // The "secure pages" setting allows a site to simultaneously use both - // secure and insecure session cookies. If enabled and both cookies - // are presented then use both keys. The session ID from the cookie is - // hashed before being stored in the database as a security measure. - if (Settings::get('mixed_mode_sessions', FALSE)) { - $insecure_session_name = substr(session_name(), 1); - if ($cookies->has($insecure_session_name)) { - $key['sid'] = Crypt::hashBase64($cookies->get($insecure_session_name)); - } + // Either ssid or sid or both will be added from $key below. + $fields = array( + 'uid' => $user->id(), + 'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(), + 'session' => $value, + 'timestamp' => REQUEST_TIME, + ); + // Use the session ID as 'sid' and an empty string as 'ssid' by default. + // read() does not allow empty strings so that's a safe default. + $key = array('sid' => Crypt::hashBase64($sid), 'ssid' => ''); + // On HTTPS connections, use the session ID as both 'sid' and 'ssid'. + if ($this->requestStack->getCurrentRequest()->isSecure()) { + $key['ssid'] = Crypt::hashBase64($sid); + $cookies = $this->requestStack->getCurrentRequest()->cookies; + // The "secure pages" setting allows a site to simultaneously use both + // secure and insecure session cookies. If enabled and both cookies + // are presented then use both keys. The session ID from the cookie is + // hashed before being stored in the database as a security measure. + if ($this->sessionManager->isMixedMode()) { + $insecure_session_name = $this->sessionManager->getInsecureName(); + if ($cookies->has($insecure_session_name)) { + $key['sid'] = Crypt::hashBase64($cookies->get($insecure_session_name)); } } - elseif (Settings::get('mixed_mode_sessions', FALSE)) { - unset($key['ssid']); - } - $this->connection->merge('sessions') - ->keys($key) - ->fields($fields) - ->execute(); } + elseif ($this->sessionManager->isMixedMode()) { + unset($key['ssid']); + } + $this->connection->merge('sessions') + ->keys($key) + ->fields($fields) + ->execute(); + // Likewise, do not update access time more than once per 180 seconds. if ($user->isAuthenticated() && REQUEST_TIME - $user->getLastAccessedTime() > Settings::get('session_write_interval', 180)) { $this->connection->update('users') @@ -252,12 +233,12 @@ public function destroy($sid) { $user = new AnonymousUserSession(); // Unset the session cookies. - $this->deleteCookie(session_name()); + $this->deleteCookie($this->getName()); if ($is_https) { - $this->deleteCookie(substr(session_name(), 1), FALSE); + $this->deleteCookie($this->sessionManager->getInsecureName(), FALSE); } - elseif (Settings::get('mixed_mode_sessions', FALSE)) { - $this->deleteCookie('S' . session_name(), TRUE); + elseif ($this->sessionManager->isMixedMode()) { + $this->deleteCookie('S' . $this->getName(), TRUE); } return TRUE; } diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php index 6a08c6d64a..f656b2d7d8 100644 --- a/core/lib/Drupal/Core/Session/SessionManager.php +++ b/core/lib/Drupal/Core/Session/SessionManager.php @@ -13,11 +13,36 @@ use Drupal\Core\Session\SessionHandler; use Drupal\Core\Site\Settings; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler; +use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetadataBag; +use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage; /** * Manages user sessions. + * + * This class implements the custom session management code inherited from + * Drupal 7 on top of the corresponding Symfony component. Regrettably the name + * NativeSessionStorage is not quite accurate. In fact the responsibility for + * storing and retrieving session data has been extracted from it in Symfony 2.1 + * but the class name was not changed. + * + * @todo + * In fact the NativeSessionStorage class already implements all of the + * functionality required by a typical Symfony application. Normally it is not + * necessary to subclass it at all. In order to reach the point where Drupal + * can use the Symfony session management unmodified, the code implemented + * here needs to be extracted either into a dedicated session handler proxy + * (e.g. mixed mode SSL, sid-hashing) or relocated to the authentication + * subsystem. */ -class SessionManager implements SessionManagerInterface { +class SessionManager extends NativeSessionStorage implements SessionManagerInterface { + + /** + * Whether or not the session manager is operating in mixed mode SSL. + * + * @var bool + */ + protected $mixedMode; /** * The request stack. @@ -54,14 +79,30 @@ class SessionManager implements SessionManagerInterface { /** * Constructs a new session manager instance. * - * @param \Symfony\Component\HttpFoundation\RequestStack $request + * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack * The request stack. * @param \Drupal\Core\Database\Connection $connection * The database connection. + * @param \Symfony\Component\HttpFoundation\Session\Storage\MetadataBag $metadata_bag + * The session metadata bag. + * @param \Drupal\Core\Site\Settings $settings + * The settings instance. */ - public function __construct(RequestStack $request_stack, Connection $connection) { + public function __construct(RequestStack $request_stack, Connection $connection, SymfonyMetadataBag $metadata_bag, Settings $settings) { + parent::__construct(); $this->requestStack = $request_stack; $this->connection = $connection; + $this->setMetadataBag($metadata_bag); + + $this->setMixedMode($settings->get('mixed_mode_sessions', FALSE)); + + // @todo When not using the Symfony Session object, the list of bags in the + // NativeSessionStorage will remain uninitialized. This will lead to + // errors in NativeSessionHandler::loadSession. Remove this after + // https://drupal.org/node/2229145, when we will be using the Symfony + // session object (which registers an attribute bag with the + // manager upon instantiation). + $this->bags = array(); } /** @@ -72,12 +113,14 @@ public function initialize() { // Register the default session handler. // @todo Extract session storage from session handler into a service. - $handler = new SessionHandler($this, $this->requestStack, $this->connection); - session_set_save_handler($handler, TRUE); + $save_handler = new SessionHandler($this, $this->requestStack, $this->connection); + $write_check_handler = new WriteCheckSessionHandler($save_handler); + $this->setSaveHandler($write_check_handler); $is_https = $this->requestStack->getCurrentRequest()->isSecure(); $cookies = $this->requestStack->getCurrentRequest()->cookies; - if (($cookies->has(session_name()) && ($session_name = $cookies->get(session_name()))) || ($is_https && Settings::get('mixed_mode_sessions', FALSE) && ($cookies->has(substr(session_name(), 1))) && ($session_name = $cookies->get(substr(session_name(), 1))))) { + $insecure_session_name = $this->getInsecureName(); + if (($cookies->has($this->getName()) && ($session_name = $cookies->get($this->getName()))) || ($is_https && $this->isMixedMode() && ($cookies->has($insecure_session_name) && ($session_name = $cookies->get($insecure_session_name))))) { // If a session cookie exists, initialize the session. Otherwise the // session is only started on demand in save(), making // anonymous users not use a session cookie unless something is stored in @@ -94,12 +137,8 @@ public function initialize() { // session ID in advance. $this->lazySession = TRUE; $user = new AnonymousUserSession(); - // Less random sessions (which are much faster to generate) are used for - // anonymous users than are generated in regenerate() when - // a user becomes authenticated. - session_id(Crypt::randomBytesBase64()); - if ($is_https && Settings::get('mixed_mode_sessions', FALSE)) { - $insecure_session_name = substr(session_name(), 1); + $this->setId(Crypt::randomBytesBase64()); + if ($is_https && $this->isMixedMode()) { $session_id = Crypt::randomBytesBase64(); $cookies->set($insecure_session_name, $session_id); } @@ -113,18 +152,20 @@ public function initialize() { * {@inheritdoc} */ public function start() { - if ($this->isCli() || $this->isStarted()) { + if (!$this->isEnabled() || $this->isCli()) { return; } // Save current session data before starting it, as PHP will destroy it. $session_data = isset($_SESSION) ? $_SESSION : NULL; - session_start(); + $result = parent::start(); // Restore session data. if (!empty($session_data)) { $_SESSION += $session_data; } + + return $result; } /** @@ -141,7 +182,7 @@ public function save() { if ($user->isAnonymous() && $this->isSessionObsolete()) { // There is no session data to store, destroy the session if it was // previously started. - if ($this->isStarted()) { + if ($this->getSaveHandler()->isActive()) { session_destroy(); } } @@ -150,8 +191,8 @@ public function save() { // started. if (!$this->isStarted()) { $this->start(); - if ($this->requestStack->getCurrentRequest()->isSecure() && Settings::get('mixed_mode_sessions', FALSE)) { - $insecure_session_name = substr(session_name(), 1); + if ($this->requestStack->getCurrentRequest()->isSecure() && $this->isMixedMode()) { + $insecure_session_name = $this->getInsecureName(); $params = session_get_cookie_params(); $expire = $params['lifetime'] ? REQUEST_TIME + $params['lifetime'] : 0; $cookie_params = $this->requestStack->getCurrentRequest()->cookies; @@ -159,21 +200,14 @@ public function save() { } } // Write the session data. - session_write_close(); + parent::save(); } } /** * {@inheritdoc} */ - public function isStarted() { - return session_status() === \PHP_SESSION_ACTIVE; - } - - /** - * {@inheritdoc} - */ - public function regenerate() { + public function regenerate($destroy = FALSE, $lifetime = NULL) { global $user; // Nothing to do if we are not allowed to change the session. @@ -181,11 +215,17 @@ public function regenerate() { return; } + // We do not support the optional $destroy and $lifetime parameters as long + // as #2238561 remains open. + if ($destroy || isset($lifetime)) { + throw new \InvalidArgumentException('The optional parameters $destroy and $lifetime of SessionManager::regenerate() are not supported currently'); + } + $is_https = $this->requestStack->getCurrentRequest()->isSecure(); $cookies = $this->requestStack->getCurrentRequest()->cookies; - if ($is_https && Settings::get('mixed_mode_sessions', FALSE)) { - $insecure_session_name = substr(session_name(), 1); + if ($is_https && $this->isMixedMode()) { + $insecure_session_name = $this->getInsecureName(); if (!isset($this->lazySession) && $cookies->has($insecure_session_name)) { $old_insecure_session_id = $cookies->get($insecure_session_name); } @@ -200,14 +240,13 @@ public function regenerate() { } if ($this->isStarted()) { - $old_session_id = session_id(); + $old_session_id = $this->getId(); } session_id(Crypt::randomBytesBase64()); - // @todo As soon as https://drupal.org/node/2238087 lands, the token seed - // can be moved onto Drupal\Core\Session\MetadataBag. The session manager - // then needs to notify the metadata bag when the token should be - // regenerated. + // @todo The token seed can be moved onto \Drupal\Core\Session\MetadataBag. + // The session manager then needs to notify the metadata bag when the + // token should be regenerated. https://drupal.org/node/2256257 if (!empty($_SESSION)) { unset($_SESSION['csrf_token_seed']); } @@ -215,13 +254,13 @@ public function regenerate() { if (isset($old_session_id)) { $params = session_get_cookie_params(); $expire = $params['lifetime'] ? REQUEST_TIME + $params['lifetime'] : 0; - setcookie(session_name(), session_id(), $expire, $params['path'], $params['domain'], $params['secure'], $params['httponly']); - $fields = array('sid' => Crypt::hashBase64(session_id())); + setcookie($this->getName(), $this->getId(), $expire, $params['path'], $params['domain'], $params['secure'], $params['httponly']); + $fields = array('sid' => Crypt::hashBase64($this->getId())); if ($is_https) { - $fields['ssid'] = Crypt::hashBase64(session_id()); + $fields['ssid'] = Crypt::hashBase64($this->getId()); // If the "secure pages" setting is enabled, use the newly-created // insecure session identifier as the regenerated sid. - if (Settings::get('mixed_mode_sessions', FALSE)) { + if ($this->isMixedMode()) { $fields['sid'] = Crypt::hashBase64($session_id); } } @@ -235,7 +274,7 @@ public function regenerate() { // the secure site but a session was active on the insecure site, update // the insecure session with the new session identifiers. $this->connection->update('sessions') - ->fields(array('sid' => Crypt::hashBase64($session_id), 'ssid' => Crypt::hashBase64(session_id()))) + ->fields(array('sid' => Crypt::hashBase64($session_id), 'ssid' => Crypt::hashBase64($this->getId()))) ->condition('sid', Crypt::hashBase64($old_insecure_session_id)) ->execute(); } @@ -286,6 +325,27 @@ public function enable() { return $this; } + /** + * {@inheritdoc} + */ + public function isMixedMode() { + return $this->mixedMode; + } + + /** + * {@inheritdoc} + */ + public function setMixedMode($mixed_mode) { + $this->mixedMode = (bool) $mixed_mode; + } + + /** + * {@inheritdoc} + */ + public function getInsecureName() { + return substr($this->getName(), 1); + } + /** * Returns whether the current PHP process runs on CLI. * @@ -305,11 +365,29 @@ protected function isCli() { * destroyed. */ protected function isSessionObsolete() { - // Return early when $_SESSION is empty or not initialized. + $used_session_keys = array_filter($this->getSessionDataMask()); + return empty($used_session_keys); + } + + /** + * Returns a map specifying which session key is containing user data. + * + * @return array + * An array where keys correspond to the session keys and the values are + * booleans specifying whether the corresponding session key contains any + * user data. + */ + protected function getSessionDataMask() { if (empty($_SESSION)) { - return TRUE; + return array(); } + // Start out with a completely filled mask. + $mask = array_fill_keys(array_keys($_SESSION), TRUE); + + // Ignore the metadata bag, it does not contain any user data. + $mask[$this->metadataBag->getStorageKey()] = FALSE; + // Ignore the CSRF token seed. // // @todo Anonymous users should not get a CSRF token at any time, or if they @@ -317,10 +395,18 @@ protected function isSessionObsolete() { // session once obsolete. Since that is not guaranteed to be the case, // this check force-ignores the CSRF token, so as to avoid performance // regressions. - // As soon as https://drupal.org/node/2238087 lands, the token seed can be - // moved onto \Drupal\Core\Session\MetadataBag. This will result in the - // CSRF token to be ignored automatically. - return count(array_diff_key($_SESSION, array('csrf_token_seed' => TRUE))) == 0; + // The token seed can be moved onto \Drupal\Core\Session\MetadataBag. This + // will result in the CSRF token being ignored automatically. + // https://drupal.org/node/2256257 + $mask['csrf_token_seed'] = FALSE; + + // Ignore attribute bags when they do not contain any data. + foreach ($this->bags as $bag) { + $key = $bag->getStorageKey(); + $mask[$key] = empty($_SESSION[$key]); + } + + return array_intersect_key($mask, $_SESSION); } } diff --git a/core/lib/Drupal/Core/Session/SessionManagerInterface.php b/core/lib/Drupal/Core/Session/SessionManagerInterface.php index 346bc0da41..dc9aa4050a 100644 --- a/core/lib/Drupal/Core/Session/SessionManagerInterface.php +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php @@ -7,10 +7,12 @@ namespace Drupal\Core\Session; +use Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface; + /** * Defines the session manager interface. */ -interface SessionManagerInterface { +interface SessionManagerInterface extends SessionStorageInterface { /** * Initializes the session handler, starting a session if needed. @@ -19,28 +21,6 @@ interface SessionManagerInterface { */ public function initialize(); - /** - * Starts a session forcefully, preserving already set session data. - */ - public function start(); - - /** - * Commits the current session, if necessary. - * - * If an anonymous user already have an empty session, destroy it. - */ - public function save(); - - /** - * Returns whether a session has been started. - */ - public function isStarted(); - - /** - * Called when an anonymous user becomes authenticated or vice-versa. - */ - public function regenerate(); - /** * Ends a specific user's session(s). * @@ -77,4 +57,28 @@ public function disable(); */ public function enable(); + /** + * Returns whether mixed mode SSL sessions are enabled in the session manager. + * + * @return bool + * Value of the mixed mode SSL sessions flag. + */ + public function isMixedMode(); + + /** + * Enables or disables mixed mode SSL sessions in the session manager. + * + * @param bool $mixed_mode + * New value for the mixed mode SSL sessions flag. + */ + public function setMixedMode($mixed_mode); + + /** + * Returns the name of the insecure session when operating in mixed mode SSL. + * + * @return string + * The name of the insecure session. + */ + public function getInsecureName(); + } -- GitLab