From d805349855c35b41bcc4d167afdd5ddc2f293968 Mon Sep 17 00:00:00 2001 From: Martin Trigaux Date: Fri, 24 Feb 2023 15:37:55 +0000 Subject: [PATCH] [FIX] base,mail: properly remove the user from an implied group In mail, changing the setting "Restrict mail templates edition and QWEB placeholders usage" to false should remove all users but admins from the group, including the template user. Before this commit, you could have weird behaviou such as 1. Disable the restrictiong (group_mail_template_editor in group_user.implied_ids) 2. Install mass_mailing (group_mail_template_editor in group_mass_mailing_user.implied_ids) 3. Create a user test with minimum employee roles (still has group_mail_template_editor as employee) 4. Check the box "Restrict mail templates edition and QWEB placeholders usage" -> test still has the template group nothing implied it The issue was a combinaison of recomputation of implied groups https://github.com/odoo/odoo/blob/b2f760cae74201d1622e56a0027dae67dcff9e33/odoo/addons/base/models/res_users.py#L1292-L1295 that triggered the synchronisation of groups on template user https://github.com/odoo/odoo/blob/b2f760cae74201d1622e56a0027dae67dcff9e33/odoo/addons/base/models/res_users.py#L611-L616 By removing the users already in a group, we avoid falling into the condition added at 121cd0d6084cb2878 where the default user gets a group removed, readded, hence added for everybody. closes odoo/odoo#115108 X-original-commit: 6398dd287f07641c85807f291d9fe078fcb8a231 Signed-off-by: Martin Trigaux (mat) --- addons/mail/models/ir_config_parameter.py | 4 +- addons/mail/tests/test_mail_template.py | 16 ++++++++ odoo/addons/base/models/res_users.py | 13 ++++++- odoo/addons/base/tests/test_base.py | 46 +++++++++++++++++++---- 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/addons/mail/models/ir_config_parameter.py b/addons/mail/models/ir_config_parameter.py index 09b6080f33b..7fe7407e413 100644 --- a/addons/mail/models/ir_config_parameter.py +++ b/addons/mail/models/ir_config_parameter.py @@ -30,10 +30,8 @@ class IrConfigParameter(models.Model): group_user.implied_ids |= group_mail_template_editor elif value and group_mail_template_editor in group_user.implied_ids: - group_user.implied_ids -= group_mail_template_editor # remove existing users, including inactive template user # admin will regain the right via implied_ids on group_system - group_mail_template_editor.with_context(active_test=False).users.write( - {'groups_id': [(3, group_mail_template_editor.id)]}) + group_user._remove_group(group_mail_template_editor) return super(IrConfigParameter, self).set_param(key, value) diff --git a/addons/mail/tests/test_mail_template.py b/addons/mail/tests/test_mail_template.py index 14b0ffdaf75..4d26f315b80 100644 --- a/addons/mail/tests/test_mail_template.py +++ b/addons/mail/tests/test_mail_template.py @@ -53,7 +53,9 @@ class TestMailTemplate(MailCommon): def test_mail_template_acl(self): # Sanity check self.assertTrue(self.user_admin.has_group('mail.group_mail_template_editor')) + self.assertTrue(self.user_admin.has_group('base.group_sanitize_override')) self.assertFalse(self.user_employee.has_group('mail.group_mail_template_editor')) + self.assertFalse(self.user_employee.has_group('base.group_sanitize_override')) # Group System can create / write / unlink mail template mail_template = self.env['mail.template'].with_user(self.user_admin).create({'name': 'Test template'}) @@ -186,3 +188,17 @@ class TestMailTemplateReset(MailCommon): # subject is not there in the data file template, so it should be set to False self.assertFalse(mail_template.subject, "Subject should be set to False") + +@tagged('-at_install', 'post_install') +class TestConfigRestrictEditor(MailCommon): + + def test_switch_icp_value(self): + # Sanity check + self.assertTrue(self.user_employee.has_group('mail.group_mail_template_editor')) + self.assertFalse(self.user_employee.has_group('base.group_system')) + + self.env['ir.config_parameter'].set_param('mail.restrict.template.rendering', True) + self.assertFalse(self.user_employee.has_group('mail.group_mail_template_editor')) + + self.env['ir.config_parameter'].set_param('mail.restrict.template.rendering', False) + self.assertTrue(self.user_employee.has_group('mail.group_mail_template_editor')) diff --git a/odoo/addons/base/models/res_users.py b/odoo/addons/base/models/res_users.py index ee8a6d60457..3efa10cda13 100644 --- a/odoo/addons/base/models/res_users.py +++ b/odoo/addons/base/models/res_users.py @@ -1252,8 +1252,17 @@ class GroupsImplied(models.Model): groups = self.filtered(lambda g: implied_group in g.implied_ids) if groups: groups.write({'implied_ids': [Command.unlink(implied_group.id)]}) - if groups.users: - implied_group.write({'users': [Command.unlink(user.id) for user in groups.users]}) + # if user belongs to implied_group thanks to another group, don't remove him + # this avoids readding the template user and triggering the mechanism at 121cd0d6084cb28 + users_to_unlink = [ + user + for user in groups.with_context(active_test=False).users + if implied_group not in (user.groups_id - implied_group).trans_implied_ids + ] + if users_to_unlink: + # do not remove inactive users (e.g. default) + implied_group.with_context(active_test=False).write( + {'users': [Command.unlink(user.id) for user in users_to_unlink]}) class UsersImplied(models.Model): _inherit = 'res.users' diff --git a/odoo/addons/base/tests/test_base.py b/odoo/addons/base/tests/test_base.py index 34eb98ad28f..a68ec268587 100644 --- a/odoo/addons/base/tests/test_base.py +++ b/odoo/addons/base/tests/test_base.py @@ -789,22 +789,52 @@ class TestGroups(TransactionCase): self.assertIn(a, c.implied_ids) def test_remove_groups(self): - u = self.env['res.users'].create({'login': 'u', 'name': 'U'}) + u1 = self.env['res.users'].create({'login': 'u1', 'name': 'U1'}) + u2 = self.env['res.users'].create({'login': 'u2', 'name': 'U2'}) + default = self.env.ref('base.default_user') + portal = self.env.ref('base.group_portal') + p = self.env['res.users'].create({'login': 'p', 'name': 'P', 'groups_id': [Command.set([portal.id])]}) - a = self.env['res.groups'].create({'name': 'A', 'users': [Command.set(u.ids)]}) - b = self.env['res.groups'].create({'name': 'B', 'users': [Command.set(u.ids)]}) - c = self.env['res.groups'].create({'name': 'C', 'implied_ids': [Command.set(a.ids)]}) + a = self.env['res.groups'].create({'name': 'A', 'users': [Command.set(u1.ids)]}) + b = self.env['res.groups'].create({'name': 'B', 'users': [Command.set(u1.ids)]}) + c = self.env['res.groups'].create({'name': 'C', 'implied_ids': [Command.set(a.ids)], 'users': [Command.set([p.id, u2.id, default.id])]}) + d = self.env['res.groups'].create({'name': 'D', 'implied_ids': [Command.set(a.ids)], 'users': [Command.set([u2.id, default.id])]}) + + def assertUsersEqual(users, group): + self.assertEqual( + sorted([r.login for r in users]), + sorted([r.login for r in group.with_context(active_test=False).users]) + ) + # sanity checks + assertUsersEqual([u1, u2, p, default], a) + assertUsersEqual([u1], b) + assertUsersEqual([u2, p, default], c) + assertUsersEqual([u2, default], d) # C already implies A, we want none of B+C to imply A (b + c)._remove_group(a) self.assertNotIn(a, b.implied_ids) self.assertNotIn(a, c.implied_ids) + self.assertIn(a, d.implied_ids) - # Since B didn't imply A, removing A from the implied groups of (B+C) - # should not remove user U from A, even though C implied A, since C does - # not have U as a user - self.assertIn(u, a.users) + # - Since B didn't imply A, removing A from the implied groups of (B+C) + # should not remove user U1 from A, even though C implied A, since C does + # not have U1 as a user + # - P should be removed as was only added via inheritance to C + # - U2 should not be removed from A since it is implied via C but also via D + assertUsersEqual([u1, u2, default], a) + assertUsersEqual([u1], b) + assertUsersEqual([u2, p, default], c) + assertUsersEqual([u2, default], d) + + # When adding the template user to a new group, it should add it to existing internal users + e = self.env['res.groups'].create({'name': 'E'}) + default.write({'groups_id': [Command.link(e.id)]}) + self.assertIn(u1, e.users) + self.assertIn(u2, e.users) + self.assertIn(default, e.with_context(active_test=False).users) + self.assertNotIn(p, e.users) class TestUsers(TransactionCase):