From a1b01751b558ac3f85478e3f1ea273f950083894 Mon Sep 17 00:00:00 2001 From: Siddarth Gajjar Date: Tue, 28 Jan 2020 10:45:46 +0000 Subject: [PATCH] [IMP] mail: prevent from re-using aliases PURPOSE Make aliases usage more unique, prevent re-using bounce or catchall aliases and remove auto-uniqueness of aliases. SPECIFICATIONS Before this commit, user could create an email alias having same name as catchall/bounce email alias, which should not happen. Also, while creating or duplicating alias, if the same name was already available, a new unique name was generated by adding a sequence number to existing name. This behavior is not considered as a good one as it magically creates aliases different from what user expects. User could even not own the newly-created alias, leading to a broken mail gateway. In this commit we improve that behavior by ensuring that no duplicate alias name should be entered while creation / updation for both catcall/bounce and mail alias. Also, while duplicating an alias, name will now be blank by default to force user to enter the name. Finally when creating an alias an error is raised if the name is already taken. Task ID 2160070 --- addons/mail/models/__init__.py | 1 + addons/mail/models/ir_config_parameter.py | 20 +++++++ addons/mail/models/mail_alias.py | 54 ++++++++----------- addons/maintenance/data/maintenance_demo.xml | 5 -- addons/maintenance/tests/test_maintenance.py | 3 +- .../tests/test_maintenance_multicompany.py | 6 +-- addons/test_mail/tests/test_mail_gateway.py | 38 +++++++++++++ 7 files changed, 86 insertions(+), 41 deletions(-) create mode 100644 addons/mail/models/ir_config_parameter.py diff --git a/addons/mail/models/__init__.py b/addons/mail/models/__init__.py index 7f45f11aece..0421976df53 100644 --- a/addons/mail/models/__init__.py +++ b/addons/mail/models/__init__.py @@ -26,6 +26,7 @@ from . import ir_action_act_window from . import ir_actions from . import ir_attachment from . import ir_autovacuum +from . import ir_config_parameter from . import ir_http from . import ir_model from . import ir_model_fields diff --git a/addons/mail/models/ir_config_parameter.py b/addons/mail/models/ir_config_parameter.py new file mode 100644 index 00000000000..865d58e7a26 --- /dev/null +++ b/addons/mail/models/ir_config_parameter.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo import api, models + + +class IrConfigParameter(models.Model): + _inherit = 'ir.config_parameter' + + @api.model_create_multi + def create(self, vals_list): + for vals in vals_list: + if vals.get('key') in ['mail.bounce.alias', 'mail.catchall.alias']: + vals['value'] = self.env['mail.alias']._clean_and_check_unique(vals.get('value')) + return super().create(vals_list) + + def write(self, vals): + if 'value' in vals and self.key in ['mail.bounce.alias', 'mail.catchall.alias']: + vals['value'] = self.env['mail.alias']._clean_and_check_unique(vals.get('value')) + return super().write(vals) diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index 43c2350fdb3..e1c4874f7d5 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -6,7 +6,7 @@ import logging import re from odoo import _, api, fields, models -from odoo.exceptions import ValidationError +from odoo.exceptions import ValidationError, UserError from odoo.tools import remove_accents, is_html_empty _logger = logging.getLogger(__name__) @@ -30,7 +30,7 @@ class Alias(models.Model): _rec_name = 'alias_name' _order = 'alias_model_id, alias_name' - alias_name = fields.Char('Alias Name', help="The name of the email alias, e.g. 'jobs' if you want to catch emails for ") + alias_name = fields.Char('Alias Name', copy=False, help="The name of the email alias, e.g. 'jobs' if you want to catch emails for ") alias_model_id = fields.Many2one('ir.model', 'Aliased Model', required=True, ondelete="cascade", help="The model (Odoo Document Kind) to which this alias " "corresponds. Any incoming email that does not reply to an " @@ -92,15 +92,15 @@ class Alias(models.Model): @api.model def create(self, vals): """ Creates an email.alias record according to the values provided in ``vals``, - with 2 alterations: the ``alias_name`` value may be suffixed in order to - make it unique (and certain unsafe characters replaced), and - he ``alias_model_id`` value will set to the model ID of the ``model_name`` - context value, if provided. + with 2 alterations: the ``alias_name`` value may be cleaned by replacing + certain unsafe characters, and the ``alias_model_id`` value will set to the + model ID of the ``model_name`` context value, if provided. Also, it raises + UserError if given alias name is already assigned. """ model_name = self._context.get('alias_model_name') parent_model_name = self._context.get('alias_parent_model_name') if vals.get('alias_name'): - vals['alias_name'] = self._clean_and_make_unique(vals.get('alias_name')) + vals['alias_name'] = self._clean_and_check_unique(vals.get('alias_name')) if model_name: model = self.env['ir.model']._get(model_name) vals['alias_model_id'] = model.id @@ -110,9 +110,9 @@ class Alias(models.Model): return super(Alias, self).create(vals) def write(self, vals): - """"give a unique alias name if given alias name is already assigned""" + """"Raises UserError if given alias name is already assigned""" if vals.get('alias_name') and self.ids: - vals['alias_name'] = self._clean_and_make_unique(vals.get('alias_name'), alias_ids=self.ids) + vals['alias_name'] = self._clean_and_check_unique(vals.get('alias_name')) return super(Alias, self).write(vals) def name_get(self): @@ -130,29 +130,21 @@ class Alias(models.Model): res.append((record['id'], _("Inactive Alias"))) return res - @api.model - def _find_unique(self, name, alias_ids=False): - """Find a unique alias name similar to ``name``. If ``name`` is - already taken, make a variant by adding an integer suffix until - an unused alias is found. - """ - sequence = None - while True: - new_name = "%s%s" % (name, sequence) if sequence is not None else name - domain = [('alias_name', '=', new_name)] - if alias_ids: - domain += [('id', 'not in', alias_ids)] - if not self.search(domain): - break - sequence = (sequence + 1) if sequence else 2 - return new_name + def _clean_and_check_unique(self, name): + """When an alias name appears to already be an email, we keep the local + part only. A sanitizing / cleaning is also performed on the name. If + name already exists an UserError is raised. """ + sanitized_name = remove_accents(name).lower().split('@')[0] + sanitized_name = re.sub(r'[^\w+.]+', '-', sanitized_name) - @api.model - def _clean_and_make_unique(self, name, alias_ids=False): - # when an alias name appears to already be an email, we keep the local part only - name = remove_accents(name).lower().split('@')[0] - name = re.sub(r'[^\w+.]+', '-', name) - return self._find_unique(name, alias_ids=alias_ids) + catchall_alias = self.env['ir.config_parameter'].sudo().get_param('mail.catchall.alias') + bounce_alias = self.env['ir.config_parameter'].sudo().get_param('mail.bounce.alias') + domain = [('alias_name', '=', sanitized_name)] + if self: + domain += [('id', 'not in', self.ids)] + if sanitized_name in [catchall_alias, bounce_alias] or self.search_count(domain): + raise UserError(_('The e-mail alias is already used. Please enter another one.')) + return sanitized_name def open_document(self): if not self.alias_model_id or not self.alias_force_thread_id: diff --git a/addons/maintenance/data/maintenance_demo.xml b/addons/maintenance/data/maintenance_demo.xml index 69e91400fa6..5707b3e6782 100644 --- a/addons/maintenance/data/maintenance_demo.xml +++ b/addons/maintenance/data/maintenance_demo.xml @@ -14,26 +14,21 @@ Computers - Software - Printers - Monitors 3 - Phones - diff --git a/addons/maintenance/tests/test_maintenance.py b/addons/maintenance/tests/test_maintenance.py index e4acceb1767..6cd04bd4192 100644 --- a/addons/maintenance/tests/test_maintenance.py +++ b/addons/maintenance/tests/test_maintenance.py @@ -37,8 +37,7 @@ class TestEquipment(TransactionCase): )) self.equipment_monitor = self.env['maintenance.equipment.category'].create({ - 'name': 'Monitors', - 'alias_id': self.env.ref('maintenance.mail_alias_equipment').id, + 'name': 'Monitors - Test', }) def test_10_equipment_request_category(self): diff --git a/addons/maintenance/tests/test_maintenance_multicompany.py b/addons/maintenance/tests/test_maintenance_multicompany.py index 7c615921f4b..5ec2a6bce71 100644 --- a/addons/maintenance/tests/test_maintenance_multicompany.py +++ b/addons/maintenance/tests/test_maintenance_multicompany.py @@ -78,21 +78,21 @@ class TestEquipmentMulticompany(TransactionCase): # create equipment category for equipment manager category_1 = Category.with_user(equipment_manager).with_context(allowed_company_ids=cids).create({ - 'name': 'Monitors', + 'name': 'Monitors - Test', 'company_id': company_b.id, 'technician_user_id': equipment_manager.id, }) # create equipment category for equipment manager Category.with_user(equipment_manager).with_context(allowed_company_ids=cids).create({ - 'name': 'Computers', + 'name': 'Computers - Test', 'company_id': company_b.id, 'technician_user_id': equipment_manager.id, }) # create equipment category for equipment user Category.with_user(equipment_manager).create({ - 'name': 'Phones', + 'name': 'Phones - Test', 'company_id': company_a.id, 'technician_user_id': equipment_manager.id, }) diff --git a/addons/test_mail/tests/test_mail_gateway.py b/addons/test_mail/tests/test_mail_gateway.py index 5a63940756b..d536d0236ae 100644 --- a/addons/test_mail/tests/test_mail_gateway.py +++ b/addons/test_mail/tests/test_mail_gateway.py @@ -77,6 +77,44 @@ class TestMailAlias(TestMailCommon): alias = self.env['mail.alias'].with_context(alias_model_name='mail.test').create({'alias_name': 'b4r+_#_R3wl$$'}) self.assertEqual(alias.alias_name, 'b4r+_-_r3wl-', 'Disallowed chars should be replaced by hyphens') + def test_alias_name_unique(self): + alias_model_id = self.env['ir.model']._get('mail.test.gateway').id + catchall_alias = self.env['ir.config_parameter'].sudo().get_param('mail.catchall.alias') + bounce_alias = self.env['ir.config_parameter'].sudo().get_param('mail.bounce.alias') + + # test you cannot create aliases matching bounce / catchall + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + self.env['mail.alias'].create({'alias_model_id': alias_model_id, 'alias_name': catchall_alias}) + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + self.env['mail.alias'].create({'alias_model_id': alias_model_id, 'alias_name': bounce_alias}) + + new_mail_alias = self.env['mail.alias'].create({ + 'alias_model_id': alias_model_id, + 'alias_name': 'unused.test.alias' + }) + + # test that re-using catchall and bounce alias raises UserError + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + new_mail_alias.write({ + 'alias_name': catchall_alias + }) + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + new_mail_alias.write({ + 'alias_name': bounce_alias + }) + + new_mail_alias.write({'alias_name': 'another.unused.test.alias'}) + + # test that duplicating an alias should have blank name + copy_new_mail_alias = new_mail_alias.copy() + self.assertFalse(copy_new_mail_alias.alias_name) + + # cannot set catchall / bounce to used alias + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + self.env['ir.config_parameter'].sudo().set_param('mail.catchall.alias', new_mail_alias.alias_name) + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + self.env['ir.config_parameter'].sudo().set_param('mail.bounce.alias', new_mail_alias.alias_name) + @tagged('mail_gateway') class TestMailgateway(TestMailCommon):