diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php
index db064ce357..cdadf2e389 100644
--- a/phpBB/phpbb/session.php
+++ b/phpBB/phpbb/session.php
@@ -956,72 +956,96 @@ class session
{
global $db, $config, $phpbb_container, $phpbb_dispatcher;
- $batch_size = 10;
-
if (!$this->time_now)
{
$this->time_now = time();
}
- // Firstly, delete guest sessions
+ /**
+ * Get expired sessions for registered users, only most recent for each user
+ * Inner SELECT gets most recent expired sessions for unique session_user_id
+ * Outer SELECT gets data for them
+ */
+ $sql_select = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time
+ FROM ' . SESSIONS_TABLE . ' AS s1
+ INNER JOIN (
+ SELECT session_user_id, MAX(session_time) AS recent_time
+ FROM ' . SESSIONS_TABLE . '
+ WHERE session_time < ' . ($this->time_now - (int) $config['session_length']) . '
+ AND session_user_id <> ' . ANONYMOUS . '
+ GROUP BY session_user_id
+ ) AS s2
+ ON s1.session_user_id = s2.session_user_id
+ AND s1.session_time = s2.recent_time';
+
+ switch ($db->get_sql_layer())
+ {
+ case 'sqlite3':
+ if (phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>='))
+ {
+ // For SQLite versions 3.8.3+ which support Common Table Expressions (CTE)
+ $sql = "WITH s3 (session_page, session_user_id, session_time) AS ($sql_select)
+ UPDATE " . USERS_TABLE . '
+ SET (user_lastpage, user_lastvisit) = (SELECT session_page, session_time FROM s3 WHERE session_user_id = user_id)
+ WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)';
+ $db->sql_query($sql);
+
+ break;
+ }
+
+ // No break, for SQLite versions prior to 3.8.3 and Oracle
+ case 'oracle':
+ $result = $db->sql_query($sql_select);
+ while ($row = $db->sql_fetchrow($result))
+ {
+ $sql = 'UPDATE ' . USERS_TABLE . '
+ SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "'
+ WHERE user_id = " . (int) $row['session_user_id'];
+ $db->sql_query($sql);
+ }
+ $db->sql_freeresult($result);
+ break;
+
+ case 'mysqli':
+ $sql = 'UPDATE ' . USERS_TABLE . " u,
+ ($sql_select) s3
+ SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page
+ WHERE u.user_id = s3.session_user_id";
+ $db->sql_query($sql);
+ break;
+
+ default:
+ $sql = 'UPDATE ' . USERS_TABLE . "
+ SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page
+ FROM ($sql_select) s3
+ WHERE user_id = s3.session_user_id";
+ $db->sql_query($sql);
+ break;
+ }
+
+ // Delete all expired sessions
$sql = 'DELETE FROM ' . SESSIONS_TABLE . '
- WHERE session_user_id = ' . ANONYMOUS . '
- AND session_time < ' . (int) ($this->time_now - $config['session_length']);
+ WHERE session_time < ' . ($this->time_now - (int) $config['session_length']);
$db->sql_query($sql);
- // Get expired sessions, only most recent for each user
- $sql = 'SELECT session_user_id, session_page, MAX(session_time) AS recent_time
- FROM ' . SESSIONS_TABLE . '
- WHERE session_time < ' . ($this->time_now - $config['session_length']) . '
- GROUP BY session_user_id, session_page';
- $result = $db->sql_query_limit($sql, $batch_size);
+ // Update gc timer
+ $config->set('session_last_gc', $this->time_now, false);
- $del_user_id = array();
- $del_sessions = 0;
-
- while ($row = $db->sql_fetchrow($result))
+ if ($config['max_autologin_time'])
{
- $sql = 'UPDATE ' . USERS_TABLE . '
- SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "'
- WHERE user_id = " . (int) $row['session_user_id'];
- $db->sql_query($sql);
-
- $del_user_id[] = (int) $row['session_user_id'];
- $del_sessions++;
- }
- $db->sql_freeresult($result);
-
- if (count($del_user_id))
- {
- // Delete expired sessions
- $sql = 'DELETE FROM ' . SESSIONS_TABLE . '
- WHERE ' . $db->sql_in_set('session_user_id', $del_user_id) . '
- AND session_time < ' . ($this->time_now - $config['session_length']);
+ $sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . '
+ WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time']));
$db->sql_query($sql);
}
- if ($del_sessions < $batch_size)
- {
- // Less than 10 users, update gc timer ... else we want gc
- // called again to delete other sessions
- $config->set('session_last_gc', $this->time_now, false);
+ // only called from CRON; should be a safe workaround until the infrastructure gets going
+ /* @var \phpbb\captcha\factory $captcha_factory */
+ $captcha_factory = $phpbb_container->get('captcha.factory');
+ $captcha_factory->garbage_collect($config['captcha_plugin']);
- if ($config['max_autologin_time'])
- {
- $sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . '
- WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time']));
- $db->sql_query($sql);
- }
-
- // only called from CRON; should be a safe workaround until the infrastructure gets going
- /* @var $captcha_factory \phpbb\captcha\factory */
- $captcha_factory = $phpbb_container->get('captcha.factory');
- $captcha_factory->garbage_collect($config['captcha_plugin']);
-
- $sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . '
- WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']);
- $db->sql_query($sql);
- }
+ $sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . '
+ WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']);
+ $db->sql_query($sql);
/**
* Event to trigger extension on session_gc
diff --git a/tests/session/fixtures/sessions_garbage.xml b/tests/session/fixtures/sessions_garbage.xml
index 5eace839d0..59a2dc2ebe 100644
--- a/tests/session/fixtures/sessions_garbage.xml
+++ b/tests/session/fixtures/sessions_garbage.xml
@@ -5,11 +5,23 @@
username_clean
user_permissions
user_sig
+ user_lastpage
+ user_lastvisit
4
bar
+ oldpage_user_bar.php
+ 1400000000
+
+
+ 5
+ foo
+
+
+ oldpage_user_foo.php
+ 1400000000
@@ -18,12 +30,16 @@
session_ip
session_browser
session_admin
+ session_page
+ session_time
anon_session00000000000000000000
1
127.0.0.1
anonymous user agent
0
+
+ 1500000005
bar_session000000000000000000000
@@ -31,6 +47,35 @@
127.0.0.1
user agent
1
+ newpage_user_bar.php
+ 1500000000
+
+
+ bar_session000000000000000000002
+ 4
+ 127.0.0.1
+ user agent
+ 1
+ oldpage_user_bar.php
+ 1400000000
+
+
+ foo_session000000000000000000000
+ 5
+ 127.0.0.1
+ user agent
+ 0
+ newpage_user_foo.php
+ 1500000000
+
+
+ foo_session000000000000000000002
+ 5
+ 127.0.0.1
+ user agent
+ 0
+ oldpage_user_foo.php
+ 1400000000
diff --git a/tests/session/garbage_collection_test.php b/tests/session/garbage_collection_test.php
index d361e022da..ec248b2904 100644
--- a/tests/session/garbage_collection_test.php
+++ b/tests/session/garbage_collection_test.php
@@ -41,19 +41,91 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
);
}
+ public function test_session_gc()
+ {
+ global $config;
+ $config['session_length'] = 3600;
+
+ $this->check_expired_sessions_recent(
+ [
+ [
+ 'session_user_id' => 4,
+ 'recent_time' => 1500000000,
+ ],
+ [
+ 'session_user_id' => 5,
+ 'recent_time' => 1500000000,
+ ],
+ ],
+ 'Before test, should get recent expired sessions only.'
+ );
+
+ $this->check_user_session_data(
+ [
+ [
+ 'username_clean' => 'bar',
+ 'user_lastvisit' => 1400000000,
+ 'user_lastpage' => 'oldpage_user_bar.php',
+ ],
+ [
+ 'username_clean' => 'foo',
+ 'user_lastvisit' => 1400000000,
+ 'user_lastpage' => 'oldpage_user_foo.php',
+ ],
+ ],
+ 'Before test, users session data is not updated yet.'
+ );
+
+ // There is an error unless the captcha plugin is set
+ $config['captcha_plugin'] = 'core.captcha.plugins.nogd';
+ $this->session->session_gc();
+ $this->check_expired_sessions_recent(
+ [],
+ 'After garbage collection, all expired sessions should be removed.'
+ );
+
+ $this->check_user_session_data(
+ [
+ [
+ 'username_clean' => 'bar',
+ 'user_lastvisit' => '1500000000',
+ 'user_lastpage' => 'newpage_user_bar.php',
+ ],
+ [
+ 'username_clean' => 'foo',
+ 'user_lastvisit' => '1500000000',
+ 'user_lastpage' => 'newpage_user_foo.php',
+ ],
+ ],
+ 'After garbage collection, users session data should be updated to the recent expired sessions data.'
+ );
+ }
+
public function test_cleanup_all()
{
$this->check_sessions_equals(
- array(
- array(
+ [
+ [
'session_id' => 'anon_session00000000000000000000',
'session_user_id' => 1,
- ),
- array(
+ ],
+ [
'session_id' => 'bar_session000000000000000000000',
'session_user_id' => 4,
- ),
- ),
+ ],
+ [
+ 'session_id' => 'bar_session000000000000000000002',
+ 'session_user_id' => 4,
+ ],
+ [
+ 'session_id' => 'foo_session000000000000000000000',
+ 'session_user_id' => 5,
+ ],
+ [
+ 'session_id' => 'foo_session000000000000000000002',
+ 'session_user_id' => 5,
+ ],
+ ],
'Before test, should have some sessions.'
);
// Set session length so it clears all
@@ -63,7 +135,7 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
$config['captcha_plugin'] = 'core.captcha.plugins.nogd';
$this->session->session_gc();
$this->check_sessions_equals(
- array(),
+ [],
'After setting session time to 0, should remove all.'
);
}
diff --git a/tests/test_framework/phpbb_session_test_case.php b/tests/test_framework/phpbb_session_test_case.php
index 86333298fd..18da5e9b08 100644
--- a/tests/test_framework/phpbb_session_test_case.php
+++ b/tests/test_framework/phpbb_session_test_case.php
@@ -46,11 +46,33 @@ abstract class phpbb_session_test_case extends phpbb_database_test_case
new phpbb_session_testable_facade($this->db, $this->session_factory);
}
+ protected function check_user_session_data($expected_session_data, $message)
+ {
+ $sql= 'SELECT username_clean, user_lastvisit, user_lastpage
+ FROM ' . USERS_TABLE . '
+ ORDER BY user_id';
+
+ $this->assertSqlResultEquals($expected_session_data, $sql, $message);
+ }
+
+ protected function check_expired_sessions_recent($expected_sessions, $message)
+ {
+ global $config;
+ $time_now = time();
+ $sql = 'SELECT session_user_id, MAX(session_time) AS recent_time
+ FROM ' . SESSIONS_TABLE . '
+ WHERE session_time < ' . ($time_now - (int) $config['session_length']) . '
+ AND session_user_id <> ' . ANONYMOUS . '
+ GROUP BY session_user_id';
+
+ $this->assertSqlResultEquals($expected_sessions, $sql, $message);
+ }
+
protected function check_sessions_equals($expected_sessions, $message)
{
$sql = 'SELECT session_id, session_user_id
FROM phpbb_sessions
- ORDER BY session_user_id';
+ ORDER BY session_user_id, session_id';
$this->assertSqlResultEquals($expected_sessions, $sql, $message);
}