Page MenuHomePhabricator
Files F15331270

D7962.diff
No OneTemporary
Actions

D7962.diff
View Options

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 @@
+<?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;
+ }
+
+}
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);

File Metadata

Mime Type
text/plain
Expires
Sat, Mar 8, 8:30 AM (4 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7381414
Default Alt Text
D7962.diff (21 KB)

Event Timeline

Phabricator · Built by Phacility