Repository: frappe/frappe
Issue: Session expiry is not reliable

idk how to describe this, but once a week I get randomly logged out while I am actively using the system. This feels buggy based on how sessions work. 


Not much further info, will just have read and check all session management code to find something 

Changed files:
frappe/sessions.py, frappe/tests/test_auth.py

diff --git a/frappe/sessions.py b/frappe/sessions.py
index 64a1a6b663..2709de8fab 100644
--- a/frappe/sessions.py
+++ b/frappe/sessions.py
@@ -18,10 +18,9 @@ import frappe.translate
 import frappe.utils
 from frappe import _
 from frappe.cache_manager import clear_user_cache
-from frappe.query_builder import DocType, Order
-from frappe.query_builder.functions import Now
-from frappe.query_builder.utils import PseudoColumn
+from frappe.query_builder import Order
 from frappe.utils import cint, cstr, get_assets_json
+from frappe.utils.data import add_to_date
 
 
 @frappe.whitelist()
@@ -62,7 +61,7 @@ def get_sessions_to_clear(user=None, keep_current=False):
 		simultaneous_sessions = frappe.db.get_value("User", user, "simultaneous_sessions") or 1
 		offset = simultaneous_sessions - 1
 
-	session = DocType("Sessions")
+	session = frappe.qb.DocType("Sessions")
 	session_id = frappe.qb.from_(session).where(session.user == user)
 	if keep_current:
 		session_id = session_id.where(session.sid != frappe.session.sid)
@@ -88,7 +87,7 @@ def delete_session(sid=None, user=None, reason="Session Expired"):
 	frappe.cache.hdel("session", sid)
 	frappe.cache.hdel("last_db_session_update", sid)
 	if sid and not user:
-		table = DocType("Sessions")
+		table = frappe.qb.DocType("Sessions")
 		user_details = (
 			frappe.qb.from_(table).where(table.sid == sid).select(table.user).run(as_dict=True)
 		)
@@ -112,17 +111,12 @@ def clear_all_sessions(reason=None):
 def get_expired_sessions():
 	"""Returns list of expired sessions"""
 
-	sessions = DocType("Sessions")
-
-	return frappe.db.get_values(
-		sessions,
-		filters=(
-			PseudoColumn(f"({Now()} - {sessions.lastupdate.get_sql()})") > get_expiry_period_for_query()
-		),
-		fieldname="sid",
-		order_by=None,
-		pluck=True,
-	)
+	sessions = frappe.qb.DocType("Sessions")
+	return (
+		frappe.qb.from_(sessions)
+		.select(sessions.sid)
+		.where(sessions.lastupdate < get_expired_threshold())
+	).run(pluck=True)
 
 
 def clear_expired_sessions():
@@ -232,7 +226,7 @@ class Session:
 			sid = frappe.generate_hash()
 
 		self.data.user = self.user
-		self.data.sid = sid
+		self.sid = self.data.sid = sid
 		self.data.data.user = self.user
 		self.data.data.session_ip = frappe.local.request_ip
 		if self.user != "Guest":
@@ -268,14 +262,17 @@ class Session:
 			frappe.db.commit()
 
 	def insert_session_record(self):
-		frappe.db.sql(
-			"""insert into `tabSessions`
-			(`sessiondata`, `user`, `lastupdate`, `sid`, `status`)
-			values (%s , %s, NOW(), %s, 'Active')""",
-			(str(self.data["data"]), self.data["user"], self.data["sid"]),
-		)
 
-		# also add to memcache
+		Sessions = frappe.qb.DocType("Sessions")
+		now = frappe.utils.now()
+
+		(
+			frappe.qb.into(Sessions)
+			.columns(
+				Sessions.sessiondata, Sessions.user, Sessions.lastupdate, Sessions.sid, Sessions.status
+			)
+			.insert((str(self.data["data"]), self.data["user"], now, self.data["sid"], "Active"))
+		).run()
 		frappe.cache.hset("session", self.data.sid, self.data)
 
 	def resume(self):
@@ -338,20 +335,18 @@ class Session:
 		return data and data.data
 
 	def get_session_data_from_db(self):
-		sessions = DocType("Sessions")
-		rec = frappe.db.get_values(
-			sessions,
-			filters=(sessions.sid == self.sid)
-			& (
-				PseudoColumn(f"({Now()} - {sessions.lastupdate.get_sql()})") < get_expiry_period_for_query()
-			),
-			fieldname=["user", "sessiondata"],
-			order_by=None,
-		)
-
-		if rec:
-			data = frappe._dict(frappe.safe_eval(rec and rec[0][1] or "{}"))
-			data.user = rec[0][0]
+		sessions = frappe.qb.DocType("Sessions")
+
+		record = (
+			frappe.qb.from_(sessions)
+			.select(sessions.user, sessions.sessiondata)
+			.where(sessions.sid == self.sid)
+			.where(sessions.lastupdate > get_expired_threshold())
+		).run()
+
+		if record:
+			data = frappe._dict(frappe.safe_eval(record and record[0][1] or "{}"))
+			data.user = record[0][0]
 		else:
 			self._delete_session()
 			data = None
