From 6014d44b2ba081da7148f37c02d89132776785e2 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 10 Jul 2014 13:42:17 +0200 Subject: [PATCH 1/6] [IMP] add read access to all users for product.pricelist.type In new API, selection field value is checked (against the selection's possible values) at both read and write. For pricelist.type this requires read access to product.pricelist.type. --- addons/product/security/ir.model.access.csv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/product/security/ir.model.access.csv b/addons/product/security/ir.model.access.csv index e8c9bfd145e..fd9274df9e4 100644 --- a/addons/product/security/ir.model.access.csv +++ b/addons/product/security/ir.model.access.csv @@ -8,7 +8,7 @@ access_product_packaging_user,product.packaging.user,model_product_packaging,bas access_product_supplierinfo_user,product.supplierinfo.user,model_product_supplierinfo,base.group_user,1,0,0,0 access_pricelist_partnerinfo_user,pricelist.partnerinfo.user,model_pricelist_partnerinfo,base.group_user,1,0,0,0 access_product_price_type_user,product.price.type.user,model_product_price_type,base.group_user,1,0,0,0 -access_product_pricelist_type_user,product.pricelist.type.user,model_product_pricelist_type,base.group_user,1,0,0,0 +access_product_pricelist_type_user,product.pricelist.type.user,model_product_pricelist_type,,1,0,0,0 access_product_pricelist_user,product.pricelist.user,model_product_pricelist,base.group_user,1,0,0,0 access_product_pricelist_version_user,product.pricelist.version.user,model_product_pricelist_version,base.group_user,1,0,0,0 access_product_pricelist_item_user,product.pricelist.item.user,model_product_pricelist_item,base.group_user,1,0,0,0 From d5eda984c9d3b858623b2002ae2a1ddcf78924a5 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 11 Jul 2014 12:42:07 +0200 Subject: [PATCH 2/6] [ADD] demonstrate broken handling of rules access denial in new API --- openerp/addons/test_access_rights/__init__.py | 2 ++ .../addons/test_access_rights/__openerp__.py | 6 ++++ .../test_access_rights/ir.model.access.csv | 2 ++ openerp/addons/test_access_rights/models.py | 6 ++++ .../test_access_rights/tests/__init__.py | 1 + .../test_access_rights/tests/test_ir_rules.py | 33 +++++++++++++++++++ 6 files changed, 50 insertions(+) create mode 100644 openerp/addons/test_access_rights/__init__.py create mode 100644 openerp/addons/test_access_rights/__openerp__.py create mode 100644 openerp/addons/test_access_rights/ir.model.access.csv create mode 100644 openerp/addons/test_access_rights/models.py create mode 100644 openerp/addons/test_access_rights/tests/__init__.py create mode 100644 openerp/addons/test_access_rights/tests/test_ir_rules.py diff --git a/openerp/addons/test_access_rights/__init__.py b/openerp/addons/test_access_rights/__init__.py new file mode 100644 index 00000000000..89d26e2f597 --- /dev/null +++ b/openerp/addons/test_access_rights/__init__.py @@ -0,0 +1,2 @@ +# -*- coding: utf-8 -*- +import models diff --git a/openerp/addons/test_access_rights/__openerp__.py b/openerp/addons/test_access_rights/__openerp__.py new file mode 100644 index 00000000000..bcccb90fd99 --- /dev/null +++ b/openerp/addons/test_access_rights/__openerp__.py @@ -0,0 +1,6 @@ +{ + 'name': 'test of access rights and rules', + 'description': "Testing of access restrictions", + 'version': '0.0.1', + 'data': ['ir.model.access.csv'], +} diff --git a/openerp/addons/test_access_rights/ir.model.access.csv b/openerp/addons/test_access_rights/ir.model.access.csv new file mode 100644 index 00000000000..a0bd5facb02 --- /dev/null +++ b/openerp/addons/test_access_rights/ir.model.access.csv @@ -0,0 +1,2 @@ +id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink +access_test_access_right_some_obj,access_test_access_right_some_obj,model_test_access_right_some_obj,,1,1,1,1 diff --git a/openerp/addons/test_access_rights/models.py b/openerp/addons/test_access_rights/models.py new file mode 100644 index 00000000000..b7752cb3665 --- /dev/null +++ b/openerp/addons/test_access_rights/models.py @@ -0,0 +1,6 @@ +from openerp import fields, models + +class SomeObj(models.Model): + _name = 'test_access_right.some_obj' + + val = fields.Integer() diff --git a/openerp/addons/test_access_rights/tests/__init__.py b/openerp/addons/test_access_rights/tests/__init__.py new file mode 100644 index 00000000000..6e1c696fc37 --- /dev/null +++ b/openerp/addons/test_access_rights/tests/__init__.py @@ -0,0 +1 @@ +import test_ir_rules diff --git a/openerp/addons/test_access_rights/tests/test_ir_rules.py b/openerp/addons/test_access_rights/tests/test_ir_rules.py new file mode 100644 index 00000000000..6cfb1bf9de7 --- /dev/null +++ b/openerp/addons/test_access_rights/tests/test_ir_rules.py @@ -0,0 +1,33 @@ +import openerp.exceptions +from openerp.tests.common import TransactionCase + +class TestRules(TransactionCase): + def setUp(self): + super(TestRules, self).setUp() + + self.id1 = self.env['test_access_right.some_obj']\ + .create({'val': 1}).id + self.id2 = self.env['test_access_right.some_obj']\ + .create({'val': -1}).id + # create a global rule forbidding access to records with a negative + # (or zero) val + self.env['ir.rule'].create({ + 'name': 'Forbid negatives', + 'model_id': self.browse_ref('test_access_rights.model_test_access_right_some_obj').id, + 'domain_force': "[('val', '>', 0)]" + }) + + def test_basic_access(self): + env = self.env(user=self.browse_ref('base.public_user')) + + # put forbidden record in cache + browse2 = env['test_access_right.some_obj'].browse(self.id2) + # this is the one we want + browse1 = env['test_access_right.some_obj'].browse(self.id1) + + # this should not blow up + self.assertEqual(browse1.val, 1) + + # but this should + with self.assertRaises(openerp.exceptions.AccessError): + self.assertEqual(browse2.val, -1) From 785018cc9c6429eb79b09ae84c9c28c9da24f85a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 10 Jul 2014 13:43:45 +0200 Subject: [PATCH 3/6] [FIX] only prefetch other cached records with read field uncached during _read_from_database If expansion of the recordset is done during _prefetch_field, if one of the prefetches (not the base record(s) but one of those selected by BaseModel._in_cache_without) can't be read by the current user (due to an access rule or for field reading reasons, or whatever) the whole read is failed, even if the record which was specifically asked for could be read on its own. By only expanding the read set in _read_from_database, the cache is correctly set but read() and _prefetch_field() only check the records explicitly asked for for AccessDenied, prefetched records will only be asked if they are ever accessed. fixes #1013 --- openerp/models.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/openerp/models.py b/openerp/models.py index a801257b81e..73b0981f513 100644 --- a/openerp/models.py +++ b/openerp/models.py @@ -3101,10 +3101,10 @@ class BaseModel(object): instance) for `self` in cache. """ # fetch the records of this model without field_name in their cache - records = self._in_cache_without(field) + records = self # by default, simply fetch field - fnames = set((field.name,)) + fnames = {field.name} if self.pool._init: # columns may be missing from database, do not prefetch other fields @@ -3176,8 +3176,14 @@ class BaseModel(object): 'order': self._parent_order or self._order, } + empty = self.browse() + records = self.browse(set(itertools.chain.from_iterable( + (self._in_cache_without(field) - self.env.todo.get(field, empty)).ids + for field in (self._fields[name] for name in field_names) + ))) + result = [] - for sub_ids in cr.split_for_in_conditions(self.ids): + for sub_ids in cr.split_for_in_conditions(records.ids): cr.execute(query, [tuple(sub_ids)] + rule_params) result.extend(cr.dictfetchall()) @@ -3250,9 +3256,9 @@ class BaseModel(object): # store failed values in cache for the records that could not be read fetched = self.browse(ids) - missing = self - fetched + missing = records - fetched if missing: - extras = fetched - self + extras = fetched - records if extras: raise AccessError( _("Database fetch misses ids ({}) and has extra ids ({}), may be caused by a type incoherence in a previous request").format( From 269a6ee128247d36c391df85cae9d7ade123ed7d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 11 Jul 2014 14:07:05 +0200 Subject: [PATCH 4/6] [FIX] filtering out of records which shouldn't be fetched/prefetched A todo would only filter out records selectioned by the same field's caching, it should filter out on the whole prefetching selection or an other field could/would just add it back to the set of records to fetch (and lead to Bad Things). Note: this probably deserves a test somehow, but I'm not quite sure how the todos thing works so... --- openerp/models.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openerp/models.py b/openerp/models.py index 73b0981f513..f48aa5bc14a 100644 --- a/openerp/models.py +++ b/openerp/models.py @@ -3177,10 +3177,12 @@ class BaseModel(object): } empty = self.browse() - records = self.browse(set(itertools.chain.from_iterable( - (self._in_cache_without(field) - self.env.todo.get(field, empty)).ids - for field in (self._fields[name] for name in field_names) - ))) + prefetch = set() + todo = set() + for field in (self._fields[name] for name in field_names): + prefetch.update(self._in_cache_without(field).ids) + todo.update(self.env.todo.get(field, empty).ids) + records = self.browse(prefetch - todo) result = [] for sub_ids in cr.split_for_in_conditions(records.ids): From 3491b7de34c9599f73bfb047e44b1abca4bb3c3f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 14 Jul 2014 10:45:34 +0200 Subject: [PATCH 5/6] [REM] cache setting in _prefetch_field --- openerp/models.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/openerp/models.py b/openerp/models.py index f48aa5bc14a..7d19cc74e08 100644 --- a/openerp/models.py +++ b/openerp/models.py @@ -3123,12 +3123,11 @@ class BaseModel(object): # fetch records with read() assert self in records and field.name in fnames + result = [] try: result = records.read(list(fnames), load='_classic_write') - except AccessError as e: - # update cache with the exception - records._cache[field] = FailedValue(e) - result = [] + except AccessError: + pass # check the cache, and update it if necessary if field not in self._cache: From a1de79b875ca4283eea88c9a7f557124423548ed Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 14 Jul 2014 15:41:40 +0200 Subject: [PATCH 6/6] [FIX] calendar: invalidate cache to avoid (pre)fetching invalid virtual IDs Because the new API basically browses everything, the virtual IDs used by calendar.event are added to the cache and prefetching tries to get them from the database resulting in conversion errors. These ids have to be forcibly evicted to avoid the error. --- addons/calendar/calendar.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/addons/calendar/calendar.py b/addons/calendar/calendar.py index a77eb5ea6f6..920a05c3ebf 100644 --- a/addons/calendar/calendar.py +++ b/addons/calendar/calendar.py @@ -1567,6 +1567,12 @@ class calendar_event(osv.Model): select = [ids] else: select = ids + + # FIXME: find a better way to not push virtual ids in the cache + # (leading to their prefetching and ultimately a type error when + # postgres tries to convert '14-3489274297' to an integer) + self.invalidate_cache(cr, uid, context=context) + select = map(lambda x: (x, calendar_id2real_id(x)), select) result = [] real_data = super(calendar_event, self).read(cr, uid, [real_id for calendar_id, real_id in select], fields=fields2, context=context, load=load)