[IMP] finalize PoC

base_mixin_restrict_field_access/README.rst

@@ -6,7 +6,7 @@ Restrict field access
 =====================
 
 This module was written to help developers restricting access to fields in a
-secure and flexible manner.
+secure and flexible manner on record level.
 
 If you're not a developer, this module is not for you as you need to write code
 in order to actually use it.
@@ -14,7 +14,9 @@ in order to actually use it.
 Usage
 =====
 
-To use this module, you need to:
+To use this module, you need to inherit this mixin for the model whose fields
+you want to restrict, and implement at least the following methods to do
+something useful:
 
 .. code:: python
 
@@ -77,6 +79,9 @@ The example code here will allow only reading a few fields for partners of
 which the current user is neither the sales person nor in this partner's sales
 team.
 
+Read the comments of the mixin, that's part of the documentation. Also have a
+look at the tests, that's another example on how to use this code.
+
 For further information, please visit:
 
 * https://www.odoo.com/forum/help-1

base_mixin_restrict_field_access/__openerp__.py

@@ -11,5 +11,6 @@
     "certain fields base on some condition",
     "depends": [
         'web',
+        'base_suspend_security',
     ],
 }

base_mixin_restrict_field_access/models/restrict_field_access_mixin.py

@@ -5,6 +5,8 @@ import json
 from lxml import etree
 from openerp import _, api, fields, models, SUPERUSER_ID
 from openerp.osv import expression  # pylint: disable=W0402
+from openerp.addons.base_suspend_security.base_suspend_security import\
+    BaseSuspendSecurityUid
 
 
 class RestrictFieldAccessMixin(models.AbstractModel):
@@ -64,10 +66,14 @@ class RestrictFieldAccessMixin(models.AbstractModel):
         result = super(RestrictFieldAccessMixin, self).read(
             fields=fields, load=load)
         for record in result:
+            this = self.browse(record['id'])
             for field in record:
-                if not self._restrict_field_access_is_field_accessible(field):
+                if not this._restrict_field_access_is_field_accessible(field):
                     record[field] = self._fields[field].convert_to_read(
                         self._fields[field].null(self.env))
+                    if self._fields[field] in self.env.cache:
+                        self.env.cache[self._fields[field]].pop(
+                            record['id'], False)
         return result
 
     @api.model
@@ -129,8 +135,12 @@ class RestrictFieldAccessMixin(models.AbstractModel):
                     if not this._restrict_field_access_is_field_accessible(
                             path[0],
                     ) and row[i]:
-                        row[i] = self._fields[path[0]].convert_to_export(
-                            self._fields[path[0]].null(self.env), self.env
+                        field = self._fields[path[0]]
+                        row[i] = field.convert_to_export(
+                            field.convert_to_cache(
+                                field.null(self.env), this, validate=False,
+                            ),
+                            self.env
                         )
             result.extend(rows)
         return result
@@ -189,7 +199,9 @@ class RestrictFieldAccessMixin(models.AbstractModel):
             cr, uid, view_id=view_id, view_type=view_type, context=context,
             toolbar=toolbar, submenu=submenu)
 
-        if view_type == 'search':
+        # TODO: for editable trees, we'll have to inject this into the
+        # form the editable list view creates on the fly
+        if view_type != 'form':
             return result
 
         # inject modifiers to make forbidden fields readonly
@@ -260,14 +272,12 @@ class RestrictFieldAccessMixin(models.AbstractModel):
     @api.model
     def _restrict_field_access_suspend(self):
         """set a marker that we don't want to restrict field access"""
-        # TODO: this is insecure. in the end, we need something in the lines of
-        # base_suspend_security's uid-hack
-        return self.with_context(_restrict_field_access_suspend=True)
+        return self.suspend_security()
 
     @api.model
     def _restrict_field_access_get_is_suspended(self):
         """return True if we shouldn't check for field access restrictions"""
-        return self.env.context.get('_restrict_field_access_suspend')
+        return isinstance(self.env.uid, BaseSuspendSecurityUid)
 
     @api.multi
     def _restrict_field_access_filter_vals(self, vals, action='read'):
