[FIX] orm,expression: sanity checks for domain terms, cleanup, tests

bzr revid: odo@openerp.com-20110924020303-2amkx95ij1yp8ofk

3 files changed, 83 insertions, 45 deletions

diff --git a/openerp/addons/base/test/test_osv_expression.yml b/openerp/addons/base/test/test_osv_expression.yml
index 3e9a0f6fb3e..43fea5c16cf 100644
--- a/openerp/addons/base/test/test_osv_expression.yml
+++ b/openerp/addons/base/test/test_osv_expression.yml
@@ -471,4 +471,33 @@
 -
     !python {model: res.country }: |
         ids = self.search(cr, uid, [('create_date', '<', '2001-01-01 12:00:00')])
+
+
 -
+    Verify that invalid expressions are refused, even for magic fields
+-
+    !python {model: res.country }: |
+        try:
+            self.search(cr, uid, [('does_not_exist', '=', 'foo')])
+            raise AssertionError('Invalid fields should not be accepted')
+        except ValueError:
+            pass
+
+        try:
+            self.search(cr, uid, [('create_date', '>>', 'foo')])
+            raise AssertionError('Invalid operators should not be accepted')
+        except ValueError:
+            pass
+
+        import psycopg2
+        try:
+            cr._default_log_exceptions = False
+            cr.execute('SAVEPOINT expression_failure_test')
+            self.search(cr, uid, [('create_date', '=', "1970-01-01'); --")])
+            # if the above search gives no error, the operand was not escaped!
+            cr.execute('RELEASE SAVEPOINT expression_failure_test')
+            raise AssertionError('Operands should always be SQL escaped')
+        except psycopg2.DataError:
+            # Should give: 'DataError: invalid input syntax for type timestamp' or similar
+            cr.execute('ROLLBACK TO SAVEPOINT expression_failure_test')
+
diff --git a/openerp/osv/expression.py b/openerp/osv/expression.py
index 272c1945d25..cb8cfea583f 100644
--- a/openerp/osv/expression.py
+++ b/openerp/osv/expression.py
@@ -126,6 +126,7 @@ import logging
 from openerp.tools import flatten, reverse_enumerate
 import fields
 import openerp.modules
+from openerp.osv.orm import MAGIC_COLUMNS
 
 #.apidoc title: Domain Expressions
 
@@ -413,7 +414,7 @@ class expression(object):
 
             # check if the expression is valid
             if not is_leaf(e):
-                raise ValueError('Bad domain expression: %r, %r is not a valid term.' % (exp, e))
+                raise ValueError("Invalid term %r in domain expression %r" % (e, exp))
 
             # normalize the leaf's operator
             e = normalize_leaf(*e)
@@ -421,41 +422,47 @@ class expression(object):
             left, operator, right = e
 
             working_table = table # The table containing the field (the name provided in the left operand)
-            fargs = left.split('.', 1)
+            field_path = left.split('.', 1)
 
             # If the field is _inherits'd, search for the working_table,
             # and extract the field.
-            if fargs[0] in table._inherit_fields:
+            if field_path[0] in table._inherit_fields:
                 while True:
-                    field = working_table._columns.get(fargs[0])
+                    field = working_table._columns.get(field_path[0])
                     if field:
                         self.__field_tables[i] = working_table
                         break
-                    next_table = working_table.pool.get(working_table._inherit_fields[fargs[0]][0])
+                    next_table = working_table.pool.get(working_table._inherit_fields[field_path[0]][0])
                     if next_table not in self.__all_tables:
                         self.__joins.append('%s."%s"=%s."%s"' % (next_table._table, 'id', working_table._table, working_table._inherits[next_table._name]))
                         self.__all_tables.add(next_table)
                     working_table = next_table
             # Or (try to) directly extract the field.
             else:
-                field = working_table._columns.get(fargs[0])
+                field = working_table._columns.get(field_path[0])
 
             if not field:
                 if left == 'id' and operator == 'child_of':
                     ids2 = to_ids(right, table)
                     dom = child_of_domain(left, ids2, working_table)
                     self.__exp = self.__exp[:i] + dom + self.__exp[i+1:]
+                else:
+                    # field could not be found in model columns, it's probably invalid, unless
+                    # it's one of the _log_access special fields
+                    # TODO: make these fields explicitly available in self.columns instead!
+                    if field_path[0] not in MAGIC_COLUMNS:
+                        raise ValueError("Invalid field %r in domain expression %r" % (left, exp))
                 continue
 
             field_obj = table.pool.get(field._obj)
