Review and deprecate OMERO.web file-based sessions backend

[sbesson]

The default session engine for OMERO.web is omeroweb.filessessionstore which was derived from django.contrib.sessions.backends.file to handle the cleanup of expired file-based sessions. As indicated in the original commit the intent was to drop this custom backend in favor of Django core file backend when the issue has been fixed upstream. From the original ticket, it looks like the relevant fix was implemented in https://github.com/django/django/commit/c055224763e11b29cce0a7c10751354c40dac63e and was available in Django 1.10 and above.

Looking at the diff between our session store and the current upstream store (Django 3.2.19), the logic is largely identical

(base) sbesson@Sebastiens-MacBook-Pro-2 GitHub % diff -u omero-web/omeroweb/filesessionstore.py django/django/contrib/sessions/backends/file.py 
--- omero-web/omeroweb/filesessionstore.py  2022-10-10 17:01:30
+++ django/django/contrib/sessions/backends/file.py 2023-05-04 11:13:08
@@ -1,52 +1,39 @@
 import datetime
-import errno
 import logging
 import os
 import shutil
 import tempfile

 from django.conf import settings
-from django.contrib.sessions.backends.base import SessionBase, CreateError
-from django.contrib.sessions.backends.base import VALID_KEY_CHARS
-from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured
+from django.contrib.sessions.backends.base import (
+    VALID_KEY_CHARS, CreateError, SessionBase, UpdateError,
+)
+from django.contrib.sessions.exceptions import InvalidSessionKey
+from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
 from django.utils import timezone
-from django.utils.encoding import force_text

-from django.contrib.sessions.exceptions import InvalidSessionKey

-# Aleksandra Tarkowska:
-# This is temporary solution to fix clearout of expiered sessions
-# See: https://code.djangoproject.com/ticket/22938
-
-logger = logging.getLogger(__name__)
-
-
 class SessionStore(SessionBase):
     """
-    Implements a file based session store.
+    Implement a file based session store.
     """
-
     def __init__(self, session_key=None):
-        self.storage_path = type(self)._get_storage_path()
+        self.storage_path = self._get_storage_path()
         self.file_prefix = settings.SESSION_COOKIE_NAME
-        super(SessionStore, self).__init__(session_key)
+        super().__init__(session_key)

     @classmethod
     def _get_storage_path(cls):
         try:
             return cls._storage_path
         except AttributeError:
-            storage_path = getattr(settings, "SESSION_FILE_PATH", None)
-            if not storage_path:
-                storage_path = tempfile.gettempdir()
-
+            storage_path = getattr(settings, 'SESSION_FILE_PATH', None) or tempfile.gettempdir()
             # Make sure the storage path is valid.
             if not os.path.isdir(storage_path):
                 raise ImproperlyConfigured(
-                    "The session storage path %r doesn't exist. Please set"
-                    " your SESSION_FILE_PATH setting to an existing directory"
-                    " in which Django can store session data." % storage_path
-                )
+                    "The session storage path %r doesn't exist. Please set your"
+                    " SESSION_FILE_PATH setting to an existing directory in which"
+                    " Django can store session data." % storage_path)

             cls._storage_path = storage_path
             return storage_path
@@ -61,8 +48,9 @@
         # Make sure we're not vulnerable to directory traversal. Session keys
         # should always be md5s, so they should never contain directory
         # components.
-        if not set(session_key).issubset(set(VALID_KEY_CHARS)):
-            raise InvalidSessionKey("Invalid characters in session key")
+        if not set(session_key).issubset(VALID_KEY_CHARS):
+            raise InvalidSessionKey(
+                "Invalid characters in session key")

         return os.path.join(self.storage_path, self.file_prefix + session_key)

@@ -73,26 +61,21 @@
         modification = os.stat(self._key_to_file()).st_mtime
         if settings.USE_TZ:
             modification = datetime.datetime.utcfromtimestamp(modification)
-            modification = modification.replace(tzinfo=timezone.utc)
-        else:
-            modification = datetime.datetime.fromtimestamp(modification)
-        return modification
+            return modification.replace(tzinfo=timezone.utc)
+        return datetime.datetime.fromtimestamp(modification)

     def _expiry_date(self, session_data):
         """
         Return the expiry time of the file storing the session's content.
         """
-        expiry = session_data.get("_session_expiry", None)
-        if expiry is None:
-            expiry = self._last_modification() + datetime.timedelta(
-                seconds=settings.SESSION_COOKIE_AGE
-            )
-        return expiry
+        return session_data.get('_session_expiry') or (
+            self._last_modification() + datetime.timedelta(seconds=self.get_session_cookie_age())
+        )

     def load(self):
         session_data = {}
         try:
