[FIX] mail: fix performance of channel is_member search

Part 1: _search_is_member
-------------------------

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.

`sudo` is added as well because the rules for the member don't need to
be checked as no information is leaked.

Part 2: clean rules
-------------------

The rule for reading "self" is included in the rule for reading other
members. It can be disabled for "read" to avoid duplicate.
It also checked is_member again, but is_self necessarily implies it.

Part 3: clean tests
-------------------

The opportunity is taken to fix the tests. The tests where considering
as "access error" when there was an assert error inside the test (for
example not finding the channel or the member), but those needed to be
considered as failure regardless of expected outcome of access check.
Extra mute loggers are added to clean the test output.

closes odoo/odoo#153697

Signed-off-by: Olivier Dony (odo) <odo@odoo.com>

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):

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:

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.
             -->
             <field name="perm_create" eval="False"/>
+            <!--
+                read() is controlled by other rules, in particular the current rule for reading self member is
+                "contained" within the rule for reading any member of accessible channel which is more generic.
+            -->
+            <field name="perm_read" eval="False"/>
         </record>
 
         <record id="ir_rule_discuss_channel_member_read_all" model="ir.rule">

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