Index: src/__phutil_library_map__.php =================================================================== --- src/__phutil_library_map__.php +++ src/__phutil_library_map__.php @@ -1203,6 +1203,7 @@ '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 @@ 0 => 'PhabricatorAuthDAO', 1 => 'PhabricatorPolicyInterface', ), + 'PhabricatorAuthSessionEngine' => 'Phobject', 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', 'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController', Index: src/applications/auth/controller/PhabricatorAuthController.php =================================================================== --- src/applications/auth/controller/PhabricatorAuthController.php +++ src/applications/auth/controller/PhabricatorAuthController.php @@ -81,7 +81,8 @@ $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. Index: src/applications/auth/controller/PhabricatorLogoutController.php =================================================================== --- src/applications/auth/controller/PhabricatorLogoutController.php +++ src/applications/auth/controller/PhabricatorLogoutController.php @@ -34,7 +34,13 @@ // 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'); Index: src/applications/auth/engine/PhabricatorAuthSessionEngine.php =================================================================== --- /dev/null +++ src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -0,0 +1,182 @@ +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; + } + +} Index: src/applications/auth/query/PhabricatorAuthSessionQuery.php =================================================================== --- src/applications/auth/query/PhabricatorAuthSessionQuery.php +++ src/applications/auth/query/PhabricatorAuthSessionQuery.php @@ -4,12 +4,24 @@ 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 @@ $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); Index: src/applications/auth/storage/PhabricatorAuthSession.php =================================================================== --- src/applications/auth/storage/PhabricatorAuthSession.php +++ src/applications/auth/storage/PhabricatorAuthSession.php @@ -36,6 +36,16 @@ 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 )----------------------------------------- */ Index: src/applications/base/controller/PhabricatorController.php =================================================================== --- src/applications/base/controller/PhabricatorController.php +++ src/applications/base/controller/PhabricatorController.php @@ -35,20 +35,12 @@ } else { $user = new PhabricatorUser(); - $phusr = $request->getCookie('phusr'); $phsid = $request->getCookie('phsid'); - - if (strlen($phusr) && $phsid) { - $info = queryfx_one( - $user->establishConnection('r'), - 'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID - AND s.type LIKE %> AND s.sessionKey = %s', - $user->getTableName(), - PhabricatorUser::SESSION_TABLE, - 'web-', - PhabricatorHash::digest($phsid)); - if ($info) { - $user->loadFromArray($info); + if ($phsid) { + $session_user = id(new PhabricatorAuthSessionEngine()) + ->loadUserForSession('web', $phsid); + if ($session_user) { + $user = $session_user; } } Index: src/applications/conduit/controller/PhabricatorConduitAPIController.php =================================================================== --- src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -279,28 +279,13 @@ ); } - $session = queryfx_one( - id(new PhabricatorUser())->establishConnection('r'), - '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 PhabricatorAuthSessionEngine()) + ->loadUserForSession('conduit', $session_key); - $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.', ); } Index: src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php =================================================================== --- src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php +++ src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php @@ -142,7 +142,8 @@ 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'); } Index: src/applications/people/storage/PhabricatorUser.php =================================================================== --- src/applications/people/storage/PhabricatorUser.php +++ src/applications/people/storage/PhabricatorUser.php @@ -292,170 +292,6 @@ 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) { Index: src/applications/settings/panel/PhabricatorSettingsPanelConduit.php =================================================================== --- src/applications/settings/panel/PhabricatorSettingsPanelConduit.php +++ src/applications/settings/panel/PhabricatorSettingsPanelConduit.php @@ -34,13 +34,14 @@ ->setDialog($dialog); } - $conn = $user->establishConnection('w'); - queryfx( - $conn, - 'DELETE FROM %T WHERE userPHID = %s AND type LIKE %>', - PhabricatorUser::SESSION_TABLE, - $user->getPHID(), - 'conduit'); + $sessions = id(new PhabricatorAuthSessionQuery()) + ->setViewer($user) + ->withIdentityPHIDs(array($user->getPHID())) + ->withSessionTypes(array('conduit')) + ->execute(); + foreach ($sessions as $session) { + $session->delete(); + } // This implicitly regenerates the certificate. $user->setConduitCertificate(null);