diff --git a/conf/default.conf.php b/conf/default.conf.php index 8b31f89792..65ec47cbbb 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -543,15 +543,6 @@ return array( // -- 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 new file mode 100644 index 0000000000..80c316f64a --- /dev/null +++ b/resources/sql/autopatches/20140115.auth.3.unlimit.php @@ -0,0 +1,26 @@ +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 index 838d0e9e88..0db6871ee5 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -14,10 +14,10 @@ final class PhabricatorAuthSessionEngine extends Phobject { $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 @@ final class PhabricatorAuthSessionEngine extends Phobject { $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); + + // This has a side effect of validating the session type. $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); - - // 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(); + id(new PhabricatorAuthSession()) + ->setUserPHID($identity_phid) + ->setType($session_type) + ->setSessionKey(PhabricatorHash::digest($session_key)) + ->setSessionStart(time()) + ->setSessionExpires(time() + $session_ttl) + ->save(); - $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, + $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 index 32be395c0b..d1e4dca895 100644 --- a/src/applications/auth/query/PhabricatorAuthSessionQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSessionQuery.php @@ -81,14 +81,10 @@ final class PhabricatorAuthSessionQuery } 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); @@ -96,10 +92,6 @@ final class PhabricatorAuthSessionQuery 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 index eb1cfc0027..fd12e035cd 100644 --- a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php +++ b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php @@ -146,6 +146,10 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck { '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 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck { '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 index dac2dbfb33..7e761085a8 100644 --- a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php +++ b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php @@ -13,22 +13,6 @@ final class PhabricatorAuthenticationConfigOptions 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(