[FIX] mail, portal, account: clean up portal attachment

Follow up of 61de1c263a7a40a36f2b58b8a107c350de6b6c7f

- Remove the dangerous **kw from the route in favor of specifying useful args.

- Use the existing method to link attachments to message instead of duplicating
the logic in an incomplete manner.

- Fix an issue where the actual email would not be sent if the message was empty
but an attachment was defined.

- Ensure the send attempt is done immediately instead of after the cr commit.
The goal is to show potential error messages correctly to the user with the
crash manager instead of a generic error page.

This also increases the send timeout and prevents the error in the first
place in most situations.

closes odoo/odoo#35263

Signed-off-by: Christophe Simonis <chs@odoo.com>

7 files changed, 98 insertions, 103 deletions

diff --git a/addons/account/tests/__init__.py b/addons/account/tests/__init__.py
index 5c822cca737..e4b29d36e38 100644
--- a/addons/account/tests/__init__.py
+++ b/addons/account/tests/__init__.py
@@ -19,4 +19,5 @@ from . import test_templates_consistency
 from . import test_account_fiscal_year
 from . import test_account_all_l10n
 from . import test_reconciliation_matching_rules
-from . import test_account_onboarding
\ No newline at end of file
+from . import test_account_onboarding
+from . import test_portal_attachment
diff --git a/addons/portal/tests/test_attachment.py b/addons/account/tests/test_portal_attachment.py
index a3f8c3b11db..2aa1b4eff2c 100644
--- a/addons/portal/tests/test_attachment.py
+++ b/addons/account/tests/test_portal_attachment.py
@@ -12,18 +12,20 @@ class TestUi(tests.HttpCase):
 
     @mute_logger('odoo.addons.website.models.ir_http', 'odoo.http')
     def test_01_portal_attachment(self):
+        """Test the portal chatter attachment route."""
+
         base_url = self.env['ir.config_parameter'].sudo().get_param('web.base.url')
