[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

@@ -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,

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);
 

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 = ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline')) ? $data['user_lastvisit'] : '';
-	{
-		$last_active = (!empty($data['session_time'])) ? $data['session_time'] : $data['user_lastvisit'];
-	}
-	else
-	{
-		$last_active = '';
-	}
 
 	$age = '';
 

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...
+			// Get recent session viewonline flags
-			$sql = 'SELECT session_user_id, MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline
+			$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;
 			}

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']]],
 		];
 	}

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
+				// Only sync user last visit time in a minute or so 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']))
+				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
+	* Effectively delete any sessions, autologin keys and login attempts data
-	* deleting any sessions older than an admin definable limit. Due to the
+	* older than an admin definable limits.
-	* way in which we maintain session data we have to ensure we update user
+	*
-	* data before those sessions are destroyed. In addition this method
+	* @return void
-	* removes autologin key information that is older than an admin defined
-	* limit.
 	*/
 	function session_gc()
 	{
 		global $db, $config, $phpbb_container, $phpbb_dispatcher;
 
-		if (!$this->time_now)
+		$this->time_now = $this->time_now ?: time();
-		{
-			$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 . '
@@ -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);