[FIX] base: fix portal rights update

Commit c3717f3018 added a (5, 0, 0) command when modifying groups_id of any
non-internal user, to avoid the situation where the various implications would
end up giving the portal user group_user rights.
This risk is avoided by commit f206714af08, which added a constraint
check_one_user_type, which would raise in that specific case.
The last missing piece was 9badedcd98f; when groups are added through settings,
they are added through the implied_groups of the group model.

The (5,) command has the unfortunate side-effect to remove the portal group
from a user if a write is triggered programmatically.
If that happens, then the 'signing up' process end-destination is a 500 error.

Now we simplify the logic that keeping the same behaviour for implied_ids for
internal (group_user) and non-internal users.
We keep the (5,) command in one case: if a user is demoted to portal or public,
he may have groups should be reserved to internal users, but that do not imply
group_user (which would raise).
To avoid that, we remove all its groups, since there is no clear distinction
between technical display groups and actual privilege-granting groups.

opw 2041606

closes odoo/odoo#37144

X-original-commit: 29723e351d5dc5e5ea055c80da86f4e871909de9
Signed-off-by: Nans Lefebvre (len) <len@odoo.com>

1 files changed, 115 insertions, 27 deletions

diff --git a/odoo/addons/base/tests/test_user_has_group.py b/odoo/addons/base/tests/test_user_has_group.py
index 98e699a0718..d0b9884c39f 100644
--- a/odoo/addons/base/tests/test_user_has_group.py
+++ b/odoo/addons/base/tests/test_user_has_group.py
@@ -28,6 +28,8 @@ class TestHasGroup(TransactionCase):
         self.grp_internal = self.env.ref(self.grp_internal_xml_id)
         self.grp_portal_xml_id = 'base.group_portal'
         self.grp_portal = self.env.ref(self.grp_portal_xml_id)
+        self.grp_public_xml_id = 'base.group_public'
+        self.grp_public = self.env.ref(self.grp_public_xml_id)
 
     def test_env_uid(self):
         Users = self.env['res.users'].sudo(self.test_user)
@@ -51,6 +53,10 @@ class TestHasGroup(TransactionCase):
         )
 
     def test_portal_creation(self):
+        """Here we check that portal user creation fails if it tries to create a user
+           who would also have group_user by implied_group.
+           Otherwise, it succeeds with the groups we asked for.
+        """
         grp_public = self.env.ref('base.group_public')
         grp_test_portal_xml_id = 'test_user_has_group.portal_implied_group'
         grp_test_portal = self.env['res.groups']._load_records([
@@ -64,14 +70,16 @@ class TestHasGroup(TransactionCase):
             dict(xml_id=grp_test_internal2_xml_id, values={'name': 'Test Group Internal 2'})
         ])
         self.grp_portal.implied_ids = grp_test_portal
-        grp_test_internal1.implied_ids = self.grp_internal
-        grp_test_internal2.implied_ids = self.grp_internal
+
+        grp_test_internal1.implied_ids = False
+        grp_test_internal2.implied_ids = False
+
         portal_user = self.env['res.users'].create({
             'login': 'portalTest',
             'name': 'Portal test',
             'sel_groups_%s_%s_%s' % (self.grp_internal.id, self.grp_portal.id, grp_public.id): self.grp_portal.id,
             'sel_groups_%s_%s' % (grp_test_internal1.id, grp_test_internal2.id): grp_test_internal2.id,
-            })
+        })
 
         self.assertTrue(
             portal_user.has_group(self.grp_portal_xml_id),
@@ -81,45 +89,52 @@ class TestHasGroup(TransactionCase):
             portal_user.has_group(grp_test_portal_xml_id),
             "The portal user should belong to '%s'" % grp_test_portal_xml_id,
         )
