From 84e2d6ea556ea3093360fe053952e53c44fda4d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 10 Oct 2023 14:40:21 +0200 Subject: [PATCH] [REF] mail: remove 'alias_user_id' 'alias_user_id' field allows to set a user when creating records through the mail gateway. This is however quite wrong and eases spoofing. The alias owner is not the creator of any record, nor responsible. Current possible ways of being owner / responsible of records created through the mailgateway * when you send an email to an alias: if you are recognized you are already set as creating user and logged message author; * it is possible to use alias_defaults notably to set fields like 'user_id' if you want to be notified / responsible of records created through this specific alias; Those usages are therefore sufficient, no need to have another way to spoof users. Moreover it is hidden in technical view of aliases, no model allows to configure it by default. Moreover since odoo/odoo@3edf181 no default value is given to alias_user_id as it adds more (ACLs / creator) issues than really helping setting up mail gateway flows. Task-3453482 Prepares Task-36879 (Mail: Multi-Domain Aliases) closes odoo/odoo#138213 Related: odoo/upgrade#5259 Related: odoo/enterprise#48692 Signed-off-by: Thibault Delavallee (tde) --- addons/crm/data/crm_team_data.xml | 1 - addons/crm/models/crm_lead.py | 4 +- addons/crm/models/crm_team.py | 4 - addons/hr_expense/data/mail_alias_data.xml | 1 - addons/hr_recruitment/models/hr_job.py | 2 - addons/mail/models/mail_alias.py | 91 +++++++++++-------- addons/mail/models/mail_thread.py | 19 +--- addons/mail/views/mail_alias_views.xml | 7 +- addons/test_mail/tests/test_mail_gateway.py | 70 +++++--------- addons/test_mail/tests/test_mail_message.py | 1 - .../test_mail/tests/test_mail_multicompany.py | 1 - addons/test_mail/tests/test_message_track.py | 1 - addons/test_mass_mailing/tests/common.py | 1 - 13 files changed, 82 insertions(+), 121 deletions(-) diff --git a/addons/crm/data/crm_team_data.xml b/addons/crm/data/crm_team_data.xml index 855a62ca636..1dda51401e1 100644 --- a/addons/crm/data/crm_team_data.xml +++ b/addons/crm/data/crm_team_data.xml @@ -2,7 +2,6 @@ info - diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 5f148e4d630..c7276a4a06f 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -2016,8 +2016,8 @@ class Lead(models.Model): # remove default author when going through the mail gateway. Indeed we # do not want to explicitly set an user as responsible. We prefer that # assignment is done automatically (scoring) or manually. Otherwise it - # would always be either root (gateway user) either alias owner (through - # alias_user_id). It also allows to exclude portal / public users. + # would always be root (gateway user). It also allows to exclude portal + # and public users. self = self.with_context(default_user_id=False) if custom_values is None: diff --git a/addons/crm/models/crm_team.py b/addons/crm/models/crm_team.py index 7a8c483e685..9761f3d6c19 100644 --- a/addons/crm/models/crm_team.py +++ b/addons/crm/models/crm_team.py @@ -52,10 +52,6 @@ class Team(models.Model): string='# Overdue Opportunities', compute='_compute_opportunities_overdue_data') opportunities_overdue_amount = fields.Monetary( string='Overdue Opportunities Revenues', compute='_compute_opportunities_overdue_data',) - # alias: improve fields coming from _inherits, use inherited to avoid replacing them - alias_user_id = fields.Many2one( - 'res.users', related='alias_id.alias_user_id', readonly=False, inherited=True, - domain=lambda self: [('groups_id', 'in', self.env.ref('sales_team.group_sale_salesman_all_leads').id)]) # properties lead_properties_definition = fields.PropertiesDefinition('Lead Properties') diff --git a/addons/hr_expense/data/mail_alias_data.xml b/addons/hr_expense/data/mail_alias_data.xml index 7c8470c5cb5..550db7bcb3f 100644 --- a/addons/hr_expense/data/mail_alias_data.xml +++ b/addons/hr_expense/data/mail_alias_data.xml @@ -5,7 +5,6 @@ expense - employees diff --git a/addons/hr_recruitment/models/hr_job.py b/addons/hr_recruitment/models/hr_job.py index 82826671d43..b169b71bb07 100644 --- a/addons/hr_recruitment/models/hr_job.py +++ b/addons/hr_recruitment/models/hr_job.py @@ -221,8 +221,6 @@ class Job(models.Model): def create(self, vals_list): for vals in vals_list: vals['favorite_user_ids'] = vals.get('favorite_user_ids', []) + [(4, self.env.uid)] - if vals.get('alias_name'): - vals['alias_user_id'] = False jobs = super().create(vals_list) utm_linkedin = self.env.ref("utm.utm_source_linkedin", raise_if_not_found=False) if utm_linkedin: diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index c873cfee3a9..bfed641636e 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -47,11 +47,6 @@ class Alias(models.Model): # hack to only allow selecting mail_thread models (we might # (have a few false positives, though) domain="[('field_id.name', '=', 'message_ids')]") - alias_user_id = fields.Many2one('res.users', 'Owner', - help="The owner of records created upon receiving emails on this alias. " - "If this field is not set the system will attempt to find the right owner " - "based on the sender (From) address, or will use the Administrator account " - "if no system user is found for that address.") alias_defaults = fields.Text('Default Values', required=True, default='{}', help="A Python dictionary that will be evaluated to provide " "default values when creating new records for this alias.") @@ -306,7 +301,35 @@ class Alias(models.Model): # MAIL GATEWAY # ------------------------------------------------------------ + def _get_alias_bounced_body(self, message_dict): + """Get the body of the email return in case of bounced email when the + alias does not accept incoming email e.g. contact is not allowed. + + :param dict message_dict: dictionary holding parsed message variables + + :return: HTML to use as email body + """ + lang_author = False + if message_dict.get('author_id'): + try: + lang_author = self.env['res.partner'].browse(message_dict['author_id']).lang + except Exception: + pass + + if lang_author: + self = self.with_context(lang=lang_author) + + if not is_html_empty(self.alias_bounced_content): + body = self.alias_bounced_content + else: + body = self._get_alias_bounced_body_fallback(message_dict) + return self.env['ir.qweb']._render('mail.mail_bounce_alias_security', { + 'body': body, + 'message': message_dict + }, minimal_qcontext=True) + def _get_alias_bounced_body_fallback(self, message_dict): + """ Default body of bounced emails. See '_get_alias_bounced_body' """ contact_description = self._get_alias_contact_description() default_email = self.env.company.partner_id.email_formatted if self.env.company.partner_id.email else self.env.company.name content = Markup( @@ -331,9 +354,12 @@ class Alias(models.Model): return _('some specific addresses') def _get_alias_invalid_body(self, message_dict): - """Get the body of the bounced email returned when the alias is misconfigured (ex.: error in alias_defaults). + """Get the body of the bounced email returned when the alias is incorrectly + configured e.g. error in alias_defaults. - :param message_dict: dictionary of mail values + :param dict message_dict: dictionary holding parsed message variables + + :return: HTML to use as email body """ content = Markup( _("""The message below could not be accepted by the address %(alias_display_name)s. @@ -352,44 +378,29 @@ Please try again later or contact %(company_name)s instead.""" 'message': message_dict }, minimal_qcontext=True) - def _get_alias_bounced_body(self, message_dict): - """Get the body of the email return in case of bounced email. - - :param message_dict: dictionary of mail values - """ - lang_author = False - if message_dict.get('author_id'): - try: - lang_author = self.env['res.partner'].browse(message_dict['author_id']).lang - except: - pass - - if lang_author: - self = self.with_context(lang=lang_author) - - if not is_html_empty(self.alias_bounced_content): - body = self.alias_bounced_content - else: - body = self._get_alias_bounced_body_fallback(message_dict) - return self.env['ir.qweb']._render('mail.mail_bounce_alias_security', { - 'body': body, - 'message': message_dict - }, minimal_qcontext=True) - - def _set_alias_invalid(self, message, message_dict): + def _alias_bounce_incoming_email(self, message, message_dict, set_invalid=True): """Set alias status to invalid and create bounce message to the sender and the alias responsible. This method must be called when a message received on the alias has caused an error due to the mis-configuration of the alias. - :param EmailMessage message: email message that has caused the error - :param dict message_dict: dictionary of mail values + :param EmailMessage message: email message that is invalid and is about + to bounce; + :param dict message_dict: dictionary holding parsed message variables + :param bool set_invalid: set alias as invalid, to be done notably if + bounce is considered as coming from a configuration error instead of + being rejected due to alias rules; """ self.ensure_one() - self.alias_status = 'invalid' - body = self._get_alias_invalid_body(message_dict) - self.env['mail.thread']._routing_create_bounce_email(message_dict['email_from'], body, message, - references=message_dict['message_id'], - # add the alias responsible as recipient if set - recipient_ids=self.alias_user_id.partner_id.ids) + if set_invalid: + self.alias_status = 'invalid' + body = self._get_alias_invalid_body(message_dict) + else: + body = self._get_alias_bounced_body(message_dict) + self.env['mail.thread']._routing_create_bounce_email( + message_dict['email_from'], body, message, + references=message_dict['message_id'], + # add the alias creator as recipient if set + recipient_ids=self.create_uid.partner_id.ids if self.create_uid.active else [], + ) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 03df01d7b0c..c0a3bc396df 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -803,7 +803,7 @@ class MailThread(models.AbstractModel): if not model: self._routing_warn(_('target model unspecified'), message_id, route, raise_exception) return () - elif model not in self.env: + if model not in self.env: self._routing_warn(_('unknown target model %s', model), message_id, route, raise_exception) return () record_set = self.env[model].browse(thread_id) if thread_id else self.env[model] @@ -855,11 +855,7 @@ class MailThread(models.AbstractModel): route, False ) - if error.is_config_error: - alias._set_alias_invalid(message, message_dict) - else: - body = alias._get_alias_bounced_body(message_dict) - self._routing_create_bounce_email(email_from, body, message, references=message_id) + alias._alias_bounce_incoming_email(message, message_dict, set_invalid=error.is_config_error) return False return (model, thread_id, route[2], route[3], route[4]) @@ -1165,8 +1161,8 @@ class MailThread(models.AbstractModel): except Exception: if alias: with self.pool.cursor() as new_cr: - self.with_env(self.env(cr=new_cr)).env['mail.alias'].browse(alias.id) \ - ._set_alias_invalid(message, message_dict) + self.with_env(self.env(cr=new_cr)).env['mail.alias'].browse(alias.id + )._alias_bounce_incoming_email(message, message_dict, set_invalid=True) raise else: if alias and alias.alias_status != 'valid': @@ -1845,13 +1841,6 @@ class MailThread(models.AbstractModel): if not matching_user: std_users = self.env['res.users'].sudo().search([('email_normalized', '=', normalized_email)], limit=1) matching_user = std_users[0] if std_users else self.env['res.users'] - if matching_user: - return matching_user - - if not matching_user and alias and alias.alias_user_id: - matching_user = alias and alias.alias_user_id - if matching_user: - return matching_user return matching_user diff --git a/addons/mail/views/mail_alias_views.xml b/addons/mail/views/mail_alias_views.xml index d4295579d91..d7572778253 100644 --- a/addons/mail/views/mail_alias_views.xml +++ b/addons/mail/views/mail_alias_views.xml @@ -38,7 +38,6 @@ - @@ -58,7 +57,6 @@ - @@ -85,14 +83,15 @@ + - + ', - [alias_valid.alias_user_id.partner_id.email_formatted], + [self.user_admin.email_formatted], subject='Re: Invalid') # Not sent to self.email_from because a return path is present in MAIL_TEMPLATE self.assertSentEmail(f'"MAILER-DAEMON" <{self.alias_bounce}@{self.alias_domain}>', @@ -568,49 +567,34 @@ class TestMailgateway(MailCommon): body=alias_valid._get_alias_invalid_body(message_dict)) @mute_logger('odoo.addons.mail.models.mail_thread', 'odoo.models') - @patch('odoo.addons.mail.models.mail_alias.Alias._set_alias_invalid') - def test_message_process_alias_defaults(self, _set_alias_invalid): + def test_message_process_alias_defaults(self): """ Test alias defaults and inner values """ self.alias.write({ - 'alias_user_id': self.user_employee.id, 'alias_defaults': "{'custom_field': 'defaults_custom'}" }) - self.assertEqual(self.alias.alias_status, 'not_tested') - record = self.format_and_process(MAIL_TEMPLATE, self.email_from, f'groups@{self.alias_domain}', subject='Specific') - - _set_alias_invalid.assert_not_called() + record = self.format_and_process( + MAIL_TEMPLATE, self.email_from, f'groups@{self.alias_domain}', + subject='Specific' + ) self.assertEqual(self.alias.alias_status, 'valid') self.assertEqual(len(record), 1) - res = record.get_metadata()[0].get('create_uid') or [None] - self.assertEqual(res[0], self.user_employee.id) self.assertEqual(record.name, 'Specific') self.assertEqual(record.custom_field, 'defaults_custom') self.alias.write({'alias_defaults': '""'}) self.assertEqual(self.alias.alias_status, 'not_tested', 'Updating alias_defaults must reset status') - record = self.format_and_process(MAIL_TEMPLATE, self.email_from, f'groups@{self.alias_domain}', subject='Specific2') - _set_alias_invalid.assert_not_called() + record = self.format_and_process( + MAIL_TEMPLATE, self.email_from, f'groups@{self.alias_domain}', + subject='Specific2' + ) self.assertEqual(len(record), 1) - res = record.get_metadata()[0].get('create_uid') or [None] - self.assertEqual(res[0], self.user_employee.id) self.assertEqual(record.name, 'Specific2') self.assertFalse(record.custom_field) - self.assertEqual(self.alias.alias_status, 'valid') - @mute_logger('odoo.addons.mail.models.mail_thread', 'odoo.models') - def test_message_process_alias_user_id(self): - """ Test alias ownership """ - self.alias.write({'alias_user_id': self.user_employee.id}) - - record = self.format_and_process(MAIL_TEMPLATE, self.email_from, f'groups@{self.alias_domain}') - self.assertEqual(len(record), 1) - res = record.get_metadata()[0].get('create_uid') or [None] - self.assertEqual(res[0], self.user_employee.id) - @mute_logger('odoo.addons.mail.models.mail_thread', 'odoo.models') def test_message_process_alias_everyone(self): """ Incoming email: everyone: new record + message_new """ @@ -825,7 +809,6 @@ class TestMailgateway(MailCommon): # test@.. will cause the creation of new mail.test new_alias_2 = self.env['mail.alias'].create({ 'alias_name': 'test', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -852,7 +835,6 @@ class TestMailgateway(MailCommon): # test@.. will cause the creation of new mail.test new_alias_2 = self.env['mail.alias'].create({ 'alias_name': 'test', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -882,7 +864,6 @@ class TestMailgateway(MailCommon): # test@.. will cause the creation of new mail.test new_alias_2 = self.env['mail.alias'].create({ 'alias_name': 'test', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -908,7 +889,6 @@ class TestMailgateway(MailCommon): # test@.. will cause the creation of new mail.test new_alias_2 = self.env['mail.alias'].create({ 'alias_name': 'test', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -934,7 +914,6 @@ class TestMailgateway(MailCommon): 'alias_contact': 'everyone', 'alias_model_id': self.env['ir.model']._get_id('mail.test.container.mc'), 'alias_name': 'test', - 'alias_user_id': False, }) test_domain = 'hello.com' @@ -1460,7 +1439,6 @@ class TestMailgateway(MailCommon): """ Incoming email using references but with alias forward should not go into references destination """ self.env['mail.alias'].create({ 'alias_name': 'test.alias', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -1481,7 +1459,6 @@ class TestMailgateway(MailCommon): """ Incoming email using references but with alias forward on same model should be considered as a reply """ self.env['mail.alias'].create({ 'alias_name': 'test.alias', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.gateway').id, 'alias_contact': 'everyone', }) @@ -1501,7 +1478,6 @@ class TestMailgateway(MailCommon): """ Incoming email using references but with alias forward in CC should be considered as a repy (To > Cc) """ self.env['mail.alias'].create({ 'alias_name': 'test.alias', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -1724,7 +1700,6 @@ class TestMailgateway(MailCommon): alias = self.env['mail.alias'].create({ 'alias_name': 'test', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, 'alias_contact': 'everyone', }) @@ -1880,7 +1855,6 @@ class TestMailThreadCC(MailCommon): 'alias_contact': 'everyone', 'alias_model_id': cls.env['ir.model']._get('mail.test.cc').id, 'alias_name': 'cc_record', - 'alias_user_id': False, }) @mute_logger('odoo.addons.mail.models.mail_thread') diff --git a/addons/test_mail/tests/test_mail_message.py b/addons/test_mail/tests/test_mail_message.py index 005976f4d74..2389ba305dd 100644 --- a/addons/test_mail/tests/test_mail_message.py +++ b/addons/test_mail/tests/test_mail_message.py @@ -294,7 +294,6 @@ class TestMessageValues(MailCommon): test_record = self.env['mail.test.simple'].create({'name': 'Test', 'email_from': 'ignasse@example.com'}) alias = self.env['mail.alias'].create({ 'alias_name': 'MegaLias', - 'alias_user_id': False, 'alias_model_id': self.env['ir.model']._get('mail.test.simple').id, 'alias_parent_model_id': self.env['ir.model']._get('mail.test.simple').id, 'alias_parent_thread_id': test_record.id, diff --git a/addons/test_mail/tests/test_mail_multicompany.py b/addons/test_mail/tests/test_mail_multicompany.py index 09783424a26..32702e2f0fd 100644 --- a/addons/test_mail/tests/test_mail_multicompany.py +++ b/addons/test_mail/tests/test_mail_multicompany.py @@ -45,7 +45,6 @@ class TestMailMCCommon(MailCommon, TestRecipients): 'alias_contact': 'everyone', 'alias_model_id': cls.test_model.id, 'alias_name': 'groups', - 'alias_user_id': False, }) # Set a first message on public group to test update and hierarchy diff --git a/addons/test_mail/tests/test_message_track.py b/addons/test_mail/tests/test_message_track.py index 90c38a94afc..f1ecbef7010 100644 --- a/addons/test_mail/tests/test_message_track.py +++ b/addons/test_mail/tests/test_message_track.py @@ -236,7 +236,6 @@ class TestTracking(MailCommon): 'mail_template': self.env.ref('test_mail.mail_test_ticket_tracking_tpl').id} self.env['mail.alias'].create({ 'alias_name': 'groups', - 'alias_user_id': False, 'alias_model_id': test_model.id, 'alias_contact': 'everyone', 'alias_defaults': custom_values}) diff --git a/addons/test_mass_mailing/tests/common.py b/addons/test_mass_mailing/tests/common.py index 1fb131c32ae..99c2727a544 100644 --- a/addons/test_mass_mailing/tests/common.py +++ b/addons/test_mass_mailing/tests/common.py @@ -14,7 +14,6 @@ class TestMassMailCommon(MassSMSCommon): cls.test_alias = cls.env['mail.alias'].create({ 'alias_name': 'test.alias', - 'alias_user_id': False, 'alias_model_id': cls.env['ir.model']._get('mailing.test.simple').id, 'alias_contact': 'everyone' })