[FIX] base: fix portal rights update

Commit c3717f30 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 f206714a, which added a constraint
check_one_user_type, which would raise in that specific case.
The last missing piece was 9badedcd; 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#35347

Signed-off-by: Nans Lefebvre (len) <len@odoo.com>

odoo/addons/base/models/res_users.py

@@ -947,24 +947,20 @@ class UsersImplied(models.Model):
             if 'groups_id' in values:
                 # complete 'groups_id' with implied groups
                 user = self.new(values)
-                group_public = self.env.ref('base.group_public', raise_if_not_found=False)
-                group_portal = self.env.ref('base.group_portal', raise_if_not_found=False)
-                if group_public and group_public in user.groups_id:
-                    gs = self.env.ref('base.group_public') | self.env.ref('base.group_public').trans_implied_ids
-                elif group_portal and group_portal in user.groups_id:
-                    gs = self.env.ref('base.group_portal') | self.env.ref('base.group_portal').trans_implied_ids
-                else:
-                    gs = user.groups_id | user.groups_id.mapped('trans_implied_ids')
+                gs = user.groups_id | user.groups_id.mapped('trans_implied_ids')
                 values['groups_id'] = type(self).groups_id.convert_to_write(gs, user.groups_id)
         return super(UsersImplied, self).create(vals_list)
 
     @api.multi
     def write(self, values):
+        users_before = self.filtered(lambda u: u.has_group('base.group_user'))
         res = super(UsersImplied, self).write(values)
         if values.get('groups_id'):
             # add implied groups for all users
             for user in self.with_context({}):
-                if not user.has_group('base.group_user'):
+                if not user.has_group('base.group_user') and user in users_before:
+                    # if we demoted a user, we strip him of all its previous privileges
+                    # (but we should not do it if we are simply adding a technical group to a portal user)
                     vals = {'groups_id': [(5, 0, 0)] + values['groups_id']}
                     super(UsersImplied, user).write(vals)
                 gs = set(concat(g.trans_implied_ids for g in user.groups_id))

odoo/addons/base/tests/__init__.py

@@ -36,4 +36,3 @@ from . import test_res_partner
 from . import test_res_partner_bank
 from . import test_reports
 from . import test_tests_tags
-from . import test_non_regression

odoo/addons/base/tests/test_non_regression.py

-# -*- coding: utf-8 -*-
-"""
-    Non-Regression Tests
-"""
-
-from openerp.tests.common import TransactionCase
-
-
-class TestNR(TransactionCase):
-    def test_issue26036(self):
-        # Coming from https://github.com/odoo/odoo/pull/26036
-        U = self.env["res.users"]
-        G = self.env["res.groups"]
-        group_user = self.env.ref('base.group_user')
-        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])]})
-        group_C = G.create({"name": "C"})
-
-        user_a = U.create({"name": "a", "login": "a", "groups_id": [(6, 0, [group_AA.id, group_user.id])]})
-        user_b = U.create({"name": "b", "login": "b", "groups_id": [(6, 0, [group_BB.id])]})
-
-        self.assertEqual(user_a.groups_id, (group_AA + group_A + group_user + group_no_one))
-
-        (user_a + user_b).write({"groups_id": [(4, group_C.id)]})
-
-        self.assertEqual(user_a.groups_id, (group_AA + group_A + group_C + group_user + group_no_one))
-        # As user_b is not an internal user, all its groups are removed
-        self.assertEqual(user_b.groups_id, group_C)

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)]})