From 459e6dc1d296c6fca11984c88046aa17ad32b026 Mon Sep 17 00:00:00 2001 From: Nans Lefebvre Date: Fri, 2 Aug 2019 11:59:05 +0000 Subject: [PATCH] [FIX] base: check group constraints when adding implied groups Since commit 5f12e244f6e, the write of implied groups is done directly in SQL, bypassing the ORM writes, and thus bypassing the python constraints checks. In particular such a constraint was added in f206714af08. We add back an explicit check of this constraint; however we rewrite it in SQL to be fast. closes odoo/odoo#35411 closes odoo/odoo#36769 Signed-off-by: Nans Lefebvre (len) Signed-off-by: Nans Lefebvre (len) --- addons/account/i18n/account.pot | 1 - addons/account/models/res_users.py | 13 +++---- odoo/addons/base/models/res_users.py | 38 ++++++++++++++++++- odoo/addons/base/tests/test_user_has_group.py | 18 +++++++++ 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/addons/account/i18n/account.pot b/addons/account/i18n/account.pot index f02dee6340f2c..91346b0d6a500 100644 --- a/addons/account/i18n/account.pot +++ b/addons/account/i18n/account.pot @@ -688,7 +688,6 @@ msgstr "" #: code:addons/account/models/res_users.py:22 #, python-format msgid "A user cannot have both Tax B2B and Tax B2C.\n" -"Problematic user(s): %s\n" "You should go in General Settings, and choose to display Product Prices\n" "either in 'Tax-Included' or in 'Tax-Excluded' mode\n" "(or switch twice the mode if you are already in the desired one)." diff --git a/addons/account/models/res_users.py b/addons/account/models/res_users.py index 8030423e74d3b..013acde54ffbe 100644 --- a/addons/account/models/res_users.py +++ b/addons/account/models/res_users.py @@ -13,14 +13,11 @@ class Users(models.Model): def _check_one_user_type(self): super(Users, self)._check_one_user_type() - users_with_both_groups = self.filtered(lambda user: - user.has_group('account.group_show_line_subtotals_tax_included') and - user.has_group('account.group_show_line_subtotals_tax_excluded') - ) - if users_with_both_groups: - names = ", ".join(users_with_both_groups.mapped('name')) + g1 = self.env.ref('account.group_show_line_subtotals_tax_included') + g2 = self.env.ref('account.group_show_line_subtotals_tax_excluded') + + if self._has_multiple_groups([g1.id, g2.id]): raise ValidationError(_("A user cannot have both Tax B2B and Tax B2C.\n" - "Problematic user(s): %s\n" "You should go in General Settings, and choose to display Product Prices\n" "either in 'Tax-Included' or in 'Tax-Excluded' mode\n" - "(or switch twice the mode if you are already in the desired one).") % names) + "(or switch twice the mode if you are already in the desired one).")) diff --git a/odoo/addons/base/models/res_users.py b/odoo/addons/base/models/res_users.py index b524a6770ea11..002693ee7815f 100644 --- a/odoo/addons/base/models/res_users.py +++ b/odoo/addons/base/models/res_users.py @@ -409,10 +409,43 @@ def _check_action_id(self): @api.multi @api.constrains('groups_id') def _check_one_user_type(self): - for user in self: - if len(user.groups_id.filtered(lambda x: x.category_id.xml_id == 'base.module_category_user_type')) > 1: + """We check that no users are both portal and users (same with public). + This could typically happen because of implied groups. + """ + user_types_category = self.env.ref('base.module_category_user_type', raise_if_not_found=False) + user_types_groups = self.env['res.groups'].search( + [('category_id', '=', user_types_category.id)]) if user_types_category else False + if user_types_groups: # needed at install + if self._has_multiple_groups(user_types_groups.ids): raise ValidationError(_('The user cannot have more than one user types.')) + @api.multi + def _has_multiple_groups(self, group_ids): + """The method is not fast if the list of ids is very long; + so we rather check all users than limit to the size of the group + :param group_ids: list of group ids + :return: boolean: is there at least a user in at least 2 of the provided groups + """ + if group_ids: + args = [tuple(group_ids)] + if len(self.ids) == 1: + where_clause = "AND r.uid = %s" + args.append(self.id) + else: + where_clause = "" # default; we check ALL users (actually pretty efficient) + query = """ + SELECT 1 FROM res_groups_users_rel WHERE EXISTS( + SELECT r.uid + FROM res_groups_users_rel r + WHERE r.gid IN %s""" + where_clause + """ + GROUP BY r.uid HAVING COUNT(r.gid) > 1 + ) + """ + self.env.cr.execute(query, args) + return bool(self.env.cr.fetchall()) + else: + return False + @api.multi def toggle_active(self): for user in self: @@ -955,6 +988,7 @@ def write(self, values): JOIN group_imply i ON (r.gid = i.hid) WHERE i.gid = %(gid)s """, dict(gid=group.id)) + self._check_one_user_type() return res class UsersImplied(models.Model): diff --git a/odoo/addons/base/tests/test_user_has_group.py b/odoo/addons/base/tests/test_user_has_group.py index 0151bd2354c64..98e699a071841 100644 --- a/odoo/addons/base/tests/test_user_has_group.py +++ b/odoo/addons/base/tests/test_user_has_group.py @@ -143,3 +143,21 @@ def test_two_user_types(self): }) with self.assertRaises(ValidationError): self.grp_internal.users = [(4, test_user.id)] + + def test_two_user_types_implied_groups(self): + """Contrarily to test_two_user_types, we simply add an implied_id to a group. + This will trigger the addition of the relevant users to the relevant groups; + if, say, this was done in SQL and thus bypassing the ORM, it would bypass the constraints + and thus give us a case uncovered by the aforementioned test. + """ + grp_test = self.env["res.groups"].create( + {"name": "test", "implied_ids": [(6, 0, [self.grp_internal.id])]}) + + test_user = self.env['res.users'].create({ + 'login': 'test_user_portal', + 'name': "Test User with one user types", + 'groups_id': [(6, 0, [grp_test.id])] + }) + + with self.assertRaises(ValidationError): + grp_test.write({'implied_ids': [(4, self.grp_portal.id)]})