From acb141cf52c168c6669e0d5c3dcfa93cd07fd691 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 15 Jan 2014 13:56:16 -0800 Subject: [PATCH] Expire and garbage collect unused sessions Summary: Ref T3720. Ref T4310. Currently, we limit the maximum number of concurrent sessions of each type. This is primarily because sessions predate garbage collection and we had no way to prevent the session table from growing fairly quickly and without bound unless we did this. Now that we have GC (and it's modular!) we can just expire unused sessions after a while and throw them away: - Add a `sessionExpires` column to the table, with a key. - Add a GC for old sessions. - When we establish a session, set `sessionExpires` to the current time plus the session TTL. - When a user uses a session and has used up more than 20% of the time on it, extend the session. In addition to this, we could also rotate sessions, but I think that provides very little value. If we do want to implement it, we should hold it until after T3720 / T4310. Test Plan: - Ran schema changes. - Looked at database. - Tested GC: - Started GC. - Set expires on one row to the past. - Restarted GC. - Verified GC nuked the session. - Logged in. - Logged out. - Ran Conduit method. - Tested refresh: - Set threshold to 0.0001% instead of 20%. - Loaded page. - Saw a session extension ever few page loads. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4310, T3720 Differential Revision: https://secure.phabricator.com/D7976 --- .../autopatches/20140115.auth.2.expires.sql | 8 ++++ src/__phutil_library_map__.php | 2 + .../engine/PhabricatorAuthSessionEngine.php | 44 ++++++++++++++++--- ...PhabricatorAuthSessionGarbageCollector.php | 18 ++++++++ .../auth/storage/PhabricatorAuthSession.php | 13 ++++++ .../PhabricatorSettingsPanelSessions.php | 3 ++ 6 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 resources/sql/autopatches/20140115.auth.2.expires.sql create mode 100644 src/applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php diff --git a/resources/sql/autopatches/20140115.auth.2.expires.sql b/resources/sql/autopatches/20140115.auth.2.expires.sql new file mode 100644 index 0000000000..94b36af7da --- /dev/null +++ b/resources/sql/autopatches/20140115.auth.2.expires.sql @@ -0,0 +1,8 @@ +ALTER TABLE {$NAMESPACE}_user.phabricator_session + ADD sessionExpires INT UNSIGNED NOT NULL; + +UPDATE {$NAMESPACE}_user.phabricator_session + SET sessionExpires = UNIX_TIMESTAMP() + (60 * 60 * 24 * 30); + +ALTER TABLE {$NAMESPACE}_user.phabricator_session + ADD KEY `key_expires` (sessionExpires); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0be73ee579..df30647529 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1211,6 +1211,7 @@ phutil_register_library_map(array( 'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php', 'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php', 'PhabricatorAuthSessionEngine' => 'applications/auth/engine/PhabricatorAuthSessionEngine.php', + 'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php', 'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', 'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php', @@ -3791,6 +3792,7 @@ phutil_register_library_map(array( 1 => 'PhabricatorPolicyInterface', ), 'PhabricatorAuthSessionEngine' => 'Phobject', + 'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', 'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController', diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index 7735db3fa1..838d0e9e88 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -5,11 +5,15 @@ 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'); + $conn_r = $session_table->establishConnection('r'); + + // NOTE: We're being clever here because this happens on every page load, + // and by joining we can save a query. $info = queryfx_one( $conn_r, - 'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID + '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', $user_table->getTableName(), $session_table->getTableName(), @@ -20,6 +24,29 @@ final class PhabricatorAuthSessionEngine extends Phobject { return null; } + $expires = $info['_sessionExpires']; + $id = $info['_sessionID']; + unset($info['_sessionExpires']); + unset($info['_sessionID']); + + $ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type); + + // If more than 20% of the time on this session has been used, refresh the + // TTL back up to the full duration. The idea here is that sessions are + // good forever if used regularly, but get GC'd when they fall out of use. + + if (time() + (0.80 * $ttl) > $expires) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $conn_w = $session_table->establishConnection('w'); + queryfx( + $conn_w, + 'UPDATE %T SET sessionExpires = UNIX_TIMESTAMP() + %d WHERE id = %d', + $session_table->getTableName(), + $ttl, + $id); + unset($unguarded); + } + return $user_table->loadFromArray($info); } @@ -84,6 +111,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { // 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( @@ -119,12 +147,14 @@ final class PhabricatorAuthSessionEngine extends Phobject { // care if we race here or not. queryfx( $conn_w, - 'INSERT IGNORE INTO %T (userPHID, type, sessionKey, sessionStart) - VALUES (%s, %s, %s, 0)', + '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)); + PhabricatorHash::digest($session_key), + $session_ttl); break; } } @@ -144,10 +174,12 @@ final class PhabricatorAuthSessionEngine extends Phobject { queryfx( $conn_w, - 'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP() + '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, $identity_phid, $establish_type, $expect_key); diff --git a/src/applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php b/src/applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php new file mode 100644 index 0000000000..f9994c2fd6 --- /dev/null +++ b/src/applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php @@ -0,0 +1,18 @@ +establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE sessionExpires <= UNIX_TIMESTAMP() LIMIT 100', + $session_table->getTableName()); + + return ($conn_w->getAffectedRows() == 100); + } + +} diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php index 6f389fa4ad..bb5230dbe2 100644 --- a/src/applications/auth/storage/PhabricatorAuthSession.php +++ b/src/applications/auth/storage/PhabricatorAuthSession.php @@ -10,6 +10,7 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO protected $type; protected $sessionKey; protected $sessionStart; + protected $sessionExpires; private $identityObject = self::ATTACHABLE; @@ -38,6 +39,18 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO return $this->assertAttached($this->identityObject); } + public static function getSessionTypeTTL($session_type) { + switch ($session_type) { + case self::TYPE_WEB: + return (60 * 60 * 24 * 30); // 30 days + case self::TYPE_CONDUIT: + return (60 * 60 * 24); // 24 hours + default: + throw new Exception(pht('Unknown session type "%s".', $session_type)); + } + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelSessions.php b/src/applications/settings/panel/PhabricatorSettingsPanelSessions.php index 0923541a98..4d2d126ee9 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelSessions.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelSessions.php @@ -60,6 +60,7 @@ final class PhabricatorSettingsPanelSessions substr($session->getSessionKey(), 0, 12), $session->getType(), phabricator_datetime($session->getSessionStart(), $viewer), + phabricator_datetime($session->getSessionExpires(), $viewer), ); } @@ -72,6 +73,7 @@ final class PhabricatorSettingsPanelSessions pht('Session'), pht('Type'), pht('Created'), + pht('Expires'), )); $table->setColumnClasses( array( @@ -79,6 +81,7 @@ final class PhabricatorSettingsPanelSessions 'n', '', 'right', + 'right', ));