From 0ad4209b6dd09a68306705692e8e680689a8c89d Mon Sep 17 00:00:00 2001 From: std-odoo Date: Fri, 8 Apr 2022 14:14:01 +0000 Subject: [PATCH 1/2] [FIX] mail: hide restricted the attachments the current user doesn't have access to Bug === When the admin open an email, he might get an error message if he can not access one of the attachment of the email. Now the admin is able to view / edit the attachments he has access to. Task-2464212 Part-of: odoo/odoo#78734 --- addons/mail/models/mail_mail.py | 21 ++++++++ addons/mail/views/mail_mail_views.xml | 8 ++- addons/test_mail/tests/test_mail_mail.py | 68 ++++++++++++++++++++++-- odoo/addons/base/models/ir_attachment.py | 21 ++++++++ 4 files changed, 114 insertions(+), 4 deletions(-) diff --git a/addons/mail/models/mail_mail.py b/addons/mail/models/mail_mail.py index 752ad9eca30..bf2aebaf1fe 100644 --- a/addons/mail/models/mail_mail.py +++ b/addons/mail/models/mail_mail.py @@ -43,6 +43,9 @@ class MailMail(models.Model): body_html = fields.Text('Rich-text Contents', help="Rich-text/HTML message") references = fields.Text('References', help='Message references, such as identifiers of previous messages', readonly=1) headers = fields.Text('Headers', copy=False) + restricted_attachment_count = fields.Integer('Restricted attachments', compute='_compute_restricted_attachments') + unrestricted_attachment_ids = fields.Many2many('ir.attachment', string='Unrestricted Attachments', + compute='_compute_restricted_attachments', inverse='_inverse_unrestricted_attachment_ids') # Auto-detected based on create() - if 'mail_message_id' was passed then this mail is a notification # and during unlink() we will not cascade delete the parent and its attachments is_notification = fields.Boolean('Notification Email', help='Mail has been created to notify people of an existing mail.message') @@ -81,6 +84,24 @@ class MailMail(models.Model): scheduled_date = fields.Char('Scheduled Send Date', help="If set, the queue manager will send the email after the date. If not set, the email will be send as soon as possible.") + @api.depends('attachment_ids') + def _compute_restricted_attachments(self): + """We might not have access to all the attachments of the emails. + Compute the attachments we have access to, + and the number of attachments we do not have access to. + """ + IrAttachment = self.env['ir.attachment'] + for mail_sudo, mail in zip(self.sudo(), self): + mail.unrestricted_attachment_ids = IrAttachment._filter_attachment_access(mail_sudo.attachment_ids.ids) + mail.restricted_attachment_count = len(mail_sudo.attachment_ids) - len(mail.unrestricted_attachment_ids) + + def _inverse_unrestricted_attachment_ids(self): + """We can only remove the attachments we have access to.""" + IrAttachment = self.env['ir.attachment'] + for mail_sudo, mail in zip(self.sudo(), self): + restricted_attaments = mail_sudo.attachment_ids - IrAttachment._filter_attachment_access(mail_sudo.attachment_ids.ids) + mail_sudo.attachment_ids = restricted_attaments | mail.unrestricted_attachment_ids + @api.model_create_multi def create(self, values_list): # notification field: if not set, set if mail comes from an existing mail.message diff --git a/addons/mail/views/mail_mail_views.xml b/addons/mail/views/mail_mail_views.xml index 6a6a1121b72..49815bae1f2 100644 --- a/addons/mail/views/mail_mail_views.xml +++ b/addons/mail/views/mail_mail_views.xml @@ -51,7 +51,13 @@ - + + diff --git a/addons/test_mail/tests/test_mail_mail.py b/addons/test_mail/tests/test_mail_mail.py index 90688b004b8..94aeff5513a 100644 --- a/addons/test_mail/tests/test_mail_mail.py +++ b/addons/test_mail/tests/test_mail_mail.py @@ -6,12 +6,13 @@ import smtplib from OpenSSL.SSL import Error as SSLError from socket import gaierror, timeout -from unittest.mock import call +from unittest.mock import call, patch -from odoo import api +from odoo import api, Command from odoo.addons.base.models.ir_mail_server import MailDeliveryException from odoo.addons.test_mail.tests.common import TestMailCommon -from odoo.tests import common, tagged +from odoo.exceptions import AccessError +from odoo.tests import common, tagged, users from odoo.tools import mute_logger @@ -60,6 +61,67 @@ class TestMailMail(TestMailCommon): self.test_mail.write({'failure_reason': False, 'failure_type': False, 'state': 'outgoing'}) self.test_notification.write({'failure_type': False, 'notification_status': 'ready'}) + @users('admin') + def test_mail_mail_attachment_access(self): + mail = self.env['mail.mail'].create({ + 'body_html': 'Test', + 'email_to': 'test@example.com', + 'partner_ids': [(4, self.user_employee.partner_id.id)], + 'attachment_ids': [ + (0, 0, {'name': 'file 1', 'datas': 'c2VjcmV0'}), + (0, 0, {'name': 'file 2', 'datas': 'c2VjcmV0'}), + (0, 0, {'name': 'file 3', 'datas': 'c2VjcmV0'}), + (0, 0, {'name': 'file 4', 'datas': 'c2VjcmV0'}), + ], + }) + + def _patched_check(self, *args, **kwargs): + if self.env.is_superuser(): + return + if any(attachment.name in ('file 2', 'file 4') for attachment in self): + raise AccessError('No access') + + mail.invalidate_cache(ids=mail.ids) + + new_attachment = self.env['ir.attachment'].create({ + 'name': 'new file', + 'datas': 'c2VjcmV0', + }) + + with patch.object(type(self.env['ir.attachment']), 'check', _patched_check): + # Sanity check + self.assertEqual(mail.restricted_attachment_count, 2) + self.assertEqual(len(mail.unrestricted_attachment_ids), 2) + self.assertEqual(mail.unrestricted_attachment_ids.mapped('name'), ['file 1', 'file 3']) + + # Add a new attachment + mail.write({ + 'unrestricted_attachment_ids': [Command.link(new_attachment.id)], + }) + self.assertEqual(mail.restricted_attachment_count, 2) + self.assertEqual(len(mail.unrestricted_attachment_ids), 3) + self.assertEqual(mail.unrestricted_attachment_ids.mapped('name'), ['file 1', 'file 3', 'new file']) + self.assertEqual(len(mail.attachment_ids), 5) + + # Remove an attachment + mail.write({ + 'unrestricted_attachment_ids': [Command.unlink(new_attachment.id)], + }) + self.assertEqual(mail.restricted_attachment_count, 2) + self.assertEqual(len(mail.unrestricted_attachment_ids), 2) + self.assertEqual(mail.unrestricted_attachment_ids.mapped('name'), ['file 1', 'file 3']) + self.assertEqual(len(mail.attachment_ids), 4) + + # Reset command + mail.invalidate_cache(ids=mail.ids) + mail.write({'unrestricted_attachment_ids': [Command.clear()]}) + self.assertEqual(len(mail.unrestricted_attachment_ids), 0) + self.assertEqual(len(mail.attachment_ids), 2) + + # Read in SUDO + mail.invalidate_cache(ids=mail.ids) + self.assertEqual(mail.sudo().restricted_attachment_count, 2) + self.assertEqual(len(mail.sudo().unrestricted_attachment_ids), 0) @mute_logger('odoo.addons.mail.models.mail_mail') def test_mail_mail_recipients(self): diff --git a/odoo/addons/base/models/ir_attachment.py b/odoo/addons/base/models/ir_attachment.py index c9bae38e7ef..6d1e6924b91 100644 --- a/odoo/addons/base/models/ir_attachment.py +++ b/odoo/addons/base/models/ir_attachment.py @@ -466,6 +466,27 @@ class IrAttachment(models.Model): records.check_access_rights(access_mode) records.check_access_rule(access_mode) + @api.model + def _filter_attachment_access(self, attachment_ids): + """Filter the given attachment to return only the records the current user have access to. + + :param attachment_ids: List of attachment ids we want to filter + :return: the current user have access to + """ + ret_attachments = self.env['ir.attachment'] + attachments = self.browse(attachment_ids) + if not attachments.check_access_rights('read', raise_exception=False): + return ret_attachments + + for attachment in attachments.sudo(): + # Use SUDO here to not raise an error during the prefetch + # And then drop SUDO right to check if we can access it + try: + attachment.sudo(False).check('read') + ret_attachments |= attachment + except AccessError: + continue + return ret_attachments def _read_group_allowed_fields(self): return ['type', 'company_id', 'res_id', 'create_date', 'create_uid', 'name', 'mimetype', 'id', 'url', 'res_field', 'res_model'] From 0afd1f672f2822d3b828830286faa59dfa1d6fe1 Mon Sep 17 00:00:00 2001 From: std-odoo Date: Thu, 14 Apr 2022 13:45:16 +0000 Subject: [PATCH 2/2] [FIX] mail: allow the admin to open any emails Bug === When the admin open an email, he might get an error message if he can not access the mail message. Technical ========= To allow to bypass mail message ACl without adding extra SQL queries, we need to overwrite "_add_inherited_fields". This trick add a related_sudo on the inherits fields, it can't be done with >>> subject = fields.Char(related='mail_message_id.subject', related_sudo=True) because the field of will be fetched two times (one time before of the inherits, and a second time because of the related), and so it will add extra SQL queries. Task-2464212 Part-of: odoo/odoo#78734 --- addons/mail/models/mail_mail.py | 20 ++++++++++++++++++++ addons/mail/views/mail_mail_views.xml | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/addons/mail/models/mail_mail.py b/addons/mail/models/mail_mail.py index bf2aebaf1fe..87defd3147b 100644 --- a/addons/mail/models/mail_mail.py +++ b/addons/mail/models/mail_mail.py @@ -40,6 +40,7 @@ class MailMail(models.Model): # content mail_message_id = fields.Many2one('mail.message', 'Message', required=True, ondelete='cascade', index=True, auto_join=True) + mail_message_id_int = fields.Integer(compute='_compute_mail_message_id_int', compute_sudo=True) body_html = fields.Text('Rich-text Contents', help="Rich-text/HTML message") references = fields.Text('References', help='Message references, such as identifiers of previous messages', readonly=1) headers = fields.Text('Headers', copy=False) @@ -84,6 +85,10 @@ class MailMail(models.Model): scheduled_date = fields.Char('Scheduled Send Date', help="If set, the queue manager will send the email after the date. If not set, the email will be send as soon as possible.") + def _compute_mail_message_id_int(self): + for mail in self: + mail.mail_message_id_int = mail.mail_message_id.id + @api.depends('attachment_ids') def _compute_restricted_attachments(self): """We might not have access to all the attachments of the emails. @@ -135,6 +140,21 @@ class MailMail(models.Model): self.env['mail.message'].browse(mail_msg_cascade_ids).unlink() return res + @api.model + def _add_inherited_fields(self): + """Allow to bypass ACLs for some mail message fields. + + This trick add a related_sudo on the inherits fields, it can't be done with + >>> subject = fields.Char(related='mail_message_id.subject', related_sudo=True) + because the field of will be fetched two times (one time before of + the inherits, and a second time because of the related), and so it will add extra + SQL queries. + """ + super()._add_inherited_fields() + cls = type(self) + for field in ('email_from', 'reply_to', 'subject'): + cls._fields[field].related_sudo = True + def action_retry(self): self.filtered(lambda mail: mail.state == 'exception').mark_outgoing() diff --git a/addons/mail/views/mail_mail_views.xml b/addons/mail/views/mail_mail_views.xml index 49815bae1f2..1b1cb862061 100644 --- a/addons/mail/views/mail_mail_views.xml +++ b/addons/mail/views/mail_mail_views.xml @@ -13,13 +13,13 @@ - +