[IMP] core: don't save visitor default session

Every request comes with a session, a dictionary that is persisted on
the filesystem and that saves various information such as the user
cart on the ecommerce.

When a user simply visits the website, a default session is created and
saved on disk, this bloats the filestore with many sessions. Creating
the session on-the-fly is cheaper than loading it from the filesystem.
With this work the default session is not saved on disk anymore unless
explicitly asked via `session.touch()`.

An exception to the statement "creating the session on-the-fly is
cheaper" is geoip, the ip geolocalization is not cheap. In this work,
geoip have been moved from http_routing/request.session.geoip to a
lazy property core/request.geoip. When requested the info is persisted
on the session. Like other keys from the default session, geoip will not
be persisted unless there is non-default stuff in the session.

Because the CSRF-TOKEN is based on the session-id, it is important the
session-id stays the same across multiples requests even when the
session is not persisted on disk. Even when a session is not persisted
on disk, the session-id cookie is still set so that the next session
created on-the-fly uses the same session-id.

Technical note regarding the session, it has been decided to drop the
session-snapshot protocol and to reintroduce a "modified" flag. It has
been decided not to use werkzeug's session (which natively comes with a
"modified" flag) and to keep our own session object. We decided to
extend MutableMapping instead of dict; using MutableMapping we only
have to override __setitem__ and __detitem__; using dict we would had to
override update()/pop()/... too.

Task: 2789035
Part-of: odoo/odoo#86015

addons/auth_totp/controllers/home.py

@@ -53,7 +53,7 @@ class Home(web_home.Home):
                         browser=request.httprequest.user_agent.browser.capitalize(),
                         platform=request.httprequest.user_agent.platform.capitalize(),
                     )
-                    geoip = request.session.geoip
+                    geoip = request.geoip
                     if geoip:
                         name += " (%s, %s)" % (geoip['city'], geoip['country_name'])
 
@@ -66,11 +66,11 @@ class Home(web_home.Home):
                         samesite='Lax'
                     )
                 # Crapy workaround for unupdatable Odoo Mobile App iOS (Thanks Apple :@)
-                request.session.should_touch = True
+                request.session.touch()
                 return response
 
         # Crapy workaround for unupdatable Odoo Mobile App iOS (Thanks Apple :@)
