[FIX] mail: overrides of _search()

This is about the overrides of _search() on models mail.activity and
mail.message, which both make extra queries to implement their specific
access rights.  We combine both queries made in _search() to retrieve
accessible records.  This simply uses the API of the Query object to
retrieve the data that is necessary to restrict access to messages.

Move security check outside of _message_format() for performance.  The
call to check_access_rule() inside _message_format() was redundant in
many cases and generated more SQL queries than necessary.

Also for performance, accessing fields from records should not actually
check permission.  That's a bit freaky, but this reproduces the former
behavior of mail.message.

And finally, make method check_access_rule() on mail.message check
ir.rules, in order to make it consistent with method _search().

Part-of: odoo/odoo#112126

addons/mail/models/mail_activity.py

@@ -59,7 +59,7 @@ class MailActivity(models.Model):
         index=True, ondelete='cascade', required=True)
     res_model = fields.Char(
         'Related Document Model',
-        index=True, related='res_model_id.model', compute_sudo=True, store=True, readonly=True)
+        index=True, related='res_model_id.model', precompute=True, store=True, readonly=True)
     res_id = fields.Many2oneReference(string='Related Document ID', index=True, model_field='res_model')
     res_name = fields.Char(
         'Document Name', compute='_compute_res_name', compute_sudo=True, store=True,
@@ -323,17 +323,6 @@ class MailActivity(models.Model):
             ])
         return activities
 
-    def read(self, fields=None, load='_classic_read'):
-        """ When reading specific fields, read calls _read that manually applies ir rules
-        (_apply_ir_rules), instead of calling check_access_rule.
-
-        Meaning that our custom rules enforcing from '_filter_access_rules' and
-        '_filter_access_rules_python' are bypassed in that case.
-        To make sure we apply our custom security rules, we force a call to 'check_access_rule'. """
-
-        self.check_access_rule('read')
-        return super(MailActivity, self).read(fields=fields, load=load)
-
     def write(self, values):
         if values.get('user_id'):
             user_changes = self.filtered(lambda activity: activity.user_id.id != values.get('user_id'))
@@ -383,46 +372,37 @@ class MailActivity(models.Model):
         if self.env.is_superuser():
             return super()._search(domain, offset, limit, order, access_rights_uid)
 
-        ids = super()._search(domain, offset, limit, order, access_rights_uid)
-        if not ids:
-            return ids
-
-        # check read access rights before checking the actual rules on the given ids
-        super(MailActivity, self.with_user(access_rights_uid or self._uid)).check_access_rights('read')
-
+        # retrieve activities and their corresponding res_model, res_id
         self.flush_model(['res_model', 'res_id'])
-        activities_to_check = []
-        for sub_ids in self._cr.split_for_in_conditions(ids):
-            self._cr.execute("""
-                SELECT DISTINCT activity.id, activity.res_model, activity.res_id
-                FROM "%s" activity
-                WHERE activity.id = ANY (%%(ids)s) AND activity.res_id != 0""" % self._table, dict(ids=list(sub_ids)))
-            activities_to_check += self._cr.dictfetchall()
+        query = super()._search(domain, offset, limit, order, access_rights_uid)
+        query_str, params = query.select(
+            f'"{self._table}"."id"',
+            f'"{self._table}"."res_model"',
+            f'"{self._table}"."res_id"',
+        )
+        self.env.cr.execute(query_str, params)
+        rows = self.env.cr.fetchall()
 
-        activity_to_documents = {}
-        for activity in activities_to_check:
-            activity_to_documents.setdefault(activity['res_model'], set()).add(activity['res_id'])
+        # group res_ids by model, and determine accessible records
+        model_ids = defaultdict(set)
+        for _id, res_model, res_id in rows:
+            model_ids[res_model].add(res_id)
 
-        allowed_ids = set()
-        for doc_model, doc_ids in activity_to_documents.items():
+        allowed_ids = defaultdict(set)
+        for res_model, res_ids in model_ids.items():
+            records = self.env[res_model].with_user(access_rights_uid or self._uid).browse(res_ids)
             # fall back on related document access right checks. Use the same as defined for mail.thread
             # if available; otherwise fall back on read
