From 444df6affa914a6dccd05ffaaeb02335488ec720 Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Thu, 24 Mar 2011 12:17:57 +0100 Subject: [PATCH] [FIX] ir.rule,expression: domain expressions must be normalized before they can be safely combined This adds a normalize_domain() method to osv.expression, which ir.rule calls before combining expressions due to multiple rules or groups being applicable to a certain user. YAML tests are also added with a trivial unit test of normalize_domain() and some additional tests for ir_rule in order to verify that unnormalized domains are properly normalized before being combined by ir.rule. bzr revid: odo@openerp.com-20110324111757-uwuoqvm3lxkipr08 --- openerp/addons/base/ir/ir_rule.py | 10 ++-- openerp/addons/base/test/test_ir_rule.yml | 43 ++++++++++++++++- .../addons/base/test/test_osv_expression.yml | 12 +++++ openerp/osv/expression.py | 46 +++++++++++++++---- 4 files changed, 96 insertions(+), 15 deletions(-) diff --git a/openerp/addons/base/ir/ir_rule.py b/openerp/addons/base/ir/ir_rule.py index befa47e25b0..abf5744b140 100644 --- a/openerp/addons/base/ir/ir_rule.py +++ b/openerp/addons/base/ir/ir_rule.py @@ -19,7 +19,7 @@ # ############################################################################## -from osv import fields,osv +from osv import fields, osv, expression import time from operator import itemgetter from functools import partial @@ -37,7 +37,7 @@ class ir_rule(osv.osv): if rule.domain_force: eval_user_data = {'user': self.pool.get('res.users').browse(cr, 1, uid), 'time':time} - res[rule.id] = eval(rule.domain_force, eval_user_data) + res[rule.id] = expression.expression.normalize_domain(eval(rule.domain_force, eval_user_data)) else: res[rule.id] = [] return res @@ -91,7 +91,7 @@ class ir_rule(osv.osv): dom += rule.domain count += 1 if count: - return ['&'] * (count-1) + dom + return [expression.AND_OPERATOR] * (count-1) + dom return [] @tools.cache() @@ -127,9 +127,9 @@ class ir_rule(osv.osv): group_domains += group_domain count += 1 if count and global_domain: - return ['&'] + global_domain + ['|'] * (count-1) + group_domains + return [expression.AND_OPERATOR] + global_domain + [expression.OR_OPERATOR] * (count-1) + group_domains if count: - return ['|'] * (count-1) + group_domains + return [expression.OR_OPERATOR] * (count-1) + group_domains return global_domain return [] diff --git a/openerp/addons/base/test/test_ir_rule.yml b/openerp/addons/base/test/test_ir_rule.yml index 9352f798508..b5114cbedcb 100644 --- a/openerp/addons/base/test/test_ir_rule.yml +++ b/openerp/addons/base/test/test_ir_rule.yml @@ -132,6 +132,47 @@ !python {model: res.partner }: | ids = self.search(cr, ref('base.user_demo'), []) assert ids, "Demo user should see some partner." +- + Modify the ir_rule for employee so that demo is not + allowed to see partners anymore. We use a domain + with implicit AND operator for later tests on normalization. +- + !record {model: ir.rule, id: test_rule2}: + domain_force: "[('id','=',False),('name','=',False)]" +- + Check that demo user does not see partners anymore. +- + !python {model: res.partner }: | + ids = self.search(cr, ref('base.user_demo'), []) + assert (not ids), "Demo user should not see any partner anymore" +- + Create a new group with demo user in it, and a complex rule that should + re-allow demo to see partners, because he belongs to both groups. +- + !record {model: res.groups, id: test_group}: + name: Test Group + users: + - base.user_demo +- + Add the rule to the new group, with a domain containing an implicit AND operator, + which is more tricky because it will have to be normalized before combining it. +- + !record {model: ir.rule, id: test_rule3}: + model_id: base.model_res_partner + domain_force: "[('name', '!=', False),('id', '!=', False)]" + name: test_rule4 + groups: + - test_group + perm_unlink: 1 + perm_write: 1 + perm_read: 1 + perm_create: 1 +- + Read the partners again as demo user, which should give results again. +- + !python {model: res.partner }: | + ids = self.search(cr, ref('base.user_demo'), []) + assert ids, "Demo user should see some partners again, due to combined rules." - Delete global domains (to combine only group domains). - @@ -140,7 +181,7 @@ assert ids, "Demo user should see some partner." self.unlink(cr, uid, ids) - - Read as demo user the partners (three 1=1 domains, no global domain). + Read as demo user the partners (several group domains, no global domain). - !python {model: res.partner }: | ids = self.search(cr, ref('base.user_demo'), []) diff --git a/openerp/addons/base/test/test_osv_expression.yml b/openerp/addons/base/test/test_osv_expression.yml index 4916fe06203..61a686b3e35 100644 --- a/openerp/addons/base/test/test_osv_expression.yml +++ b/openerp/addons/base/test/test_osv_expression.yml @@ -110,4 +110,16 @@ res_ids = self.search(cr, uid, [('company_id.partner_id', 'not in', [])]) res_ids.sort() assert res_ids == all_ids, "Searching against empty set failed, returns %r" % res_ids +- + Verify that normalize_domain() works. +- + !python {model: res.partner}: | + from osv.expression import expression + norm_domain = domain = ['&',(1,'=',1),('a','=','b')] + assert norm_domain == expression.normalize_domain(domain), "Normalized domains should be left untouched" + domain = [('x','in',['y','z']),('a.v','=','e'),'|','|',('a','=','b'),'!',('c','>','d'),('e','!=','f'),('g','=','h')] + norm_domain = ['&','&','&'] + domain + assert norm_domain == expression.normalize_domain(domain), "Non-normalized domains should be properly normalized" + + diff --git a/openerp/osv/expression.py b/openerp/osv/expression.py index 1922d5e8388..0283935ea34 100644 --- a/openerp/osv/expression.py +++ b/openerp/osv/expression.py @@ -23,6 +23,9 @@ from openerp.tools import flatten, reverse_enumerate import fields +NOT_OPERATOR = '!' +OR_OPERATOR = '|' +AND_OPERATOR = '&' class expression(object): """ @@ -32,10 +35,12 @@ class expression(object): For more info: http://christophe-simonis-at-tiny.blogspot.com/2008/08/new-new-domain-notation.html """ - def _is_operator(self, element): - return isinstance(element, (str, unicode)) and element in ['&', '|', '!'] + @classmethod + def _is_operator(cls, element): + return isinstance(element, (str, unicode)) and element in [AND_OPERATOR, OR_OPERATOR, NOT_OPERATOR] - def _is_leaf(self, element, internal=False): + @classmethod + def _is_leaf(cls, element, internal=False): OPS = ('=', '!=', '<>', '<=', '<', '>', '>=', '=?', '=like', '=ilike', 'like', 'not like', 'ilike', 'not ilike', 'in', 'not in', 'child_of') INTERNAL_OPS = OPS + ('inselect',) return (isinstance(element, tuple) or isinstance(element, list)) \ @@ -43,6 +48,30 @@ class expression(object): and (((not internal) and element[1] in OPS) \ or (internal and element[1] in INTERNAL_OPS)) + @classmethod + def normalize_domain(cls, domain_expr): + """Returns a normalized version of ``domain_expr``, where all implicit '&' operators + have been made explicit. One property of normalized domain expressions is that they + can be easily combined together as if they were single domain components. + """ + assert isinstance(domain_expr, (list, tuple)), "Domain to normalize must have a 'domain' form: a list or tuple of domain components" + missing_operators = -1 + for item in domain_expr: + if cls._is_operator(item): + if item != NOT_OPERATOR: + missing_operators -= 1 + else: + missing_operators += 1 + return [AND_OPERATOR] * missing_operators + domain_expr + + def normalize(self): + """Make this expression normalized, i.e. change it so that all implicit '&' + operator become explicit. If the expression had already been parsed, + there is no need to do it again. + """ + self.__exp = expression.normalize_domain(self.__exp) + return self + def __execute_recursive_in(self, cr, s, f, w, ids, op, type): # todo: merge into parent query as sub-query res = [] @@ -92,8 +121,8 @@ class expression(object): doms = [] for o in table.browse(cr, uid, ids, context=context): if doms: - doms.insert(0, '|') - doms += ['&', ('parent_left', '<', o.parent_right), ('parent_left', '>=', o.parent_left)] + doms.insert(0, OR_OPERATOR) + doms += [AND_OPERATOR, ('parent_left', '<', o.parent_right), ('parent_left', '>=', o.parent_left)] if prefix: return [(left, 'in', table.search(cr, uid, doms, context=context))] return doms @@ -172,7 +201,7 @@ class expression(object): else: # we assume that the expression is valid # we create a dummy leaf for forcing the parsing of the resulting expression - self.__exp[i] = '&' + self.__exp[i] = AND_OPERATOR self.__exp.insert(i + 1, self.__DUMMY_LEAF) for j, se in enumerate(subexp): self.__exp.insert(i + 2 + j, se) @@ -387,7 +416,6 @@ class expression(object): ] self.__exp[i] = ('id', 'inselect', (query1, query2)) - return self def __leaf_to_sql(self, leaf, table): @@ -491,10 +519,10 @@ class expression(object): params.insert(0, p) stack.append(q) else: - if e == '!': + if e == NOT_OPERATOR: stack.append('(NOT (%s))' % (stack.pop(),)) else: - ops = {'&': ' AND ', '|': ' OR '} + ops = {AND_OPERATOR: ' AND ', OR_OPERATOR: ' OR '} q1 = stack.pop() q2 = stack.pop() stack.append('(%s %s %s)' % (q1, ops[e], q2,))