-        self.assertFalse(
+        self.assertTrue(
             portal_user.has_group(grp_test_internal2_xml_id),
-            "The portal user should not belong to '%s'" % grp_test_internal2_xml_id
+            "The portal user should belong to '%s'" % grp_test_internal2_xml_id
         )
         self.assertFalse(
             portal_user.has_group(self.grp_internal_xml_id),
             "The portal user should not belong to '%s'" % self.grp_internal_xml_id
         )
 
+        portal_user.unlink()  # otherwise, badly modifying the implication would raise
+
+        grp_test_internal1.implied_ids = self.grp_internal
+        grp_test_internal2.implied_ids = self.grp_internal
+
+        with self.assertRaises(ValidationError): # current group implications forbid to create a portal user
+            portal_user = self.env['res.users'].create({
+                'login': 'portalFail',
+                'name': 'Portal fail',
+                'sel_groups_%s_%s_%s' % (self.grp_internal.id, self.grp_portal.id, grp_public.id): self.grp_portal.id,
+                'sel_groups_%s_%s' % (grp_test_internal1.id, grp_test_internal2.id): grp_test_internal2.id,
+            })
+
     def test_portal_write(self):
-        grp_remove_xml_id = 'test_portal_write.group_to_remove'
-        grp_remove = self.env['res.groups']._load_records([
-            dict(xml_id=grp_remove_xml_id, values={'name': 'Group to remove'})
-        ])
+        """Check that adding a new group to a portal user works as expected,
+           except if it implies group_user/public, in chich case it should raise.
+        """
+        grp_test_portal = self.env["res.groups"].create({"name": "implied by portal"})
+        self.grp_portal.implied_ids = grp_test_portal
+
         portal_user = self.env['res.users'].create({
             'login': 'portalTest2',
             'name': 'Portal test 2',
-            'groups_id': [(6, 0, [grp_remove.id])],
+            'groups_id': [(6, 0, [self.grp_portal.id])],
             })
-        grp_test_portal_xml_id = 'test_portal_write.portal_implied_group'
-        grp_test_portal = self.env['res.groups']._load_records([
-            dict(xml_id=grp_test_portal_xml_id, values={'name': 'Test Group Portal'})
-        ])
-        self.grp_portal.implied_ids = grp_test_portal
-        portal_user.write({'groups_id': [(4, self.grp_portal.id, 0)]})
 
-        self.assertFalse(
-            portal_user.has_group(grp_remove_xml_id),
-            "The portal user should not belong to '%s'" % grp_remove_xml_id
-        )
-        self.assertTrue(
-            portal_user.has_group(self.grp_portal_xml_id),
-            "The portal user should belong to '%s'" % self.grp_portal_xml_id,
-        )
-        self.assertTrue(
-            portal_user.has_group(grp_test_portal_xml_id),
-            "The portal user should belong to '%s'" % grp_test_portal_xml_id,
+        self.assertEqual(
+            portal_user.groups_id, (self.grp_portal + grp_test_portal),
+            "The portal user should have the implied group.",
         )
 
+        grp_fail = self.env["res.groups"].create(
+            {"name": "fail", "implied_ids": [(6, 0, [self.grp_internal.id])]})
+
+        with self.assertRaises(ValidationError):
+            portal_user.write({'groups_id': [(4, grp_fail.id)]})
+
     def test_two_user_types(self):
         #Create a user with two groups of user types kind (Internal and Portal)
         grp_test = self.env['res.groups']._load_records([
@@ -161,3 +176,76 @@ class TestHasGroup(TransactionCase):
 
         with self.assertRaises(ValidationError):
             grp_test.write({'implied_ids': [(4, self.grp_portal.id)]})
+
+    def test_demote_user(self):
+        """When a user is demoted to the status of portal/public,
+           we should strip him of all his (previous) rights
+        """
+        group_0 = self.env.ref(self.group0)  # the group to which test_user already belongs
+        group_U = self.env["res.groups"].create({"name": "U", "implied_ids": [(6, 0, [self.grp_internal.id])]})
+        self.grp_internal.implied_ids = False  # only there to simplify the test by not having to care about its trans_implied_ids
+
+        self.test_user.write({'groups_id': [(4, group_U.id)]})
+        self.assertEqual(
+            self.test_user.groups_id, (group_0 + group_U + self.grp_internal),
+            "We should have our 2 groups and the implied user group",
+        )
+
+        # Now we demote him. The JS framework sends 3 and 4 commands,
+        # which is what we write here, but it should work even with a 5 command or whatever.
+        self.test_user.write({'groups_id': [
+            (3, self.grp_internal.id),
+            (3, self.grp_public.id),
+            (4, self.grp_portal.id),
+        ]})
+
+        # if we screw up the removing groups/adding the implied ids, we could end up in two situations:
+        # 1. we have a portal user with way too much rights (e.g. 'Contact Creation', which does not imply any other group)
+        # 2. because a group may be (transitively) implying group_user, then it would raise an exception
+        # so as a compromise we remove all groups when demoting a user
+        # (even technical display groups, e.g. TaxB2B, which could be re-added later)
+        self.assertEqual(
+            self.test_user.groups_id, (self.grp_portal),
+            "Here the portal group does not imply any other group, so we should only have this group.",
+        )
+
+    def test_implied_groups(self):
+        """ We check that the adding of implied ids works correctly for normal users and portal users.
+            In the second case, working normally means raising if a group implies to give 'group_user'
+            rights to a portal user.
+        """
+        U = self.env["res.users"]
+        G = self.env["res.groups"]
+        group_user = self.env.ref('base.group_user')
+        group_portal = self.env.ref('base.group_portal')
+        group_no_one = self.env.ref('base.group_no_one')
+
+        group_A = G.create({"name": "A"})
+        group_AA = G.create({"name": "AA", "implied_ids": [(6, 0, [group_A.id])]})
+        group_B = G.create({"name": "B"})
+        group_BB = G.create({"name": "BB", "implied_ids": [(6, 0, [group_B.id])]})
+
+        # user_a is a normal user, so we expect groups to be added when we add them,
+        # as well as 'implied_groups'; otherwise nothing else should happen.
+        # By contrast, for a portal user we want implied groups not to be added
+        # if and only if it would not give group_user (or group_public) privileges
+        user_a = U.create({"name": "a", "login": "a", "groups_id": [(6, 0, [group_AA.id, group_user.id])]})
+        self.assertEqual(user_a.groups_id, (group_AA + group_A + group_user + group_no_one))
+
+        user_b = U.create({"name": "b", "login": "b", "groups_id": [(6, 0, [group_portal.id, group_AA.id])]})
+        self.assertEqual(user_b.groups_id, (group_AA + group_A + group_portal))
+
+        # user_b is not an internal user, but giving it a new group just added a new group
+        (user_a + user_b).write({"groups_id": [(4, group_BB.id)]})
+        self.assertEqual(user_a.groups_id, (group_AA + group_A + group_BB + group_B + group_user + group_no_one))
+        self.assertEqual(user_b.groups_id, (group_AA + group_A + group_BB + group_B + group_portal))
+
+        # now we create a group that implies the group_user
+        # adding it to a user should work normally, whereas adding it to a portal user should raise
+        group_C = G.create({"name": "C", "implied_ids": [(6, 0, [group_user.id])]})
+
+        user_a.write({"groups_id": [(4, group_C.id)]})
+        self.assertEqual(user_a.groups_id, (group_AA + group_A + group_BB + group_B + group_C + group_user + group_no_one))
+
+        with self.assertRaises(ValidationError):
+            user_b.write({"groups_id": [(4, group_C.id)]})