Remove session limits and sequencing

epriestley · epriestley · commit 2ec45d42a689 · 2014-01-15T17:27:59.000-08:00
Summary: Ref T4310. Fixes T3720. This change: - Removes concurrent session limits. Instead, unused sessions are GC'd after a while. - Collapses all existing "web-1", "web-2", etc., sessions into "web" sessions. - Dramatically simplifies the code for establishing a session (like omg). Test Plan: Ran migration, checked Sessions panel and database for sanity. Used existing session. Logged out, logged in. Ran Conduit commands. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4310, T3720 Differential Revision: https://secure.phabricator.com/D7978
diff --git a/conf/default.conf.php b/conf/default.conf.php
@@ -543,15 +543,6 @@
 
 // -- Auth ------------------------------------------------------------------ //
 
-  // Maximum number of simultaneous web sessions each user is permitted to have.
-  // Setting this to "1" will prevent a user from logging in on more than one
-  // browser at the same time.
-  'auth.sessions.web'           => 5,
-
-  // Maximum number of simultaneous Conduit sessions each user is permitted
-  // to have.
-  'auth.sessions.conduit'       => 5,
-
   // If true, email addresses must be verified (by clicking a link in an
   // email) before a user can login. By default, verification is optional
   // unless 'auth.email-domains' is nonempty (see below).
diff --git a/resources/sql/autopatches/20140115.auth.3.unlimit.php b/resources/sql/autopatches/20140115.auth.3.unlimit.php
@@ -0,0 +1,26 @@
+<?php
+
+// Prior to this patch, we issued sessions "web-1", "web-2", etc., up to some
+// limit. This collapses all the "web-X" sessions into "web" sessions.
+
+$session_table = new PhabricatorAuthSession();
+$conn_w = $session_table->establishConnection('w');
+
+foreach (new LiskMigrationIterator($session_table) as $session) {
+  $id = $session->getID();
+
+  echo "Migrating session {$id}...\n";
+  $old_type = $session->getType();
+  $new_type = preg_replace('/-.*$/', '', $old_type);
+
+  if ($old_type !== $new_type) {
+    queryfx(
+      $conn_w,
+      'UPDATE %T SET type = %s WHERE id = %d',
+      $session_table->getTableName(),
+      $new_type,
+      $id);
+  }
+}
+
+echo "Done.\n";
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -14,10 +14,10 @@ public function loadUserForSession($session_type, $session_key) {
       $conn_r,
       'SELECT s.sessionExpires AS _sessionExpires, s.id AS _sessionID, u.*
         FROM %T u JOIN %T s ON u.phid = s.userPHID
-        AND s.type LIKE %> AND s.sessionKey = %s',
+        AND s.type = %s AND s.sessionKey = %s',
       $user_table->getTableName(),
       $session_table->getTableName(),
-      $session_type.'-',
+      $session_type,
       PhabricatorHash::digest($session_key));
 
     if (!$info) {
@@ -72,142 +72,35 @@ public function establishSession($session_type, $identity_phid) {
     $session_table = new PhabricatorAuthSession();
     $conn_w = $session_table->establishConnection('w');
 
-    if (strpos($session_type, '-') !== false) {
-      throw new Exception("Session type must not contain hyphen ('-')!");
-    }
-
-    // We allow multiple sessions of the same type, so when a caller requests
-    // a new session of type "web", we give them the first available session in
-    // "web-1", "web-2", ..., "web-N", up to some configurable limit. If none
-    // of these sessions is available, we overwrite the oldest session and
-    // reissue a new one in its place.
-
-    $session_limit = 1;
-    switch ($session_type) {
-      case PhabricatorAuthSession::TYPE_WEB:
-        $session_limit = PhabricatorEnv::getEnvConfig('auth.sessions.web');
-        break;
-      case PhabricatorAuthSession::TYPE_CONDUIT:
-        $session_limit = PhabricatorEnv::getEnvConfig('auth.sessions.conduit');
-        break;
-      default:
-        throw new Exception("Unknown session type '{$session_type}'!");
-    }
-
-    $session_limit = (int)$session_limit;
-    if ($session_limit <= 0) {
-      throw new Exception(
-        "Session limit for '{$session_type}' must be at least 1!");
-    }
-
-    // NOTE: Session establishment is sensitive to race conditions, as when
-    // piping `arc` to `arc`:
-    //
-    //   arc export ... | arc paste ...
-    //
-    // To avoid this, we overwrite an old session only if it hasn't been
-    // re-established since we read it.
-
     // Consume entropy to generate a new session key, forestalling the eventual
     // heat death of the universe.
     $session_key = Filesystem::readRandomCharacters(40);
-    $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
-
-    // Load all the currently active sessions.
-    $sessions = queryfx_all(
-      $conn_w,
-      'SELECT type, sessionKey, sessionStart FROM %T
-        WHERE userPHID = %s AND type LIKE %>',
-      $session_table->getTableName(),
-      $identity_phid,
-      $session_type.'-');
-    $sessions = ipull($sessions, null, 'type');
-    $sessions = isort($sessions, 'sessionStart');
 
-    $existing_sessions = array_keys($sessions);
+    // This has a side effect of validating the session type.
+    $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
 
-    // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
+    // Logging-in users don't have CSRF stuff yet, so we have to unguard this
+    // write.
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
-    $retries = 0;
-    while (true) {
-
-      // Choose which 'type' we'll actually establish, i.e. what number we're
-      // going to append to the basic session type. To do this, just check all
-      // the numbers sequentially until we find an available session.
-      $establish_type = null;
-      for ($ii = 1; $ii <= $session_limit; $ii++) {
-        $try_type = $session_type.'-'.$ii;
-        if (!in_array($try_type, $existing_sessions)) {
-          $establish_type = $try_type;
-          $expect_key = PhabricatorHash::digest($session_key);
-          $existing_sessions[] = $try_type;
-
-          // Ensure the row exists so we can issue an update below. We don't
-          // care if we race here or not.
-          queryfx(
-            $conn_w,
-            'INSERT IGNORE INTO %T
-              (userPHID, type, sessionKey, sessionStart, sessionExpires)
-              VALUES (%s, %s, %s, 0, UNIX_TIMESTAMP() + %d)',
-            $session_table->getTableName(),
-            $identity_phid,
-            $establish_type,
-            PhabricatorHash::digest($session_key),
-            $session_ttl);
-          break;
-        }
-      }
-
-      // If we didn't find an available session, choose the oldest session and
-      // overwrite it.
-      if (!$establish_type) {
-        $oldest = reset($sessions);
-        $establish_type = $oldest['type'];
-        $expect_key = $oldest['sessionKey'];
-      }
-
-      // This is so that we'll only overwrite the session if it hasn't been
-      // refreshed since we read it. If it has, the session key will be
-      // different and we know we're racing other processes. Whichever one
-      // won gets the session, we go back and try again.
-
-      queryfx(
-        $conn_w,
-        'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP(),
-            sessionExpires = UNIX_TIMESTAMP() + %d
-          WHERE userPHID = %s AND type = %s AND sessionKey = %s',
-        $session_table->getTableName(),
-        PhabricatorHash::digest($session_key),
-        $session_ttl,
+      id(new PhabricatorAuthSession())
+        ->setUserPHID($identity_phid)
+        ->setType($session_type)
+        ->setSessionKey(PhabricatorHash::digest($session_key))
+        ->setSessionStart(time())
+        ->setSessionExpires(time() + $session_ttl)
+        ->save();
+
+      $log = PhabricatorUserLog::initializeNewLog(
+        null,
         $identity_phid,
-        $establish_type,
-        $expect_key);
-
-      if ($conn_w->getAffectedRows()) {
-        // The update worked, so the session is valid.
-        break;
-      } else {
-        // We know this just got grabbed, so don't try it again.
-        unset($sessions[$establish_type]);
-      }
-
-      if (++$retries > $session_limit) {
-        throw new Exception("Failed to establish a session!");
-      }
-    }
-
-    $log = PhabricatorUserLog::initializeNewLog(
-      null,
-      $identity_phid,
-      PhabricatorUserLog::ACTION_LOGIN);
-    $log->setDetails(
-      array(
-        'session_type' => $session_type,
-        'session_issued' => $establish_type,
-      ));
-    $log->setSession($session_key);
-    $log->save();
+        PhabricatorUserLog::ACTION_LOGIN);
+      $log->setDetails(
+        array(
+          'session_type' => $session_type,
+        ));
+      $log->setSession($session_key);
+      $log->save();
+    unset($unguarded);
 
     return $session_key;
   }
diff --git a/src/applications/auth/query/PhabricatorAuthSessionQuery.php b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
@@ -81,25 +81,17 @@ protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
     }
 
     if ($this->sessionTypes) {
-      $clauses = array();
-      foreach ($this->sessionTypes as $session_type) {
-        $clauses[] = qsprintf(
-          $conn_r,
-          'type LIKE %>',
-          $session_type.'-');
-      }
-      $where[] = '('.implode(') OR (', $clauses).')';
+      $where[] = qsprintf(
+        $conn_r,
+        'type IN (%Ls)',
+        $this->sessionTypes);
     }
 
     $where[] = $this->buildPagingClause($conn_r);
 
     return $this->formatWhereClause($where);
   }
 
