Skip to content

Commit acb141c

Browse files
author
epriestley
committedJan 15, 2014
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
1 parent a64228b commit acb141c

File tree

6 files changed

+82
-6
lines changed

6 files changed

+82
-6
lines changed
 
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
ALTER TABLE {$NAMESPACE}_user.phabricator_session
2+
ADD sessionExpires INT UNSIGNED NOT NULL;
3+
4+
UPDATE {$NAMESPACE}_user.phabricator_session
5+
SET sessionExpires = UNIX_TIMESTAMP() + (60 * 60 * 24 * 30);
6+
7+
ALTER TABLE {$NAMESPACE}_user.phabricator_session
8+
ADD KEY `key_expires` (sessionExpires);

‎src/__phutil_library_map__.php

+2
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,7 @@
12111211
'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php',
12121212
'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php',
12131213
'PhabricatorAuthSessionEngine' => 'applications/auth/engine/PhabricatorAuthSessionEngine.php',
1214+
'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php',
12141215
'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
12151216
'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
12161217
'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php',
@@ -3791,6 +3792,7 @@
37913792
1 => 'PhabricatorPolicyInterface',
37923793
),
37933794
'PhabricatorAuthSessionEngine' => 'Phobject',
3795+
'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector',
37943796
'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
37953797
'PhabricatorAuthStartController' => 'PhabricatorAuthController',
37963798
'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController',

‎src/applications/auth/engine/PhabricatorAuthSessionEngine.php

+38-6
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@ final class PhabricatorAuthSessionEngine extends Phobject {
55
public function loadUserForSession($session_type, $session_key) {
66
$session_table = new PhabricatorAuthSession();
77
$user_table = new PhabricatorUser();
8-
$conn_r = $session_table->establishConnection('w');
8+
$conn_r = $session_table->establishConnection('r');
9+
10+
// NOTE: We're being clever here because this happens on every page load,
11+
// and by joining we can save a query.
912

1013
$info = queryfx_one(
1114
$conn_r,
12-
'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID
15+
'SELECT s.sessionExpires AS _sessionExpires, s.id AS _sessionID, u.*
16+
FROM %T u JOIN %T s ON u.phid = s.userPHID
1317
AND s.type LIKE %> AND s.sessionKey = %s',
1418
$user_table->getTableName(),
1519
$session_table->getTableName(),
@@ -20,6 +24,29 @@ public function loadUserForSession($session_type, $session_key) {
2024
return null;
2125
}
2226

27+
$expires = $info['_sessionExpires'];
28+
$id = $info['_sessionID'];
29+
unset($info['_sessionExpires']);
30+
unset($info['_sessionID']);
31+
32+
$ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
33+
34+
// If more than 20% of the time on this session has been used, refresh the
35+
// TTL back up to the full duration. The idea here is that sessions are
36+
// good forever if used regularly, but get GC'd when they fall out of use.
37+
38+
if (time() + (0.80 * $ttl) > $expires) {
39+
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
40+
$conn_w = $session_table->establishConnection('w');
41+
queryfx(
42+
$conn_w,
43+
'UPDATE %T SET sessionExpires = UNIX_TIMESTAMP() + %d WHERE id = %d',
44+
$session_table->getTableName(),
45+
$ttl,
46+
$id);
47+
unset($unguarded);
48+
}
49+
2350
return $user_table->loadFromArray($info);
2451
}
2552

@@ -84,6 +111,7 @@ public function establishSession($session_type, $identity_phid) {
84111
// Consume entropy to generate a new session key, forestalling the eventual
85112
// heat death of the universe.
86113
$session_key = Filesystem::readRandomCharacters(40);
114+
$session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
87115

88116
// Load all the currently active sessions.
89117
$sessions = queryfx_all(
@@ -119,12 +147,14 @@ public function establishSession($session_type, $identity_phid) {
119147
// care if we race here or not.
120148
queryfx(
121149
$conn_w,
122-
'INSERT IGNORE INTO %T (userPHID, type, sessionKey, sessionStart)
123-
VALUES (%s, %s, %s, 0)',
150+
'INSERT IGNORE INTO %T
151+
(userPHID, type, sessionKey, sessionStart, sessionExpires)
152+
VALUES (%s, %s, %s, 0, UNIX_TIMESTAMP() + %d)',
124153
$session_table->getTableName(),
125154
$identity_phid,
126155
$establish_type,
127-
PhabricatorHash::digest($session_key));
156+
PhabricatorHash::digest($session_key),
157+
$session_ttl);
128158
break;
129159
}
130160
}
@@ -144,10 +174,12 @@ public function establishSession($session_type, $identity_phid) {
144174

145175
queryfx(
146176
$conn_w,
147-
'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP()
177+
'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP(),
178+
sessionExpires = UNIX_TIMESTAMP() + %d
148179
WHERE userPHID = %s AND type = %s AND sessionKey = %s',
149180
$session_table->getTableName(),
150181
PhabricatorHash::digest($session_key),
182+
$session_ttl,
151183
$identity_phid,
152184
$establish_type,
153185
$expect_key);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
3+
final class PhabricatorAuthSessionGarbageCollector
4+
extends PhabricatorGarbageCollector {
5+
6+
public function collectGarbage() {
7+
$session_table = new PhabricatorAuthSession();
8+
$conn_w = $session_table->establishConnection('w');
9+
10+
queryfx(
11+
$conn_w,
12+
'DELETE FROM %T WHERE sessionExpires <= UNIX_TIMESTAMP() LIMIT 100',
13+
$session_table->getTableName());
14+
15+
return ($conn_w->getAffectedRows() == 100);
16+
}
17+
18+
}

‎src/applications/auth/storage/PhabricatorAuthSession.php

+13
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
1010
protected $type;
1111
protected $sessionKey;
1212
protected $sessionStart;
13+
protected $sessionExpires;
1314

1415
private $identityObject = self::ATTACHABLE;
1516

@@ -38,6 +39,18 @@ public function getIdentityObject() {
3839
return $this->assertAttached($this->identityObject);
3940
}
4041

42+
public static function getSessionTypeTTL($session_type) {
43+
switch ($session_type) {
44+
case self::TYPE_WEB:
45+
return (60 * 60 * 24 * 30); // 30 days
46+
case self::TYPE_CONDUIT:
47+
return (60 * 60 * 24); // 24 hours
48+
default:
49+
throw new Exception(pht('Unknown session type "%s".', $session_type));
50+
}
51+
}
52+
53+
4154
/* -( PhabricatorPolicyInterface )----------------------------------------- */
4255

4356

‎src/applications/settings/panel/PhabricatorSettingsPanelSessions.php

+3
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ public function processRequest(AphrontRequest $request) {
6060
substr($session->getSessionKey(), 0, 12),
6161
$session->getType(),
6262
phabricator_datetime($session->getSessionStart(), $viewer),
63+
phabricator_datetime($session->getSessionExpires(), $viewer),
6364
);
6465
}
6566

@@ -72,13 +73,15 @@ public function processRequest(AphrontRequest $request) {
7273
pht('Session'),
7374
pht('Type'),
7475
pht('Created'),
76+
pht('Expires'),
7577
));
7678
$table->setColumnClasses(
7779
array(
7880
'wide',
7981
'n',
8082
'',
8183
'right',
84+
'right',
8285
));
8386

8487

0 commit comments

Comments
 (0)
Failed to load comments.