@@ -373,6 +368,8 @@ class Session:
 
 		now = frappe.utils.now()
 
+		Sessions = frappe.qb.DocType("Sessions")
+
 		self.data["data"]["last_updated"] = now
 		self.data["data"]["lang"] = str(frappe.lang)
 
@@ -384,17 +381,14 @@ class Session:
 		updated_in_db = False
 		if (force or (time_diff is None) or (time_diff > 600)) and not frappe.flags.read_only:
 			# update sessions table
-			frappe.db.sql(
-				"""update `tabSessions` set sessiondata=%s,
-				lastupdate=NOW() where sid=%s""",
-				(str(self.data["data"]), self.data["sid"]),
-			)
+			(
+				frappe.qb.update(Sessions)
+				.where(Sessions.sid == self.data["sid"])
+				.set(Sessions.sessiondata, str(self.data["data"]))
+				.set(Sessions.lastupdate, now)
+			).run()
 
-			# update last active in user table
-			frappe.db.sql(
-				"""update `tabUser` set last_active=%(now)s where name=%(name)s""",
-				{"now": now, "name": frappe.session.user},
-			)
+			frappe.db.set_value("User", frappe.session.user, "last_active", now, update_modified=False)
 
 			frappe.db.commit()
 			frappe.cache.hset("last_db_session_update", self.sid, now)
@@ -421,6 +415,15 @@ def get_expiry_in_seconds(expiry=None):
 	return (cint(parts[0]) * 3600) + (cint(parts[1]) * 60) + cint(parts[2])
 
 
+def get_expired_threshold():
+	"""Get cutoff time before which all sessions are considered expired."""
+
+	now = frappe.utils.now()
+	expiry_in_seconds = get_expiry_in_seconds()
+
+	return add_to_date(now, seconds=-expiry_in_seconds, as_string=True)
+
+
 def get_expiry_period():
 	exp_sec = frappe.defaults.get_global_default("session_expiry") or "06:00:00"
 
diff --git a/frappe/tests/test_auth.py b/frappe/tests/test_auth.py
index d4cfe4451e..6aa8d2cc2f 100644
--- a/frappe/tests/test_auth.py
+++ b/frappe/tests/test_auth.py
@@ -1,14 +1,18 @@
 # Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 import time
+from unittest.mock import patch
 
 import requests
 
 import frappe
-import frappe.utils
 from frappe.auth import LoginAttemptTracker
 from frappe.frappeclient import AuthError, FrappeClient
+from frappe.sessions import Session, get_expired_sessions, get_expiry_in_seconds
+from frappe.tests.test_api import FrappeAPITestCase
 from frappe.tests.utils import FrappeTestCase
+from frappe.utils import get_site_url, now
+from frappe.utils.data import add_to_date
 from frappe.www.login import _generate_temporary_login_link
 
 
@@ -26,9 +30,7 @@ class TestAuth(FrappeTestCase):
 	@classmethod
 	def setUpClass(cls):
 		super().setUpClass()
-		cls.HOST_NAME = frappe.get_site_config().host_name or frappe.utils.get_site_url(
-			frappe.local.site
-		)
+		cls.HOST_NAME = frappe.get_site_config().host_name or get_site_url(frappe.local.site)
 		cls.test_user_email = "test_auth@test.com"
 		cls.test_user_name = "test_auth_user"
 		cls.test_user_mobile = "+911234567890"
@@ -197,3 +199,27 @@ class TestLoginAttemptTracker(FrappeTestCase):
 
 		tracker.add_failure_attempt()
 		self.assertTrue(tracker.is_user_allowed())
+
+
+class TestSessionExpirty(FrappeAPITestCase):
+	def test_session_expires(self):
+		sid = self.sid  # triggers login for test case login
+		s: Session = frappe.local.session_obj
+
+		expiry_in = get_expiry_in_seconds()
+		session_created = now()
+
+		# Try with 1% increments of times, it should always work
+		for step in range(0, 100, 1):
+			seconds_elapsed = expiry_in * step / 100
+
+			time_now = add_to_date(session_created, seconds=seconds_elapsed, as_string=True)
+			with patch("frappe.utils.now", return_value=time_now):
+				data = s.get_session_data_from_db()
+				self.assertEqual(data.user, "Administrator")
+
+		# 1% higher should immediately expire
+		time_now = add_to_date(session_created, seconds=expiry_in * 1.01, as_string=True)
+		with patch("frappe.utils.now", return_value=time_now):
+			self.assertIn(sid, get_expired_sessions())
+			self.assertFalse(s.get_session_data_from_db())