[MERGE] mail, maintenance: prevent re-using alias

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 merge we improve that behavior by ensuring that no duplicate alias
name should be entered while creation / update 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.

TaskID 2160070

closes odoo/odoo#47567

Related: odoo/enterprise#9211
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>

9 files changed, 190 insertions, 141 deletions

diff --git a/addons/mail/models/__init__.py b/addons/mail/models/__init__.py
index 7f45f11aece..871e24ed5ab 100644
--- a/addons/mail/models/__init__.py
+++ b/addons/mail/models/__init__.py
@@ -4,6 +4,7 @@
 from . import mail_message_subtype
 from . import mail_tracking_value
 from . import mail_alias
+from . import mail_alias_mixin
 from . import mail_followers
 from . import mail_notification
 from . import mail_render_mixin
@@ -26,6 +27,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..1c781edff7a 100644
--- a/addons/mail/models/mail_alias.py
+++ b/addons/mail/models/mail_alias.py
@@ -2,15 +2,12 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
 import ast
-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__)
-
 
 class Alias(models.Model):
     """A Mail Alias is a mapping of an email address with a given Odoo Document
@@ -30,7 +27,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 <jobs@example.odoo.com>")
+    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 <jobs@example.odoo.com>")
     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 +89,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 +107,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 +127,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
-
-    @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)
+    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)
+
+        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:
@@ -203,97 +192,3 @@ Only allowed people can contact us at this address.""" % message_dict.get('to'))
             'body': body,
             'message': message_dict
         }, engine='ir.qweb', minimal_qcontext=True)
-
-
-class AliasMixin(models.AbstractModel):
-    """ A mixin for models that inherits mail.alias. This mixin initializes the
-        alias_id column in database, and manages the expected one-to-one
-        relation between your model and mail aliases.
-    """
-    _name = 'mail.alias.mixin'
-    _inherits = {'mail.alias': 'alias_id'}
-    _description = 'Email Aliases Mixin'
-
-    alias_id = fields.Many2one('mail.alias', string='Alias', ondelete="restrict", required=True)
-
-    def get_alias_model_name(self, vals):
-        """ Return the model name for the alias. Incoming emails that are not
-            replies to existing records will cause the creation of a new record
-            of this alias model. The value may depend on ``vals``, the dict of
-            values passed to ``create`` when a record of this model is created.
-        """
-        return None
-
-    def get_alias_values(self):
-        """ Return values to create an alias, or to write on the alias after its
-            creation.
-        """
-        return {'alias_parent_thread_id': self.id}
-
-    @api.model
-    def create(self, vals):
-        """ Create a record with ``vals``, and create a corresponding alias. """
-        record = super(AliasMixin, self.with_context(
-            alias_model_name=self.get_alias_model_name(vals),
-            alias_parent_model_name=self._name,
-        )).create(vals)
-        record.alias_id.sudo().write(record.get_alias_values())
-        return record
-
-    def unlink(self):
-        """ Delete the given records, and cascade-delete their corresponding alias. """
-        aliases = self.mapped('alias_id')
-        res = super(AliasMixin, self).unlink()
-        aliases.unlink()
-        return res
-
-    def _init_column(self, name):
-        """ Create aliases for existing rows. """
-        super(AliasMixin, self)._init_column(name)
-        if name != 'alias_id':
-            return
-
-        # both self and the alias model must be present in 'ir.model'
-        IM = self.env['ir.model']
-        IM._reflect_model(self)
-        IM._reflect_model(self.env[self.get_alias_model_name({})])
-
-        alias_ctx = {
-            'alias_model_name': self.get_alias_model_name({}),
-            'alias_parent_model_name': self._name,
-        }
-        alias_model = self.env['mail.alias'].sudo().with_context(alias_ctx).browse([])
-
-        child_ctx = {
-            'active_test': False,       # retrieve all records
-            'prefetch_fields': False,   # do not prefetch fields on records
-        }
-        child_model = self.sudo().with_context(child_ctx).browse([])
-
-        for record in child_model.search([('alias_id', '=', False)]):
-            # create the alias, and link it to the current record
-            alias = alias_model.create(record.get_alias_values())
-            record.with_context(mail_notrack=True).alias_id = alias
-            _logger.info('Mail alias created for %s %s (id %s)',
-                         record._name, record.display_name, record.id)
-
-    def _alias_check_contact(self, message, message_dict, alias):
-        """ Main mixin method that inheriting models may inherit in order
-        to implement a specifc behavior. """
-        return self._alias_check_contact_on_record(self, message, message_dict, alias)
-
-    def _alias_check_contact_on_record(self, record, message, message_dict, alias):
-        """ Generic method that takes a record not necessarily inheriting from
-        mail.alias.mixin. """
-        author = self.env['res.partner'].browse(message_dict.get('author_id', False))
-        if alias.alias_contact == 'followers':
-            if not record.ids:
-                return _('incorrectly configured alias (unknown reference record)')
-            if not hasattr(record, "message_partner_ids") or not hasattr(record, "message_channel_ids"):
-                return _('incorrectly configured alias')
-            accepted_partner_ids = record.message_partner_ids | record.message_channel_ids.mapped('channel_partner_ids')
-            if not author or author not in accepted_partner_ids:
-                return _('restricted to followers')
-        elif alias.alias_contact == 'partners' and not author:
-            return _('restricted to known authors')
-        return True
diff --git a/addons/mail/models/mail_alias_mixin.py b/addons/mail/models/mail_alias_mixin.py
new file mode 100644
index 00000000000..b8e8091b72e
--- /dev/null
+++ b/addons/mail/models/mail_alias_mixin.py
@@ -0,0 +1,102 @@
+# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+import logging
+
+from odoo import _, api, fields, models
+
+_logger = logging.getLogger(__name__)
+
+
+class AliasMixin(models.AbstractModel):
+    """ A mixin for models that inherits mail.alias. This mixin initializes the
+        alias_id column in database, and manages the expected one-to-one
+        relation between your model and mail aliases.
+    """
+    _name = 'mail.alias.mixin'
+    _inherits = {'mail.alias': 'alias_id'}
+    _description = 'Email Aliases Mixin'
+
+    alias_id = fields.Many2one('mail.alias', string='Alias', ondelete="restrict", required=True)
+
+    def get_alias_model_name(self, vals):
+        """ Return the model name for the alias. Incoming emails that are not
+            replies to existing records will cause the creation of a new record
+            of this alias model. The value may depend on ``vals``, the dict of
+            values passed to ``create`` when a record of this model is created.
+        """
+        return None
+
+    def get_alias_values(self):
+        """ Return values to create an alias, or to write on the alias after its
+            creation.
+        """
+        return {'alias_parent_thread_id': self.id}
+
+    @api.model
+    def create(self, vals):
+        """ Create a record with ``vals``, and create a corresponding alias. """
+        record = super(AliasMixin, self.with_context(
+            alias_model_name=self.get_alias_model_name(vals),
+            alias_parent_model_name=self._name,
+        )).create(vals)
+        record.alias_id.sudo().write(record.get_alias_values())
+        return record
+
+    def unlink(self):
+        """ Delete the given records, and cascade-delete their corresponding alias. """
+        aliases = self.mapped('alias_id')
+        res = super(AliasMixin, self).unlink()
+        aliases.unlink()
+        return res
+
+    def _init_column(self, name):
+        """ Create aliases for existing rows. """
+        super(AliasMixin, self)._init_column(name)
+        if name != 'alias_id':
+            return
+
+        # both self and the alias model must be present in 'ir.model'
+        IM = self.env['ir.model']
+        IM._reflect_model(self)
+        IM._reflect_model(self.env[self.get_alias_model_name({})])
+
+        alias_ctx = {
+            'alias_model_name': self.get_alias_model_name({}),
+            'alias_parent_model_name': self._name,
+        }
+        alias_model = self.env['mail.alias'].sudo().with_context(alias_ctx).browse([])
+
+        child_ctx = {
+            'active_test': False,       # retrieve all records
+            'prefetch_fields': False,   # do not prefetch fields on records
+        }
+        child_model = self.sudo().with_context(child_ctx).browse([])
+
+        for record in child_model.search([('alias_id', '=', False)]):
+            # create the alias, and link it to the current record
+            alias = alias_model.create(record.get_alias_values())
+            record.with_context(mail_notrack=True).alias_id = alias
+            _logger.info('Mail alias created for %s %s (id %s)',
+                         record._name, record.display_name, record.id)
+
+    def _alias_check_contact(self, message, message_dict, alias):
+        """ Main mixin method that inheriting models may inherit in order
+        to implement a specifc behavior. """
+        return self._alias_check_contact_on_record(self, message, message_dict, alias)
+
+    def _alias_check_contact_on_record(self, record, message, message_dict, alias):
+        """ Generic method that takes a record not necessarily inheriting from
+        mail.alias.mixin. """
+        author = self.env['res.partner'].browse(message_dict.get('author_id', False))
+        if alias.alias_contact == 'followers':
+            if not record.ids:
+                return _('incorrectly configured alias (unknown reference record)')
+            if not hasattr(record, "message_partner_ids") or not hasattr(record, "message_channel_ids"):
+                return _('incorrectly configured alias')
+            accepted_partner_ids = record.message_partner_ids | record.message_channel_ids.mapped('channel_partner_ids')
+            if not author or author not in accepted_partner_ids:
+                return _('restricted to followers')
+        elif alias.alias_contact == 'partners' and not author:
+            return _('restricted to known authors')
+        return True
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 @@
     <!-- Equipment categories -->
     <record id="equipment_computer" model="maintenance.equipment.category">
         <field name="name">Computers</field>