-            if len(fargs) > 1:
+            if len(field_path) > 1:
                 if field._type == 'many2one':
-                    right = field_obj.search(cr, uid, [(fargs[1], operator, right)], context=context)
-                    self.__exp[i] = (fargs[0], 'in', right)
+                    right = field_obj.search(cr, uid, [(field_path[1], operator, right)], context=context)
+                    self.__exp[i] = (field_path[0], 'in', right)
                 # Making search easier when there is a left operand as field.o2m or field.m2m
                 if field._type in ['many2many', 'one2many']:
-                    right = field_obj.search(cr, uid, [(fargs[1], operator, right)], context=context)
-                    right1 = table.search(cr, uid, [(fargs[0], 'in', right)], context=context)
+                    right = field_obj.search(cr, uid, [(field_path[1], operator, right)], context=context)
+                    right1 = table.search(cr, uid, [(field_path[0], 'in', right)], context=context)
                     self.__exp[i] = ('id', 'in', right1)
 
                 if not isinstance(field, fields.property):
@@ -657,6 +664,12 @@ class expression(object):
     def __leaf_to_sql(self, leaf, table):
         left, operator, right = leaf
 
+        # final sanity checks - should never fail
+        assert operator in (TERM_OPERATORS + ('inselect',)), \
+            "Invalid operator %r in domain term %r" % (operator, leaf)
+        assert leaf in (TRUE_LEAF, FALSE_LEAF) or left in table._all_columns \
+            or left in MAGIC_COLUMNS, "Invalid field %r in domain term %r" % (left, leaf)
+
         if leaf == TRUE_LEAF:
             query = 'TRUE'
             params = []
@@ -704,8 +717,8 @@ class expression(object):
                     query = '(%s OR %s."%s" IS NULL)' % (query, table._table, left)
                 elif check_nulls and operator == 'not in':
                     query = '(%s AND %s."%s" IS NOT NULL)' % (query, table._table, left) # needed only for TRUE.
-            else: # Must not happen.
-                pass
+            else: # Must not happen
+                raise ValueError("Invalid domain term %r" % (leaf,))
 
         elif right == False and (left in table._columns) and table._columns[left]._type=="boolean" and (operator == '='):
             query = '(%s."%s" IS NULL or %s."%s" = false )' % (table._table, left, table._table, left)
@@ -725,15 +738,12 @@ class expression(object):
 
         elif (operator == '=?'):
             if (right is False or right is None):
+                # '=?' is a short-circuit that makes the term TRUE if right is None or False
                 query = 'TRUE'
                 params = []
-            elif left in table._columns:
-                format = table._columns[left]._symbol_set[0]
-                query = '(%s."%s" = %s)' % (table._table, left, format)
-                params = table._columns[left]._symbol_set[1](right)
             else:
-                query = "(%s.\"%s\" = '%%s')" % (table._table, left)
-                params = right
+                # '=?' behaves like '=' in other cases
+                query, params = self.__leaf_to_sql((left, '=', right), table)
 
         elif left == 'id':
             query = '%s.id %s %%s' % (table._table, operator)
@@ -749,15 +759,11 @@ class expression(object):
                     query = '(unaccent(%s."%s") %s unaccent(%s))' % (table._table, left, sql_operator, format)
                 else:
                     query = '(%s."%s" %s %s)' % (table._table, left, sql_operator, format)
-            else:
-                # Ugly case to support columns present in database but not in
-                # _columns. This will probably be removed in the future, but
-                # we have to keep it for now.
-                _logger.warning("The domain term '%s' specify a column not declared in _columns." % ((left, operator, right),))
-                if self.has_unaccent and sql_operator in ('ilike', 'not ilike'):
-                    query = "(unaccent(%s.\"%s\") %s unaccent('%s'))" % (table._table, left, sql_operator, right)
-                else:
-                    query = "(%s.\"%s\" %s '%s')" % (table._table, left, sql_operator, right)
+            elif left in MAGIC_COLUMNS:
+                    query = "(%s.\"%s\" %s %%s)" % (table._table, left, sql_operator)
+                    params = right
+            else: # Must not happen
+                raise ValueError("Invalid field %r in domain term %r" % (left, leaf))
 
             add_null = False
             if need_wildcard:
@@ -771,10 +777,6 @@ class expression(object):
                 add_null = not str_utf8
             elif left in table._columns:
                 params = table._columns[left]._symbol_set[1](right)
-            else:
-                # Matching else clause for the above else clause, where the
-                # params are actually already in the query.
-                params = []
 
             if add_null:
                 query = '(%s OR %s."%s" IS NULL)' % (query, table._table, left)