-        request.session.should_touch = True
+        request.session.touch()
         return request.render('auth_totp.auth_totp_form', {
             'user': user,
             'error': error,

addons/http_routing/models/ir_http.py

@@ -1,4 +1,4 @@
-# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
 
 import contextlib
 import logging
@@ -27,8 +27,6 @@ from odoo.http import request
 from odoo.osv import expression
 from odoo.tools import config, ustr, pycompat
 
-from ..geoipresolver import GeoIPResolver
-
 _logger = logging.getLogger(__name__)
 
 # global resolver (GeoIP API is thread-safe, for multithreaded workers)
@@ -520,33 +518,10 @@ class IrHttp(models.AbstractModel):
         threading.current_thread().url = httprequest.url
         request.httprequest = httprequest
 
-    @classmethod
-    def _geoip_setup_resolver(cls):
-        # Lazy init of GeoIP resolver
-        if odoo._geoip_resolver is not None:
-            return
-        geofile = config.get('geoip_database')
-        try:
-            odoo._geoip_resolver = GeoIPResolver.open(geofile) or False
-        except Exception as e:
-            _logger.warning('Cannot load GeoIP: %s', ustr(e))
-
-    @classmethod
-    def _geoip_resolve(cls):
-        if 'geoip' not in request.session:
-            record = {}
-            if odoo._geoip_resolver and request.httprequest.remote_addr:
-                record = odoo._geoip_resolver.resolve(request.httprequest.remote_addr) or {}
-            request.session['geoip'] = record
-
-
     @classmethod
     def _pre_dispatch(cls, rule, args):
         super()._pre_dispatch(rule, args)
 
-        cls._geoip_setup_resolver()
-        cls._geoip_resolve()
-
         if request.is_frontend:
             cls._frontend_pre_dispatch()
 

addons/web/controllers/home.py

@@ -44,7 +44,7 @@ class Home(http.Controller):
             raise http.SessionExpiredException("Session expired")
 
         # Side-effect, refresh the session lifetime
-        request.session.should_touch = True
+        request.session.touch()
 
         # Restore the user on the environment, it was lost due to auth="none"
         request.update_env(user=request.session.uid)

addons/web/controllers/session.py

@@ -24,7 +24,7 @@ class Session(http.Controller):
     @http.route('/web/session/get_session_info', type='json', auth="user")
     def get_session_info(self):
         # Crapy workaround for unupdatable Odoo Mobile App iOS (Thanks Apple :@)
-        request.session.should_touch = True
+        request.session.touch()
         return request.env['ir.http'].session_info()
 
     @http.route('/web/session/authenticate', type='json', auth="none")

addons/website/tests/test_page.py

@@ -1,7 +1,10 @@
-# coding: utf-8
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
-from odoo.addons.website.tools import MockRequest
+
+from unittest.mock import patch
+from odoo.http import root
 from odoo.tests import common, HttpCase, tagged
 from odoo.tools import mute_logger
+from odoo.addons.website.tools import MockRequest
 
 
 @tagged('-at_install', 'post_install')
@@ -273,3 +276,10 @@ class WithContext(HttpCase):
             r = self.url_open(self.page.url)
             self.assertEqual(r.status_code, 500, "15/0 raise a 500 error page (2)")
             self.assertIn('ZeroDivisionError: division by zero', r.text, "Error should be shown in debug.")
+
+    def test_04_visitor_no_session(self):
+        with patch.object(root.session_store, 'save') as session_save,\
+             MockRequest(self.env, website=self.env['website'].browse(1)):
+            # no session should be saved for website visitor
+            self.url_open(self.page.url).raise_for_status()
+            session_save.assert_not_called()

addons/website/tools.py

@@ -47,6 +47,7 @@ def MockRequest(
             sale_order_id=sale_order_id,
             website_sale_current_pl=website_sale_current_pl,
         ),
+        geoip={},
         db=None,
         env=env,
         registry=env.registry,

odoo/addons/base/models/ir_http.py

@@ -119,6 +119,10 @@ class IrHttp(models.AbstractModel):
             _logger.info("Exception during request Authentication.", exc_info=True)
             raise AccessDenied()
 
+    @classmethod
+    def _geoip_resolve(cls):
+        return request._geoip_resolve()
+
     @classmethod
     def _pre_dispatch(cls, rule, args):
         request.dispatcher.pre_dispatch(rule, args)

odoo/addons/base/tests/test_xmlrpc.py

@@ -128,7 +128,8 @@ class TestAPIKeys(common.HttpCase):
                 'cookies': {},
             }),
             # bypass check_identity flow
-            'session': {'identity-check-last': time.time()}
+            'session': {'identity-check-last': time.time()},
+            'geoip': {},
         })
         _request_stack.push(fake_req)
         self.addCleanup(_request_stack.pop)

odoo/addons/test_http/controllers.py

@@ -120,3 +120,15 @@ class TestHttp(http.Controller):
         ensure_db()
         assert request.db, "There should be a database"
         return request.db
+
+    # =====================================================
+    # Session
+    # =====================================================
+    @http.route('/test_http/geoip', type='http', auth='none')
+    def geoip(self):
+        return str(request.geoip)
+
+    @http.route('/test_http/save_session', type='http', auth='none')
+    def touch(self):
+        request.session.touch()
+        return ''

odoo/addons/test_http/tests/test_http.py

@@ -12,7 +12,19 @@ from odoo.tests.common import HOST, HttpCase, new_test_user
 from odoo.tools import config, file_open, mute_logger
 from odoo.tools.func import lazy_property
 from odoo.addons.test_http.controllers import CT_JSON
-from odoo.addons.test_http.utils import MemorySessionStore, HtmlTokenizer
+from odoo.addons.test_http.utils import (
+    MemoryGeoipResolver, MemorySessionStore, HtmlTokenizer
+)
+
+GEOIP_ODOO_FARM_2 = {
+    'city': 'Ramillies',
+    'country_code': 'BE',
+    'country_name': 'Belgium',
+    'latitude': 50.6314,
+    'longitude': 4.8573,
+    'region': 'WAL',
+    'time_zone': 'Europe/Brussels'
+}
 
 
 class TestHttpBase(HttpCase):
@@ -23,6 +35,7 @@ class TestHttpBase(HttpCase):
         cls.classPatch(odoo.conf, 'server_wide_modules', ['base', 'web', 'test_http'])
         lazy_property.reset_all(odoo.http.root)
         cls.classPatch(odoo.http.root, 'session_store', MemorySessionStore(session_class=Session))
