From d289d8a7d0ad7dd61b63ac80e2fc6ad3a39504d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Tue, 11 Jul 2023 17:53:48 +0000 Subject: [PATCH] [FIX] mail: make file upload consistent between guest and portal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit opw-3370926 closes odoo/odoo#128290 X-original-commit: 2e5f8a3c9e7cff8c61f66e56da7b0f9a59f9bf2e Signed-off-by: Vranckx Florian (flvr) Signed-off-by: Sébastien Theys (seb) --- addons/mail/controllers/thread.py | 8 +- addons/mail/models/ir_attachment.py | 26 ++- addons/mail/models/mail_thread.py | 2 +- .../static/src/core/common/message_service.js | 17 +- .../static/src/core/common/thread_service.js | 1 + .../mock_server/controllers/discuss.js | 23 +- .../static/tests/tours/discuss_public_tour.js | 56 +++++ addons/mail/static/tests/web/chatter_tests.js | 2 + addons/mail/tests/discuss/__init__.py | 1 + .../tests/discuss/test_message_controller.py | 208 ++++++++++++++++++ addons/portal/controllers/mail.py | 13 +- 11 files changed, 333 insertions(+), 24 deletions(-) create mode 100644 addons/mail/tests/discuss/test_message_controller.py diff --git a/addons/mail/controllers/thread.py b/addons/mail/controllers/thread.py index 02024f31219..6980e809386 100644 --- a/addons/mail/controllers/thread.py +++ b/addons/mail/controllers/thread.py @@ -63,6 +63,10 @@ class ThreadController(http.Controller): @http.route("/mail/message/post", methods=["POST"], type="json", auth="public") def mail_message_post(self, thread_model, thread_id, post_data, context=None): + guest = request.env["mail.guest"]._get_guest_from_request(request) + guest.env["ir.attachment"].browse(post_data.get("attachment_ids", []))._check_attachments_access( + post_data.get("attachment_tokens") + ) if context: request.update_context(**context) canned_response_ids = tuple(cid for cid in post_data.pop('canned_response_ids', []) if isinstance(cid, int)) @@ -90,8 +94,9 @@ class ThreadController(http.Controller): return message_data @http.route("/mail/message/update_content", methods=["POST"], type="json", auth="public") - def mail_message_update_content(self, message_id, body, attachment_ids, partner_ids=None): + def mail_message_update_content(self, message_id, body, attachment_ids, attachment_tokens=None, partner_ids=None): guest = request.env["mail.guest"]._get_guest_from_request(request) + guest.env["ir.attachment"].browse(attachment_ids)._check_attachments_access(attachment_tokens) message_sudo = guest.env["mail.message"].browse(message_id).sudo().exists() if not message_sudo.is_current_user_or_guest_author and not guest.env.user._is_admin(): raise NotFound() @@ -100,3 +105,4 @@ class ThreadController(http.Controller): guest.env[message_sudo.model].browse([message_sudo.res_id])._message_update_content( message_sudo, body, attachment_ids=attachment_ids, partner_ids=partner_ids ) + return message_sudo.message_format()[0] diff --git a/addons/mail/models/ir_attachment.py b/addons/mail/models/ir_attachment.py index 1912ea903d1..c88ca88129a 100644 --- a/addons/mail/models/ir_attachment.py +++ b/addons/mail/models/ir_attachment.py @@ -3,14 +3,36 @@ import contextlib -from odoo import api, models -from odoo.exceptions import AccessError +from odoo import _, api, models, SUPERUSER_ID +from odoo.exceptions import AccessError, MissingError, UserError from odoo.http import request +from odoo.tools import consteq class IrAttachment(models.Model): _inherit = 'ir.attachment' + def _check_attachments_access(self, attachment_tokens): + """This method relies on access rules/rights and therefore it should not be called from a sudo env.""" + self = self.sudo(False) + attachment_tokens = attachment_tokens or ([None] * len(self)) + if len(attachment_tokens) != len(self): + raise UserError(_("An access token must be provided for each attachment.")) + for attachment, access_token in zip(self, attachment_tokens): + try: + attachment_sudo = attachment.with_user(SUPERUSER_ID).exists() + if not attachment_sudo: + raise MissingError(_("The attachment %s does not exist.", attachment.id)) + try: + attachment.check('write') + except AccessError: + if not access_token or not attachment_sudo.access_token or not consteq(attachment_sudo.access_token, access_token): + message_sudo = self.env['mail.message'].sudo().search([('attachment_ids', 'in', attachment_sudo.ids)], limit=1) + if not message_sudo or not message_sudo.is_current_user_or_guest_author: + raise + except (AccessError, MissingError): + raise UserError(_("The attachment %s does not exist or you do not have the rights to access it.", attachment.id)) + def _post_add_create(self): """ Overrides behaviour when the attachment is created through the controller """ diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index f9d5f756a82..4b9c814dc4a 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -4093,7 +4093,7 @@ class MailThread(models.AbstractModel): 'Message': { 'id': message.id, 'body': message.body, - 'attachment_ids': message.attachment_ids._attachment_format(), + 'attachment_ids': message.attachment_ids.sorted("id")._attachment_format(), 'pinned_at': message.pinned_at, 'recipients': [{'id': p.id, 'name': p.name} for p in message.partner_ids], 'write_date': message.write_date, diff --git a/addons/mail/static/src/core/common/message_service.js b/addons/mail/static/src/core/common/message_service.js index 1bdef5603f8..56cc0bda4f0 100644 --- a/addons/mail/static/src/core/common/message_service.js +++ b/addons/mail/static/src/core/common/message_service.js @@ -9,6 +9,8 @@ import { removeFromArrayWithPredicate, replaceArrayWithCompare } from "@mail/uti import { convertBrToLineBreak, prettifyMessageContent } from "@mail/utils/common/format"; import { assignDefined, createLocalId } from "@mail/utils/common/misc"; +import { markup } from "@odoo/owl"; + import { _t } from "@web/core/l10n/translation"; import { registry } from "@web/core/registry"; import { sprintf } from "@web/core/utils/strings"; @@ -34,14 +36,22 @@ export class MessageService { return; } const validMentions = this.getMentionsFromText(rawMentions, body); - await this.rpc("/mail/message/update_content", { + const messageData = await this.rpc("/mail/message/update_content", { attachment_ids: attachments - .map(({ id }) => id) - .concat(message.attachments.map(({ id }) => id)), + .concat(message.attachments) + .map((attachment) => attachment.id), + attachment_tokens: attachments + .concat(message.attachments) + .map((attachment) => attachment.accessToken), body: await prettifyMessageContent(body, validMentions), message_id: message.id, partner_ids: validMentions?.partners?.map((partner) => partner.id), }); + this.insert( + Object.assign(messageData, { + body: messageData.body ? markup(messageData.body) : messageData.body, + }) + ); if (!message.isEmpty && this.store.hasLinkPreviewFeature) { this.rpc( "/mail/link_preview", @@ -63,6 +73,7 @@ export class MessageService { message.attachments = []; await this.rpc("/mail/message/update_content", { attachment_ids: [], + attachment_tokens: [], body: "", message_id: message.id, }); diff --git a/addons/mail/static/src/core/common/thread_service.js b/addons/mail/static/src/core/common/thread_service.js index 7e75d5bd4fd..8a4566f67b8 100644 --- a/addons/mail/static/src/core/common/thread_service.js +++ b/addons/mail/static/src/core/common/thread_service.js @@ -1006,6 +1006,7 @@ export class ThreadService { post_data: { body: await prettifyMessageContent(body, validMentions), attachment_ids: attachments.map(({ id }) => id), + attachment_tokens: attachments.map((attachment) => attachment.accessToken), canned_response_ids: cannedResponseIds, message_type: "comment", partner_ids, diff --git a/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js b/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js index 8c9610cbb3c..14fdab16009 100644 --- a/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js +++ b/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js @@ -95,17 +95,26 @@ patch(MockServer.prototype, "mail/controllers/discuss", { return this._mockRouteMailLoadMessageFailures(); } if (route === "/mail/message/post") { + const finalData = {}; + for (const allowedField of [ + "attachment_ids", + "body", + "message_type", + "partner_ids", + "subtype_xmlid", + "parent_id", + ]) { + if (args.post_data[allowedField] !== undefined) { + finalData[allowedField] = args.post_data[allowedField]; + } + } if (args.thread_model === "discuss.channel") { - return this._mockDiscussChannelMessagePost( - args.thread_id, - args.post_data, - args.context - ); + return this._mockDiscussChannelMessagePost(args.thread_id, finalData, args.context); } return this._mockMailThreadMessagePost( args.thread_model, [args.thread_id], - args.post_data, + finalData, args.context ); } @@ -124,7 +133,7 @@ patch(MockServer.prototype, "mail/controllers/discuss", { attachment_ids: this._mockIrAttachment_attachmentFormat(args.attachment_ids), }, }); - return "dummy value for mock server"; + return this._mockMailMessageMessageFormat([args.message_id])[0]; } if (route === "/mail/read_subscription_data") { const follower_id = args.follower_id; diff --git a/addons/mail/static/tests/tours/discuss_public_tour.js b/addons/mail/static/tests/tours/discuss_public_tour.js index 92e85f99c1f..80d7954e085 100644 --- a/addons/mail/static/tests/tours/discuss_public_tour.js +++ b/addons/mail/static/tests/tours/discuss_public_tour.js @@ -76,5 +76,61 @@ registry.category("web_tour.tours").add("mail/static/tests/tours/discuss_public_ trigger: '.o-mail-Message .o-mail-AttachmentCard:contains("text.txt")', run() {}, }, + { + content: "Click on more menu", + trigger: ".o-mail-Message [title='Expand']", + }, + { + content: "Click on edit", + trigger: ".o-mail-Message [title='Edit']", + }, + { + content: "Edit message", + trigger: ".o-mail-Message .o-mail-Composer-input", + run: "text vegetables", + }, + { + content: "Add one more file in composer", + trigger: ".o-mail-Message .o-mail-Composer button[aria-label='Attach files']", + async run() { + const file = await createFile({ + content: "hello 2", + contentType: "text/plain", + name: "extra.txt", + }); + inputFiles( + document.querySelector( + ".o-mail-Message .o-mail-Composer-coreMain .o_input_file" + ), + [file] + ); + }, + }, + { + content: "Check the earlier provided extra attachment is listed", + trigger: '.o-mail-Message .o-mail-Composer .o-mail-AttachmentCard[title="extra.txt"]', + extra_trigger: + ".o-mail-Message .o-mail-Composer .o-mail-AttachmentCard:not(.o-isUploading)", // waiting the attachment to be uploaded + run() {}, + }, + { + content: "Save edited message", + trigger: ".o-mail-Message a:contains(save)", + }, + { + content: "Check message is edited", + trigger: '.o-mail-Message-body:contains("vegetables")', + run() {}, + }, + { + content: "Check edited message contains the first attachment", + trigger: '.o-mail-Message .o-mail-AttachmentCard:contains("text.txt")', + run() {}, + }, + { + content: "Check edited message contains the extra attachment", + trigger: '.o-mail-Message .o-mail-AttachmentCard:contains("extra.txt")', + run() {}, + }, ], }); diff --git a/addons/mail/static/tests/web/chatter_tests.js b/addons/mail/static/tests/web/chatter_tests.js index 8d34cfc5dd1..28980cb65cf 100644 --- a/addons/mail/static/tests/web/chatter_tests.js +++ b/addons/mail/static/tests/web/chatter_tests.js @@ -52,6 +52,7 @@ QUnit.test("can post a message on a record thread", async (assert) => { post_data: { body: "hey", attachment_ids: [], + attachment_tokens: [], canned_response_ids: [], message_type: "comment", partner_ids: [], @@ -89,6 +90,7 @@ QUnit.test("can post a note on a record thread", async (assert) => { context: args.context, post_data: { attachment_ids: [], + attachment_tokens: [], body: "hey", canned_response_ids: [], message_type: "comment", diff --git a/addons/mail/tests/discuss/__init__.py b/addons/mail/tests/discuss/__init__.py index a12db1a96a8..4451a3547cb 100644 --- a/addons/mail/tests/discuss/__init__.py +++ b/addons/mail/tests/discuss/__init__.py @@ -3,5 +3,6 @@ from . import test_discuss_channel from . import test_discuss_channel_as_guest from . import test_discuss_channel_member +from . import test_message_controller from . import test_load_messages from . import test_rtc diff --git a/addons/mail/tests/discuss/test_message_controller.py b/addons/mail/tests/discuss/test_message_controller.py new file mode 100644 index 00000000000..46e1cc5784f --- /dev/null +++ b/addons/mail/tests/discuss/test_message_controller.py @@ -0,0 +1,208 @@ +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import json + +import odoo +from odoo.tools import mute_logger +from odoo.tests import HttpCase + + +@odoo.tests.tagged("-at_install", "post_install") +class TestMessageController(HttpCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.channel = cls.env["discuss.channel"].create( + { + "group_public_id": None, + "name": "Test channel", + } + ) + cls.public_user = cls.env.ref("base.public_user") + cls.attachments = ( + cls.env["ir.attachment"] + .with_user(cls.public_user) + .sudo() + .create( + [ + { + "access_token": cls.env["ir.attachment"]._generate_access_token(), + "name": "File 1", + "res_id": 0, + "res_model": "mail.compose.message", + }, + { + "access_token": cls.env["ir.attachment"]._generate_access_token(), + "name": "File 2", + "res_id": 0, + "res_model": "mail.compose.message", + }, + ] + ) + ) + cls.guest = cls.env["mail.guest"].create({"name": "Guest"}) + cls.channel.add_members(guest_ids=cls.guest.ids) + + @mute_logger("odoo.addons.http_routing.models.ir_http", "odoo.http") + def test_channel_message_attachments(self): + self.authenticate(None, None) + self.opener.cookies[ + self.guest._cookie_name + ] = f"{self.guest.id}{self.guest._cookie_separator}{self.guest.access_token}" + # test message post: token error + res1 = self.url_open( + url="/mail/message/post", + data=json.dumps( + { + "params": { + "thread_model": self.channel._name, + "thread_id": self.channel.id, + "post_data": { + "body": "test", + "attachment_ids": [self.attachments[0].id], + "attachment_tokens": ["wrong token"], + }, + }, + } + ), + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(res1.status_code, 200) + self.assertIn( + f"The attachment {self.attachments[0].id} does not exist or you do not have the rights to access it", + res1.text, + "guest should not be allowed to add attachment without token when posting message", + ) + # test message post: token ok + res2 = self.url_open( + url="/mail/message/post", + data=json.dumps( + { + "params": { + "thread_model": self.channel._name, + "thread_id": self.channel.id, + "post_data": { + "body": "test", + "attachment_ids": [self.attachments[0].id], + "attachment_tokens": [self.attachments[0].access_token], + "message_type": "comment", + }, + }, + } + ), + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(res2.status_code, 200) + message_format1 = res2.json()["result"] + self.assertEqual( + message_format1["attachment_ids"], + json.loads(json.dumps(self.attachments[0]._attachment_format())), + "guest should be allowed to add attachment with token when posting message", + ) + # test message update: token error + res3 = self.url_open( + url="/mail/message/update_content", + data=json.dumps( + { + "params": { + "message_id": message_format1["id"], + "body": "test", + "attachment_ids": [self.attachments[1].id], + "attachment_tokens": ["wrong token"], + }, + } + ), + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(res3.status_code, 200) + self.assertIn( + f"The attachment {self.attachments[1].id} does not exist or you do not have the rights to access it", + res3.text, + "guest should not be allowed to add attachment without token when updating message", + ) + # test message update: token ok + res4 = self.url_open( + url="/mail/message/update_content", + data=json.dumps( + { + "params": { + "message_id": message_format1["id"], + "body": "test", + "attachment_ids": [self.attachments[1].id], + "attachment_tokens": [self.attachments[1].access_token], + }, + } + ), + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(res4.status_code, 200) + message_format2 = res4.json()["result"] + self.assertEqual( + message_format2["attachment_ids"], + json.loads(json.dumps(self.attachments.sorted("id")._attachment_format())), + "guest should be allowed to add attachment with token when updating message", + ) + # test message update: own attachment ok + res5 = self.url_open( + url="/mail/message/update_content", + data=json.dumps( + { + "params": { + "message_id": message_format2["id"], + "body": "test", + "attachment_ids": [self.attachments[1].id], + }, + } + ), + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(res5.status_code, 200) + message_format3 = res5.json()["result"] + self.assertEqual( + message_format3["attachment_ids"], + json.loads(json.dumps(self.attachments.sorted("id")._attachment_format())), + "guest should be allowed to add own attachment without token when updating message", + ) + + @mute_logger("odoo.addons.http_routing.models.ir_http", "odoo.http") + def test_attachment_hijack(self): + att = self.env["ir.attachment"].create( + [ + { + "name": "arguments_for_firing_marc_demo", + "res_id": 0, + "res_model": "mail.compose.message", + }, + ] + ) + demo = self.authenticate("demo", "demo") + channel = self.env["discuss.channel"].create({"group_public_id": None, "name": "public_channel"}) + channel.add_members( + self.env["res.users"].browse(demo.uid).partner_id.id + ) # don't care, we just need a channel where demo is follower + no_access_request = self.url_open("/web/content/" + str(att.id)) + self.assertFalse( + no_access_request.ok + ) # if this test breaks, it might be due to a change in /web/content, or the default rules for accessing an attachment. This is not an issue but it makes this test irrelevant. + response = self.url_open( + url="/mail/message/post", + headers={"Content-Type": "application/json"}, # route called as demo + data=json.dumps( + { + "params": { + "post_data": { + "attachment_ids": [att.id], # demo does not have access to this attachment id + "body": "", + "message_type": "comment", + "partner_ids": [], + "subtype_xmlid": "mail.mt_comment", + }, + "thread_id": channel.id, + "thread_model": "discuss.channel", + } + }, + ), + ) + self.assertNotIn( + "arguments_for_firing_marc_demo", response.text + ) # demo should not be able to see the name of the document diff --git a/addons/portal/controllers/mail.py b/addons/portal/controllers/mail.py index f99c8fc2bac..987e0db1a8e 100644 --- a/addons/portal/controllers/mail.py +++ b/addons/portal/controllers/mail.py @@ -4,13 +4,12 @@ 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 import mail -from odoo.addons.portal.controllers.portal import CustomerPortal -from odoo.exceptions import AccessError, MissingError, UserError +from odoo.exceptions import AccessError def _check_special_access(res_model, res_id, token='', _hash='', pid=False): @@ -107,13 +106,7 @@ class PortalChatter(http.Controller): return ['token', 'pid'] def _portal_post_check_attachments(self, attachment_ids, attachment_tokens): - 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)) + request.env['ir.attachment'].browse(attachment_ids)._check_attachments_access(attachment_tokens) def _portal_post_has_content(self, res_model, res_id, message, attachment_ids=None, **kw): """ Tells if we can effectively post on the model based on content. """