[IMP] base_changeset: overhaul security

base_changeset/models/base.py

@@ -88,7 +88,8 @@ class Base(models.AbstractModel):
                     {
                         field_name: value
                         for field_name, value in vals.items()
-                        if self._fields[field_name].required
+                        if field_name in self._fields and
+                        self._fields[field_name].required
                         and isinstance(
                             self._fields[field_name], fields.Many2one,
                         )

base_changeset/models/changeset_field_rule.py

@@ -43,6 +43,13 @@ class ChangesetFieldRule(models.Model):
         help="Use this rule only on records where this is true. "
         "Available variables: object, user",
     )
+    validator_group_ids = fields.Many2many(
+        'res.groups', 'changeset_field_rule_validator_group_rel',
+        string='Validator Groups', default=lambda self: self.env.ref(
+            'base_changeset.group_changeset_user',
+            raise_if_not_found=False,
+        ) or self.env['res.groups'],
+    )
 
     def init(self):
         """Ensure there is at most one rule with source_model_id NULL."""

base_changeset/models/record_changeset_change.py

@@ -397,8 +397,13 @@ class RecordChangesetChange(models.Model):
     def _compute_user_can_validate_changeset(self):
         is_superuser = self.env.is_superuser()
         has_group = self.user_has_groups("base_changeset.group_changeset_user")
+        user_groups = self.env.user.groups_id
         for rec in self:
-            can_validate = rec._is_change_pending() and (is_superuser or has_group)
+            can_validate = rec._is_change_pending() and (
+                is_superuser
+                or rec.rule_id.validator_group_ids & user_groups
+                or has_group
+            )
             if rec.rule_id.prevent_self_validation:
                 can_validate = can_validate and rec.modified_by_id != self.env.user
             rec.user_can_validate_changeset = can_validate

base_changeset/readme/USAGE.rst

@@ -64,3 +64,12 @@ same that is passed in ``__changeset_rules_source_model``).
 The source is used for the application of the rules, allowing to have a
 different rule for a different source. It is also stored on the changeset for
 information.
+
+Notes on security
+-----------------
+
+Note that by default, changeset users see all changes on all configured
+rules. This circumvents read restrictions on the original records, so if you
+have restrictions on models with changeset rules, changeset users will still
+see all changes of all records, and applying a change on an inaccessible record
+will fail.

base_changeset/security/groups.xml

@@ -17,10 +17,7 @@
     <data noupdate="1">
         <record id="group_changeset_manager" model="res.groups">
             <field name="users" eval="[(4, ref('base.user_root'))]" />
-        </record>
-        <record id="group_changeset_user" model="res.groups">
-            <field name="implied_ids" eval="[(4, ref('group_changeset_manager'))]" />
-            <field name="users" eval="[(4, ref('base.user_root'))]" />
+            <field name="implied_ids" eval="[(4, ref('group_changeset_user'))]" />
         </record>
     </data>
 </odoo>

base_changeset/security/rules.xml

@@ -16,4 +16,16 @@
             name="domain_force"
         >['|',('company_id','=',False),('company_id', 'in', company_ids)]</field>
     </record>
+    <record id="rule_record_changeset_change_base_user" model="ir.rule">
+        <field name="name">Restrict changeset change access for users</field>
+        <field name="model_id" ref="model_record_changeset_change" />
+        <field name="groups" eval="[(4, ref('base.group_user'))]" />
+        <field name="domain_force">[('create_uid', '=', user.id)]</field>
+    </record>
+    <record id="rule_record_changeset_change_user" model="ir.rule">
+        <field name="name">Allow changeset change access for changeset users</field>
+        <field name="model_id" ref="model_record_changeset_change" />
+        <field name="groups" eval="[(4, ref('group_changeset_user'))]" />
+        <field name="domain_force">[('rule_id.validator_group_ids', 'in', user.groups_id.ids)]</field>
+    </record>
 </odoo>

base_changeset/tests/__init__.py

@@ -2,3 +2,4 @@ from . import test_changeset_flow
 from . import test_changeset_field_type
 from . import test_changeset_origin
 from . import test_changeset_field_rule
+from . import test_changeset_security

base_changeset/tests/test_changeset_flow.py

@@ -77,13 +77,19 @@ class TestChangesetFlow(ChangesetTestCommon, TransactionCase):
 
     def test_create_new_changeset(self):
         """ Create a new partner with a changeset """
-        new = self.env["res.partner"].with_context(
+        new = (
+            self.env["res.partner"]
+            .with_context(
                 test_record_changeset=True,
-        ).create({
+            )
+            .create(
+                {
                     "name": "partner",
                     "street": "street",
                     "street2": "street2",
-        })
+                }
+            )
+        )
         self.assertEqual(new.count_pending_changesets, 1)
         self.assert_changeset(
             new,

base_changeset/tests/test_changeset_security.py

@@ -0,0 +1,33 @@
+# Copyright 2021 Hunki Enterprises BV (<https://hunki-enterprises.com>)
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+
+from odoo.tests.common import TransactionCase
+
+from .common import ChangesetTestCommon
+
+
+class TestChangesetFlow(ChangesetTestCommon, TransactionCase):
+    """ Check that changesets don't leak information """
+
+    def setUp(self):
+        super().setUp()
+        self.env['changeset.field.rule'].search([]).unlink()
+        self.rule = self.env['changeset.field.rule'].create({
+            'model_id': self.env.ref('base.model_ir_config_parameter').id,
+            'field_id': self.env.ref('base.field_ir_config_parameter__key').id,
+            'action': 'auto',
+        })
+
+    def test_change_unprivileged_user(self):
+        """
+        Check that unprivileged users can't see changesets they didn't create
+        """
+        user = self.env.ref('base.user_demo')
+        self.env['ir.config_parameter'].with_context(
+            test_record_changeset=True,
+        ).set_param('hello', 'world')
+        changeset = self.env['record.changeset.change'].search([
+            ('rule_id', '=', self.rule.id),
+        ])
+        self.assertTrue(changeset)
+        self.assertFalse(changeset.sudo(user).search([('id', '=', changeset.id)]))

base_changeset/views/changeset_field_rule_views.xml

@@ -8,6 +8,7 @@
                 <field name="field_id" />
                 <field name="source_model_id" />
                 <field name="expression" />
+                <field name="validator_group_ids" />
                 <field
                     name="company_id"
                     groups="base.group_multi_company"
@@ -42,6 +43,7 @@
                             <field name="source_model_id" widget="selection" />
                             <field name="prevent_self_validation" />
                             <field name="expression" placeholder="True" />
+                            <field name="validator_group_ids" widget="many2many_tags" />
                         </group>
                     </group>
                 </sheet>
@@ -55,6 +57,7 @@
                 <field name="field_id" />
                 <field name="source_model_id" />
                 <field name="action" />
+                <field name="validator_group_ids" />
                 <group string="Group By" name="groupby">
                     <filter
                         name="model_groupby"