Separate session management from PhabricatorUser

Summary: Ref T4310. Ref T3720. Session operations are currently part of PhabricatorUser. This is more tightly coupled than needbe, and makes it difficult to establish login sessions for non-users. Move all the session management code to a `SessionEngine`.

Test Plan:
  - Viewed sessions.
  - Regenerated Conduit certificate.
  - Verified Conduit sessions were destroyed.
  - Logged out.
  - Logged in.
  - Ran conduit commands.
  - Viewed sessions again.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7962

src/__phutil_library_map__.php

@@ -1203,6 +1203,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthProviderPersona' => 'applications/auth/provider/PhabricatorAuthProviderPersona.php',
     'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php',
     'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php',
+    'PhabricatorAuthSessionEngine' => 'applications/auth/engine/PhabricatorAuthSessionEngine.php',
     'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
     'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
     'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php',
@@ -3768,6 +3769,7 @@ phutil_register_library_map(array(
       0 => 'PhabricatorAuthDAO',
       1 => 'PhabricatorPolicyInterface',
     ),
+    'PhabricatorAuthSessionEngine' => 'Phobject',
     'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthStartController' => 'PhabricatorAuthController',
     'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController',

src/applications/auth/controller/PhabricatorAuthController.php

@@ -81,7 +81,8 @@ abstract class PhabricatorAuthController extends PhabricatorController {
 
     $should_login = $event->getValue('shouldLogin');
     if ($should_login) {
-      $session_key = $user->establishSession($session_type);
+      $session_key = id(new PhabricatorAuthSessionEngine())
+        ->establishSession($session_type, $user->getPHID());
 
       // NOTE: We allow disabled users to login and roadblock them later, so
       // there's no check for users being disabled here.

src/applications/auth/controller/PhabricatorLogoutController.php

@@ -34,7 +34,13 @@ final class PhabricatorLogoutController
       // try to login again and tell them to clear any junk.
       $phsid = $request->getCookie('phsid');
       if ($phsid) {
-        $user->destroySession($phsid);
+        $session = id(new PhabricatorAuthSessionQuery())
+          ->setViewer($user)
+          ->withSessionKeys(array($phsid))
+          ->executeOne();
+        if ($session) {
+          $session->delete();
+        }
       }
       $request->clearCookie('phsid');
 

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -0,0 +1,182 @@
+<?php
+
+final class PhabricatorAuthSessionEngine extends Phobject {
+
+  public function loadUserForSession($session_type, $session_key) {
+    $session_table = new PhabricatorAuthSession();
+    $user_table = new PhabricatorUser();
+    $conn_r = $session_table->establishConnection('w');
+
+    $info = queryfx_one(
+      $conn_r,
+      'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID
+        AND s.type LIKE %> AND s.sessionKey = %s',
+      $user_table->getTableName(),
+      $session_table->getTableName(),
+      $session_type.'-',
+      PhabricatorHash::digest($session_key));
+
+    if (!$info) {
+      return null;
+    }
+
+    return $user_table->loadFromArray($info);
+  }
+
+
+  /**
+   * Issue a new session key for a given identity. Phabricator supports
+   * different types of sessions (like "web" and "conduit") and each session
+   * type may have multiple concurrent sessions (this allows a user to be
+   * logged in on multiple browsers at the same time, for instance).
+   *
+   * Note that this method is transport-agnostic and does not set cookies or
+   * issue other types of tokens, it ONLY generates a new session key.
+   *
+   * You can configure the maximum number of concurrent sessions for various
+   * session types in the Phabricator configuration.
+   *
+   * @param   string  Session type, like "web".
+   * @param   phid    Identity to establish a session for, usually a user PHID.
+   * @return  string  Newly generated session key.
+   */
+  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 'web':
+        $session_limit = PhabricatorEnv::getEnvConfig('auth.sessions.web');
+        break;
+      case '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);
+
+    // 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);
+
+    // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
+    $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)
+              VALUES (%s, %s, %s, 0)',
+            $session_table->getTableName(),
+            $identity_phid,
+            $establish_type,
+            PhabricatorHash::digest($session_key));
+          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()
+          WHERE userPHID = %s AND type = %s AND sessionKey = %s',
+        $session_table->getTableName(),
+        PhabricatorHash::digest($session_key),
+        $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();
+
+    return $session_key;
+  }
+
+}

