pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[IMP] mail: add sanity checks for message_post and its helpers

Browse Source 
Purpose of this commit is to clearly check input of ``message_post`` method
and its main helpers in order to prevent wrong usage of message post API.

Some values used to populate message fields should not be set directly when
posting or logging messages. Indeed they may be part of other process (like
notification process managed by ``_notify_thread``, or could be setup by
custom routes like 'reaction_ids', or custom usage of 'model' and 'res_id'
that could conflicts with record on which methods are called).

We therefore add checks and cleanup in those methods to be sure the API
is used as intended and avoid unwanted side effects.

Task-2710804 (Mail: Clean MailThread Posting API)

Part-of: odoo/odoo#99482
This commit is contained in:
Thibault Delavallée
2023-01-17 20:58:34 +01:00
parent 023bc67327
commit 2175bde607
7 changed files with 238 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/mail/models/mail_message.py
+3 -3
View File
@@ -571,7 +571,7 @@ class Message(models.Model):
if 'record_name' not in values and 'default_record_name' not in self.env.context:
values['record_name'] = self._get_record_name(values)
if 'attachment_ids' not in values:
if not values.get('attachment_ids'):
values['attachment_ids'] = []
# extract base64 images
if 'body' in values:
@@ -604,9 +604,9 @@ class Message(models.Model):
messages = super(Message, self).create(values_list)
check_attachment_access = []
if all(isinstance(command, int) or command[0] in (4, 6) for values in values_list for command in values.get('attachment_ids')):
if all(isinstance(command, int) or command[0] in (4, 6) for values in values_list for command in values['attachment_ids']):
for values in values_list:
for command in values.get('attachment_ids'):
for command in values['attachment_ids']:
if isinstance(command, int):
check_attachment_access += [command]
elif command[0] == 6:
addons/mail/models/mail_thread.py
+197 -54
View File
@@ -1821,36 +1821,61 @@ class MailThread(models.AbstractModel):
tuples in the form ``(name,content)`` or ``(name,content, info)`` where content
is NOT base64 encoded;
:param list attachment_ids: list of existing attachments to link to this message
-Should only be set by chatter
-Attachment object attached to mail.compose.message(0) will be attached
to the related document.
Should not be a list of commands. Attachment records attached to mail
composer will be attached to the related document.
Extra keyword arguments will be used either
* as default column values for the new mail.message record if they match
mail.message fields;
* propagated to notification methods;
* propagated to notification methods if not;
:return record: newly create mail.message
"""
self.ensure_one() # should always be posted on a record, use message_notify if no record
# split message additional values from notify additional values
msg_kwargs = dict((key, val) for key, val in kwargs.items() if key in self.env['mail.message']._fields)
notif_kwargs = dict((key, val) for key, val in kwargs.items() if key not in msg_kwargs)
# preliminary value safety check
partner_ids = set(partner_ids or [])
if self._name == 'mail.thread' or not self.id or message_type == 'user_notification':
raise ValueError(_('Posting a message should be done on a business document. Use message_notify to send a notification to an user.'))
if 'channel_ids' in kwargs:
raise ValueError(_("Posting a message with channels as listeners is not supported since Odoo 14.3+. Please update code accordingly."))
if 'model' in msg_kwargs or 'res_id' in msg_kwargs:
raise ValueError(_("message_post does not support model and res_id parameters anymore. Please call message_post on record."))
if 'subtype' in kwargs:
raise ValueError(_("message_post does not support subtype parameter anymore. Please give a valid subtype_id or subtype_xmlid value instead."))
if any(not isinstance(pc_id, int) for pc_id in partner_ids):
raise ValueError(_('message_post partner_ids and must be integer list, not commands.'))
self._raise_for_invalid_parameters(
set(kwargs.keys()),
forbidden_names={'model', 'res_id', 'subtype'}
)
if self._name == 'mail.thread' or not self.id:
raise ValueError(_("Posting a message should be done on a business document. Use message_notify to send a notification to an user."))
if message_type == 'user_notification':
raise ValueError(_("Use message_notify to send a notification to an user."))
if attachments:
# attachments should be a list (or tuples) of 3-elements list (or tuple)
format_error = not tools.is_list_of(attachments, list) and not tools.is_list_of(attachments, tuple)
if not format_error:
format_error = not all(len(attachment) in {2, 3} for attachment in attachments)
if format_error:
raise ValueError(
_('Posting a message should receive attachments as a list of list or tuples (received %(aids)s)',
aids=repr(attachment_ids),
)
)
if attachment_ids and not tools.is_list_of(attachment_ids, int):
raise ValueError(
_('Posting a message should receive attachments records as a list of IDs (received %(aids)s)',
aids=repr(attachment_ids),
)
)
attachment_ids = list(attachment_ids or [])
if partner_ids and not tools.is_list_of(partner_ids, int):
raise ValueError(
_('Posting a message should receive partners as a list of IDs (received %(pids)s)',
pids=repr(partner_ids),
)
)
partner_ids = list(partner_ids or [])
self = self._fallback_lang() # add lang to context immediately since it will be useful in various flows latter.
# split message additional values from notify additional values
msg_kwargs = {key: val for key, val in kwargs.items()
if key in self.env['mail.message']._fields}
notif_kwargs = {key: val for key, val in kwargs.items()
if key not in msg_kwargs}
# Add lang to context immediately since it will be useful in various flows later
self = self._fallback_lang()
# Find the message's author
if self.env.user._is_public() and 'guest' in self.env.context:
@@ -1892,12 +1917,10 @@ class MailThread(models.AbstractModel):
'partner_ids': partner_ids,
})
attachments = attachments or []
attachment_ids = attachment_ids or []
attachement_values = self._process_attachments_for_post(attachments, attachment_ids, msg_values)
msg_values.update(attachement_values) # attachement_ids, [body]
new_message = self._message_create(msg_values)
msg_values.update(
self._process_attachments_for_post(attachments, attachment_ids, msg_values)
) # attachement_ids, body
new_message = self._message_create([msg_values])
# Set main attachment field if necessary
self._message_set_main_attachment_id(msg_values['attachment_ids'])
@@ -2111,13 +2134,19 @@ class MailThread(models.AbstractModel):
:param template_id : the id of the template to render to create the body of the message
:param **kwargs : parameter to create a mail.compose.message woaerd (which inherit from mail.message)
"""
# preliminary value safety check
self._raise_for_invalid_parameters(
set(kwargs.keys()),
forbidden_names={'model', 'res_id'}
)
# Get composition mode, or force it according to the number of record in self
if not kwargs.get('composition_mode'):
kwargs['composition_mode'] = 'comment' if len(self.ids) == 1 else 'mass_mail'
if not kwargs.get('message_type'):
kwargs['message_type'] = 'notification'
res_id = kwargs.get('res_id', self.ids and self.ids[0] or 0)
res_ids = kwargs.get('res_id') and [kwargs['res_id']] or self.ids
res_id = self.ids[0] if self.ids else 0
res_ids = self.ids
# support xml based subtype id
if kwargs.get('subtype_xmlid') and not kwargs.get('subtype_id'):
@@ -2127,17 +2156,22 @@ class MailThread(models.AbstractModel):
composer = self.env['mail.compose.message'].with_context(
active_id=res_id,
active_ids=res_ids,
active_model=kwargs.get('model', self._name),
active_model=self._name,
default_composition_mode=kwargs['composition_mode'],
default_email_layout_xmlid=email_layout_xmlid,
default_model=kwargs.get('model', self._name),
default_model=self._name,
default_res_id=res_id,
default_template_id=template_id,
).create(kwargs)
# Simulate the onchange (like trigger in form the view) only
# when having a template in single-email mode
if template_id:
update_values = composer._onchange_template_id(template_id, kwargs['composition_mode'], self._name, res_id)['value']
update_values = composer._onchange_template_id(
template_id,
kwargs['composition_mode'],
self._name,
res_id
)['value']
composer.write(update_values)
return composer._action_send_mail(auto_commit=auto_commit)
@@ -2151,21 +2185,32 @@ class MailThread(models.AbstractModel):
on the user configuration, like other notifications. """
if self:
self.ensure_one()
# split message additional values from notify additional values
msg_kwargs = dict((key, val) for key, val in kwargs.items() if key in self.env['mail.message']._fields)
notif_kwargs = dict((key, val) for key, val in kwargs.items() if key not in msg_kwargs)
author_id, email_from = self._message_compute_author(author_id, email_from, raise_on_email=True)
if not partner_ids:
_logger.warning('Message notify called without recipient_ids, skipping')
return self.env['mail.message']
# preliminary value safety check
self._raise_for_invalid_parameters(
set(kwargs.keys()),
forbidden_names={'message_id', 'message_type'}
)
if not tools.is_list_of(partner_ids, int):
raise ValueError(
_('Notification should receive partners given as a list of IDs (received %(pids)s)',
pids=repr(partner_ids),
)
)
# split message additional values from notify additional values
msg_kwargs = {key: val for key, val in kwargs.items() if key in self.env['mail.message']._fields}
notif_kwargs = {key: val for key, val in kwargs.items() if key not in msg_kwargs}
author_id, email_from = self._message_compute_author(author_id, email_from, raise_on_email=True)
# allow to link a notification to a document that does not inherit from
# MailThread by supporting model / res_id
if not (model and res_id): # both value should be set or none should be set (record)
model = False
res_id = False
# MailThread by supporting model / res_id, but then both value should be set
if not model or not res_id:
model, res_id = False, False
msg_values = {
# author
@@ -2196,7 +2241,7 @@ class MailThread(models.AbstractModel):
if 'email_add_signature' not in msg_values:
msg_values['email_add_signature'] = True
new_message = self._message_create(msg_values)
new_message = self._message_create([msg_values])
self._notify_thread(new_message, msg_values, **notif_kwargs)
return new_message
@@ -2204,13 +2249,17 @@ class MailThread(models.AbstractModel):
""" Helper method to log a note using a view_id without notifying followers. """
return self._message_compose_with_view(views_or_xmlid, message_log=True, **kwargs)
def _message_log(self, *, body='', author_id=None, email_from=None, subject=False, message_type='notification', **kwargs):
def _message_log(self, *, body='', author_id=None, email_from=None, subject=False,
message_type='notification',
attachment_ids=False, tracking_value_ids=False):
""" Shortcut allowing to post note on a document. It does not perform
any notification and pre-computes some values to have a short code
as optimized as possible. This method is private as it does not check
access rights and perform the message creation as sudo to speedup
the log process. This method should be called within methods where
access rights are already granted to avoid privilege escalation. """
access rights are already granted to avoid privilege escalation.
:return: created message (as sudo)"""
self.ensure_one()
author_id, email_from = self._message_compute_author(author_id, email_from, raise_on_email=False)
@@ -2219,30 +2268,43 @@ class MailThread(models.AbstractModel):
'author_id': author_id,
'email_from': email_from,
# document
'model': kwargs.get('model', self._name),
'model': self._name,
'record_name': False,
'res_id': self.ids[0] if self.ids else False,
'res_id': self.id,
# content
'attachment_ids': attachment_ids,
'body': body,
'is_internal': True,
'message_type': message_type,
'subject': subject,
'subtype_id': self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note'),
'tracking_value_ids': tracking_value_ids,
# recipients
'email_add_signature': False, # False as no notification -> no need to compute signature
'message_id': tools.generate_tracking_message_id('message-notify'), # why? this is all but a notify
'reply_to': self.env['mail.thread']._notify_get_reply_to(default=email_from)[False],
}
msg_values.update(kwargs)
return self.sudo()._message_create(msg_values)
return self.sudo()._message_create([msg_values])
def _message_log_batch(self, bodies, author_id=None, email_from=None, subject=False, message_type='notification'):
""" Shortcut allowing to post notes on a batch of documents. It achieve the
same purpose as _message_log, done in batch to speedup quick note log.
def _message_log_batch(self, bodies, author_id=None, email_from=None, subject=False,
message_type='notification',
attachment_ids=False, tracking_value_ids=False):
""" Shortcut allowing to post notes on a batch of documents. It does not
perform any notification and pre-computes some values to have a short code
as optimized as possible. This method is private as it does not check
access rights and perform the message creation as sudo to speedup
the log process. This method should be called within methods where
access rights are already granted to avoid privilege escalation.
:param bodies: dict {record_id: body}
:param bodies: dict {record_id: body}
:return: created messages (as sudo)
"""
# protect against side-effect prone usage
if len(self) > 1 and (attachment_ids or tracking_value_ids):
raise ValueError(_('Batch log cannot support attachments or tracking values on more than 1 document'))
author_id, email_from = self._message_compute_author(author_id, email_from, raise_on_email=False)
base_message_values = {
@@ -2253,12 +2315,14 @@ class MailThread(models.AbstractModel):
'model': self._name,
'record_name': False,
# content
'attachment_ids': attachment_ids,
'message_type': message_type,
'is_internal': True,
'subject': subject,
'subtype_id': self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note'),
'tracking_value_ids': tracking_value_ids,
# recipients
'email_add_signature': False,
'email_add_signature': False, # False as no notification -> no need to compute signature
'message_id': tools.generate_tracking_message_id('message-notify'), # why? this is all but a notify
'reply_to': self.env['mail.thread']._notify_get_reply_to(default=email_from)[False],
}
@@ -2337,9 +2401,14 @@ class MailThread(models.AbstractModel):
return self.name_get()[0][1]
def _message_create(self, values_list):
if not isinstance(values_list, (list)):
values_list = [values_list]
create_values_list = []
# preliminary value safety check
self._raise_for_invalid_parameters(
{key for values in values_list for key in values.keys()},
restricting_names=self._get_message_create_valid_field_names()
)
for values in values_list:
create_values = dict(values)
# Avoid warnings about non-existing fields
@@ -2354,6 +2423,76 @@ class MailThread(models.AbstractModel):
clean_context(self.env.context)
).create(create_values_list)
def _get_message_create_valid_field_names(self):
""" Some fields should not be given when creating a mail.message from
mail.thread main API methods (in addition to some API specific check).
Those fields are generally used through UI or dedicated methods. We
therefore give an allowed field names list. """
return {
'attachment_ids',
'author_guest_id',
'author_id',
'body',
'create_date', # anyway limited to admins
'date',
'email_add_signature',
'email_from',
'email_layout_xmlid',
'is_internal',
'mail_activity_type_id',
'mail_server_id',
'message_id',
'message_type',
'model',
'parent_id',
'partner_ids',
'record_name',
'reply_to',
'reply_to_force_new',
'res_id',
'subject',
'subtype_id',
'tracking_value_ids',
}
def _get_notify_valid_parameters(self):
""" Several parameters exist for notification methods as business
flows often want to customize the standard notification experience.
In order to ease coding kwargs are frequently used. This method
acts like a filter, allowing to spot parameters that are not
supported. """
return {
'force_email_company',
'force_email_lang',
'force_send',
'mail_auto_delete',
'model_description',
'resend_existing',
'scheduled_date',
'send_after_commit',
'skip_existing',
'subtitles',
}
def _raise_for_invalid_parameters(self, parameter_names, forbidden_names=None, restricting_names=None):
""" Helper to warn about invalid parameters (or fields).
:param set parameter_names: a set of parameter names;
:param set forbidden_names: set of parameter name that should not be
present in parameter_names;
:param set restricting_names: set of parameters restricting given
parameter_names, parameters not belonging to this list are rejected;
"""
if forbidden_names:
conflicting_names = parameter_names & forbidden_names
elif restricting_names:
conflicting_names = parameter_names - restricting_names
if conflicting_names:
raise ValueError(
_('Those values are not supported when posting or notifying: %(param_names)s',
param_names=', '.join(conflicting_names))
)
# ------------------------------------------------------
# NOTIFICATION API
# ------------------------------------------------------
@@ -2426,6 +2565,10 @@ class MailThread(models.AbstractModel):
"""
# add lang to context immediately since it will be useful in various rendering later
self = self._fallback_lang()
self._raise_for_invalid_parameters(
set(kwargs.keys()),
restricting_names=self._get_notify_valid_parameters()
)
msg_vals = msg_vals if msg_vals else {}
recipients_data = self._notify_get_recipients(message, msg_vals, **kwargs)
addons/mail_group/models/mail_group_message.py
+1
View File
@@ -96,6 +96,7 @@ class MailGroupMessage(models.Model):
field: vals.pop(field)
for field in self.env['mail.message']._fields
if field in vals
and field in self.env['mail.thread']._get_message_create_valid_field_names()
}).id
return super(MailGroupMessage, self).create(values_list)
addons/rating/models/mail_thread.py
-10
View File
@@ -30,13 +30,3 @@ class MailThread(models.AbstractModel):
self.env['rating.rating'].browse(rating_id).write({'message_id': message.id})
return message
def _message_create(self, values_list):
""" Force usage of rating-specific methods and API allowing to delegate
computation to records. Keep methods optimized and skip rating_ids
support to simplify MailThrad main API. """
if not isinstance(values_list, (list)):
values_list = [values_list]
if any(values.get('rating_ids') for values in values_list):
raise ValueError(_("Posting a rating should be done using message post API."))
return super()._message_create(values_list)
addons/sms/models/mail_thread.py
+3
View File
@@ -363,6 +363,9 @@ class MailThread(models.AbstractModel):
return True
def _get_notify_valid_parameters(self):
return super()._get_notify_valid_parameters() | {'put_in_queue', 'sms_numbers', 'sms_pid_to_number'}
@api.model
def notify_cancel_by_type(self, notification_type):
super().notify_cancel_by_type(notification_type)
addons/test_mail/tests/test_mail_thread_internals.py
-1
View File
@@ -361,7 +361,6 @@ class TestDiscuss(TestMailCommon, TestRecipients):
channel = self.env['mail.channel'].create({'name': 'testChannel'})
notification_msg = channel.with_user(self.user_admin).message_notify(
body='test',
message_type='user_notification',
partner_ids=[self.partner_2.id],
)
addons/test_mail/tests/test_message_post.py
+34
View File
@@ -403,6 +403,40 @@ class TestMessageNotify(TestMessagePostCommon):
}
)
@users('employee')
def test_notify_parameters(self):
""" Test usage of parameters in notify, both for unwanted side effects
and magic parameters. """
test_record = self.test_record.with_env(self.env)
for parameters in [
{'message_type': 'comment'},
{'canned_response_ids': []},
{'child_ids': []},
{'mail_ids': []},
{'notification_ids': []},
{'notified_partner_ids': []},
{'reaction_ids': []},
{'starred_partner_ids': []},
]:
with self.subTest(parameters=parameters), \
self.mock_mail_gateway(), \
self.assertRaises(ValueError):
_new_message = test_record.message_notify(
body='<p>You will not receive a notification</p>',
partner_ids=self.partner_1.ids,
subject='This should not be accepted',
**parameters
)
# support of subtype xml id
new_message = test_record.message_notify(
body='<p>You will not receive a notification</p>',
partner_ids=self.partner_1.ids,
subtype_xmlid='mail.mt_note',
)
self.assertEqual(new_message.subtype_id, self.env.ref('mail.mt_note'))
@users('employee')
@mute_logger('odoo.addons.mail.models.mail_mail')
def test_notify_thread(self):