-            if hasattr(self.env[doc_model], '_mail_post_access'):
-                doc_operation = self.env[doc_model]._mail_post_access
-            else:
-                doc_operation = 'read'
-            DocumentModel = self.env[doc_model].with_user(access_rights_uid or self._uid)
-            right = DocumentModel.check_access_rights(doc_operation, raise_exception=False)
-            if right:
-                valid_docs = DocumentModel.browse(doc_ids)._filter_access_rules(doc_operation)
-                valid_doc_ids = set(valid_docs.ids)
-                allowed_ids.update(
-                    activity['id'] for activity in activities_to_check
-                    if activity['res_model'] == doc_model and activity['res_id'] in valid_doc_ids)
+            operation = getattr(records, '_mail_post_access', 'read')
+            if records.check_access_rights(operation, raise_exception=False):
+                allowed_ids[res_model] = set(records._filter_access_rules(operation)._ids)
 
-        # re-construct a list based on ids, because 'allowed_ids' does not keep the original order
-        id_list = [id for id in ids if id in allowed_ids]
-        return self.browse(id_list)._as_query(order)
+        activities = self.browse(
+            id_
+            for id_, res_model, res_id in rows
+            if res_id in allowed_ids[res_model]
+        )
+        return activities._as_query(order)
 
     @api.model
     def _read_group_raw(self, domain, fields, groupby, offset=0, limit=None, orderby=False, lazy=True):

addons/mail/models/mail_message.py

@@ -274,48 +274,52 @@ class Message(models.Model):
         # Rules do not apply to administrator
         if self.env.is_superuser():
             return super()._search(domain, offset, limit, order, access_rights_uid)
+
         # Non-employee see only messages with a subtype and not internal
         if not self.env['res.users'].has_group('base.group_user'):
-            domain = expression.AND([self._get_search_domain_share(), domain])
-        # Perform a super with count as False, to have the ids, not a counter
-        ids = super()._search(domain, offset, limit, order, access_rights_uid)
-        if not ids:
-            return ids
+            domain = self._get_search_domain_share() + domain
 
-        pid = self.env.user.partner_id.id
-        author_ids, partner_ids, allowed_ids = set([]), set([]), set([])
-        model_ids = {}
-
-        # check read access rights before checking the actual rules on the given ids
-        super(Message, self.with_user(access_rights_uid or self._uid)).check_access_rights('read')
+        # make the search query with the default rules
+        query = super()._search(domain, offset, limit, order, access_rights_uid)
 
+        # retrieve matching records and determine which ones are truly accessible
         self.flush_model(['model', 'res_id', 'author_id', 'message_type', 'partner_ids'])
         self.env['mail.notification'].flush_model(['mail_message_id', 'res_partner_id'])
-        for sub_ids in self._cr.split_for_in_conditions(ids):
-            self._cr.execute("""
-                SELECT DISTINCT m.id, m.model, m.res_id, m.author_id, m.message_type,
-                                COALESCE(partner_rel.res_partner_id, needaction_rel.res_partner_id)
-                FROM "%s" m
-                LEFT JOIN "mail_message_res_partner_rel" partner_rel
-                ON partner_rel.mail_message_id = m.id AND partner_rel.res_partner_id = %%(pid)s
-                LEFT JOIN "mail_notification" needaction_rel
-                ON needaction_rel.mail_message_id = m.id AND needaction_rel.res_partner_id = %%(pid)s
-                WHERE m.id = ANY (%%(ids)s)""" % self._table, dict(pid=pid, ids=list(sub_ids)))
-            for msg_id, rmod, rid, author_id, message_type, partner_id in self._cr.fetchall():
-                if author_id == pid:
-                    author_ids.add(msg_id)
-                elif partner_id == pid:
-                    partner_ids.add(msg_id)
-                elif rmod and rid and message_type != 'user_notification':
-                    model_ids.setdefault(rmod, {}).setdefault(rid, set()).add(msg_id)
 
-        allowed_ids = self._find_allowed_doc_ids(model_ids)
+        pid = self.env.user.partner_id.id
+        ids = []
+        allowed_ids = set()
+        model_ids = defaultdict(lambda: defaultdict(set))
 
