From aafb5225387809f41ed432c1606cc454d3f5c908 Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 11 Jun 2023 14:55:30 +0700 Subject: [PATCH 01/10] [ticket/16470] Update user last visit time on session begin Update user last visit time on session begin same way as on session create. PHPBB3-16470 PHPBB3-14173 --- phpBB/phpbb/session.php | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 6bdce89de26..2bc99a86dcc 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -440,6 +440,12 @@ function session_begin($update_session_page = true) // Is user banned? Are they excluded? Won't return on ban, exists within method $this->check_ban_for_current_session($config); + // Update user last visit time accordingly, but in a minute or so + if ($this->time_now - $this->data['session_time'] > 60) + { + $this->update_user_lastvisit(); + } + return true; } } @@ -684,14 +690,11 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f { $this->session_id = $this->data['session_id']; - // Only update session DB a minute or so after last update or if page changes + // Only sync user last visit time in a minute or so after last session data update or if the page changes if ($this->time_now - $this->data['session_time'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page'])) { // Update the last visit time - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $this->data['session_time'] . ' - WHERE user_id = ' . (int) $this->data['user_id']; - $db->sql_query($sql); + $this->update_user_lastvisit(); } $SID = '?sid='; @@ -1797,4 +1800,28 @@ public function id() : int { return isset($this->data['user_id']) ? (int) $this->data['user_id'] : ANONYMOUS; } + + /** + * Update user last visit time + * + * @return bool + */ + public function update_user_lastvisit() + { + global $db; + + if (!isset($this->data['session_time'], $this->data['user_id'])) + { + return false; + } + + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $this->data['session_time'] . ' + WHERE user_id = ' . (int) $this->data['user_id']; + + if ($db->sql_query($sql)) + { + return true; + } + } } From aefdd86020441bdc9f6032a82dd0da72860079ed Mon Sep 17 00:00:00 2001 From: rxu Date: Mon, 12 Jun 2023 01:37:34 +0700 Subject: [PATCH 02/10] [ticket/16470] Add migration to update existing data PHPBB3-16470 PHPBB3-14173 --- .../data/v33x/update_user_lastvisit_data.php | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php 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 new file mode 100644 index 00000000000..487e56e5983 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php @@ -0,0 +1,97 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\db\migration\data\v33x; + +class update_user_lastvisit_data extends \phpbb\db\migration\migration +{ + static public function depends_on() + { + return [ + '\phpbb\db\migration\data\v33x\v3310', + ]; + } + + public function update_data() + { + return [ + ['custom', [[$this, 'update_user_lastvisit_fields']]], + ]; + } + + public function update_user_lastvisit_fields() + { + /** + * Logic is borrowed from function session_gc() + * + * Get most recent sessions for registered users + * Inner SELECT gets most recent sessions for unique session_user_id + * Outer SELECT gets data for them + */ + $sql_select = 'SELECT 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 ($this->db->get_sql_layer()) + { + case 'sqlite3': + if (phpbb_version_compare($this->db->sql_server_info(true), '3.8.3', '>=')) + { + // For SQLite versions 3.8.3+ which support Common Table Expressions (CTE) + $sql = "WITH s3 (session_user_id, session_time) AS ($sql_select) + UPDATE " . USERS_TABLE . ' + SET (user_lastvisit) = (SELECT session_time FROM s3 WHERE session_user_id = user_id) + WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)'; + $this->db->sql_query($sql); + + break; + } + + // No break, for SQLite versions prior to 3.8.3 and Oracle + case 'oracle': + $result = $this->db->sql_query($sql_select); + while ($row = $this->db->sql_fetchrow($result)) + { + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $row['recent_time'] . ' + WHERE user_id = ' . (int) $row['session_user_id']; + $this->db->sql_query($sql); + } + $this->db->sql_freeresult($result); + break; + + case 'mysqli': + $sql = 'UPDATE ' . USERS_TABLE . " u, + ($sql_select) s3 + SET u.user_lastvisit = s3.recent_time + WHERE u.user_id = s3.session_user_id"; + $this->db->sql_query($sql); + break; + + default: + $sql = 'UPDATE ' . USERS_TABLE . " + SET user_lastvisit = s3.recent_time + FROM ($sql_select) s3 + WHERE user_id = s3.session_user_id"; + $this->db->sql_query($sql); + break; + } + } +} From a14e8f8ce5c9ea788ae0d6d86b3cece6aa486b52 Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 15 Jun 2023 23:21:34 +0700 Subject: [PATCH 03/10] [ticket/16470] Adjust method result type PHPBB3-16470 PHPBB3-14173 --- phpBB/phpbb/session.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 2bc99a86dcc..a74a25e122e 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -1806,7 +1806,7 @@ public function id() : int * * @return bool */ - public function update_user_lastvisit() + public function update_user_lastvisit(): bool { global $db; @@ -1818,10 +1818,8 @@ public function update_user_lastvisit() $sql = 'UPDATE ' . USERS_TABLE . ' SET user_lastvisit = ' . (int) $this->data['session_time'] . ' WHERE user_id = ' . (int) $this->data['user_id']; + $db->sql_query($sql); - if ($db->sql_query($sql)) - { - return true; - } + return (bool) $db->sql_affectedrows(); } } From 9e55d3cb44b7dc803192cd043c0646850a23eeea Mon Sep 17 00:00:00 2001 From: rxu Date: Sat, 17 Jun 2023 00:46:38 +0700 Subject: [PATCH 04/10] [ticket/16470] Adjust user last visit time update logic Previous logic doesn't work correctly as session_time value got updated in regular manner, so that its 60 seconds lag with time_now can be unreachable in some cases. So update user_lastvisit if it lags for 60 sec of session_time. PHPBB3-16470 PHPBB3-14173 --- phpBB/phpbb/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index a74a25e122e..2b98b34ee2c 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -441,7 +441,7 @@ function session_begin($update_session_page = true) $this->check_ban_for_current_session($config); // Update user last visit time accordingly, but in a minute or so - if ($this->time_now - $this->data['session_time'] > 60) + if ((int) $this->data['session_time'] - (int) $this->data['user_lastvisit'] > 60) { $this->update_user_lastvisit(); } @@ -691,7 +691,7 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f $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 ($this->time_now - $this->data['session_time'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page'])) + 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(); From bbe2b213108f94121e05008fff23ed9ed3424e9d Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 18 Jun 2023 23:56:44 +0700 Subject: [PATCH 05/10] [ticket/16470] Correctly handle user last visit update on session create PHPBB3-16470 PHPBB3-14173 --- phpBB/phpbb/session.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 2b98b34ee2c..e1f54f42f57 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -827,16 +827,13 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f { $this->data['session_time'] = $this->data['session_last_visit'] = $this->time_now; - // Update the last visit time - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $this->data['session_time'] . ' - WHERE user_id = ' . (int) $this->data['user_id']; - $db->sql_query($sql); - $SID = '?sid='; $_SID = ''; } + // Update the last visit time + $this->update_user_lastvisit(); + $session_data = $sql_ary; /** * Event to send new session data to extension From 735b8260632f0cd25a7a40b1f6b669ff2caa0d9f Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 21 Jun 2023 00:02:29 +0700 Subject: [PATCH 06/10] [ticket/16470] Do not sync user last visit time with expired sessions time PHPBB3-16470 PHPBB3-14173 --- phpBB/phpbb/session.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index e1f54f42f57..5f40008702b 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -960,8 +960,8 @@ function session_gc() } /** - * Get expired sessions for registered users, only most recent for each user - * Inner SELECT gets most recent expired sessions for unique session_user_id + * 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 @@ -969,8 +969,7 @@ function session_gc() 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 . ' + WHERE session_user_id <> ' . ANONYMOUS . ' GROUP BY session_user_id ) AS s2 ON s1.session_user_id = s2.session_user_id From 33dfaa478c0349a7d52a2361674875c4aa8efe35 Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 22 Jun 2023 13:02:43 +0700 Subject: [PATCH 07/10] [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 395e7214640..357cea9a324 100644 --- a/phpBB/includes/acp/acp_main.php +++ b/phpBB/includes/acp/acp_main.php @@ -403,7 +403,6 @@ function main($id, $mode) '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 1234e1aa7b2..2c14bb46d8d 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 8fd70855868..43f1d33a3f8 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 e9af6026d77..cd9d356909b 100644 --- a/phpBB/memberlist.php +++ b/phpBB/memberlist.php @@ -663,14 +663,13 @@ 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 @@ // 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 @@ 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 @@ $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 487e56e5983..0839abf95f8 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 @@ static public function depends_on() 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 5f40008702b..1357729f88d 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -441,9 +441,9 @@ function session_begin($update_session_page = true) $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f { $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f } 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 @@ function session_kill($new_session = true) /** * 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 @@ public function id() : int /** * 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); From f99d1a7a51116f04e606c149712068b1cce67fbc Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 22 Jun 2023 14:33:27 +0700 Subject: [PATCH 08/10] [ticket/16470] Remove tests for user_lastvisit vs session_time sync PHPBB3-16470 PHPBB3-14173 --- tests/session/garbage_collection_test.php | 32 ----------------------- 1 file changed, 32 deletions(-) diff --git a/tests/session/garbage_collection_test.php b/tests/session/garbage_collection_test.php index 9080478a285..d287e1145d9 100644 --- a/tests/session/garbage_collection_test.php +++ b/tests/session/garbage_collection_test.php @@ -60,22 +60,6 @@ public function test_session_gc() '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(); @@ -83,22 +67,6 @@ public function test_session_gc() [], '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() From 7ba5f8f22aa334a27aa7f5074b0be32b2a351793 Mon Sep 17 00:00:00 2001 From: rxu Date: Mon, 26 Jun 2023 10:05:47 +0700 Subject: [PATCH 09/10] [ticket/16470] Revert session_time sync back This reverts changes made by last 2 commits as they seem to break things. PHPBB3-16470 --- 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 +++++++++++++++--- tests/session/garbage_collection_test.php | 32 ++++++ 7 files changed, 143 insertions(+), 30 deletions(-) diff --git a/phpBB/includes/acp/acp_main.php b/phpBB/includes/acp/acp_main.php index 357cea9a324..395e7214640 100644 --- a/phpBB/includes/acp/acp_main.php +++ b/phpBB/includes/acp/acp_main.php @@ -403,6 +403,7 @@ function main($id, $mode) '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 2c14bb46d8d..1234e1aa7b2 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 43f1d33a3f8..8fd70855868 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 cd9d356909b..e9af6026d77 100644 --- a/phpBB/memberlist.php +++ b/phpBB/memberlist.php @@ -663,13 +663,14 @@ 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 @@ // 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 @@ 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 @@ $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 0839abf95f8..487e56e5983 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 @@ static public function depends_on() 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 1357729f88d..5f40008702b 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -441,9 +441,9 @@ function session_begin($update_session_page = true) $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f { $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f $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 @@ function session_create($user_id = false, $set_admin = false, $persist_login = f } 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 @@ function session_kill($new_session = true) /** * 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 @@ public function id() : int /** * 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 d287e1145d9..9080478a285 100644 --- a/tests/session/garbage_collection_test.php +++ b/tests/session/garbage_collection_test.php @@ -60,6 +60,22 @@ public function test_session_gc() '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 @@ public function test_session_gc() [], '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() From 9e130333c02fcf11ae23c6924124d7658871beed Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 1 Nov 2023 21:03:07 +0700 Subject: [PATCH 10/10] [ticket/16470] remove unneeded migration, adjust code PHPBB3-16470 --- .../data/v33x/update_user_lastvisit_data.php | 97 ------------------- phpBB/phpbb/session.php | 18 ++-- 2 files changed, 6 insertions(+), 109 deletions(-) delete mode 100644 phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php 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 deleted file mode 100644 index 487e56e5983..00000000000 --- a/phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php +++ /dev/null @@ -1,97 +0,0 @@ - - * @license GNU General Public License, version 2 (GPL-2.0) - * - * For full copyright and license information, please see - * the docs/CREDITS.txt file. - * - */ - -namespace phpbb\db\migration\data\v33x; - -class update_user_lastvisit_data extends \phpbb\db\migration\migration -{ - static public function depends_on() - { - return [ - '\phpbb\db\migration\data\v33x\v3310', - ]; - } - - public function update_data() - { - return [ - ['custom', [[$this, 'update_user_lastvisit_fields']]], - ]; - } - - public function update_user_lastvisit_fields() - { - /** - * Logic is borrowed from function session_gc() - * - * Get most recent sessions for registered users - * Inner SELECT gets most recent sessions for unique session_user_id - * Outer SELECT gets data for them - */ - $sql_select = 'SELECT 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 ($this->db->get_sql_layer()) - { - case 'sqlite3': - if (phpbb_version_compare($this->db->sql_server_info(true), '3.8.3', '>=')) - { - // For SQLite versions 3.8.3+ which support Common Table Expressions (CTE) - $sql = "WITH s3 (session_user_id, session_time) AS ($sql_select) - UPDATE " . USERS_TABLE . ' - SET (user_lastvisit) = (SELECT session_time FROM s3 WHERE session_user_id = user_id) - WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)'; - $this->db->sql_query($sql); - - break; - } - - // No break, for SQLite versions prior to 3.8.3 and Oracle - case 'oracle': - $result = $this->db->sql_query($sql_select); - while ($row = $this->db->sql_fetchrow($result)) - { - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $row['recent_time'] . ' - WHERE user_id = ' . (int) $row['session_user_id']; - $this->db->sql_query($sql); - } - $this->db->sql_freeresult($result); - break; - - case 'mysqli': - $sql = 'UPDATE ' . USERS_TABLE . " u, - ($sql_select) s3 - SET u.user_lastvisit = s3.recent_time - WHERE u.user_id = s3.session_user_id"; - $this->db->sql_query($sql); - break; - - default: - $sql = 'UPDATE ' . USERS_TABLE . " - SET user_lastvisit = s3.recent_time - FROM ($sql_select) s3 - WHERE user_id = s3.session_user_id"; - $this->db->sql_query($sql); - break; - } - } -} diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 5f40008702b..df88a849df4 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -1799,23 +1799,17 @@ public function id() : int /** * Update user last visit time - * - * @return bool */ - public function update_user_lastvisit(): bool + public function update_user_lastvisit() { global $db; - if (!isset($this->data['session_time'], $this->data['user_id'])) + if (isset($this->data['session_time'], $this->data['user_id'])) { - return false; + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $this->data['session_time'] . ' + WHERE user_id = ' . (int) $this->data['user_id']; + $db->sql_query($sql); } - - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $this->data['session_time'] . ' - WHERE user_id = ' . (int) $this->data['user_id']; - $db->sql_query($sql); - - return (bool) $db->sql_affectedrows(); } }