+        cls.classPatch(odoo.http.root, 'geoip_resolver', MemoryGeoipResolver())
 
     def setUp(self):
         super().setUp()
@@ -392,19 +405,6 @@ class TestHttpMisc(TestHttpBase):
             self.assertEqual(res.json()['REMOTE_ADDR'], client_ip)
             self.assertEqual(res.json()['HTTP_HOST'], host)
 
-    @mute_logger('odoo.http')  # greeting_none called ignoring args {'debug'}
-    def test_misc3_debug_mode(self):
-        session = self.authenticate(None, None)
-        self.assertEqual(session.debug, '')
-        self.db_url_open('/test_http/greeting').raise_for_status()
-        self.assertEqual(session.debug, '')
-        self.db_url_open('/test_http/greeting?debug=1').raise_for_status()
-        self.assertEqual(session.debug, '1')
-        self.db_url_open('/test_http/greeting').raise_for_status()
-        self.assertEqual(session.debug, '1')
-        self.db_url_open('/test_http/greeting?debug=').raise_for_status()
-        self.assertEqual(session.debug, '')
-
 
 @tagged('post_install', '-at_install')
 class TestHttpCors(TestHttpBase):
@@ -502,3 +502,61 @@ class TestHttpEnsureDb(TestHttpBase):
         res.raise_for_status()
         self.assertEqual(res.status_code, 200)
         self.assertEqual(res.text, 'db1')
+
+
+class TestHttpSession(TestHttpBase):
+
+    @mute_logger('odoo.http')  # greeting_none called ignoring args {'debug'}
+    def test_session0_debug_mode(self):
+        session = self.authenticate(None, None)
+        self.assertEqual(session.debug, '')
+        self.db_url_open('/test_http/greeting').raise_for_status()
+        self.assertEqual(session.debug, '')
+        self.db_url_open('/test_http/greeting?debug=1').raise_for_status()
+        self.assertEqual(session.debug, '1')
+        self.db_url_open('/test_http/greeting').raise_for_status()
+        self.assertEqual(session.debug, '1')
+        self.db_url_open('/test_http/greeting?debug=').raise_for_status()
+        self.assertEqual(session.debug, '')
+
+    def test_session1_default_session(self):
+        # The default session should not be saved on the filestore.
+        with patch.object(odoo.http.root.session_store, 'save') as mock_save:
+            res = self.db_url_open('/test_http/greeting')
+            res.raise_for_status()
+            try:
+                mock_save.assert_not_called()
+            except AssertionError as exc:
+                msg = f'save() was called with args: {mock_save.call_args}'
+                raise AssertionError(msg) from exc
+
+    def test_session2_geoip(self):
+        real_save = odoo.http.root.session_store.save
+        with patch.object(odoo.http.root.geoip_resolver, 'resolve') as mock_resolve,\
+             patch.object(odoo.http.root.session_store, 'save') as mock_save:
+            mock_resolve.return_value = GEOIP_ODOO_FARM_2
+            mock_save.side_effect = real_save
+
+            # Geoip is lazy: it should be computed only when necessary.
+            self.nodb_url_open('/test_http/greeting').raise_for_status()
+            mock_resolve.assert_not_called()
+
+            # Geoip is like the defaut session: the session should not
+            # be stored only due to geoip.
+            mock_resolve.reset_mock()
+            mock_save.reset_mock()
+            res = self.nodb_url_open('/test_http/geoip')
+            res.raise_for_status()
+            self.assertEqual(res.text, str(GEOIP_ODOO_FARM_2))
+            mock_save.assert_not_called()
+
+            # Geoip is cached on the session: we shouldn't geolocate the
+            # same ip multiple times.
+            mock_resolve.reset_mock()
+            mock_save.reset_mock()
+            self.nodb_url_open('/test_http/save_session').raise_for_status()
+            self.nodb_url_open('/test_http/geoip').raise_for_status()
+            res = self.nodb_url_open('/test_http/geoip')
+            res.raise_for_status()
+            self.assertEqual(res.text, str(GEOIP_ODOO_FARM_2))
+            mock_resolve.assert_called_once()

odoo/addons/test_http/utils.py

@@ -5,6 +5,11 @@ from odoo.http import FilesystemSessionStore
 from odoo.tools._vendor.sessions import SessionStore
 
 
+class MemoryGeoipResolver:
+    def resolve(self, ip):
+        return {}
+
+
 class MemorySessionStore(SessionStore):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)

odoo/http.py

@@ -110,6 +110,7 @@ endpoint
 
 import cgi
 import collections
+import collections.abc
 import contextlib
 import functools
 import glob
@@ -124,6 +125,7 @@ import re
 import threading
 import time
 import traceback
+import warnings
 import zlib
 from abc import ABC, abstractmethod
 from datetime import datetime
@@ -154,6 +156,7 @@ from .modules.registry import Registry
 from .service import security, model as service_model
 from .tools import (config, consteq, date_utils, profiler, resolve_attr,
                     submap, unique, ustr,)
+from .tools.geoipresolver import GeoIPResolver
 from .tools.func import filter_kwargs, lazy_property
 from .tools.mimetypes import guess_mimetype
 from .tools._vendor import sessions
@@ -703,29 +706,56 @@ class FilesystemSessionStore(sessions.FilesystemSessionStore):
                     os.unlink(path)
 
 
-class Session(dict):
+class Session(collections.abc.MutableMapping):
     """ Structure containing data persisted across requests. """
-    __slots__ = ('can_save', 'is_explicit', 'json_data', 'new', 'should_rotate', 'should_touch', 'sid')
+    __slots__ = ('can_save', 'data', 'is_dirty', 'is_explicit', 'is_new',
+                 'should_rotate', 'sid')
 
     def __init__(self, data, sid, new=False):
-        super().__init__(data)
+        self.can_save = True
-        object.__setattr__(self, 'can_save', True)
+        self.data = data
-        object.__setattr__(self, 'is_explicit', False)
+        self.is_dirty = False
-        object.__setattr__(self, 'json_data', json.dumps(data))
+        self.is_explicit = False
-        object.__setattr__(self, 'new', new)
+        self.is_new = new
-        object.__setattr__(self, 'should_rotate', False)
+        self.should_rotate = False
-        object.__setattr__(self, 'should_touch', False)
+        self.sid = sid
-        object.__setattr__(self, 'sid', sid)
+
+    #
+    # MutableMapping implementation with DocDict-like extension
+    #
+    def __getitem__(self, item):
+        if item == 'geoip':
+            warnings.warn('request.session.geoip have been moved to request.geoip', DeprecationWarning)
+            return request.geoip if request else {}
+        return self.data[item]
+
+    def __setitem__(self, item, value):
+        if item not in self.data or self.data[item] != value:
+            self.is_dirty = True
+        self.data[item] = value
+
+    def __delitem__(self, item):
+        del self.data[item]
+        self.is_dirty = True
+
+    def __len__(self):
+        return len(self.data)
+
+    def __iter__(self):
+        return iter(self.data)
 
     def __getattr__(self, attr):
         return self.get(attr, None)
 
     def __setattr__(self, key, val):
         if key in self.__slots__:
-            object.__setattr__(self, key, val)
+            super().__setattr__(key, val)
         else:
             self[key] = val
 
