From fba6ea5a4750dd5c2e553bc3ba4cf894ee6fefdf Mon Sep 17 00:00:00 2001 From: Victor Feyens Date: Mon, 16 May 2022 14:48:21 +0000 Subject: [PATCH] [IMP] *: do not force admins to be app admins For bugfix purposes, app administration groups have been given to (implied by) the "Settings" group because without those rights, opening/saving the settings crashed. 1) Do not load hidden view content This commit uses the conditional inheritance of views (depending on user groups) to avoid loading unnecessary view & record content client-side. This improves performance for admins without the specific application admin rights, but also fixes the main bugfix problem, caused by the webclient querying name_get for the records in relational fields content. Example: sale_management adds a res.config.settings field to specify the default sale.order.template for the current company. If a 'Settings' user without 'sale.group_sale_manager' opens the settings, he won't see this setting, but if a default template is specified for the current company, the webclient will still request the name_get of this template to the server, because the field was present in the view, only hidden with a groups attribute. With this commit change in sale, the field won't be in the view unless you have the Sale manager group, avoiding the error/traceback/bug. 2) Remove implied application administration groups Do not force the specific application groups on all 'Settings' user, they globally do not need those rights, and if they need it, they can add it to their account themselves. 3) Add a test to make sure settings user are able to manage settings. 4) Enforce 'settings' -> 'access rights' -> 'internal user' groups As the previous test highlighted some 'false positives' because it considered a settings user unable to read `crm.team` and `stock.warehouse` records, we also took the opportunity to enforce the fact that 'Settings' & 'Access rights' users must be internal users. It makes no sense for a portal/public user to have access to the settings, and didn't work anyway. Part-of: odoo/odoo#91909 --- addons/account/security/account_security.xml | 4 - .../views/res_config_settings_views.xml | 1 + .../crm/views/res_config_settings_views.xml | 1 + .../event/views/res_config_settings_views.xml | 1 + .../fleet/views/res_config_settings_views.xml | 1 + addons/hr/views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + addons/lunch/views/res_config_settings.xml | 1 + .../views/res_config_settings_views.xml | 1 + addons/mrp/security/mrp_security.xml | 4 - .../mrp/views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../views/res_config_settings_views.xml | 1 + .../sale/views/res_config_settings_views.xml | 3 +- .../stock/views/res_config_settings_views.xml | 1 + addons/website/security/website_security.xml | 4 - .../views/res_config_settings_views.xml | 17 +++- .../views/res_config_settings_views.xml | 6 +- .../security/website_slides_security.xml | 4 - .../views/res_config_settings_views.xml | 1 + odoo/addons/base/security/base_groups.xml | 1 + odoo/addons/base/tests/test_res_config.py | 88 ++++++++++++++++++- .../tests/test_bindings.py | 2 +- 27 files changed, 123 insertions(+), 27 deletions(-) diff --git a/addons/account/security/account_security.xml b/addons/account/security/account_security.xml index fc5279f05f8..662c803525f 100644 --- a/addons/account/security/account_security.xml +++ b/addons/account/security/account_security.xml @@ -78,10 +78,6 @@ - - - - A warning can be set on a partner (Account) diff --git a/addons/account/views/res_config_settings_views.xml b/addons/account/views/res_config_settings_views.xml index 2bbd7ee59d5..687c9a52047 100644 --- a/addons/account/views/res_config_settings_views.xml +++ b/addons/account/views/res_config_settings_views.xml @@ -17,6 +17,7 @@ res.config.settings + diff --git a/addons/crm/views/res_config_settings_views.xml b/addons/crm/views/res_config_settings_views.xml index b9860e0089a..20f9755e8f0 100644 --- a/addons/crm/views/res_config_settings_views.xml +++ b/addons/crm/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/event/views/res_config_settings_views.xml b/addons/event/views/res_config_settings_views.xml index e186e10b852..77e85b5223b 100644 --- a/addons/event/views/res_config_settings_views.xml +++ b/addons/event/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/fleet/views/res_config_settings_views.xml b/addons/fleet/views/res_config_settings_views.xml index 27a22aa9e0a..0d7dda2c55b 100644 --- a/addons/fleet/views/res_config_settings_views.xml +++ b/addons/fleet/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/hr/views/res_config_settings_views.xml b/addons/hr/views/res_config_settings_views.xml index e1b04429aff..38e37473187 100644 --- a/addons/hr/views/res_config_settings_views.xml +++ b/addons/hr/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings +
diff --git a/addons/hr_attendance/views/res_config_settings_views.xml b/addons/hr_attendance/views/res_config_settings_views.xml index 54d49173cec..dbf5e0d562a 100644 --- a/addons/hr_attendance/views/res_config_settings_views.xml +++ b/addons/hr_attendance/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings +
diff --git a/addons/hr_expense/views/res_config_settings_views.xml b/addons/hr_expense/views/res_config_settings_views.xml index ba85a1ad6a8..d483082ae29 100644 --- a/addons/hr_expense/views/res_config_settings_views.xml +++ b/addons/hr_expense/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/hr_recruitment/views/res_config_settings_views.xml b/addons/hr_recruitment/views/res_config_settings_views.xml index a3062ae56de..d110ab2fc5f 100644 --- a/addons/hr_recruitment/views/res_config_settings_views.xml +++ b/addons/hr_recruitment/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/hr_timesheet/views/res_config_settings_views.xml b/addons/hr_timesheet/views/res_config_settings_views.xml index 09df4cd64bc..122079aa1cc 100644 --- a/addons/hr_timesheet/views/res_config_settings_views.xml +++ b/addons/hr_timesheet/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings + hr_timesheet_config_form diff --git a/addons/lunch/views/res_config_settings.xml b/addons/lunch/views/res_config_settings.xml index e454a423005..fade4fc8a0f 100644 --- a/addons/lunch/views/res_config_settings.xml +++ b/addons/lunch/views/res_config_settings.xml @@ -5,6 +5,7 @@ res.config.settings +
diff --git a/addons/mass_mailing/views/res_config_settings_views.xml b/addons/mass_mailing/views/res_config_settings_views.xml index 0ef06f24ac0..3a0273ef0ce 100644 --- a/addons/mass_mailing/views/res_config_settings_views.xml +++ b/addons/mass_mailing/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings +
diff --git a/addons/mrp/security/mrp_security.xml b/addons/mrp/security/mrp_security.xml index 97000a88b69..e94d13d7f57 100644 --- a/addons/mrp/security/mrp_security.xml +++ b/addons/mrp/security/mrp_security.xml @@ -40,10 +40,6 @@ - - - - Use Operation Dependencies diff --git a/addons/mrp/views/res_config_settings_views.xml b/addons/mrp/views/res_config_settings_views.xml index 86b5097627b..41b2da5937d 100644 --- a/addons/mrp/views/res_config_settings_views.xml +++ b/addons/mrp/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/point_of_sale/views/res_config_settings_views.xml b/addons/point_of_sale/views/res_config_settings_views.xml index b7617a841f1..4a8bbb1adfd 100644 --- a/addons/point_of_sale/views/res_config_settings_views.xml +++ b/addons/point_of_sale/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings + diff --git a/addons/project/views/res_config_settings_views.xml b/addons/project/views/res_config_settings_views.xml index 7b54e651e2e..4242c2beffb 100644 --- a/addons/project/views/res_config_settings_views.xml +++ b/addons/project/views/res_config_settings_views.xml @@ -5,6 +5,7 @@ res.config.settings +
diff --git a/addons/purchase/views/res_config_settings_views.xml b/addons/purchase/views/res_config_settings_views.xml index ae98295a2fc..8a24c27f347 100644 --- a/addons/purchase/views/res_config_settings_views.xml +++ b/addons/purchase/views/res_config_settings_views.xml @@ -6,6 +6,7 @@ res.config.settings +
diff --git a/addons/sale/views/res_config_settings_views.xml b/addons/sale/views/res_config_settings_views.xml index 2b778e70fbe..cce4f730b83 100644 --- a/addons/sale/views/res_config_settings_views.xml +++ b/addons/sale/views/res_config_settings_views.xml @@ -5,7 +5,8 @@ res.config.settings.view.form.inherit.sale res.config.settings - + +
res.config.settings +
diff --git a/addons/website/security/website_security.xml b/addons/website/security/website_security.xml index 566fac8df09..95e55cc6bac 100644 --- a/addons/website/security/website_security.xml +++ b/addons/website/security/website_security.xml @@ -23,10 +23,6 @@ - - - - diff --git a/addons/website/views/res_config_settings_views.xml b/addons/website/views/res_config_settings_views.xml index 57a40c89a6b..3e946198e4d 100644 --- a/addons/website/views/res_config_settings_views.xml +++ b/addons/website/views/res_config_settings_views.xml @@ -1,15 +1,24 @@ + + res.config.settings.view.form.inherit.website + res.config.settings + + + + + + + + res.config.settings.view.form.inherit.website res.config.settings + - - - 1 -
- - 1 - + + diff --git a/addons/website_slides/security/website_slides_security.xml b/addons/website_slides/security/website_slides_security.xml index cd789eb8bff..3a3e5d9d23b 100644 --- a/addons/website_slides/security/website_slides_security.xml +++ b/addons/website_slides/security/website_slides_security.xml @@ -20,10 +20,6 @@ - - - - diff --git a/addons/website_slides/views/res_config_settings_views.xml b/addons/website_slides/views/res_config_settings_views.xml index a71df327f24..ff640c692c0 100644 --- a/addons/website_slides/views/res_config_settings_views.xml +++ b/addons/website_slides/views/res_config_settings_views.xml @@ -4,6 +4,7 @@ res.config.settings.view.form.inherit.website.slides res.config.settings +
diff --git a/odoo/addons/base/security/base_groups.xml b/odoo/addons/base/security/base_groups.xml index 73b2580a54b..cf53bcda031 100644 --- a/odoo/addons/base/security/base_groups.xml +++ b/odoo/addons/base/security/base_groups.xml @@ -9,6 +9,7 @@ --> Access Rights + diff --git a/odoo/addons/base/tests/test_res_config.py b/odoo/addons/base/tests/test_res_config.py index 552d8759715..fe8cb885dd5 100644 --- a/odoo/addons/base/tests/test_res_config.py +++ b/odoo/addons/base/tests/test_res_config.py @@ -1,10 +1,11 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from collections import defaultdict import logging -from odoo import exceptions -from odoo.tests.common import TransactionCase, tagged +from odoo import exceptions, Command +from odoo.tests.common import Form, TransactionCase, tagged _logger = logging.getLogger(__name__) @@ -97,3 +98,86 @@ class TestResConfigExecute(TransactionCase): for config_settings in all_config_settings: _logger.info("Testing %s" % (config_settings.name)) self.env[config_settings.name].create({}).execute() + + def test_settings_access(self): + """Check that settings user are able to open & save settings + + Also check that user with settings rights + any one of the groups restricting + a conditional view inheritance of res.config.settings view is also able to + open & save the settings (considering the added conditional content) + """ + ResUsers = self.env['res.users'] + group_system = self.env.ref('base.group_system') + self.settings_view = self.env.ref('base.res_config_settings_view_form') + settings_only_user = ResUsers.create({ + 'name': 'Sleepy Joe', + 'login': 'sleepy', + 'groups_id': [Command.link(group_system.id)], + }) + + _logger.info("Testing settings access for group %s", group_system.full_name) + forbidden_models = self._test_user_settings_fields_access(settings_only_user) + self._test_user_settings_view_save(settings_only_user) + + for model in forbidden_models: + _logger.warning("Settings user doesn\'t have read access to the model %s", model) + + settings_view_conditional_groups = self.env['ir.ui.view'].search([ + ('model', '=', 'res.config.settings'), + ]).groups_id + + for group in settings_view_conditional_groups: + group_name = group.full_name + _logger.info("Testing settings access for group %s", group_name) + create_values = { + 'name': f'Test {group_name}', + 'login': group_name, + 'groups_id': [Command.link(group_system.id), Command.link(group.id)] + } + user = ResUsers.create(create_values) + self._test_user_settings_view_save(user) + forbidden_models_fields = self._test_user_settings_fields_access(user) + + for model, fields in forbidden_models_fields.items(): + _logger.warning( + "Settings + %s user doesn\'t have read access to the model %s" + "linked to settings records by the field(s) %s", + group_name, model, ", ".join(str(field) for field in fields) + ) + + def _test_user_settings_fields_access(self, user): + """Verify that settings user are able to create & save settings.""" + settings = self.env['res.config.settings'].with_user(user).create({}) + + # Save the settings + settings.set_values() + + # Check user has access to all models of relational fields in view + # because the webclient makes a name_get request for all specified records + # even if they are not shown to the user. + settings_view_arch = self.settings_view.with_user(user)._get_combined_arch() + seen_fields = set() + for node in settings_view_arch.iterdescendants(tag='field'): + seen_fields.add(node.get('name')) + + models_to_check = defaultdict(set) + for field_name in seen_fields: + field = settings._fields[field_name] + if field.relational: + models_to_check[field.comodel_name].add(field) + + forbidden_models_fields = defaultdict(set) + for model in models_to_check: + has_read_access = self.env[model].with_user(user).check_access_rights( + 'read', raise_exception=False) + if not has_read_access: + forbidden_models_fields[model] = models_to_check[model] + + return forbidden_models_fields + + def _test_user_settings_view_save(self, user): + """Verify that settings user are able to save the settings form.""" + ResConfigSettings = self.env['res.config.settings'].with_user(user) + + settings_form = Form(ResConfigSettings) + settings_form.save() diff --git a/odoo/addons/test_action_bindings/tests/test_bindings.py b/odoo/addons/test_action_bindings/tests/test_bindings.py index cb9ba4561ed..e7bdcc66f0b 100644 --- a/odoo/addons/test_action_bindings/tests/test_bindings.py +++ b/odoo/addons/test_action_bindings/tests/test_bindings.py @@ -33,7 +33,7 @@ class TestActionBindings(common.TransactionCase): ) # add a group on an action, and check that it is not returned - group = self.env.ref('base.group_user') + group = self.env.ref('base.group_system') action2.groups_id += group self.env.user.groups_id -= group