@@ -288,10 +298,14 @@ class RestrictFieldAccessMixin(models.AbstractModel):
         """return True if the current user can perform specified action on
         all records in self. Override for your own logic.
         This function is also called with an empty recordset to get a list
-        of fields which are accessible unconditionally"""
+        of fields which are accessible unconditionally.
+        Note that this function is called *very* often. Even small things
+        like saying self.env.user.id instead of self.env.uid will give you a
+        massive performance penalty"""
         if self._restrict_field_access_get_is_suspended() or\
-                self.env.user.id == SUPERUSER_ID or\
-                not self and action == 'read':
+                self.env.uid == SUPERUSER_ID or\
+                not self and action == 'read' and\
+                self._fields[field_name].required:
             return True
         whitelist = self._restrict_field_access_get_field_whitelist(
             action=action)

base_mixin_restrict_field_access/tests/test_base_mixin_restrict_field_access.py

@@ -1,17 +1,50 @@
 # -*- coding: utf-8 -*-
 # © 2016 Therp BV <http://therp.nl>
 # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).
+from openerp import api, models
+from openerp.osv import expression
 from openerp.tests.common import TransactionCase
-from openerp import models
 
 
 class TestBaseMixinRestrictFieldAccess(TransactionCase):
     def test_base_mixin_restrict_field_access(self):
-        # inherit from our mixin
+        # inherit from our mixin. Here we want to restrict access to
+        # all fields when the partner has a credit limit of less than 42
+        # and the current user is not an admin
         class ResPartner(models.Model):
             _inherit = ['restrict.field.access.mixin', 'res.partner']
             _name = 'res.partner'
 
+            # implement a record specific whitelist: credit limit is only
+            # visible for normal users if it's below 42
+            @api.multi
+            def _restrict_field_access_is_field_accessible(
+                    self, field_name, action='read'
+            ):
+                result = super(ResPartner, self)\
+                    ._restrict_field_access_is_field_accessible(
+                        field_name, action=action
+                    )
+                if not self._restrict_field_access_get_is_suspended() and\
+                   not self.env.user.has_group('base.group_system') and\
+                   field_name not in models.MAGIC_COLUMNS and self:
+                    result = all(
+                        this.sudo().credit_limit < 42 for this in self
+                    )
+                return result
+
+            # and as the documentation says, we need to add a domain to enforce
+            # this
+            def _restrict_field_access_inject_restrict_field_access_domain(
+                    self, domain
+            ):
+                domain[:] = expression.AND([
+                    expression.normalize_domain(domain),
+                    [('credit_limit', '<', 42)]
+                ])
+        # call base-suspend_security's register hook
+        self.env['ir.rule']._register_hook()
+
         # setup the model
         res_partner = ResPartner._build_model(self.registry, self.cr).browse(
             self.cr, self.uid, [], context={})
@@ -19,15 +52,55 @@ class TestBaseMixinRestrictFieldAccess(TransactionCase):
         res_partner._setup_base(False)
         res_partner._setup_fields()
         res_partner._setup_complete()
+
         # run tests as nonprivileged user
-        partner = res_partner.sudo(self.env.ref('base.user_demo').id).create({
+        partner_model = res_partner.sudo(self.env.ref('base.user_demo').id)
+        partner = partner_model.create({
             'name': 'testpartner',
         })
-        partner.copy()
-        partner.write({
-            'name': 'testpartner2',
-        })
-        partner.search([])
+        self.assertFalse(partner.restrict_field_access)
+        partner.sudo().write({'credit_limit': 42})
+        partner.invalidate_cache()
         self.assertTrue(partner.restrict_field_access)
-        partner.fields_view_get()
-        # TODO: a lot more tests
+        self.assertFalse(partner.credit_limit)
+        self.assertTrue(partner.sudo().credit_limit)
+        # not searching for some restricted field should yield the partner
+        self.assertIn(partner, partner_model.search([]))
+        # but searching for it should not
+        self.assertNotIn(
+            partner,
+            partner_model.search([
+                ('credit_limit', '=', 42)
+            ])
+        )
+        # when we copy stuff, restricted fields should be copied, but still
+        # be inaccessible
+        new_partner = partner.copy()
+        self.assertFalse(new_partner.credit_limit)
+        self.assertTrue(new_partner.sudo().credit_limit)
+        # check that our field injection works
+        fields_view_get = partner.fields_view_get()
+        self.assertIn('restrict_field_access', fields_view_get['arch'])
+        # check that the export does null offending values
+        export = partner._BaseModel__export_rows([['id'], ['credit_limit']])
+        self.assertEqual(export[0][1], '0.0')
+        # but that it does export the value when it's fine
+        partner.sudo().write({'credit_limit': 41})
+        partner.invalidate_cache()
+        export = partner._BaseModel__export_rows([['id'], ['credit_limit']])
+        self.assertEqual(export[0][1], '41.0')
+        # read_group should behave like search: restrict to records with our
+        # field accessible if a restricted field is requested, unrestricted
+        # otherwise
+        data = partner_model.read_group(
+            [], [], ['user_id']
+        )
+        self.assertEqual(data[0]['credit_limit'], 41)
+        # but users with permissions should see the sum for all records
+        data = partner_model.sudo().read_group(
+            [], [], ['user_id']
+        )
+        self.assertEqual(
+            data[0]['credit_limit'],
+            sum(partner_model.sudo().search([]).mapped('credit_limit'))
+        )