src/applications/auth/query/PhabricatorAuthSessionQuery.php

@@ -4,12 +4,24 @@ final class PhabricatorAuthSessionQuery
   extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $identityPHIDs;
+  private $sessionKeys;
+  private $sessionTypes;
 
   public function withIdentityPHIDs(array $identity_phids) {
     $this->identityPHIDs = $identity_phids;
     return $this;
   }
 
+  public function withSessionKeys(array $keys) {
+    $this->sessionKeys = $keys;
+    return $this;
+  }
+
+  public function withSessionTypes(array $types) {
+    $this->sessionTypes = $types;
+    return $this;
+  }
+
   protected function loadPage() {
     $table = new PhabricatorAuthSession();
     $conn_r = $table->establishConnection('r');
@@ -57,6 +69,28 @@ final class PhabricatorAuthSessionQuery
         $this->identityPHIDs);
     }
 
+    if ($this->sessionKeys) {
+      $hashes = array();
+      foreach ($this->sessionKeys as $session_key) {
+        $hashes[] = PhabricatorHash::digest($session_key);
+      }
+      $where[] = qsprintf(
+        $conn_r,
+        'sessionKey IN (%Ls)',
+        $hashes);
+    }
+
+    if ($this->sessionTypes) {
+      $clauses = array();
+      foreach ($this->sessionTypes as $session_type) {
+        $clauses[] = qsprintf(
+          $conn_r,
+          'type LIKE %>',
+          $session_type.'-');
+      }
+      $where[] = '('.implode(') OR (', $clauses).')';
+    }
+
     $where[] = $this->buildPagingClause($conn_r);
 
     return $this->formatWhereClause($where);

src/applications/auth/storage/PhabricatorAuthSession.php

@@ -36,6 +36,16 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
     return $this->assertAttached($this->identityObject);
   }
 
+  public function delete() {
+    // TODO: We don't have a proper `id` column yet, so make this work as
+    // expected until we do.
+    queryfx(
+      $this->establishConnection('w'),
+      'DELETE FROM %T WHERE sessionKey = %s',
+      $this->getTableName(),
+      $this->getSessionKey());
+    return $this;
+  }
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

src/applications/base/controller/PhabricatorController.php

@@ -35,20 +35,12 @@ abstract class PhabricatorController extends AphrontController {
     } else {
       $user = new PhabricatorUser();
 
-      $phusr = $request->getCookie('phusr');
       $phsid = $request->getCookie('phsid');
-
+      if ($phsid) {
-      if (strlen($phusr) && $phsid) {
+        $session_user = id(new PhabricatorAuthSessionEngine())
-        $info = queryfx_one(
+          ->loadUserForSession('web', $phsid);
-          $user->establishConnection('r'),
+        if ($session_user) {
-          'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID
+          $user = $session_user;
-            AND s.type LIKE %> AND s.sessionKey = %s',
-          $user->getTableName(),
-          PhabricatorUser::SESSION_TABLE,
-          'web-',
-          PhabricatorHash::digest($phsid));
-        if ($info) {
-          $user->loadFromArray($info);
         }
       }
 

src/applications/conduit/controller/PhabricatorConduitAPIController.php

@@ -279,28 +279,13 @@ final class PhabricatorConduitAPIController
       );
     }
 
-    $session = queryfx_one(
+    $user = id(new PhabricatorAuthSessionEngine())
-      id(new PhabricatorUser())->establishConnection('r'),
+      ->loadUserForSession('conduit', $session_key);
-      'SELECT * FROM %T WHERE sessionKey = %s',
-      PhabricatorUser::SESSION_TABLE,
-      PhabricatorHash::digest($session_key));
-    if (!$session) {
-      return array(
-        'ERR-INVALID-SESSION',
-        'Session key is invalid.',
-      );
-    }
 
-    // TODO: Make sessions timeout.
-    // TODO: When we pull a session, read connectionID from the session table.
-
-    $user = id(new PhabricatorUser())->loadOneWhere(
-      'phid = %s',
-      $session['userPHID']);
     if (!$user) {
       return array(
         'ERR-INVALID-SESSION',
-        'Session is for nonexistent user.',
+        'Session key is invalid.',
       );
     }
 

src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php

@@ -142,7 +142,8 @@ final class ConduitAPI_conduit_connect_Method extends ConduitAPIMethod {
       if ($valid != $signature) {
         throw new ConduitException('ERR-INVALID-CERTIFICATE');
       }
-      $session_key = $user->establishSession('conduit');
+      $session_key = id(new PhabricatorAuthSessionEngine())
+        ->establishSession('conduit', $user->getPHID());
     } else {
       throw new ConduitException('ERR-NO-CERTIFICATE');
     }

src/applications/people/storage/PhabricatorUser.php

@@ -292,170 +292,6 @@ final class PhabricatorUser
     return substr(PhabricatorHash::digest($vec), 0, $len);
   }
 
