From 47fbaeda11b4b685d1f31c4929239adf852582f5 Mon Sep 17 00:00:00 2001 From: Holger Brunn Date: Thu, 4 Nov 2021 14:07:59 +0100 Subject: [PATCH] [IMP] base_changeset: overhaul security --- base_changeset/models/base.py | 3 +- base_changeset/models/changeset_field_rule.py | 7 ++++ .../models/record_changeset_change.py | 7 +++- base_changeset/readme/USAGE.rst | 9 +++++ base_changeset/security/groups.xml | 5 +-- base_changeset/security/rules.xml | 12 +++++++ base_changeset/tests/__init__.py | 1 + base_changeset/tests/test_changeset_flow.py | 20 +++++++---- .../tests/test_changeset_security.py | 33 +++++++++++++++++++ .../views/changeset_field_rule_views.xml | 3 ++ 10 files changed, 87 insertions(+), 13 deletions(-) create mode 100644 base_changeset/tests/test_changeset_security.py diff --git a/base_changeset/models/base.py b/base_changeset/models/base.py index d6b300d04..923e89810 100644 --- a/base_changeset/models/base.py +++ b/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, ) diff --git a/base_changeset/models/changeset_field_rule.py b/base_changeset/models/changeset_field_rule.py index ed43068b9..f3b308abf 100644 --- a/base_changeset/models/changeset_field_rule.py +++ b/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.""" diff --git a/base_changeset/models/record_changeset_change.py b/base_changeset/models/record_changeset_change.py index 8058c1e31..a5e46ea91 100644 --- a/base_changeset/models/record_changeset_change.py +++ b/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 diff --git a/base_changeset/readme/USAGE.rst b/base_changeset/readme/USAGE.rst index b8babe803..302872864 100644 --- a/base_changeset/readme/USAGE.rst +++ b/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. diff --git a/base_changeset/security/groups.xml b/base_changeset/security/groups.xml index 852751849..7b7263950 100644 --- a/base_changeset/security/groups.xml +++ b/base_changeset/security/groups.xml @@ -17,10 +17,7 @@ - - - - + diff --git a/base_changeset/security/rules.xml b/base_changeset/security/rules.xml index 670568065..fd9037712 100644 --- a/base_changeset/security/rules.xml +++ b/base_changeset/security/rules.xml @@ -16,4 +16,16 @@ name="domain_force" >['|',('company_id','=',False),('company_id', 'in', company_ids)] + + Restrict changeset change access for users + + + [('create_uid', '=', user.id)] + + + Allow changeset change access for changeset users + + + [('rule_id.validator_group_ids', 'in', user.groups_id.ids)] + diff --git a/base_changeset/tests/__init__.py b/base_changeset/tests/__init__.py index debb66919..c3d0a95ed 100644 --- a/base_changeset/tests/__init__.py +++ b/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 diff --git a/base_changeset/tests/test_changeset_flow.py b/base_changeset/tests/test_changeset_flow.py index a14203693..e10f5ca39 100644 --- a/base_changeset/tests/test_changeset_flow.py +++ b/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( - test_record_changeset=True, - ).create({ - "name": "partner", - "street": "street", - "street2": "street2", - }) + new = ( + self.env["res.partner"] + .with_context( + test_record_changeset=True, + ) + .create( + { + "name": "partner", + "street": "street", + "street2": "street2", + } + ) + ) self.assertEqual(new.count_pending_changesets, 1) self.assert_changeset( new, diff --git a/base_changeset/tests/test_changeset_security.py b/base_changeset/tests/test_changeset_security.py new file mode 100644 index 000000000..90d05686d --- /dev/null +++ b/base_changeset/tests/test_changeset_security.py @@ -0,0 +1,33 @@ +# Copyright 2021 Hunki Enterprises BV () +# 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)])) diff --git a/base_changeset/views/changeset_field_rule_views.xml b/base_changeset/views/changeset_field_rule_views.xml index 1d577a7e8..2ee54bd43 100644 --- a/base_changeset/views/changeset_field_rule_views.xml +++ b/base_changeset/views/changeset_field_rule_views.xml @@ -8,6 +8,7 @@ + + @@ -55,6 +57,7 @@ +