diff --git a/openerp/osv/orm.py b/openerp/osv/orm.py
index 5f19ddf2378..0dd7d487bba 100644
--- a/openerp/osv/orm.py
+++ b/openerp/osv/orm.py
@@ -638,6 +638,7 @@ class orm_template(object):
     _log_create = False
 
     CONCURRENCY_CHECK_FIELD = '__last_update'
+
     def log(self, cr, uid, id, message, secondary=False, context=None):
         if context and context.get('disable_log'):
             return True
@@ -2546,12 +2547,24 @@ class orm_memory(orm_template):
         for id in ids:
             self._check_access(uid, id, operation)
 
+# Definition of log access columns, automatically added to models if
+# self._log_access is True
+LOG_ACCESS_COLUMNS = {
+    'create_uid': 'INTEGER REFERENCES res_users ON DELETE SET NULL',
+    'create_date': 'TIMESTAMP',
+    'write_uid': 'INTEGER REFERENCES res_users ON DELETE SET NULL',
+    'write_date': 'TIMESTAMP'
+}
+# special columns automatically created by the ORM
+MAGIC_COLUMNS =  ['id'] + LOG_ACCESS_COLUMNS.keys()
+
 class orm(orm_template):
     _sql_constraints = []
     _table = None
     _protected = ['read', 'write', 'create', 'default_get', 'perm_read', 'unlink', 'fields_get', 'fields_view_get', 'search', 'name_get', 'distinct_field_get', 'name_search', 'copy', 'import_data', 'search_count', 'exists']
     __logger = logging.getLogger('orm')
     __schema = logging.getLogger('orm.schema')
+
     def read_group(self, cr, uid, domain, fields, groupby, offset=0, limit=None, context=None, orderby=False):
         """
         Get the list of records in list view grouped by the given ``groupby`` fields
@@ -2780,7 +2793,7 @@ class orm(orm_template):
         # iterate on the database columns to drop the NOT NULL constraints
         # of fields which were required but have been removed (or will be added by another module)
         columns = [c for c in self._columns if not (isinstance(self._columns[c], fields.function) and not self._columns[c].store)]
-        columns += ('id', 'write_uid', 'write_date', 'create_uid', 'create_date') # openerp access columns
+        columns += MAGIC_COLUMNS
         cr.execute("SELECT a.attname, a.attnotnull"
                    "  FROM pg_class c, pg_attribute a"
                    " WHERE c.relname=%s"
@@ -2847,7 +2860,7 @@ class orm(orm_template):
             column_data = self._select_column_data(cr)
 
             for k, f in self._columns.iteritems():
-                if k in ('id', 'write_uid', 'write_date', 'create_uid', 'create_date'):
+                if k in MAGIC_COLUMNS:
                     continue
                 # Don't update custom (also called manual) fields
                 if f.manual and not update_custom_fields:
@@ -3149,23 +3162,17 @@ class orm(orm_template):
 
 
     def _add_log_columns(self, cr):
-        logs = {
-            'create_uid': 'INTEGER REFERENCES res_users ON DELETE SET NULL',
-            'create_date': 'TIMESTAMP',
-            'write_uid': 'INTEGER REFERENCES res_users ON DELETE SET NULL',
-            'write_date': 'TIMESTAMP'
-        }
-        for k in logs:
+        for field, field_def in LOG_ACCESS_COLUMNS.iteritems():
             cr.execute("""
                 SELECT c.relname
                   FROM pg_class c, pg_attribute a
                  WHERE c.relname=%s AND a.attname=%s AND c.oid=a.attrelid
-                """, (self._table, k))
+                """, (self._table, field))
             if not cr.rowcount:
-                cr.execute('ALTER TABLE "%s" ADD COLUMN "%s" %s' % (self._table, k, logs[k]))
+                cr.execute('ALTER TABLE "%s" ADD COLUMN "%s" %s' % (self._table, field, field_def))
                 cr.commit()
                 self.__schema.debug("Table '%s': added column '%s' with definition=%s",
-                                    self._table, k, logs[k])
+                                    self._table, field, field_def)
 
 
     def _select_column_data(self, cr):
@@ -4641,7 +4648,7 @@ class orm(orm_template):
         for f in fields:
             ftype = fields[f]['type']
 
-            if self._log_access and f in ('create_date', 'create_uid', 'write_date', 'write_uid'):
+            if self._log_access and f in LOG_ACCESS_COLUMNS:
                 del data[f]
 
             if f in default: