diff options
-rw-r--r-- | phpBB/phpbb/session.php | 124 | ||||
-rw-r--r-- | tests/session/fixtures/sessions_garbage.xml | 45 | ||||
-rw-r--r-- | tests/session/garbage_collection_test.php | 86 | ||||
-rw-r--r-- | tests/test_framework/phpbb_session_test_case.php | 24 |
4 files changed, 221 insertions, 58 deletions
diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 7c76c08b73..6851bc8188 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -954,72 +954,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 - $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' - WHERE session_user_id = ' . ANONYMOUS . ' - AND session_time < ' . (int) ($this->time_now - $config['session_length']); - $db->sql_query($sql); + /** + * 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); - // 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); + break; + } - $del_user_id = array(); - $del_sessions = 0; + // 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; - 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); + 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; - $del_user_id[] = (int) $row['session_user_id']; - $del_sessions++; + 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; } - $db->sql_freeresult($result); - if (count($del_user_id)) + // Delete all expired sessions + $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' + WHERE session_time < ' . ($this->time_now - (int) $config['session_length']); + $db->sql_query($sql); + + // Update gc timer + $config->set('session_last_gc', $this->time_now, false); + + if ($config['max_autologin_time']) { - // 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 @@ <column>username_clean</column> <column>user_permissions</column> <column>user_sig</column> + <column>user_lastpage</column> + <column>user_lastvisit</column> <row> <value>4</value> <value>bar</value> <value></value> <value></value> + <value>oldpage_user_bar.php</value> + <value>1400000000</value> + </row> + <row> + <value>5</value> + <value>foo</value> + <value></value> + <value></value> + <value>oldpage_user_foo.php</value> + <value>1400000000</value> </row> </table> <table name="phpbb_sessions"> @@ -18,12 +30,16 @@ <column>session_ip</column> <column>session_browser</column> <column>session_admin</column> + <column>session_page</column> + <column>session_time</column> <row> <value>anon_session00000000000000000000</value> <value>1</value> <value>127.0.0.1</value> <value>anonymous user agent</value> <value>0</value> + <value></value> + <value>1500000005</value> </row> <row> <value>bar_session000000000000000000000</value> @@ -31,6 +47,35 @@ <value>127.0.0.1</value> <value>user agent</value> <value>1</value> + <value>newpage_user_bar.php</value> + <value>1500000000</value> + </row> + <row> + <value>bar_session000000000000000000002</value> + <value>4</value> + <value>127.0.0.1</value> + <value>user agent</value> + <value>1</value> + <value>oldpage_user_bar.php</value> + <value>1400000000</value> + </row> + <row> + <value>foo_session000000000000000000000</value> + <value>5</value> + <value>127.0.0.1</value> + <value>user agent</value> + <value>0</value> + <value>newpage_user_foo.php</value> + <value>1500000000</value> + </row> + <row> + <value>foo_session000000000000000000002</value> + <value>5</value> + <value>127.0.0.1</value> + <value>user agent</value> + <value>0</value> + <value>oldpage_user_foo.php</value> + <value>1400000000</value> </row> </table> <table name="phpbb_login_attempts"> 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 02722c473e..530d8c6b48 100644 --- a/tests/test_framework/phpbb_session_test_case.php +++ b/tests/test_framework/phpbb_session_test_case.php @@ -48,11 +48,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); } |