-        final_ids = author_ids | partner_ids | allowed_ids
+        rel_alias = query.left_join(
+            self._table, 'id', 'mail_message_res_partner_rel', 'mail_message_id', 'partner_ids',
+            '{rhs}.res_partner_id = %s', [pid],
+        )
+        notif_alias = query.left_join(
+            self._table, 'id', 'mail_notification', 'mail_message_id', 'notification_ids',
+            '{rhs}.res_partner_id = %s', [pid],
+        )
+        query_str, params = query.select(
+            f'"{self._table}"."id"',
+            f'"{self._table}"."model"',
+            f'"{self._table}"."res_id"',
+            f'"{self._table}"."author_id"',
+            f'"{self._table}"."message_type"',
+            f'COALESCE("{rel_alias}"."res_partner_id", "{notif_alias}"."res_partner_id")',
+        )
+        self.env.cr.execute(query_str, params)
+        for id_, model, res_id, author_id, message_type, partner_id in self.env.cr.fetchall():
+            ids.append(id_)
+            if author_id == pid:
+                allowed_ids.add(id_)
+            elif partner_id == pid:
+                allowed_ids.add(id_)
+            elif model and res_id and message_type != 'user_notification':
+                model_ids[model][res_id].add(id_)
 
-        # re-construct a list based on ids, because set did not keep the original order
-        id_list = [id for id in ids if id in final_ids]
-        return self.browse(id_list)._as_query(order)
+        allowed_ids.update(self._find_allowed_doc_ids(model_ids))
+        allowed = self.browse(id_ for id_ in ids if id_ in allowed_ids)
+        return allowed._as_query(order)
 
     @api.model
     def _find_allowed_model_wise(self, doc_model, doc_dict):
@@ -373,6 +377,10 @@ class Message(models.Model):
 
         if self.env.is_superuser():
             return
+
+        # just in case there are ir.rules
+        super().check_access_rule(operation)
+
         # Non employees see only messages with a subtype (aka, not internal logs)
         if not self.env['res.users'].has_group('base.group_user'):
             self._cr.execute('''SELECT DISTINCT message.id, message.subtype_id, subtype.internal
@@ -639,6 +647,15 @@ class Message(models.Model):
         self.check_access_rule('read')
         return super(Message, self).read(fields=fields, load=load)
 
+    def fetch(self, field_names):
+        # This freaky hack is aimed at reading data without the overhead of
+        # checking that "self" is accessible, which is already done above in
+        # methods read() and _search(). It reproduces the existing behavior
+        # before the introduction of method fetch(), where the low-lever
+        # reading method _read() did not enforce any actual permission.
+        self = self.sudo()
+        return super().fetch(field_names)
+
     def write(self, vals):
         record_changed = 'model' in vals or 'res_id' in vals
         if record_changed or 'message_type' in vals:
@@ -828,7 +845,6 @@ class Message(models.Model):
 
     def _message_format(self, fnames, format_reply=True, legacy=False):
         """Reads values from messages and formats them for the web client."""
-        self.check_access_rule('read')
         vals_list = self._read_format(fnames)
 
         thread_ids_by_model_name = defaultdict(set)
@@ -951,6 +967,7 @@ class Message(models.Model):
                     'parentMessage': {...}, # formatted message that this message is a reply to. Only present if format_reply is True
                 }
         """
+        self.check_access_rule('read')
         vals_list = self._message_format(self._get_message_format_fields(), format_reply=format_reply)
 
         com_id = self.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment')

addons/portal/models/mail_message.py

@@ -8,6 +8,7 @@ class MailMessage(models.Model):
     _inherit = 'mail.message'
 
     def portal_message_format(self):
+        self.check_access_rule('read')
         return self._portal_message_format([
             'id', 'body', 'date', 'author_id', 'email_from',  # base message fields
             'message_type', 'subtype_id', 'is_internal', 'subject',  # message specific

addons/test_mail/tests/test_mail_thread_internals.py

@@ -389,7 +389,7 @@ class TestDiscuss(TestMailCommon, TestRecipients):
         )
 
         with self.assertRaises(exceptions.AccessError):
-            notification_msg.with_env(self.env)._message_format(['id', 'body', 'date', 'author_id', 'email_from'])
+            notification_msg.with_env(self.env).message_format(['id', 'body', 'date', 'author_id', 'email_from'])
 
         channel_message = self.env['mail.message'].sudo().search([('model', '=', 'mail.channel'), ('res_id', 'in', channel.ids)])
         self.assertEqual(len(channel_message), 1, "Test message should have been posted")