Issue #2238087 by znerol, YesCT: Rebase SessionManager onto Symfony NativeSessionStorage.

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' ]

core/lib/Drupal/Core/Session/MetadataBag.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Session\MetadataBag.
+ */
+
+namespace Drupal\Core\Session;
+
+use Drupal\Core\Site\Settings;
+use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetadataBag;
+
+/**
+ * Provides a container for application specific session metadata.
+ */
+class MetadataBag extends SymfonyMetadataBag {
+
+  /**
+   * Constructs a new metadata bag instance.
+   *
+   * @param \Drupal\Core\Site\Settings $settings
+   *   The settings instance.
+   */
+  public function __construct(Settings $settings) {
+    $update_threshold = $settings->get('session_write_interval', 180);
+    parent::__construct('_sf2_meta', $update_threshold);
+  }
+
+}

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;
   }

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.
+    //   The token seed can be moved onto \Drupal\Core\Session\MetadataBag. This
+    //   will result in the CSRF token being ignored automatically.