[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 121cd0d608 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) <mat@odoo.com>

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)

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'))

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'

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):