From eb0ba7f79b8a5c099a6f039af770c54169abe584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Tue, 12 Sep 2023 09:31:54 +0200 Subject: [PATCH] [IMP] core: check field groups for search domain and order In order to boost the read security of groups on field, we now check groups of field used in search domain and search order. Part-of: odoo/odoo#135111 --- odoo/addons/test_access_rights/models.py | 1 + .../test_access_rights/tests/test_feedback.py | 109 +++++++++++++----- odoo/models.py | 16 ++- 3 files changed, 92 insertions(+), 34 deletions(-) diff --git a/odoo/addons/test_access_rights/models.py b/odoo/addons/test_access_rights/models.py index 1f1c6cc8850..e42b52af283 100644 --- a/odoo/addons/test_access_rights/models.py +++ b/odoo/addons/test_access_rights/models.py @@ -9,6 +9,7 @@ class SomeObj(models.Model): val = fields.Integer() categ_id = fields.Many2one('test_access_right.obj_categ') + parent_id = fields.Many2one('test_access_right.some_obj') company_id = fields.Many2one('res.company') forbidden = fields.Integer( groups='test_access_rights.test_group,!base.group_no_one,!base.group_public', diff --git a/odoo/addons/test_access_rights/tests/test_feedback.py b/odoo/addons/test_access_rights/tests/test_feedback.py index a4a5a5a0f96..1fc90f3854a 100644 --- a/odoo/addons/test_access_rights/tests/test_feedback.py +++ b/odoo/addons/test_access_rights/tests/test_feedback.py @@ -5,19 +5,21 @@ import odoo from odoo import SUPERUSER_ID, Command from odoo.exceptions import AccessError from odoo.tests import TransactionCase +from odoo.tools.misc import mute_logger class Feedback(TransactionCase): - def setUp(self): - super().setUp() + @classmethod + def setUpClass(cls): + super().setUpClass() - self.group0 = self.env['res.groups'].create({'name': "Group 0"}) - self.group1 = self.env['res.groups'].create({'name': "Group 1"}) - self.group2 = self.env['res.groups'].create({'name': "Group 2"}) - self.user = self.env['res.users'].create({ + cls.group0 = cls.env['res.groups'].create({'name': "Group 0"}) + cls.group1 = cls.env['res.groups'].create({'name': "Group 1"}) + cls.group2 = cls.env['res.groups'].create({'name': "Group 2"}) + cls.user = cls.env['res.users'].create({ 'login': 'bob', 'name': "Bob Bobman", - 'groups_id': [Command.set([self.group2.id, self.env.ref('base.group_user').id])], + 'groups_id': [Command.set([cls.group2.id, cls.env.ref('base.group_user').id])], }) @@ -90,28 +92,30 @@ class TestSudo(Feedback): class TestACLFeedback(Feedback): """ Tests that proper feedback is returned on ir.model.access errors """ - def setUp(self): - super().setUp() - ACL = self.env['ir.model.access'] - m = self.env['ir.model'].search([('model', '=', 'test_access_right.some_obj')]) + @classmethod + def setUpClass(cls): + super().setUpClass() + + ACL = cls.env['ir.model.access'] + m = cls.env['ir.model'].search([('model', '=', 'test_access_right.some_obj')]) ACL.search([('model_id', '=', m.id)]).unlink() ACL.create({ 'name': "read", 'model_id': m.id, - 'group_id': self.group1.id, + 'group_id': cls.group1.id, 'perm_read': True, }) ACL.create({ 'name': "create-and-read", 'model_id': m.id, - 'group_id': self.group0.id, + 'group_id': cls.group0.id, 'perm_read': True, 'perm_create': True, }) - self.record = self.env['test_access_right.some_obj'].create({'val': 5}) + cls.record = cls.env['test_access_right.some_obj'].create({'val': 5}) # values are in cache, clear them up for the test - self.env.flush_all() - self.env.invalidate_all() + cls.env.flush_all() + cls.env.invalidate_all() def test_no_groups(self): """ Operation is never allowed @@ -159,14 +163,14 @@ Contact your administrator to request access if necessary.""" class TestIRRuleFeedback(Feedback): """ Tests that proper feedback is returned on ir.rule errors """ - def setUp(self): - super().setUp() - self.env.ref('base.group_user').write({'users': [Command.link(self.user.id)]}) - self.model = self.env['ir.model'].search([('model', '=', 'test_access_right.some_obj')]) - self.record = self.env['test_access_right.some_obj'].create({ + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.env.ref('base.group_user').write({'users': [Command.link(cls.user.id)]}) + cls.model = cls.env['ir.model'].search([('model', '=', 'test_access_right.some_obj')]) + cls.record = cls.env['test_access_right.some_obj'].create({ 'val': 0, - }).with_user(self.user) - + }).with_user(cls.user) def debug_mode(self): odoo.http._request_stack.push(Mock(db=self.env.cr.dbname, env=self.env, debug=True)) @@ -174,7 +178,6 @@ class TestIRRuleFeedback(Feedback): self.env.flush_all() self.env.invalidate_all() - def _make_rule(self, name, domain, global_=False, attr='write'): res = self.env['ir.rule'].create({ 'name': name, @@ -410,13 +413,17 @@ Contact your administrator to request access if necessary.""" % (self.record.dis class TestFieldGroupFeedback(Feedback): - def setUp(self): - super().setUp() - self.record = self.env['test_access_right.some_obj'].create({ + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.record = cls.env['test_access_right.some_obj'].create({ 'val': 0, - }).with_user(self.user) - + }).with_user(cls.user) + cls.inherits_record = cls.env['test_access_right.inherits'].create({ + 'some_id': cls.record.id, + }).with_user(cls.user) + @mute_logger('odoo.models') def test_read(self): self.env.ref('base.group_no_one').write( {'users': [Command.link(self.user.id)]}) @@ -449,6 +456,7 @@ Fields: - forbidden3 (always forbidden)""" % self.user.id ) + @mute_logger('odoo.models') def test_write(self): self.env.ref('base.group_no_one').write( {'users': [Command.link(self.user.id)]}) @@ -468,3 +476,46 @@ Fields: - forbidden2 (allowed for groups 'Test Group')""" % self.user.id ) + + @mute_logger('odoo.models') + def test_check_field_access_rights_domain(self): + with self.assertRaises(AccessError): + self.record.search([('forbidden3', '=like', 'blu%')]) + + with self.assertRaises(AccessError): + self.record.search([('parent_id.forbidden3', '=like', 'blu%')]) + + with self.assertRaises(AccessError): + self.record.search([('parent_id', 'any', [('forbidden3', '=like', 'blu%')])]) + + with self.assertRaises(AccessError): + self.inherits_record.search([('forbidden3', '=like', 'blu%')]) + + @mute_logger('odoo.models') + def test_check_field_access_rights_order(self): + self.record.search([], order='val') + + with self.assertRaises(AccessError): + self.record.search([], order='forbidden3 DESC') + + with self.assertRaises(AccessError): + self.record.search([], order='forbidden3') + + with self.assertRaises(AccessError): + self.record.search([], order='val DESC, forbidden3 DESC') + + @mute_logger('odoo.models') + def test_check_field_access_rights_read_group(self): + self.record._read_group([], ['val'], []) + + with self.assertRaises(AccessError): + self.record._read_group([('forbidden3', '=like', 'blu%')], ['val']) + + with self.assertRaises(AccessError): + self.record._read_group([('parent_id.forbidden3', '=like', 'blu%')], ['val']) + + with self.assertRaises(AccessError): + self.record._read_group([], ['forbidden3']) + + with self.assertRaises(AccessError): + self.record._read_group([], [], ['forbidden3:array_agg']) diff --git a/odoo/models.py b/odoo/models.py index b96a8a17a84..165551a0ad5 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -5250,10 +5250,6 @@ class BaseModel(metaclass=MetaModel): # return the model found by traversing all fields (used in collect_from_domain) return model - # also take into account the fields in the record rules - domain = list(domain) + (self.env['ir.rule']._compute_domain(self._name, 'read') or []) - collect_from_domain(self, domain) - # flush the order fields if order: for order_part in order.split(','): @@ -5265,9 +5261,19 @@ class BaseModel(metaclass=MetaModel): comodel = self.env[field.comodel_name] comodel._flush_search([], order=comodel._order, seen=seen) - if self._active_name: + if self._active_name and self.env.context.get('active_test', True): to_flush[self._name].add(self._active_name) + collect_from_domain(self, domain) + + # Check access of fields with groups + for model_name, field_names in to_flush.items(): + self.env[model_name].check_field_access_rights('read', field_names) + + # also take into account the fields in the record rules + if ir_rule_domain := self.env['ir.rule']._compute_domain(self._name, 'read'): + collect_from_domain(self, ir_rule_domain) + # flush model dependencies (recursively) if self._depends: models = [self]