-        # For this test we need a parent document with an access_token field:
-        # attachment itself has an access_token so we can use it as parent too.
-        record = self.env['ir.attachment'].create({
+        # For this test we need a parent document with an access_token field
+        # (in this case from portal.mixin) and also inheriting mail.thread.
+        invoice = self.env['account.move'].with_context(tracking_disable=True).create({
             'name': 'a record with an access_token field',
         })
 
         # Test public user can't create attachment without token of document
         create_data = {
             'name': "new attachment",
-            'res_model': record._name,
-            'res_id': record.id,
+            'res_model': invoice._name,
+            'res_id': invoice.id,
             'csrf_token': http.WebRequest.csrf_token(self),
         }
         create_url = base_url + '/portal/attachment/add'
@@ -33,7 +35,7 @@ class TestUi(tests.HttpCase):
         self.assertTrue("you do not have the rights" in res.text)
 
         # Test public user can create attachment with token
-        create_data['access_token'] = record.generate_access_token()[0]
+        create_data['access_token'] = invoice._portal_ensure_token()
         res = self.url_open(url=create_url, data=create_data, files=files)
         self.assertEqual(res.status_code, 200)
         create_res = json.loads(res.content.decode('utf-8'))
@@ -82,21 +84,23 @@ class TestUi(tests.HttpCase):
         self.assertTrue(remove_res is True)
 
         # Test attachment can't be removed if not "pending" state
+        attachment = self.env['ir.attachment'].create({
+            'name': "an attachment",
+            'access_token': self.env['ir.attachment']._generate_access_token(),
+        })
         remove_data = {
-            'attachment_id': record.id,
-            'access_token': record.access_token,
+            'attachment_id': attachment.id,
+            'access_token': attachment.access_token,
         }
         res = self.opener.post(url=remove_url, json={'params': remove_data})
         self.assertEqual(res.status_code, 200)
-        self.assertTrue(self.env['ir.attachment'].search([('id', '=', record.id)]))
+        self.assertTrue(self.env['ir.attachment'].search([('id', '=', attachment.id)]))
         self.assertTrue("not in a pending state" in res.text)
 
         # Test attachment can't be removed if attached to a message
-        attachment = self.env['ir.attachment'].create({
-            'name': 'a record with an access_token field',
+        attachment.write({
             'res_model': 'mail.compose.message',
             'res_id': 0,
-            'access_token': self.env['ir.attachment']._generate_access_token(),
         })
         message = self.env['mail.message'].create({
             'attachment_ids': [(6, 0, attachment.ids)],
@@ -111,42 +115,60 @@ class TestUi(tests.HttpCase):
         self.assertTrue("it is linked to a message" in res.text)
         message.unlink()
 
-        # Need a `mail.thread` record for the following test
-        thread_record = self.env['mail.channel'].create({
-            'name': 'channel',
-            'public': 'public',
-        })
-        # Authenticate because public user can't create `mail.message` without
-        # token, and `mail.channel` does not have an access_token.
-        self.authenticate('portal', 'portal')
-
-        # Test attachment can't be associated if no token.
+        # Test attachment can't be associated if no attachment token.
         post_url = base_url + '/mail/chatter_post'
         post_data = {
-            'res_model': thread_record._name,
-            'res_id': thread_record.id,
-            'message': "test message",
+            'res_model': invoice._name,
+            'res_id': invoice.id,
+            'message': "test message 1",
             'attachment_ids': attachment.id,
             'attachment_tokens': 'false',
             'csrf_token': http.WebRequest.csrf_token(self),
         }
-        self.assertFalse(thread_record.message_ids)
         res = self.url_open(url=post_url, data=post_data)
-        thread_record.invalidate_cache(fnames=['message_ids'], ids=thread_record.ids)
-        message = thread_record.message_ids[0]
-        self.assertFalse(message.attachment_ids)
+        self.assertEqual(res.status_code, 400)
+        self.assertIn("The attachment %s does not exist or you do not have the rights to access it." % attachment.id, res.text)
 
-        # Test attachment can't be associated if not "pending" state
+        # Test attachment can't be associated if no main document token
         post_data['attachment_tokens'] = attachment.access_token
+        res = self.url_open(url=post_url, data=post_data)
+        self.assertEqual(res.status_code, 403)
+        self.assertIn("Sorry, you are not allowed to access documents of type 'Journal Entries' (account.move).", res.text)
+
+        # Test attachment can't be associated if not "pending" state
+        post_data['token'] = invoice._portal_ensure_token()
+        self.assertFalse(invoice.message_ids)
         attachment.write({'res_model': 'model'})
         res = self.url_open(url=post_url, data=post_data)
-        thread_record.invalidate_cache(fnames=['message_ids'], ids=thread_record.ids)
-        message = thread_record.message_ids[0]
-        self.assertFalse(message.attachment_ids)
+        self.assertEqual(res.status_code, 200)
+        invoice.invalidate_cache(fnames=['message_ids'], ids=invoice.ids)
+        self.assertEqual(len(invoice.message_ids), 1)
+        self.assertEqual(invoice.message_ids.body, "<p>test message 1</p>")
+        self.assertFalse(invoice.message_ids.attachment_ids)
 
-        # Test attachment can be associated if all good
+        # Test attachment can't be associated if not correct user
         attachment.write({'res_model': 'mail.compose.message'})
+        post_data['message'] = "test message 2"
+        res = self.url_open(url=post_url, data=post_data)
+        self.assertEqual(res.status_code, 200)
+        invoice.invalidate_cache(fnames=['message_ids'], ids=invoice.ids)
+        self.assertEqual(len(invoice.message_ids), 2)
+        self.assertEqual(invoice.message_ids[0].body, "<p>test message 2</p>")
+        self.assertFalse(invoice.message_ids.attachment_ids)
+
+        # Test attachment can be associated if all good (complete flow)
+        create_data['name'] = "final attachment"
+        res = self.url_open(url=create_url, data=create_data, files=files)
+        self.assertEqual(res.status_code, 200)
+        create_res = json.loads(res.content.decode('utf-8'))
+        self.assertEqual(create_res['name'], "final attachment")
+
+        post_data['message'] = "test message 3"
+        post_data['attachment_ids'] = create_res['id']
+        post_data['attachment_tokens'] = create_res['access_token']
         res = self.url_open(url=post_url, data=post_data)
-        thread_record.invalidate_cache(fnames=['message_ids'], ids=thread_record.ids)
-        message = thread_record.message_ids[0]
-        self.assertEqual(len(message.attachment_ids), 1)
+        self.assertEqual(res.status_code, 200)
+        invoice.invalidate_cache(fnames=['message_ids'], ids=invoice.ids)
+        self.assertEqual(len(invoice.message_ids), 3)
+        self.assertEqual(invoice.message_ids[0].body, "<p>test message 3</p>")
+        self.assertEqual(len(invoice.message_ids[0].attachment_ids), 1)
diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py
index 701690039ca..68254b2bde0 100644
--- a/addons/mail/models/mail_thread.py
+++ b/addons/mail/models/mail_thread.py
@@ -1619,7 +1619,7 @@ class MailThread(models.AbstractModel):
                 lambda a: a.res_model == 'mail.compose.message' and a.create_uid.id == self._uid)
             if filtered_attachment_ids:
                 filtered_attachment_ids.write({'res_model': model, 'res_id': res_id})
-            m2m_attachment_ids += [(4, id) for id in attachment_ids]
+            m2m_attachment_ids += [(4, id) for id in filtered_attachment_ids.ids]
         # Handle attachments parameter, that is a dictionary of attachments
 
         if attachments: # generate 
diff --git a/addons/portal/controllers/mail.py b/addons/portal/controllers/mail.py
index dec8fccff5c..f06f14a1b56 100644
--- a/addons/portal/controllers/mail.py
+++ b/addons/portal/controllers/mail.py
@@ -5,13 +5,13 @@ import werkzeug
 from werkzeug import urls
 from werkzeug.exceptions import NotFound, Forbidden
 
-from odoo import http
+from odoo import http, _
 from odoo.http import request
 from odoo.osv import expression
 from odoo.tools import consteq, plaintext2html
 from odoo.addons.mail.controllers.main import MailController
 from odoo.addons.portal.controllers.portal import CustomerPortal
-from odoo.exceptions import AccessError, MissingError
+from odoo.exceptions import AccessError, MissingError, UserError
 
 
 def _check_special_access(res_model, res_id, token='', _hash='', pid=False):
@@ -25,7 +25,7 @@ def _check_special_access(res_model, res_id, token='', _hash='', pid=False):
         raise Forbidden()
 
 
-def _message_post_helper(res_model, res_id, message, token='', nosubscribe=True, **kw):
+def _message_post_helper(res_model, res_id, message, token='', _hash=False, pid=False, nosubscribe=True, **kw):
     """ Generic chatter function, allowing to write on *any* object that inherits mail.thread. We
         distinguish 2 cases:
             1/ If a token is specified, all logged in users will be able to write a message regardless
@@ -55,9 +55,9 @@ def _message_post_helper(res_model, res_id, message, token='', nosubscribe=True,
 
     # check if user can post with special token/signed token. The "else" will try to post message with the
     # current user access rights (_mail_post_access use case).
-    if token or (kw.get('hash') and kw.get('pid')):
-        pid = int(kw['pid']) if kw.get('pid') else False
-        if _check_special_access(res_model, res_id, token=token, _hash=kw.get('hash'), pid=pid):
+    if token or (_hash and pid):
+        pid = int(pid) if pid else False
+        if _check_special_access(res_model, res_id, token=token, _hash=_hash, pid=pid):
             record = record.sudo()
         else:
             raise Forbidden()
@@ -75,15 +75,9 @@ def _message_post_helper(res_model, res_id, message, token='', nosubscribe=True,
             if not author_id:
                 raise NotFound()
     # Signed Token Case: author_id is forced
-    elif kw.get('hash') and kw.get('pid'):
-        author_id = int(kw.get('pid'))
-
-    kw.pop('csrf_token', None)
-    kw.pop('redirect', None)
-    kw.pop('attachment_ids', None)
-    kw.pop('attachment_tokens', None)
-    kw.pop('hash', None)
-    kw.pop('pid', None)
+    elif _hash and pid:
+        author_id = pid
+
     return record.with_context(mail_create_nosubscribe=nosubscribe).message_post(
         body=message, message_type=kw.pop('message_type', "comment"),
         subtype=kw.pop('subtype', "mt_comment"), author_id=author_id, **kw)
@@ -91,45 +85,8 @@ def _message_post_helper(res_model, res_id, message, token='', nosubscribe=True,
 
 class PortalChatter(http.Controller):
 
-    def _post_process_portal_attachments(self, message, res_model, res_id, attachment_ids, attachment_tokens):
-        """Associate "pending" attachments to a message and its main record.
-
-        The user must have the rights to all attachments or he must provide a
-        valid access_token for each of them. The attachments have to be on a
-        "pending" state as well: res_id=0 and res_model='mail.compose.message'.
-
-        :param message: the related message on which to link the attachments
-        :type message: recordset of one `mail.message`
-
-        :param res_model: the related model that will be saved on the attachment
-        :type res_model: string
-
-        :param res_id: the id of the record that will be saved on the attachment
-        :type res_id: int
-
-        :param attachment_ids: id of the attachments to associate
-        :type attachment_ids: iterable of int
-
-        :param attachment_tokens: access_token of the attachments to associate.
-            Must always be the same length as `attachment_ids`, but only has to
-            contain a valid access_token if the user does not have the rights to
-            access the attachment without token.
-        :type attachment_tokens: iterable of string
-        """
-        message.ensure_one()
-        attachments = request.env['ir.attachment'].sudo()
-        for (attachment_id, access_token) in zip(attachment_ids, attachment_tokens):
-            try:
-                attachment = CustomerPortal._document_check_access(self, 'ir.attachment', attachment_id, access_token)
-                if attachment.res_model == 'mail.compose.message' and attachment.res_id == 0:
-                    attachments += attachment
-            except (AccessError, MissingError):
-                pass
-        attachments.write({'res_model': res_model, 'res_id': res_id})
-        message.attachment_ids |= attachments
-
     @http.route(['/mail/chatter_post'], type='http', methods=['POST'], auth='public', website=True)
-    def portal_chatter_post(self, res_model, res_id, message, redirect=None, attachment_ids=None, attachment_tokens=None, **kw):
+    def portal_chatter_post(self, res_model, res_id, message, redirect=None, attachment_ids='', attachment_tokens='', **kw):
         """Create a new `mail.message` with the given `message` and/or
         `attachment_ids` and redirect the user to the newly created message.
 
@@ -138,18 +95,33 @@ class PortalChatter(http.Controller):
         must provide valid identifiers through `kw`. See `_message_post_helper`.
         """
         url = redirect or (request.httprequest.referrer and request.httprequest.referrer + "#discussion") or '/my'
-        if message or (attachment_ids and attachment_tokens):
-            res_id = int(res_id)
+
+        res_id = int(res_id)
+
+        attachment_ids = [int(res_id) for res_id in attachment_ids.split(',')]
+        attachment_tokens = attachment_tokens.split(',')
+        if len(attachment_tokens) != len(attachment_ids):
+            raise UserError(_("An access token must be provided for each attachment."))
+        for (attachment_id, access_token) in zip(attachment_ids, attachment_tokens):
+            try:
+                CustomerPortal._document_check_access(self, 'ir.attachment', attachment_id, access_token)
+            except (AccessError, MissingError):
+                raise UserError(_("The attachment %s does not exist or you do not have the rights to access it.") % attachment_id)
+
+        if message or attachment_ids:
             # message is received in plaintext and saved in html
             if message:
                 message = plaintext2html(message)
-            message = _message_post_helper(res_model=res_model, res_id=res_id, message=message, **kw)
-
-            if attachment_ids and attachment_tokens and message:
-                attachment_ids = [int(res_id) for res_id in attachment_ids.split(',')]
-                attachment_tokens = attachment_tokens.split(',')
-                self._post_process_portal_attachments(message=message, res_model=res_model, res_id=res_id,
-                    attachment_ids=attachment_ids, attachment_tokens=attachment_tokens)
+            message = _message_post_helper(
+                res_model=res_model,
+                res_id=res_id,
+                token=kw.get('token'),
+                _hash=kw.get('hash'),
+                pid=kw.get('pid'),
+                message=message,
+                send_after_commit=False,
+                attachment_ids=attachment_ids
+            )
 
         return request.redirect(url)
 
diff --git a/addons/portal/controllers/portal.py b/addons/portal/controllers/portal.py
index 286cd970a0c..4425266df38 100644
--- a/addons/portal/controllers/portal.py
+++ b/addons/portal/controllers/portal.py
@@ -241,7 +241,7 @@ class CustomerPortal(Controller):
 
         # At this point the related message does not exist yet, so we assign
         # those specific res_model and res_is. They will be correctly set
-        # when the message is created: see `_post_process_portal_attachments`,
+        # when the message is created: see `portal_chatter_post`,
         # or garbage collected otherwise: see  `_garbage_collect_attachments`.
         attachment = IrAttachment.create({
             'name': name,
diff --git a/addons/portal/tests/__init__.py b/addons/portal/tests/__init__.py
index 752ffa6f96c..06a8d4cd529 100644
--- a/addons/portal/tests/__init__.py
+++ b/addons/portal/tests/__init__.py
@@ -2,4 +2,3 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
 from . import test_load_process
-from . import test_attachment
diff --git a/addons/test_mail/tests/test_odoobot.py b/addons/test_mail/tests/test_odoobot.py
index af73b325311..a494a84bf53 100644
--- a/addons/test_mail/tests/test_odoobot.py
+++ b/addons/test_mail/tests/test_odoobot.py
@@ -64,6 +64,7 @@ class TestOdoobot(BaseFunctionalTest, MockEmails, TestRecipients):
         attachment = self.env['ir.attachment'].with_user(self.user_employee).create({
             'datas': 'bWlncmF0aW9uIHRlc3Q=',
             'name': 'picture_of_your_dog.doc',
+            'res_model': 'mail.compose.message',
         })
         kwargs['attachment_ids'] = [attachment.id]
         last_message = self.assertNextMessage(