diff --git a/addons/mail/models/discuss/discuss_channel.py b/addons/mail/models/discuss/discuss_channel.py index ac4f7fe3cbf..42503e08a34 100644 --- a/addons/mail/models/discuss/discuss_channel.py +++ b/addons/mail/models/discuss/discuss_channel.py @@ -171,7 +171,21 @@ class Channel(models.Model): def _search_is_member(self, operator, operand): is_in = (operator == '=' and operand) or (operator == '!=' and not operand) - return [('channel_member_ids', "any" if is_in else "not any", [('is_self', '=', True)])] + # Separate query to fetch candidate channels because the sub-select that _search would + # generate leads psql query plan to take bad decisions. When candidate ids are explicitly + # given it doesn't need to make (incorrect) guess, at the cost of one extra but fast query. + # It is expected to return hundreds of channels, a thousand at most, which is acceptable. + # A "join" would be ideal, but the ORM is currently not able to generate it from the domain. + current_partner, current_guest = self.env["res.partner"]._get_current_persona() + if current_guest: + # sudo: discuss.channel - sudo for performance, just checking existence + channels = current_guest.sudo().channel_ids + elif current_partner: + # sudo: discuss.channel - sudo for performance, just checking existence + channels = current_partner.sudo().channel_ids + else: + channels = self.env["discuss.channel"] + return [('id', "in" if is_in else "not in", channels.ids)] @api.depends('channel_member_ids') def _compute_member_count(self): diff --git a/addons/mail/models/discuss/discuss_channel_member.py b/addons/mail/models/discuss/discuss_channel_member.py index 8e65a1fd974..125312c79e3 100644 --- a/addons/mail/models/discuss/discuss_channel_member.py +++ b/addons/mail/models/discuss/discuss_channel_member.py @@ -138,7 +138,11 @@ class ChannelMember(models.Model): raise UserError( _("Adding more members to this chat isn't possible; it's designed for just two people.") ) - return super().create(vals_list) + res = super().create(vals_list) + # help the ORM to detect changes + res.partner_id.invalidate_recordset(["channel_ids"]) + res.guest_id.invalidate_recordset(["channel_ids"]) + return res def write(self, vals): for channel_member in self: diff --git a/addons/mail/security/mail_security.xml b/addons/mail/security/mail_security.xml index 279825269cb..97d727f7379 100644 --- a/addons/mail/security/mail_security.xml +++ b/addons/mail/security/mail_security.xml @@ -46,9 +46,7 @@ [ ('is_self', '=', True), "|", - "&", - ("channel_id.channel_type", "!=", "channel"), - ("channel_id.is_member", "=", True), + ("channel_id.channel_type", "!=", "channel"), "&", ("channel_id.channel_type", "=", "channel"), '|', @@ -61,6 +59,11 @@ its data, which means just using 'is_self' would allow any user to add themselves in any channel. --> + + diff --git a/addons/mail/tests/discuss/test_discuss_channel_access.py b/addons/mail/tests/discuss/test_discuss_channel_access.py index 3b83e155e87..2fefd0f019a 100644 --- a/addons/mail/tests/discuss/test_discuss_channel_access.py +++ b/addons/mail/tests/discuss/test_discuss_channel_access.py @@ -1,7 +1,10 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. +from psycopg2.errors import UniqueViolation + from odoo.addons.mail.tests.common import mail_new_test_user from odoo.addons.mail.tests.common import MailCommon +from odoo.exceptions import AccessError, UserError from odoo.tests.common import tagged from odoo.tools import mute_logger @@ -145,17 +148,29 @@ class TestDiscussChannelAccess(MailCommon): for user_key, channel_key, membership, operation, result in tests: if result: try: - self._execute_action_channel(user_key, channel_key, membership, operation, result) - except Exception as e: # noqa: BLE001 - re-raising, just with a more contextual message - raise AssertionError(f"{user_key, channel_key, membership, operation} should not raise") from e + self._execute_action_channel( + user_key, channel_key, membership, operation, result + ) + except ( + Exception + ) as e: # noqa: BLE001 - re-raising, just with a more contextual message + raise AssertionError( + f"{user_key, channel_key, membership, operation} should not raise" + ) from e else: try: - with self.assertRaises(Exception), mute_logger("odoo.sql_db"), mute_logger( - "odoo.addons.base.models.ir_rule" + with self.assertRaises(AccessError), mute_logger("odoo.sql_db"), mute_logger( + "odoo.addons.base.models.ir_model" + ), mute_logger("odoo.addons.base.models.ir_rule"), mute_logger( + "odoo.models.unlink" ): - self._execute_action_channel(user_key, channel_key, membership, operation, result) + self._execute_action_channel( + user_key, channel_key, membership, operation, result + ) except AssertionError as e: - raise AssertionError(f"{user_key, channel_key, membership, operation} should raise") from e + raise AssertionError( + f"{user_key, channel_key, membership, operation} should raise" + ) from e def test_10_discuss_channel_member_access(self): """Exhaustive list of operations on channel.member that would make sense in various setup @@ -384,18 +399,29 @@ class TestDiscussChannelAccess(MailCommon): if result: try: self._execute_action_member(channel_id, user_key, target, operation, result) - except Exception as e: # noqa: BLE001 - re-raising, just with a more contextual message + except ( + Exception + ) as e: # noqa: BLE001 - re-raising, just with a more contextual message raise AssertionError( f"{user_key, channel_key, membership, target, operation} should not raise" ) from e else: try: - with self.assertRaises(Exception), mute_logger("odoo.sql_db"), mute_logger( - "odoo.addons.base.models.ir_rule" + with self.assertRaises(AccessError), mute_logger("odoo.sql_db"), mute_logger( + "odoo.addons.base.models.ir_model" + ), mute_logger("odoo.addons.base.models.ir_rule"), mute_logger( + "odoo.models.unlink" ): - self._execute_action_member(channel_id, user_key, target, operation, result) + try: + self._execute_action_member( + channel_id, user_key, target, operation, result + ) + except (UniqueViolation, UserError) as e: + raise AccessError("expected errors as access error") from e except AssertionError as e: - raise AssertionError(f"{user_key, channel_key, membership, target, operation} should raise") from e + raise AssertionError( + f"{user_key, channel_key, membership, target, operation} should raise access error" + ) from e def _get_channel_id(self, user_key, channel_key, membership): partner = self.env["res.partner"] if user_key == "public" else self.users[user_key].partner_id