-  public function getPagingColumn() {
-    return 'sessionKey';
-  }
-
   public function getQueryApplicationClass() {
     return 'PhabricatorApplicationAuth';
   }
diff --git a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php
@@ -146,6 +146,10 @@ public static function getAncientConfig() {
       'PhabricatorRemarkupCustomInlineRule or '.
       'PhabricatorRemarkupCustomBlockRule.');
 
+    $session_reason = pht(
+      'Sessions now expire and are garbage collected rather than having an '.
+      'arbitrary concurrency limit.');
+
     $ancient_config += array(
       'phid.external-loaders' =>
         pht(
@@ -172,6 +176,8 @@ public static function getAncientConfig() {
         'multiple maps. See T4222.'),
       'metamta.send-immediately' => pht(
         'Mail is now always delivered by the daemons.'),
+      'auth.sessions.conduit' => $session_reason,
+      'auth.sessions.web' => $session_reason,
     );
 
     return $ancient_config;
diff --git a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php
@@ -13,22 +13,6 @@ public function getDescription() {
 
   public function getOptions() {
     return array(
-      $this->newOption('auth.sessions.web', 'int', 5)
-        ->setSummary(
-          pht("Number of web sessions a user can have simultaneously."))
-        ->setDescription(
-          pht(
-            "Maximum number of simultaneous web sessions each user is ".
-            "permitted to have. Setting this to '1' will prevent a user from ".
-            "logging in on more than one browser at the same time.")),
-      $this->newOption('auth.sessions.conduit', 'int', 5)
-        ->setSummary(
-          pht(
-            "Number of simultaneous Conduit sessions each user is permitted."))
-        ->setDescription(
-          pht(
-            "Maximum number of simultaneous Conduit sessions each user is ".
-            "permitted to have.")),
       $this->newOption('auth.require-email-verification', 'bool', false)
         ->setBoolOptions(
           array(
