From 055f46f29029f054a792b19619081221f41085b2 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Tue, 11 Dec 2012 13:28:44 +0100 Subject: [PATCH 1/7] [IMP] tests: add test_acl to the openerp.tests suite. bzr revid: vmt@openerp.com-20121211122844-6y060gex2lbq76hp --- openerp/tests/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openerp/tests/__init__.py b/openerp/tests/__init__.py index 92a40ab306c..2606f421669 100644 --- a/openerp/tests/__init__.py +++ b/openerp/tests/__init__.py @@ -8,6 +8,7 @@ Tests can be explicitely added to the `fast_suite` or `checks` lists or not. See the :ref:`test-framework` section in the :ref:`features` list. """ +from . import test_acl from . import test_expression, test_mail, test_ir_sequence, test_orm, \ test_fields, test_basecase, \ test_view_validation, test_uninstall, test_misc, test_db_cursor, \ @@ -20,6 +21,7 @@ fast_suite = [ ] checks = [ + test_acl, test_expression, test_mail, test_db_cursor, From 559e2ff23c7cfc46fdb5d3afbb11d43d61f77e59 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Tue, 11 Dec 2012 15:49:03 +0100 Subject: [PATCH 2/7] [FIX] demo: add the group_partner_manager group to the demo user (needed to pass the test_acl sanity check). bzr revid: vmt@openerp.com-20121211144903-737sc04h41hmzaro --- openerp/addons/base/base_demo.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openerp/addons/base/base_demo.xml b/openerp/addons/base/base_demo.xml index 69cc4994f82..519bcf10eea 100644 --- a/openerp/addons/base/base_demo.xml +++ b/openerp/addons/base/base_demo.xml @@ -14,7 +14,7 @@ -- Mr Demo - + /9j/4AAQSkZJRgABAQEASABIAAD/2wBDAAUDBAQEAwUEBAQFBQUGBwwIBwcHBw8LCwkMEQ8SEhEP ERETFhwXExQaFRERGCEYGh0dHx8fExciJCIeJBweHx7/2wBDAQUFBQcGBw4ICA4eFBEUHh4eHh4e Hh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh7/wAARCACEAIQDASIA From 9d2afcae3fa570e1ac97697cdf2f75872a81fbb2 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Wed, 12 Dec 2012 12:36:47 +0100 Subject: [PATCH 3/7] [IMP] orm: check groups-based access rights on model fields in read() and write(). The commented-out tests present in test_acl.py now pass. Other tests now fail :-(. bzr revid: vmt@openerp.com-20121212113647-11y3buulifg6tyhj --- openerp/osv/orm.py | 27 +++++++++++++++++++++++++-- openerp/tests/test_acl.py | 16 +++++++++------- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/openerp/osv/orm.py b/openerp/osv/orm.py index 060d6da0277..1c0afa7131a 100644 --- a/openerp/osv/orm.py +++ b/openerp/osv/orm.py @@ -3536,6 +3536,29 @@ class BaseModel(object): return res + def check_field_access_rights(self, cr, user, operation, fields, context=None): + """ + Check the user access rights on the given fields. This raises Access + Denied if the user does not have the rights. Otherwise it returns the + fields (as is if the fields is not falsy, or the readable/writable + fields if fields is falsy). + """ + def p(field_name): + """Predicate to test if the user has access to the given field name.""" + field = self._all_columns[field_name].column + if field.groups: + return self.user_has_groups(cr, user, groups=field.groups, context=context) + else: + return True + if not fields: + fields = filter(p, self._all_columns.keys()) + else: + filtered_fields = filter(lambda a: not p(a), fields) + if filtered_fields: + _logger.warning('Access Denied by ACLs for operation: %s, uid: %s, model: %s, fields: %s', operation, user, self._name, ', '.join(filtered_fields)) + raise except_orm(_('Access Denied'), 'TODO') + return fields + def read(self, cr, user, ids, fields=None, context=None, load='_classic_read'): """ Read records with given ids with the given fields @@ -3561,8 +3584,7 @@ class BaseModel(object): if not context: context = {} self.check_access_rights(cr, user, 'read') - if not fields: - fields = list(set(self._columns.keys() + self._inherit_fields.keys())) + fields = self.check_field_access_rights(cr, user, 'read', fields) if isinstance(ids, (int, long)): select = [ids] else: @@ -4018,6 +4040,7 @@ class BaseModel(object): """ readonly = None + self.check_field_access_rights(cr, user, 'write', vals.keys()) for field in vals.copy(): fobj = None if field in self._columns: diff --git a/openerp/tests/test_acl.py b/openerp/tests/test_acl.py index a10a705092c..1d8d6bfb22a 100644 --- a/openerp/tests/test_acl.py +++ b/openerp/tests/test_acl.py @@ -1,6 +1,9 @@ import unittest2 from lxml import etree +import openerp +from openerp.tools.misc import mute_logger + import common # test group that demo user should not have @@ -55,6 +58,7 @@ class TestACL(common.TransactionCase): self.tech_group.write({'users': [(3, self.demo_uid)]}) self.res_currency._columns['rate'].groups = False + @mute_logger('openerp.osv.orm') def test_field_crud_restriction(self): "Read/Write RPC access to restricted field should be forbidden" # Verify the test environment first @@ -65,12 +69,10 @@ class TestACL(common.TransactionCase): # Now restrict access to the field and check it's forbidden self.res_partner._columns['bank_ids'].groups = GROUP_TECHNICAL_FEATURES - # FIXME TODO: enable next tests when access rights checks per field are implemented - # from openerp.osv.orm import except_orm - # with self.assertRaises(except_orm): - # self.res_partner.read(self.cr, self.demo_uid, [1], ['bank_ids']) - # with self.assertRaises(except_orm): - # self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []}) + with self.assertRaises(openerp.osv.orm.except_orm): + self.res_partner.read(self.cr, self.demo_uid, [1], ['bank_ids']) + with self.assertRaises(openerp.osv.orm.except_orm): + self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []}) # Add the restricted group, and check that it works again self.tech_group.write({'users': [(4, self.demo_uid)]}) @@ -86,4 +88,4 @@ class TestACL(common.TransactionCase): if __name__ == '__main__': unittest2.main() -# vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4: \ No newline at end of file +# vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4: From 1a4fd7188093d49ff688f153be0d589cf4e5d47d Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Wed, 12 Dec 2012 14:47:55 +0100 Subject: [PATCH 4/7] [FIX] test_fields: _all_columns must be modified similarly to _columns. (Because of the newly added method check_field_access_rights().) bzr revid: vmt@openerp.com-20121212134755-ykfdykqepwzti5uf --- openerp/osv/fields.py | 2 +- openerp/tests/test_fields.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/openerp/osv/fields.py b/openerp/osv/fields.py index 3d2c83b2621..a963b25b9e6 100644 --- a/openerp/osv/fields.py +++ b/openerp/osv/fields.py @@ -1560,7 +1560,7 @@ class column_info(object): def __str__(self): return '%s(%s, %s, %s, %s, %s)' % ( - self.__name__, self.name, self.column, + self.__class__.__name__, self.name, self.column, self.parent_model, self.parent_column, self.original_parent) # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4: diff --git a/openerp/tests/test_fields.py b/openerp/tests/test_fields.py index ae401e77032..6f463a1ac7a 100644 --- a/openerp/tests/test_fields.py +++ b/openerp/tests/test_fields.py @@ -53,31 +53,46 @@ class TestRelatedField(common.TransactionCase): def test_1_single_related(self): """ test a related field with a single indirection like fields.related('foo') """ # add a related field test_related_company_id on res.partner + # and simulate a _inherits_reload() to populate _all_columns. old_columns = self.partner._columns + old_all_columns = self.partner._all_columns self.partner._columns = dict(old_columns) + self.partner._all_columns = dict(old_all_columns) self.partner._columns.update({ 'single_related_company_id': fields.related('company_id', type='many2one', obj='res.company'), }) + self.partner._all_columns.update({ + 'single_related_company_id': fields.column_info('single_related_company_id', self.partner._columns['single_related_company_id'], None, None, None) + }) self.do_test_company_field('single_related_company_id') # restore res.partner fields self.partner._columns = old_columns + self.partner._all_columns = old_all_columns def test_2_related_related(self): """ test a related field referring to a related field """ # add a related field on a related field on res.partner + # and simulate a _inherits_reload() to populate _all_columns. old_columns = self.partner._columns + old_all_columns = self.partner._all_columns self.partner._columns = dict(old_columns) + self.partner._all_columns = dict(old_all_columns) self.partner._columns.update({ 'single_related_company_id': fields.related('company_id', type='many2one', obj='res.company'), 'related_related_company_id': fields.related('single_related_company_id', type='many2one', obj='res.company'), }) + self.partner._all_columns.update({ + 'single_related_company_id': fields.column_info('single_related_company_id', self.partner._columns['single_related_company_id'], None, None, None) + 'related_related_company_id': fields.column_info('related_related_company_id', self.partner._columns['related_related_company_id'], None, None, None) + }) self.do_test_company_field('related_related_company_id') # restore res.partner fields self.partner._columns = old_columns + self.partner._all_columns = old_all_columns def test_3_read_write(self): """ write on a related field """ From eb0fcc35d2233b15d095b64c53587ef0d0f81b89 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Wed, 12 Dec 2012 16:15:09 +0100 Subject: [PATCH 5/7] [FIX] check_field_access_rights: ignore nonexisting fields It seems name_alias is requested but does not exist. bzr revid: vmt@openerp.com-20121212151509-22494edob4e3fqaf --- openerp/osv/orm.py | 4 ++++ openerp/tests/test_fields.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/openerp/osv/orm.py b/openerp/osv/orm.py index 1c0afa7131a..ce2f8fc671a 100644 --- a/openerp/osv/orm.py +++ b/openerp/osv/orm.py @@ -3545,6 +3545,10 @@ class BaseModel(object): """ def p(field_name): """Predicate to test if the user has access to the given field name.""" + # Ignore requested field if it doesn't exist. This is ugly but + # it seems to happen at least with 'name_alias' on res.partner. + if field_name not in self._all_columns: + return True field = self._all_columns[field_name].column if field.groups: return self.user_has_groups(cr, user, groups=field.groups, context=context) diff --git a/openerp/tests/test_fields.py b/openerp/tests/test_fields.py index 6f463a1ac7a..efa7c67126d 100644 --- a/openerp/tests/test_fields.py +++ b/openerp/tests/test_fields.py @@ -84,7 +84,7 @@ class TestRelatedField(common.TransactionCase): 'related_related_company_id': fields.related('single_related_company_id', type='many2one', obj='res.company'), }) self.partner._all_columns.update({ - 'single_related_company_id': fields.column_info('single_related_company_id', self.partner._columns['single_related_company_id'], None, None, None) + 'single_related_company_id': fields.column_info('single_related_company_id', self.partner._columns['single_related_company_id'], None, None, None), 'related_related_company_id': fields.column_info('related_related_company_id', self.partner._columns['related_related_company_id'], None, None, None) }) From 9ebbe7d2303a14beff884cb049324a7953d9f5d2 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Fri, 14 Dec 2012 16:30:48 +0100 Subject: [PATCH 6/7] [IMP] doc/security: removed note stating the groups attribute was partially implemented. bzr revid: vmt@openerp.com-20121214153048-pehjjfp05uc9gdu4 --- doc/04_security.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/doc/04_security.rst b/doc/04_security.rst index 99d7fd3d17f..bb0ce468de5 100644 --- a/doc/04_security.rst +++ b/doc/04_security.rst @@ -141,10 +141,6 @@ for users who do not belong to the authorized groups: .. note:: The tests related to this feature are in ``openerp/tests/test_acl.py``. -.. warning:: At the time of writing the implementation of this feature is partial - and does not yet restrict read/write RPC access to the field. - The corresponding test is written already but currently disabled. - Workflow transition rules +++++++++++++++++++++++++ From 91ee5eae52846518b84e07b658d72f5c3c6f9016 Mon Sep 17 00:00:00 2001 From: Vo Minh Thu Date: Fri, 14 Dec 2012 16:53:13 +0100 Subject: [PATCH 7/7] [IMP] orm/acl: proper message instead of `TODO`. bzr revid: vmt@openerp.com-20121214155313-76lncslpx7ugrp6x --- openerp/osv/orm.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/openerp/osv/orm.py b/openerp/osv/orm.py index ce2f8fc671a..1457013035b 100644 --- a/openerp/osv/orm.py +++ b/openerp/osv/orm.py @@ -3560,7 +3560,11 @@ class BaseModel(object): filtered_fields = filter(lambda a: not p(a), fields) if filtered_fields: _logger.warning('Access Denied by ACLs for operation: %s, uid: %s, model: %s, fields: %s', operation, user, self._name, ', '.join(filtered_fields)) - raise except_orm(_('Access Denied'), 'TODO') + raise except_orm( + _('Access Denied'), + _('The requested operation cannot be completed due to security restrictions. ' + 'Please contact your system administrator.\n\n(Document type: %s, Operation: %s)') % \ + (self._description, operation)) return fields def read(self, cr, user, ids, fields=None, context=None, load='_classic_read'):