From 04e972660b3f38c8dfa42111b1fe88adbcff4699 Mon Sep 17 00:00:00 2001 From: Julien Castiaux Date: Fri, 25 Mar 2022 12:19:55 +0000 Subject: [PATCH] [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 | 6 +- addons/http_routing/models/ir_http.py | 27 +--- addons/web/controllers/home.py | 2 +- addons/web/controllers/session.py | 2 +- addons/website/tests/test_page.py | 14 +- addons/website/tools.py | 1 + odoo/addons/base/models/ir_http.py | 4 + odoo/addons/base/tests/test_xmlrpc.py | 3 +- odoo/addons/test_http/controllers.py | 12 ++ odoo/addons/test_http/tests/test_http.py | 86 +++++++++-- odoo/addons/test_http/utils.py | 5 + odoo/http.py | 134 +++++++++++++----- odoo/service/model.py | 3 +- .../tools}/geoipresolver.py | 0 14 files changed, 215 insertions(+), 84 deletions(-) rename {addons/http_routing => odoo/tools}/geoipresolver.py (100%) diff --git a/addons/auth_totp/controllers/home.py b/addons/auth_totp/controllers/home.py index cb458358cc6..cb97c404f25 100644 --- a/addons/auth_totp/controllers/home.py +++ b/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, diff --git a/addons/http_routing/models/ir_http.py b/addons/http_routing/models/ir_http.py index 4528b6be325..62eea049830 100644 --- a/addons/http_routing/models/ir_http.py +++ b/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() diff --git a/addons/web/controllers/home.py b/addons/web/controllers/home.py index 6f01251d01d..13a7e6b1b41 100644 --- a/addons/web/controllers/home.py +++ b/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) diff --git a/addons/web/controllers/session.py b/addons/web/controllers/session.py index 2dab618ae7d..cc07b3dc7aa 100644 --- a/addons/web/controllers/session.py +++ b/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") diff --git a/addons/website/tests/test_page.py b/addons/website/tests/test_page.py index 15964e49093..8e820e28685 100644 --- a/addons/website/tests/test_page.py +++ b/addons/website/tests/test_page.py @@ -1,7 +1,10 @@ -# coding: utf-8 -from odoo.addons.website.tools import MockRequest +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +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() diff --git a/addons/website/tools.py b/addons/website/tools.py index c84bc41e700..f61530be192 100644 --- a/addons/website/tools.py +++ b/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, diff --git a/odoo/addons/base/models/ir_http.py b/odoo/addons/base/models/ir_http.py index 61b0f4f6a7f..4738a569dc3 100644 --- a/odoo/addons/base/models/ir_http.py +++ b/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) diff --git a/odoo/addons/base/tests/test_xmlrpc.py b/odoo/addons/base/tests/test_xmlrpc.py index 2f6ecdf394f..ed47d691e6d 100644 --- a/odoo/addons/base/tests/test_xmlrpc.py +++ b/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) diff --git a/odoo/addons/test_http/controllers.py b/odoo/addons/test_http/controllers.py index 795e1b20d14..a6a78ec7e99 100644 --- a/odoo/addons/test_http/controllers.py +++ b/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 '' diff --git a/odoo/addons/test_http/tests/test_http.py b/odoo/addons/test_http/tests/test_http.py index 38f1c82cb58..aaa711ef70c 100644 --- a/odoo/addons/test_http/tests/test_http.py +++ b/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() diff --git a/odoo/addons/test_http/utils.py b/odoo/addons/test_http/utils.py index 962ee888afc..3e26d821d02 100644 --- a/odoo/addons/test_http/utils.py +++ b/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) diff --git a/odoo/http.py b/odoo/http.py index 26e21923e54..311b1ca6b99 100644 --- a/odoo/http.py +++ b/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) - object.__setattr__(self, 'can_save', True) - object.__setattr__(self, 'is_explicit', False) - object.__setattr__(self, 'json_data', json.dumps(data)) - object.__setattr__(self, 'new', new) - object.__setattr__(self, 'should_rotate', False) - object.__setattr__(self, 'should_touch', False) - object.__setattr__(self, 'sid', sid) + self.can_save = True + self.data = data + self.is_dirty = False + self.is_explicit = False + self.is_new = new + self.should_rotate = False + 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.db = self._get_dbname() + self.session, self.db = self._get_session_and_dbname() self.registry = None self.env = None - if self.session.db != self.db: - 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): + def _get_session_and_dbname(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 self.session.db and db_filter([self.session.db], host=self.httprequest.environ['HTTP_HOST']): - return self.session.db + if session.db != dbname: + if 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 - all_dbs = db_list(force=True, host=self.httprequest.environ['HTTP_HOST']) - if len(all_dbs) == 1: - return all_dbs[0] - - # nodb - return None + session.is_dirty = False + return session, dbname # ===================================================== # 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 diff --git a/odoo/service/model.py b/odoo/service/model.py index 887e732a85f..91ea16457a1 100644 --- a/odoo/service/model.py +++ b/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.update(json.loads(request.session.json_data)) + request.session = request._get_session_and_dbname()[0] if isinstance(exc, IntegrityError): raise _as_validation_error(env, exc) from exc if exc.pgcode not in PG_CONCURRENCY_ERRORS_TO_RETRY: diff --git a/addons/http_routing/geoipresolver.py b/odoo/tools/geoipresolver.py similarity index 100% rename from addons/http_routing/geoipresolver.py rename to odoo/tools/geoipresolver.py