diff --git a/phpBB/includes/acp/acp_main.php b/phpBB/includes/acp/acp_main.php index 357cea9a32..395e721464 100644 --- a/phpBB/includes/acp/acp_main.php +++ b/phpBB/includes/acp/acp_main.php @@ -403,6 +403,7 @@ class acp_main 'session_forum_id' => $user->page['forum'], 'session_user_id' => (int) $user->data['user_id'], 'session_start' => (int) $user->data['session_start'], + 'session_last_visit' => (int) $user->data['session_last_visit'], 'session_time' => (int) $user->time_now, 'session_browser' => (string) trim(substr($user->browser, 0, 149)), 'session_forwarded_for' => (string) $user->forwarded_for, diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 2c14bb46d8..1234e1aa7b 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -3816,7 +3816,7 @@ function page_header($page_title = '', $display_online_list = false, $item_id = } // Last visit date/time - $s_last_visit = ($user->data['user_id'] != ANONYMOUS) ? $user->format_date($user->data['user_lastvisit']) : ''; + $s_last_visit = ($user->data['user_id'] != ANONYMOUS) ? $user->format_date($user->data['session_last_visit']) : ''; // Get users online list ... if required $l_online_users = $online_userlist = $l_online_record = $l_online_time = ''; @@ -3854,10 +3854,10 @@ function page_header($page_title = '', $display_online_list = false, $item_id = { if ($user->data['user_new_privmsg']) { - if (!$user->data['user_last_privmsg'] || $user->data['user_last_privmsg'] > $user->data['user_lastvisit']) + if (!$user->data['user_last_privmsg'] || $user->data['user_last_privmsg'] > $user->data['session_last_visit']) { $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_last_privmsg = ' . $user->data['user_lastvisit'] . ' + SET user_last_privmsg = ' . $user->data['session_last_visit'] . ' WHERE user_id = ' . $user->data['user_id']; $db->sql_query($sql); diff --git a/phpBB/includes/functions_display.php b/phpBB/includes/functions_display.php index 43f1d33a3f..8fd7085586 100644 --- a/phpBB/includes/functions_display.php +++ b/phpBB/includes/functions_display.php @@ -1594,14 +1594,21 @@ function phpbb_show_profile($data, $user_notes_enabled = false, $warn_user_enabl if ($config['load_onlinetrack']) { $update_time = $config['load_online_time'] * 60; - $online = (time() - $update_time < $data['user_lastvisit']) && (!empty($data['session_viewonline']) || $auth->acl_get('u_viewonline')); + $online = (time() - $update_time < $data['session_time'] && ((isset($data['session_viewonline']) && $data['session_viewonline']) || $auth->acl_get('u_viewonline'))) ? true : false; } else { $online = false; } - $last_active = ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline')) ? $data['user_lastvisit'] : ''; + if ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline')) + { + $last_active = (!empty($data['session_time'])) ? $data['session_time'] : $data['user_lastvisit']; + } + else + { + $last_active = ''; + } $age = ''; diff --git a/phpBB/memberlist.php b/phpBB/memberlist.php index cd9d356909..e9af6026d7 100644 --- a/phpBB/memberlist.php +++ b/phpBB/memberlist.php @@ -663,13 +663,14 @@ switch ($mode) if ($config['load_onlinetrack']) { - $sql = 'SELECT MIN(session_viewonline) AS session_viewonline + $sql = 'SELECT MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline FROM ' . SESSIONS_TABLE . " WHERE session_user_id = $user_id"; $result = $db->sql_query($sql); $row = $db->sql_fetchrow($result); $db->sql_freeresult($result); + $member['session_time'] = (isset($row['session_time'])) ? $row['session_time'] : 0; $member['session_viewonline'] = (isset($row['session_viewonline'])) ? $row['session_viewonline'] : 0; unset($row); } @@ -1632,8 +1633,8 @@ switch ($mode) // So, did we get any users? if (count($user_list)) { - // Get recent session viewonline flags - $sql = 'SELECT session_user_id, MIN(session_viewonline) AS session_viewonline + // Session time?! Session time... + $sql = 'SELECT session_user_id, MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline FROM ' . SESSIONS_TABLE . ' WHERE session_time >= ' . (time() - $config['session_length']) . ' AND ' . $db->sql_in_set('session_user_id', $user_list) . ' @@ -1644,6 +1645,7 @@ switch ($mode) while ($row = $db->sql_fetchrow($result)) { $session_ary[$row['session_user_id']] = [ + 'session_time' => $row['session_time'], 'session_viewonline' => $row['session_viewonline'], ]; } @@ -1709,8 +1711,9 @@ switch ($mode) $id_cache = array(); while ($row = $db->sql_fetchrow($result)) { + $row['session_time'] = $session_ary[$row['user_id']]['session_time'] ?? 0; $row['session_viewonline'] = $session_ary[$row['user_id']]['session_viewonline'] ?? 0; - $row['last_visit'] = $row['user_lastvisit']; + $row['last_visit'] = (!empty($row['session_time'])) ? $row['session_time'] : $row['user_lastvisit']; $id_cache[$row['user_id']] = $row; } diff --git a/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php b/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php index 0839abf95f..487e56e598 100644 --- a/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php +++ b/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php @@ -25,11 +25,6 @@ class update_user_lastvisit_data extends \phpbb\db\migration\migration public function update_data() { return [ - 'drop_columns' => [ - $this->table_prefix . 'sessions' => [ - 'session_last_visit', - ], - ], ['custom', [[$this, 'update_user_lastvisit_fields']]], ]; } diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 1357729f88..5f40008702 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -441,9 +441,9 @@ class session $this->check_ban_for_current_session($config); // Update user last visit time accordingly, but in a minute or so - if ((int) $this->time_now - (int) $this->data['user_lastvisit'] > 60) + if ((int) $this->data['session_time'] - (int) $this->data['user_lastvisit'] > 60) { - $this->update_user_lastvisit((int) $this->time_now); + $this->update_user_lastvisit(); } return true; @@ -643,6 +643,15 @@ class session $db->sql_freeresult($result); } + if ($this->data['user_id'] != ANONYMOUS && !$bot) + { + $this->data['session_last_visit'] = (isset($this->data['session_time']) && $this->data['session_time']) ? $this->data['session_time'] : (($this->data['user_lastvisit']) ? $this->data['user_lastvisit'] : time()); + } + else + { + $this->data['session_last_visit'] = $this->time_now; + } + // Force user id to be integer... $this->data['user_id'] = (int) $this->data['user_id']; @@ -681,11 +690,11 @@ class session { $this->session_id = $this->data['session_id']; - // Only sync user last visit time in a minute or so or if the page changes - if ((int) $this->time_now - (int) $this->data['user_lastvisit'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page'])) + // Only sync user last visit time in a minute or so after last session data update or if the page changes + if ((int) $this->data['session_time'] - (int) $this->data['user_lastvisit'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page'])) { // Update the last visit time - $this->update_user_lastvisit((int) $this->time_now); + $this->update_user_lastvisit(); } $SID = '?sid='; @@ -706,6 +715,7 @@ class session $sql_ary = array( 'session_user_id' => (int) $this->data['user_id'], 'session_start' => (int) $this->time_now, + 'session_last_visit' => (int) $this->data['session_last_visit'], 'session_time' => (int) $this->time_now, 'session_browser' => (string) trim(substr($this->browser, 0, 149)), 'session_forwarded_for' => (string) $this->forwarded_for, @@ -815,14 +825,14 @@ class session } else { - $this->data['session_time'] = $this->data['user_lastvisit'] = $this->time_now; + $this->data['session_time'] = $this->data['session_last_visit'] = $this->time_now; $SID = '?sid='; $_SID = ''; } // Update the last visit time - $this->update_user_lastvisit($this->time_now); + $this->update_user_lastvisit(); $session_data = $sql_ary; /** @@ -933,16 +943,82 @@ class session /** * Session garbage collection * - * Effectively delete any sessions, autologin keys and login attempts data - * older than an admin definable limits. - * - * @return void + * This looks a lot more complex than it really is. Effectively we are + * deleting any sessions older than an admin definable limit. Due to the + * way in which we maintain session data we have to ensure we update user + * data before those sessions are destroyed. In addition this method + * removes autologin key information that is older than an admin defined + * limit. */ function session_gc() { global $db, $config, $phpbb_container, $phpbb_dispatcher; - $this->time_now = $this->time_now ?: time(); + if (!$this->time_now) + { + $this->time_now = time(); + } + + /** + * Get most recent session for each registered user to sync user last visit with it + * Inner SELECT gets most recent sessions for each 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_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 . ' @@ -1724,20 +1800,19 @@ class session /** * Update user last visit time * - * @param int $user_lastvisit Timestamp to update user_lastvisit field to * @return bool */ - public function update_user_lastvisit(int $user_lastvisit): bool + public function update_user_lastvisit(): bool { global $db; - if (empty($this->data['user_id']) || empty($user_lastvisit)) + if (!isset($this->data['session_time'], $this->data['user_id'])) { return false; } $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $user_lastvisit . ' + SET user_lastvisit = ' . (int) $this->data['session_time'] . ' WHERE user_id = ' . (int) $this->data['user_id']; $db->sql_query($sql); diff --git a/tests/session/garbage_collection_test.php b/tests/session/garbage_collection_test.php index d287e1145d..9080478a28 100644 --- a/tests/session/garbage_collection_test.php +++ b/tests/session/garbage_collection_test.php @@ -60,6 +60,22 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case '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(); @@ -67,6 +83,22 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case [], '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()