-            with open(self._key_to_file(), "r") as session_file:
+            with open(self._key_to_file(), encoding='ascii') as session_file:
                 file_data = session_file.read()
             # Don't fail if there is no data in the session file.
             # We may have opened the empty placeholder file.
@@ -101,24 +84,18 @@
                     session_data = self.decode(file_data)
                 except (EOFError, SuspiciousOperation) as e:
                     if isinstance(e, SuspiciousOperation):
-                        log = logging.getLogger(
-                            "django.security.%s" % e.__class__.__name__
-                        )
-                        log.warning(force_text(e))
+                        logger = logging.getLogger('django.security.%s' % e.__class__.__name__)
+                        logger.warning(str(e))
                     self.create()

                 # Remove expired sessions.
-                # Fixing https://code.djangoproject.com/ticket/22938
                 expiry_age = self.get_expiry_age(expiry=self._expiry_date(session_data))
-                if expiry_age < 0:
+                if expiry_age <= 0:
                     session_data = {}
                     self.delete()
                     self.create()
-            else:
-                logger.debug("No file_data for session: %s" % self._key_to_file())
-        except (IOError, SuspiciousOperation):
-            logger.debug("Failed to load session data", exc_info=True)
-            self.create()
+        except (OSError, SuspiciousOperation):
+            self._session_key = None
         return session_data

     def create(self):
@@ -128,34 +105,32 @@
                 self.save(must_create=True)
             except CreateError:
                 continue
-            logger.debug("Session created with session_key: %s" % self._session_key)
             self.modified = True
-            self._session_cache = {}
             return

     def save(self, must_create=False):
+        if self.session_key is None:
+            return self.create()
         # Get the session data now, before we start messing
         # with the file it is stored within.
         session_data = self._get_session(no_load=must_create)

         session_file_name = self._key_to_file()
-        logger.debug(
-            "Save session to file with session_file_name: %s" % session_file_name
-        )

         try:
             # Make sure the file exists.  If it does not already exist, an
             # empty placeholder file is created.
-            flags = os.O_WRONLY | os.O_CREAT | getattr(os, "O_BINARY", 0)
+            flags = os.O_WRONLY | getattr(os, 'O_BINARY', 0)
             if must_create:
-                flags |= os.O_EXCL
+                flags |= os.O_EXCL | os.O_CREAT
             fd = os.open(session_file_name, flags)
             os.close(fd)
-
-        except OSError as e:
-            if must_create and e.errno == errno.EEXIST:
+        except FileNotFoundError:
+            if not must_create:
+                raise UpdateError
+        except FileExistsError:
+            if must_create:
                 raise CreateError
-            raise

         # Write the session file without interfering with other threads
         # or processes.  By writing to an atomically generated temporary
@@ -175,9 +150,7 @@
         dir, prefix = os.path.split(session_file_name)

         try:
-            output_file_fd, output_file_name = tempfile.mkstemp(
-                dir=dir, prefix=prefix + "_out_"
-            )
+            output_file_fd, output_file_name = tempfile.mkstemp(dir=dir, prefix=prefix + '_out_')
             renamed = False
             try:
                 try:
@@ -193,10 +166,9 @@
             finally:
                 if not renamed:
                     os.unlink(output_file_name)
+        except (EOFError, OSError):
+            pass

-        except (OSError, IOError, EOFError):
-            logger.debug("Failed to save session data", exc_info=True)
-
     def exists(self, session_key):
         return os.path.exists(self._key_to_file(session_key))

@@ -208,7 +180,7 @@
         try:
             os.unlink(self._key_to_file(session_key))
         except OSError:
-            logger.debug("Failed to delete with session_key: %s" % session_key)
+            pass

     def clean(self):
         pass
@@ -221,7 +193,7 @@
         for session_file in os.listdir(storage_path):
             if not session_file.startswith(file_prefix):
                 continue
-            session_key = session_file[len(file_prefix) :]
+            session_key = session_file[len(file_prefix):]
             session = cls(session_key)
             # When an expired session is loaded, its file is removed, and a
             # new file is immediately created. Prevent this by disabling

Alongside the effort of reducing the amount of custom code, a potential proposal for an upcoming minor OMERO.web release would be to:

• review and confirm that Django's built-in file session store properly handles the session expiry time
• mark this store as deprecated which should be removed in the next major release
• optionally update it to be a simple subclass of django.contrib.sessions.backend.file.SessionStore
• update the default session engine to django.contrib.sessions.backend.file
• test that both django.contrib.sessions.backend.file and omeroweb.filesessionstore engines remain functional

[chris-allan]

With the move to add Django 4.2.x compatibility in #480 and various timezone deprecations that affect this code it's probably a good idea to get this at least started with #480 going in.