Known users cache may not be invalidated when creating a session

[Ryan J Ollos]

The case was missed in #11868. The known users cache will not be invalidated when creating a session without setting the name and email.

session = DetachedSession(self.env, sid)
session['authenticated'] = True
session.save()

[Ryan J Ollos]

Proposed changes: [54236d4d1/rjollos.git].

[Jun Omae]

The authenticated is an attribute of session object, not an item of the object.

-session['authenticated'] = True
+session.authenticated = True

>>> from trac.test import EnvironmentStub, MockRequest
>>> from trac.web.session import Session
>>> env = EnvironmentStub()
>>> req = MockRequest(env, authname='user')
>>> req.authname
'user'
>>> sess = Session(env, req)
>>> sess.authenticated
True
>>> list(env.get_known_users())
[]
>>> sess.save()
>>> list(env.get_known_users())
[]  # should be 1 entry

Currently, new session without associated data is not be persisted, even if authenticated/anonymous session.

    def save(self):
        items = self.items()
        if not self._old and not items:
            # The session doesn't have associated data, so there's no need to
            # persist it
            return

If we should persist new authenticated session without associated data:

trac/web/session.py

@@ -136 +136 @@
                 self._old = {}
 
     def save(self):
-        items = self.items()
-        if not self._old and not items:
-            # The session doesn't have associated data, so there's no need to
-            # persist it
-            return
-
         authenticated = int(self.authenticated)
         now = int(time_now())
+        items = self.items()
+        if not authenticated and not self._old and not items:
+            # The session for anonymous doesn't have associated data,
+            # so there's no need to persist it
+            return
 
         # We can't do the session management in one big transaction,
         # as the intertwined changes to both the session and
@@ -173 +172 @@
                     db.rollback()
                     return
 
+            if authenticated and \
+                    (new or self._old.get('name') != self.get('name') or \
+                     self._old.get('email') != self.get('email')):
+                self.env.invalidate_known_users_cache()
+
             # Remove former values for session_attribute and save the
             # new ones. The last concurrent request to do so "wins".
 
             if self._old != self:
-                if new or self._old.get('name') != self.get('name') or \
-                        self._old.get('email') != self.get('email'):
-                    self.env.invalidate_known_users_cache()
                 if not items and not authenticated:
                     # No need to keep around empty unauthenticated sessions
                     db("DELETE FROM session WHERE sid=%s AND authenticated=0",

trac/web/tests/session.py

@@ -563 +563 @@
         """Setting attribute on a new session invalidates known_users cache."""
         sid = 'user'
         session = DetachedSession(self.env, sid)
-        session['authenticated'] = True
+        session.authenticated = True
 
         self.assertEqual([], list(self.env.get_known_users()))
         session.save()
@@ -573 +573 @@
         self.assertIsNone(known_users[0][1])
         self.assertIsNone(known_users[0][2])
 
+    def test_create_new_anonymous_session(self):
+        """Setting attribute on a new anonymous session doesn't invalidate
+        known_users cache."""
+        sid = 'anonymous'
+        session = DetachedSession(self.env, sid)
+        session.authenticated = False
+
+        self.assertEqual([], list(self.env.get_known_users()))
+        # insert a session record without invalidating cache
+        self.env.insert_users([('user', 'Name' 'user@example.com', 1)])
+        session.save()
+        self.assertEqual([], list(self.env.get_known_users()))
+
+        self.env.invalidate_known_users_cache()
+        self.assertEqual(1, len(list(self.env.get_known_users())))
+
     def test_session_set_email(self):
         """Setting session email invalidates known_users cache."""
         sid = 'user'

[Ryan J Ollos]

Committed with comment:2 changes in r16364, merged to trunk in r16365.