[FIX] Model.browse: prefetching should not include model-level restricted fields (with @groups)

This usually causes spurious AccessDenied errors that would
not have occurred in most cases because the restricted fields
would not be actually read/accessed.
This patch treats fields with model-level `groups` attributes
(which means access is restricted by the ORM, not just the UI)
as not prefetchable, irregardless of the actual groups that are
restricted. This agnostic  approach was chosen to save the
extra group membership lookup in most cases, which is currently
not cached (caching it would be a bigger change and come with
cache invalidation issues).

In the worst case the non-prefetching will cause an extra
SQL query for each restricted field that is browsed, but in
most cases this will make no performance hit, as this
from of access control is typically used only for a few
fields.

bzr revid: odo@openerp.com-20130820085451-e1a30wfod2hc52hf

openerp/osv/orm.py

@@ -383,16 +383,18 @@ class browse_record(object):
                 raise KeyError(error_msg)
 
             # if the field is a classic one or a many2one, we'll fetch all classic and many2one fields
-            if col._prefetch:
+            if col._prefetch and not col.groups:
                 # gen the list of "local" (ie not inherited) fields which are classic or many2one
-                fields_to_fetch = filter(lambda x: x[1]._classic_write and x[1]._prefetch, self._table._columns.items())
+                field_filter = lambda x: x[1]._classic_write and x[1]._prefetch and not x[1].groups
+                fields_to_fetch = filter(field_filter, self._table._columns.items())
                 # gen the list of inherited fields
                 inherits = map(lambda x: (x[0], x[1][2]), self._table._inherit_fields.items())
                 # complete the field list with the inherited fields which are classic or many2one
-                fields_to_fetch += filter(lambda x: x[1]._classic_write and x[1]._prefetch, inherits)
+                fields_to_fetch += filter(field_filter, inherits)
             # otherwise we fetch only that field
             else:
                 fields_to_fetch = [(name, col)]
+
             ids = filter(lambda id: name not in self._data[id], self._data.keys())
             # read the results
             field_names = map(lambda x: x[0], fields_to_fetch)

openerp/tests/test_acl.py

@@ -9,6 +9,7 @@ import common
 # test group that demo user should not have
 GROUP_TECHNICAL_FEATURES = 'base.group_no_one'
 
+
 class TestACL(common.TransactionCase):
 
     def setUp(self):
@@ -22,7 +23,7 @@ class TestACL(common.TransactionCase):
 
     def test_field_visibility_restriction(self):
         """Check that model-level ``groups`` parameter effectively restricts access to that
-           field for users who do not belong to one of the explicitly allowed groups""" 
+           field for users who do not belong to one of the explicitly allowed groups"""
         # Verify the test environment first
         original_fields = self.res_currency.fields_get(self.cr, self.demo_uid, [])
         form_view = self.res_currency.fields_view_get(self.cr, self.demo_uid, False, 'form')
@@ -40,7 +41,7 @@ class TestACL(common.TransactionCase):
         view_arch = etree.fromstring(form_view.get('arch'))
         self.assertFalse('accuracy' in fields, "'accuracy' field should be gone")
         self.assertEquals(view_arch.xpath("//field[@name='accuracy']"), [],
-                             "Field 'accuracy' must not be found in view definition")
+                          "Field 'accuracy' must not be found in view definition")
 
         # Make demo user a member of the restricted group and check that the field is back
         self.tech_group.write({'users': [(4, self.demo_uid)]})
@@ -65,7 +66,7 @@ class TestACL(common.TransactionCase):
         has_tech_feat = self.res_users.has_group(self.cr, self.demo_uid, GROUP_TECHNICAL_FEATURES)
         self.assertFalse(has_tech_feat, "`demo` user should not belong to the restricted group")
         self.assert_(self.res_partner.read(self.cr, self.demo_uid, [1], ['bank_ids']))
-        self.assert_(self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []})) 
+        self.assert_(self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []}))
 
         # Now restrict access to the field and check it's forbidden
         self.res_partner._columns['bank_ids'].groups = GROUP_TECHNICAL_FEATURES
@@ -79,12 +80,30 @@ class TestACL(common.TransactionCase):
         has_tech_feat = self.res_users.has_group(self.cr, self.demo_uid, GROUP_TECHNICAL_FEATURES)
         self.assertTrue(has_tech_feat, "`demo` user should now belong to the restricted group")
         self.assert_(self.res_partner.read(self.cr, self.demo_uid, [1], ['bank_ids']))
-        self.assert_(self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []})) 
-        
+        self.assert_(self.res_partner.write(self.cr, self.demo_uid, [1], {'bank_ids': []}))
+
         #cleanup
         self.tech_group.write({'users': [(3, self.demo_uid)]})
         self.res_partner._columns['bank_ids'].groups = False
 
+    def test_fields_browse_restriction(self):
+        """Test access to records having restricted fields"""
+        self.res_partner._columns['email'].groups = GROUP_TECHNICAL_FEATURES
+        try:
+            P = self.res_partner
+            pid = P.search(self.cr, self.demo_uid, [], limit=1)[0]
+            part = P.browse(self.cr, self.demo_uid, pid)
+            # accessing fields must no raise exceptions...
+            part.name
+            # ... except they are restricted
+            with self.assertRaises(openerp.osv.orm.except_orm) as cm:
+                part.email
+
+            self.assertEqual(cm.exception.args[0], 'Access Denied')
+        finally:
+            self.res_partner._columns['email'].groups = False
+
+
 if __name__ == '__main__':
     unittest2.main()