From 33dfaa478c0349a7d52a2361674875c4aa8efe35 Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 22 Jun 2023 13:02:43 +0700 Subject: [PATCH] [ticket/16470] Further do not rely on session_time displaying user activity session_time has not been updated during session_length, so relying on it last activity data will be incorrect, especially if session_length value is high. Thus rely on regular and properly updated user_lastvisit. Remove user_lastvisit vs with session_time sync for the same reason. Also get rid of the session_last_visit field as it floats between user_lastvisit and session_time and actually is meaningless. PHPBB3-16470 PHPBB3-14173 --- phpBB/includes/acp/acp_main.php | 1 - phpBB/includes/functions.php | 6 +- phpBB/includes/functions_display.php | 11 +- phpBB/memberlist.php | 11 +- .../data/v33x/update_user_lastvisit_data.php | 5 + phpBB/phpbb/session.php | 107 +++--------------- 6 files changed, 30 insertions(+), 111 deletions(-) diff --git a/phpBB/includes/acp/acp_main.php b/phpBB/includes/acp/acp_main.php index 395e721464..357cea9a32 100644 --- a/phpBB/includes/acp/acp_main.php +++ b/phpBB/includes/acp/acp_main.php @@ -403,7 +403,6 @@ 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 1234e1aa7b..2c14bb46d8 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['session_last_visit']) : ''; + $s_last_visit = ($user->data['user_id'] != ANONYMOUS) ? $user->format_date($user->data['user_lastvisit']) : ''; // 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['session_last_visit']) + if (!$user->data['user_last_privmsg'] || $user->data['user_last_privmsg'] > $user->data['user_lastvisit']) { $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_last_privmsg = ' . $user->data['session_last_visit'] . ' + SET user_last_privmsg = ' . $user->data['user_lastvisit'] . ' 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 8fd7085586..43f1d33a3f 100644 --- a/phpBB/includes/functions_display.php +++ b/phpBB/includes/functions_display.php @@ -1594,21 +1594,14 @@ 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['session_time'] && ((isset($data['session_viewonline']) && $data['session_viewonline']) || $auth->acl_get('u_viewonline'))) ? true : false; + $online = (time() - $update_time < $data['user_lastvisit']) && (!empty($data['session_viewonline']) || $auth->acl_get('u_viewonline')); } else { $online = false; } - 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 = ''; - } + $last_active = ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline')) ? $data['user_lastvisit'] : ''; $age = ''; diff --git a/phpBB/memberlist.php b/phpBB/memberlist.php index e9af6026d7..cd9d356909 100644 --- a/phpBB/memberlist.php +++ b/phpBB/memberlist.php @@ -663,14 +663,13 @@ switch ($mode) if ($config['load_onlinetrack']) { - $sql = 'SELECT MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline + $sql = 'SELECT 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); } @@ -1633,8 +1632,8 @@ switch ($mode) // So, did we get any users? if (count($user_list)) { - // Session time?! Session time... - $sql = 'SELECT session_user_id, MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline + // Get recent session viewonline flags + $sql = 'SELECT session_user_id, 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) . ' @@ -1645,7 +1644,6 @@ switch ($mode) while ($row = $db->sql_fetchrow($result)) { $session_ary[$row['session_user_id']] = [ - 'session_time' => $row['session_time'], 'session_viewonline' => $row['session_viewonline'], ]; } @@ -1711,9 +1709,8 @@ 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'] = (!empty($row['session_time'])) ? $row['session_time'] : $row['user_lastvisit']; + $row['last_visit'] = $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 487e56e598..0839abf95f 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,6 +25,11 @@ 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 5f40008702..1357729f88 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->data['session_time'] - (int) $this->data['user_lastvisit'] > 60) + if ((int) $this->time_now - (int) $this->data['user_lastvisit'] > 60) { - $this->update_user_lastvisit(); + $this->update_user_lastvisit((int) $this->time_now); } return true; @@ -643,15 +643,6 @@ 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']; @@ -690,11 +681,11 @@ class session { $this->session_id = $this->data['session_id']; - // 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'])) + // 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'])) { // Update the last visit time - $this->update_user_lastvisit(); + $this->update_user_lastvisit((int) $this->time_now); } $SID = '?sid='; @@ -715,7 +706,6 @@ 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, @@ -825,14 +815,14 @@ class session } else { - $this->data['session_time'] = $this->data['session_last_visit'] = $this->time_now; + $this->data['session_time'] = $this->data['user_lastvisit'] = $this->time_now; $SID = '?sid='; $_SID = ''; } // Update the last visit time - $this->update_user_lastvisit(); + $this->update_user_lastvisit($this->time_now); $session_data = $sql_ary; /** @@ -943,82 +933,16 @@ class session /** * Session garbage collection * - * 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. + * Effectively delete any sessions, autologin keys and login attempts data + * older than an admin definable limits. + * + * @return void */ function session_gc() { global $db, $config, $phpbb_container, $phpbb_dispatcher; - 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; - } + $this->time_now = $this->time_now ?: time(); // Delete all expired sessions $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' @@ -1800,19 +1724,20 @@ 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(): bool + public function update_user_lastvisit(int $user_lastvisit): bool { global $db; - if (!isset($this->data['session_time'], $this->data['user_id'])) + if (empty($this->data['user_id']) || empty($user_lastvisit)) { return false; } $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $this->data['session_time'] . ' + SET user_lastvisit = ' . (int) $user_lastvisit . ' WHERE user_id = ' . (int) $this->data['user_id']; $db->sql_query($sql);