-  /**
-   * Issue a new session key to this user. Phabricator supports different
-   * types of sessions (like "web" and "conduit") and each session type may
-   * have multiple concurrent sessions (this allows a user to be logged in on
-   * multiple browsers at the same time, for instance).
-   *
-   * Note that this method is transport-agnostic and does not set cookies or
-   * issue other types of tokens, it ONLY generates a new session key.
-   *
-   * You can configure the maximum number of concurrent sessions for various
-   * session types in the Phabricator configuration.
-   *
-   * @param   string  Session type, like "web".
-   * @return  string  Newly generated session key.
-   */
-  public function establishSession($session_type) {
-    $conn_w = $this->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 'web':
-        $session_limit = PhabricatorEnv::getEnvConfig('auth.sessions.web');
-        break;
-      case '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);
-
-    // Load all the currently active sessions.
-    $sessions = queryfx_all(
-      $conn_w,
-      'SELECT type, sessionKey, sessionStart FROM %T
-        WHERE userPHID = %s AND type LIKE %>',
-      PhabricatorUser::SESSION_TABLE,
-      $this->getPHID(),
-      $session_type.'-');
-    $sessions = ipull($sessions, null, 'type');
-    $sessions = isort($sessions, 'sessionStart');
-
-    $existing_sessions = array_keys($sessions);
-
-    // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
-    $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)
-              VALUES (%s, %s, %s, 0)',
-            self::SESSION_TABLE,
-            $this->getPHID(),
-            $establish_type,
-            PhabricatorHash::digest($session_key));
-          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()
-          WHERE userPHID = %s AND type = %s AND sessionKey = %s',
-        self::SESSION_TABLE,
-        PhabricatorHash::digest($session_key),
-        $this->getPHID(),
-        $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(
-      $this,
-      $this->getPHID(),
-      PhabricatorUserLog::ACTION_LOGIN);
-    $log->setDetails(
-      array(
-        'session_type' => $session_type,
-        'session_issued' => $establish_type,
-      ));
-    $log->setSession($session_key);
-    $log->save();
-
-    return $session_key;
-  }
-
-  public function destroySession($session_key) {
-    $conn_w = $this->establishConnection('w');
-    queryfx(
-      $conn_w,
-      'DELETE FROM %T WHERE userPHID = %s AND sessionKey = %s',
-      self::SESSION_TABLE,
-      $this->getPHID(),
-      PhabricatorHash::digest($session_key));
-  }
-
   private function generateEmailToken(
     PhabricatorUserEmail $email,
     $offset = 0) {

src/applications/settings/panel/PhabricatorSettingsPanelConduit.php

@@ -34,13 +34,14 @@ final class PhabricatorSettingsPanelConduit
           ->setDialog($dialog);
       }
 
-      $conn = $user->establishConnection('w');
+      $sessions = id(new PhabricatorAuthSessionQuery())
-      queryfx(
+        ->setViewer($user)
-        $conn,
+        ->withIdentityPHIDs(array($user->getPHID()))
-        'DELETE FROM %T WHERE userPHID = %s AND type LIKE %>',
+        ->withSessionTypes(array('conduit'))
-        PhabricatorUser::SESSION_TABLE,
+        ->execute();
-        $user->getPHID(),
+      foreach ($sessions as $session) {
-        'conduit');
+        $session->delete();
+      }
 
       // This implicitly regenerates the certificate.
       $user->setConduitCertificate(null);