+    #
+    # Session methods
+    #
     def authenticate(self, dbname, login=None, password=None):
         """
         Authenticate the current user with the given db, login and
@@ -796,6 +826,9 @@ class Session(dict):
         self.context['lang'] = request.default_lang() if request else DEFAULT_LANG
         self.should_rotate = True
 
+    def touch(self):
+        self.is_dirty = True
+
 
 # =========================================================
 # Request and Response
@@ -919,18 +952,11 @@ class Request:
         self.dispatcher = _dispatchers['http'](self)  # until we match
         #self.params = {}  # set by the Dispatcher
 
-        self.session = self._get_session()
+        self.session, self.db = self._get_session_and_dbname()
-        self.db = self._get_dbname()
         self.registry = None
         self.env = None
 
-        if self.session.db != self.db:
+    def _get_session_and_dbname(self):
-            if self.session.db:
-                _logger.warning("Logged into database %r, but dbfilter rejects it; logging session out.", self.session.db)
-                self.session.logout(keep_db=False)
-            self.session.db = self.db
-
-    def _get_session(self):
         # The session is explicit when it comes from the query-string or
         # the header. It is implicit when it comes from the cookie or
         # that is does not exist yet. The explicit session should be
@@ -948,26 +974,31 @@ class Request:
             session = root.session_store.new()
         else:
             session = root.session_store.get(sid)
-
+            session.sid = sid  # in case the session was not persisted
         session.is_explicit = is_explicit
+
         for key, val in DEFAULT_SESSION.items():
             session.setdefault(key, val)
         if not session.context.get('lang'):
             session.context['lang'] = self.default_lang()
 
-        return session
+        dbname = None
+        host = self.httprequest.environ['HTTP_HOST']
+        if session.db and db_filter([session.db], host=host):
+            dbname = session.db
+        else:
+            all_dbs = db_list(force=True, host=host)
+            if len(all_dbs) == 1:
+                dbname = all_dbs[0]  # monodb
 
-    def _get_dbname(self):
+        if session.db != dbname:
-        if self.session.db and db_filter([self.session.db], host=self.httprequest.environ['HTTP_HOST']):
+            if session.db:
-            return self.session.db
+                _logger.warning("Logged into database %r, but dbfilter rejects it; logging session out.", session.db)
+                session.logout(keep_db=False)
+            session.db = dbname
 
-        # monodb
+        session.is_dirty = False
-        all_dbs = db_list(force=True, host=self.httprequest.environ['HTTP_HOST'])
+        return session, dbname
-        if len(all_dbs) == 1:
-            return all_dbs[0]
-
-        # nodb
-        return None
 
     # =====================================================
     # Getters and setters
@@ -1014,6 +1045,27 @@ class Request:
 
     _cr = cr
 
+    @property
+    def geoip(self):
+        """
+        Get the remote address geolocalisation.
+
+        When geolocalization is successful, the return value is a
+        dictionary whoose format is:
+
+            {'city': str, 'country_code': str, 'country_name': str,
+             'latitude': float, 'longitude': float, 'region': str,
+             'time_zone': str}
+
+        When geolocalization fails, an empty dict is returned.
+        """
+        if '_geoip' not in self.session:
+            was_dirty = self.session.is_dirty
+            self.session._geoip = (self.registry['ir.http']._geoip_resolve()
+                                   if self.db else self._geoip_resolve())
+            self.session.is_dirty = was_dirty
+        return self.session._geoip
+
     # =====================================================
     # Helpers
     # =====================================================
@@ -1090,6 +1142,11 @@ class Request:
         except (ValueError, KeyError):
             return DEFAULT_LANG
 
+    def _geoip_resolve(self):
+        if not (root.geoip_resolver and self.httprequest.remote_addr):
+            return {}
+        return root.geoip_resolver.resolve(self.httprequest.remote_addr) or {}
+
     def get_http_params(self):
         """
         Extract key=value pairs from the query string and the forms
@@ -1212,8 +1269,10 @@ class Request:
             return
 
         if sess.should_rotate:
+            sess['_geoip'] = self.geoip
             root.session_store.rotate(sess, self.env)  # it saves
-        elif sess.should_touch or json.dumps(sess) != sess.json_data:
+        elif sess.is_dirty:
+            sess['_geoip'] = self.geoip
             root.session_store.save(sess)
 
         # We must not set the cookie if the session id was specified
@@ -1226,7 +1285,7 @@ class Request:
         #   cookie). That is a special feature of the Javascript Session.
         # - It could allow session fixation attacks.
         cookie_sid = self.httprequest.cookies.get('session_id')
-        if (sess.should_touch or cookie_sid != sess.sid and not sess.is_explicit):
+        if not sess.is_explicit and (sess.is_dirty or cookie_sid != sess.sid):
             self.future_response.set_cookie('session_id', sess.sid, max_age=SESSION_LIFETIME, httponly=True)
 
     def _set_request_dispatcher(self, rule):
@@ -1620,6 +1679,13 @@ class Application:
         _logger.debug('HTTP sessions stored in: %s', path)
         return FilesystemSessionStore(path, session_class=Session, renew_missing=True)
 
+    @lazy_property
+    def geoip_resolver(self):
+        try:
+            return GeoIPResolver.open(config.get('geoip_database'))
+        except Exception as e:
+            _logger.warning('Cannot load GeoIP: %s', e)
+
     def get_db_router(self, db):
         if not db:
             return self.nodb_routing_map

odoo/service/model.py

@@ -143,8 +143,7 @@ def retrying(func, env):
                 env.cr.rollback()
                 env.registry.reset_changes()
                 if request:
-                    request.session.clear()
+                    request.session = request._get_session_and_dbname()[0]
-                    request.session.update(json.loads(request.session.json_data))
                 if isinstance(exc, IntegrityError):
                     raise _as_validation_error(env, exc) from exc
                 if exc.pgcode not in PG_CONCURRENCY_ERRORS_TO_RETRY: