From 8c38baee8526b725536eac6b6dee04aca92a4aad Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Tue, 4 Jul 2023 08:55:11 +0000 Subject: [PATCH] [IMP] core: enforce custom fields to starts with `x_` The `api.constrains` `_check_name` on `ir.model.fields` ensures users do not create custom fields not starting with `x_`. It offers a user-friendly message when users try to do that. However, this `api.constrains` can be bypassed, for instance when updating the field name through raw SQL. This revision aims to no longer load custom fields if they do not starts with `x_`. In addition, it replaces the Python constraint with an SQL constraint in order to have a stronger constraint. Following bugs or unexpected flows, people were able to add custom fields not starting with `x_` with the `api.constrains` alone. Adding the SQL constraint will tighten the constraint. However, even without that SQL constraint, for instance if users drop the SQL constraint manually, custom field not starting by `x_` must not be loaded. A user would be able to drop that constraint through a SQL shell or a `cr.execute` in a server action. This is to prevent users to override class attributes. closes odoo/odoo#127702 Related: odoo/upgrade#4908 --- odoo/addons/base/models/ir_model.py | 9 ++++++--- odoo/addons/base/tests/test_ir_actions.py | 24 +++++++++++++++++++++-- odoo/models.py | 18 ++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/odoo/addons/base/models/ir_model.py b/odoo/addons/base/models/ir_model.py index eefbbfb66af..012bd3cd298 100644 --- a/odoo/addons/base/models/ir_model.py +++ b/odoo/addons/base/models/ir_model.py @@ -596,11 +596,9 @@ class IrModelFields(models.Model): for field in self: safe_eval(field.domain or '[]') - @api.constrains('name', 'state') + @api.constrains('name') def _check_name(self): for field in self: - if field.state == 'manual' and not field.name.startswith('x_'): - raise ValidationError(_("Custom fields must have a name that starts with 'x_'!")) try: models.check_pg_name(field.name) except ValidationError: @@ -610,6 +608,11 @@ class IrModelFields(models.Model): _sql_constraints = [ ('name_unique', 'UNIQUE(model, name)', "Field names must be unique per model."), ('size_gt_zero', 'CHECK (size>=0)', 'Size of the field cannot be negative.'), + ( + "name_manual_field", + "CHECK (state != 'manual' OR name LIKE 'x_%')", + "Custom fields must have a name that starts with 'x_'!" + ), ] def _related_field(self): diff --git a/odoo/addons/base/tests/test_ir_actions.py b/odoo/addons/base/tests/test_ir_actions.py index 81f96c266be..f9ee8c395ad 100644 --- a/odoo/addons/base/tests/test_ir_actions.py +++ b/odoo/addons/base/tests/test_ir_actions.py @@ -379,13 +379,13 @@ class TestCustomFields(common.TransactionCase): def test_create_custom(self): """ custom field names must be start with 'x_' """ - with self.assertRaises(ValidationError): + with self.assertRaises(IntegrityError), mute_logger('odoo.sql_db'): self.create_field('foo') def test_rename_custom(self): """ custom field names must be start with 'x_' """ field = self.create_field('x_foo') - with self.assertRaises(ValidationError): + with self.assertRaises(IntegrityError), mute_logger('odoo.sql_db'): field.name = 'foo' def test_create_valid(self): @@ -399,6 +399,26 @@ class TestCustomFields(common.TransactionCase): with self.assertRaises(ValidationError): field.name = 'x_foo bar' + def test_add_field_valid(self): + """ custom field names must start with 'x_', even when bypassing the constraints + + If a user bypasses all constraints to add a custom field not starting by `x_`, + it must not be loaded in the registry. + + This is to forbid users to override class attributes. + """ + field = self.create_field('x_foo') + # Drop the SQL constraint, to bypass it, + # as a user could do through a SQL shell or a `cr.execute` in a server action + self.env.cr.execute("ALTER TABLE ir_model_fields DROP CONSTRAINT ir_model_fields_name_manual_field") + self.env.cr.execute("UPDATE ir_model_fields SET name = 'foo' WHERE id = %s", [field.id]) + with self.assertLogs('odoo.addons.base.models.ir_model') as log_catcher: + # Trick to reload the registry. The above rename done through SQL didn't reload the registry. This will. + field.write({'field_description': 'foo'}) + self.assertIn( + f'The field `{field.name}` is not defined in the `{field.model}` Python class', log_catcher.output[0] + ) + def test_create_unique(self): """ one cannot create two fields with the same name on a given model """ self.create_field('x_foo') diff --git a/odoo/models.py b/odoo/models.py index 770c8dec42c..7ac14d9f25a 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -613,12 +613,28 @@ class BaseModel(metaclass=MetaModel): def _add_field(self, name, field): """ Add the given ``field`` under the given ``name`` in the class """ cls = type(self) - # add field as an attribute and in cls._fields (for reflection) + + # Assert the name is an existing field in the model, or any model in the _inherits + # or a custom field (starting by `x_`) + is_class_field = any( + isinstance(getattr(model, name, None), fields.Field) + for model in [cls] + [self.env.registry[inherit] for inherit in cls._inherits] + ) + if not (is_class_field or name.startswith('x_')): + raise ValidationError( + f"The field `{name}` is not defined in the `{cls._name}` Python class and does not start with 'x_'" + ) + + # Assert the attribute to assign is a Field + if not isinstance(field, fields.Field): + raise ValidationError("You can only add `fields.Field` objects to a model fields") + if not isinstance(getattr(cls, name, field), Field): _logger.warning("In model %r, field %r overriding existing value", cls._name, name) setattr(cls, name, field) field._toplevel = True field.__set_name__(cls, name) + # add field as an attribute and in cls._fields (for reflection) cls._fields[name] = field @api.model