-        <field name="alias_id" ref="mail_alias_equipment"/>
     </record>
     <record id="equipment_software" model="maintenance.equipment.category">
         <field name="name">Software</field>
-        <field name="alias_id" ref="mail_alias_equipment"/>
     </record>
     <record id="equipment_printer" model="maintenance.equipment.category">
         <field name="name">Printers</field>
-        <field name="alias_id" ref="mail_alias_equipment"/>
     </record>
     <record id="equipment_monitor" model="maintenance.equipment.category">
         <field name="name">Monitors</field>
         <field name="technician_user_id" ref="base.user_admin"/>
         <field name="color">3</field>
-        <field name="alias_id" ref="mail_alias_equipment"/>
     </record>
     <record id="equipment_phone" model="maintenance.equipment.category">
         <field name="name">Phones</field>
         <field name="technician_user_id" ref="base.user_admin"/>
-        <field name="alias_id" ref="mail_alias_equipment"/>
     </record>
 
     <!-- Equipments -->
diff --git a/addons/maintenance/models/maintenance.py b/addons/maintenance/models/maintenance.py
index c2f67db0412..4b8598796f4 100644
--- a/addons/maintenance/models/maintenance.py
+++ b/addons/maintenance/models/maintenance.py
@@ -64,8 +64,6 @@ class MaintenanceEquipmentCategory(models.Model):
     @api.model
     def create(self, vals):
         self = self.with_context(alias_model_name='maintenance.request', alias_parent_model_name=self._name)
-        if not vals.get('alias_name'):
-            vals['alias_name'] = vals.get('name')
         category_id = super(MaintenanceEquipmentCategory, self).create(vals)
         category_id.alias_id.write({'alias_parent_thread_id': category_id.id, 'alias_defaults': {'category_id': category_id.id}})
         return category_id
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):