diff --git a/odoo/addons/base/models/res_users.py b/odoo/addons/base/models/res_users.py index 002693ee781..cad105b0ea7 100644 --- a/odoo/addons/base/models/res_users.py +++ b/odoo/addons/base/models/res_users.py @@ -1000,24 +1000,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)) diff --git a/odoo/addons/base/tests/__init__.py b/odoo/addons/base/tests/__init__.py index 281aec864c8..6b7996daacc 100644 --- a/odoo/addons/base/tests/__init__.py +++ b/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 diff --git a/odoo/addons/base/tests/test_non_regression.py b/odoo/addons/base/tests/test_non_regression.py deleted file mode 100644 index 12faee0e1f0..00000000000 --- a/odoo/addons/base/tests/test_non_regression.py +++ /dev/null @@ -1,32 +0,0 @@ -# -*- 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) 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)]})