diff options
| author | Xavier Morel <xmo@odoo.com> | 2019-05-21 11:58:28 +0000 |
|---|---|---|
| committer | Olivier Dony <odo@odoo.com> | 2020-08-14 23:03:27 +0000 |
| commit | 083c70bbb63f27839b2c9a4b549e216947bc4dd1 (patch) | |
| tree | 00535c2ae4a0143ad002a66b967ab37490b77b02 | |
| parent | 6e6946577307e569319661d413952f7b61bafd57 (diff) | |
[IMP] core: replace dedicated uid cache by ormcache
Before this, invalidations to the UID cache is not synchronised
between workers because it's an ad-hoc solution (so a user changing
their password or an admin disabling a user would only lock out an
attacker currently using the API of one of possibly several
workers). Shift the entire thing to ormcache which already has proper
support for synchronising cache invalidation between workers.
Also simplify the cache invalidation mess in Users.write because the
caches have been unified into a single registry-level LRU, so the
half-dozen cache clears on specific ormcached methods & models is
pretty much the same as repeatedly calling clear_caches on the current
model.
**However** registry.cache is trivially accessible from server actions
and safe_eval as long as they provide access to a model (through
`model.pool.cache`). Which is common, and an issue given we're very
much putting sensible data in there.
Fix this by renaming `Registry.cache` to `Registry.__cache`, this
requires few editions and mangled names are not accessible from
safe_eval contexts.
The alternative would have been to add more bespoke handling of the
uid cache to hook it into the cache invalidation propagation
machinery.
After discussion with (@)odony, fixing LRU access and using that seems
cleaner and less error-prone.
Note on lazy_property
=====================
Make Registry.cache / Registry.__cache into a regular attribute: the
overhead of the LRU is not that high (compared to that of the registry
itself), it's rare that we *don't* need it, and it's assumed to be a
persisted attribute (it's not just a cache) so making it a normal
attribute seems fine; and lazy_property doesn't work for mangled
names: the name of the property is mangled using the name of the
definition class, but the name of the symbol (fget) is not mangled so
lazy_property would set the __cache attribute but then Python would
lookup _Registry__cache, creating a new cache every access.
And we can't (always) mangle things correctly on `__get__(obj,
owner)`: `owner` is just `type(obj)`, meaning in the case of
inheritance the type we get is the type through which the property is
accessed rather than the one it's defined on. So it would work in the
cases where no inheritance is involved (such as Registry.__cache) but
not in general (lest we want to play around walking the MRO ourselves
to find the definition source, which doesn't seem worth it).
lazy_property *could* be made to work properly on Python 3.6+: the
descriptor protocol gains `__set_name__(name, owner)`, which is called
with the properly mangled name — and with the definition class to boot
(though there might still be issues when overriding lazy properties as
the override will be mangled & named differently... or maybe that's a
feature?). However we're still supporting 3.5 at this point, AFAIK, so
that's not an option. Plus it feels unnecessary / not very useful.
However add an assertion to `lazy_property` so it signals when we try
to use it on a mangled method (as otherwise it kinda sorta work in the
sense that the property / object is accessible but is in effect a
slower way to write a regular property).
| -rw-r--r-- | addons/web/controllers/main.py | 2 | ||||
| -rw-r--r-- | odoo/addons/base/models/res_users.py | 39 | ||||
| -rw-r--r-- | odoo/modules/registry.py | 13 | ||||
| -rw-r--r-- | odoo/service/security.py | 1 | ||||
| -rw-r--r-- | odoo/tools/cache.py | 4 | ||||
| -rw-r--r-- | odoo/tools/func.py | 2 |
6 files changed, 25 insertions, 36 deletions
diff --git a/addons/web/controllers/main.py b/addons/web/controllers/main.py index 383308061de..b2f9b268dea 100644 --- a/addons/web/controllers/main.py +++ b/addons/web/controllers/main.py @@ -918,7 +918,7 @@ class Home(http.Controller): uid = request.env.user.id if request.env.user._is_system(): uid = request.session.uid = odoo.SUPERUSER_ID - request.env['res.users']._invalidate_session_cache() + request.env['res.users'].clear_caches() request.session.session_token = security.compute_session_token(request.session, request.env) return http.local_redirect(self._login_redirect(uid), keep_hash=True) diff --git a/odoo/addons/base/models/res_users.py b/odoo/addons/base/models/res_users.py index 6f7509ac133..a54c9c2b21f 100644 --- a/odoo/addons/base/models/res_users.py +++ b/odoo/addons/base/models/res_users.py @@ -249,7 +249,6 @@ class Users(models.Model): _description = 'Users' _inherits = {'res.partner': 'partner_id'} _order = 'name, login' - __uid_cache = defaultdict(dict) # {dbname: {uid: password}} # User can write on a few of his own fields (but not his groups for example) SELF_WRITEABLE_FIELDS = ['signature', 'action_id', 'company_id', 'email', 'name', 'image_1920', 'lang', 'tz'] @@ -580,26 +579,24 @@ class Users(models.Model): # DLE P139: Calling invalidate_cache on a new, well you lost everything as you wont be able to take it back from the cache # `test_00_equipment_multicompany_user` self.env['ir.model.access'].call_cache_clearing_methods() - self.env['ir.rule'].clear_caches() - self.has_group.clear_cache(self) - if any(key.startswith('context_') or key in ('lang', 'tz') for key in values): - self.context_get.clear_cache(self) - if any(key in values for key in ['active'] + USER_PRIVATE_FIELDS): - db = self._cr.dbname - for id in self.ids: - self.__uid_cache[db].pop(id, None) - if any(key in values for key in self._get_session_token_fields()): - self._invalidate_session_cache() + + # per-method / per-model caches have been removed so the various + # clear_cache/clear_caches methods pretty much just end up calling + # Registry._clear_cache + invalidation_fields = { + 'groups_id', 'active', 'lang', 'tz', 'company_id', + *USER_PRIVATE_FIELDS, + *self._get_session_token_fields() + } + if (invalidation_fields & values.keys()) or any(key.startswith('context_') for key in values): + self.clear_caches() return res def unlink(self): if SUPERUSER_ID in self.ids: raise UserError(_('You can not remove the admin user as it is used internally for resources created by Odoo (updates, module installation, ...)')) - db = self._cr.dbname - for id in self.ids: - self.__uid_cache[db].pop(id, None) - self._invalidate_session_cache() + self.clear_caches() return super(Users, self).unlink() @api.model @@ -719,22 +716,20 @@ class Users(models.Model): return uid @classmethod + @tools.ormcache('uid', 'passwd') def check(cls, db, uid, passwd): """Verifies that the given (uid, password) is authorized for the database ``db`` and raise an exception if it is not.""" if not passwd: # empty passwords disallowed for obvious security reasons raise AccessDenied() - db = cls.pool.db_name - if cls.__uid_cache[db].get(uid) == passwd: - return + with contextlib.closing(cls.pool.cursor()) as cr: self = api.Environment(cr, uid, {})[cls._name] with self._assert_can_auth(): if not self.env.user.active: raise AccessDenied() self._check_credentials(passwd, {'interactive': False}) - cls.__uid_cache[db][uid] = passwd def _get_session_token_fields(self): return {'id', 'login', 'password', 'active'} @@ -748,7 +743,7 @@ class Users(models.Model): FROM res_users WHERE id=%%s""" % (session_fields), (self.id,)) if self.env.cr.rowcount != 1: - self._invalidate_session_cache() + self.clear_caches() return False data_fields = self.env.cr.fetchone() # generate hmac key @@ -759,10 +754,6 @@ class Users(models.Model): # keep in the cache the token return h.hexdigest() - def _invalidate_session_cache(self): - """ Clear the sessions cache """ - self._compute_session_token.clear_cache(self) - @api.model def change_password(self, old_passwd, new_passwd): """Change current user password. Old password must be provided explicitly diff --git a/odoo/modules/registry.py b/odoo/modules/registry.py index f660fccf52c..19b631fc4c3 100644 --- a/odoo/modules/registry.py +++ b/odoo/modules/registry.py @@ -114,6 +114,7 @@ class Registry(Mapping): self._fields_by_model = None self._ordinary_tables = None self._constraint_queue = deque() + self.__cache = LRU(8192) # modules fully loaded (maintained during init phase by `loading` module) self._init_modules = set() @@ -218,6 +219,7 @@ class Registry(Mapping): """ from .. import models + self.clear_caches() lazy_property.reset_all(self) # Instantiate registered classes (via the MetaModel automatic discovery @@ -234,6 +236,7 @@ class Registry(Mapping): """ Complete the setup of models. This must be called after loading modules and before using the ORM. """ + self.clear_caches() lazy_property.reset_all(self) self.registry_invalidated = True env = odoo.api.Environment(cr, SUPERUSER_ID, {}) @@ -504,15 +507,9 @@ class Registry(Mapping): for table in missing_tables: _logger.error("Model %s has no table.", table2model[table]) - @lazy_property - def cache(self): - """ A cache for model methods. """ - # this lazy_property is automatically reset by lazy_property.reset_all() - return LRU(8192) - def _clear_cache(self): """ Clear the cache and mark it as invalidated. """ - self.cache.clear() + self.__cache.clear() self.cache_invalidated = True def clear_caches(self): @@ -618,7 +615,7 @@ class Registry(Mapping): self.setup_models(cr) self.registry_invalidated = False if self.cache_invalidated: - self.cache.clear() + self.__cache.clear() self.cache_invalidated = False @contextmanager diff --git a/odoo/service/security.py b/odoo/service/security.py index 23d89b6c691..c247c6d9ac5 100644 --- a/odoo/service/security.py +++ b/odoo/service/security.py @@ -17,5 +17,4 @@ def check_session(session, env): expected = self._compute_session_token(session.sid) if expected and odoo.tools.misc.consteq(expected, session.session_token): return True - self._invalidate_session_cache() return False diff --git a/odoo/tools/cache.py b/odoo/tools/cache.py index 0e244f63a4b..46652736b93 100644 --- a/odoo/tools/cache.py +++ b/odoo/tools/cache.py @@ -76,7 +76,7 @@ class ormcache(object): def lru(self, model): counter = STAT[(model.pool.db_name, model._name, self.method)] - return model.pool.cache, (model._name, self.method), counter + return model.pool._Registry__cache, (model._name, self.method), counter def lookup(self, method, *args, **kwargs): d, key0, counter = self.lru(args[0]) @@ -207,7 +207,7 @@ def log_ormcache_stats(sig=None, frame=None): for dbname, reg in sorted(Registry.registries.d.items()): # set logger prefix to dbname me.dbname = dbname - entries = Counter(k[:2] for k in reg.cache.d) + entries = Counter(k[:2] for k in reg._Registry__cache.d) # show entries sorted by model name, method name for key in sorted(entries, key=lambda key: (key[0], key[1].__name__)): model, method = key diff --git a/odoo/tools/func.py b/odoo/tools/func.py index deae2bdec04..17b5f85ab24 100644 --- a/odoo/tools/func.py +++ b/odoo/tools/func.py @@ -16,6 +16,8 @@ class lazy_property(object): get it again. """ def __init__(self, fget): + assert not fget.__name__.startswith('__'),\ + "lazy_property does not support mangled names" self.fget = fget def __get__(self, obj, cls): |
