[FW][FIX] mail, portal: improve mail/view redirections

Global purpose is to make mail/view controller more resilient by
improving redirections when not having access to the record.

Notably
 * correctly route internal users to discuss, portal users to /my
   and unlogged users to login;
 * keep original mail/view route when performing redirection after
   login so that we try the routing again;
 * fix various issues in redirect computation, such as lost access
   token parameter or partial reconstruction of URLs;

Improve test coverage.

Task-4685166

closes odoo#213889

Forward-port-of: odoo#213750
Forward-port-of: odoo#212545
Signed-off-by: Thibault Delavallee (tde) <[email protected]>

Author: robodoo

addons/crm/controllers/main.py

@@ -4,7 +4,6 @@
 
 from odoo.addons.mail.controllers.mail import MailController
 from odoo import http
-from odoo.http import request
 
 _logger = logging.getLogger(__name__)
 
@@ -19,7 +18,7 @@ def crm_lead_case_mark_won(self, res_id, token):
                 record.action_set_won_rainbowman()
             except Exception:
                 _logger.exception("Could not mark crm.lead as won")
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('crm.lead', res_id)
         return redirect
 
     @http.route('/lead/case_mark_lost', type='http', auth='user', methods=['GET'])
@@ -30,7 +29,7 @@ def crm_lead_case_mark_lost(self, res_id, token):
                 record.action_set_lost()
             except Exception:
                 _logger.exception("Could not mark crm.lead as lost")
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('crm.lead', res_id)
         return redirect
 
     @http.route('/lead/convert', type='http', auth='user', methods=['GET'])
@@ -41,5 +40,5 @@ def crm_lead_convert(self, res_id, token):
                 record.convert_opportunity(record.partner_id)
             except Exception:
                 _logger.exception("Could not convert crm.lead to opportunity")
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('crm.lead', res_id)
         return redirect

addons/hr_holidays/controllers/main.py

@@ -24,7 +24,7 @@ def hr_holidays_request_validate(self, res_id, token):
             try:
                 record.action_validate()
             except Exception:
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('hr.leave', res_id)
         return redirect
 
     @http.route('/leave/refuse', type='http', auth='user', methods=['GET'])
@@ -34,7 +34,7 @@ def hr_holidays_request_refuse(self, res_id, token):
             try:
                 record.action_refuse()
             except Exception:
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('hr.leave', res_id)
         return redirect
 
     @http.route('/allocation/validate', type='http', auth='user', methods=['GET'])
@@ -44,7 +44,7 @@ def hr_holidays_allocation_validate(self, res_id, token):
             try:
                 record.action_approve()
             except Exception:
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('hr.leave.allocation', res_id)
         return redirect
 
     @http.route('/allocation/refuse', type='http', auth='user', methods=['GET'])
@@ -54,5 +54,5 @@ def hr_holidays_allocation_refuse(self, res_id, token):
             try:
                 record.action_refuse()
             except Exception:
-                return MailController._redirect_to_messaging()
+                return MailController._redirect_to_generic_fallback('hr.leave.allocation', res_id)
         return redirect

addons/mail/controllers/mail.py

@@ -19,11 +19,33 @@
 class MailController(http.Controller):
     _cp_path = '/mail'
 
+    @classmethod
+    def _redirect_to_generic_fallback(cls, model, res_id, access_token=None, **kwargs):
+        if request.session.uid is None:
+            return cls._redirect_to_login_with_mail_view(
+                model, res_id, access_token=access_token, **kwargs,
+            )
+        return cls._redirect_to_messaging()
+
     @classmethod
     def _redirect_to_messaging(cls):
         url = '/odoo/action-mail.action_discuss'
         return request.redirect(url)
 
+    @classmethod
+    def _redirect_to_login_with_mail_view(cls, model, res_id, access_token=None, **kwargs):
+        url_base = '/mail/view'
+        url_params = request.env['mail.thread']._get_action_link_params(
+            'view', **{
+                'model': model,
+                'res_id': res_id,
+                'access_token': access_token,
+                **kwargs,
+            }
+        )
+        mail_view_url = f'{url_base}?{url_encode(url_params, sort=True)}'
+        return request.redirect(f'/web/login?{url_encode({"redirect": mail_view_url})}')
+
     @classmethod
     def _check_token(cls, token):
         base_link = request.httprequest.path
@@ -37,12 +59,12 @@ def _check_token_and_record_or_redirect(cls, model, res_id, token):
         comparison = cls._check_token(token)
         if not comparison:
             _logger.warning('Invalid token in route %s', request.httprequest.url)
-            return comparison, None, cls._redirect_to_messaging()
+            return comparison, None, cls._redirect_to_generic_fallback(model, res_id)
         try:
             record = request.env[model].browse(res_id).exists()
         except Exception:
             record = None
-            redirect = cls._redirect_to_messaging()
+            redirect = cls._redirect_to_generic_fallback(model, res_id)
         else:
             redirect = cls._redirect_to_record(model, res_id)
         return comparison, record, redirect
@@ -57,20 +79,26 @@ def _redirect_to_record(cls, model, res_id, access_token=None, **kwargs):
 
         # no model / res_id, meaning no possible record -> redirect to login
         if not model or not res_id or model not in request.env:
-            return cls._redirect_to_messaging()
+            return cls._redirect_to_generic_fallback(
+                model, res_id, access_token=access_token, **kwargs,
+            )
 
         # find the access action using sudo to have the details about the access link
         RecordModel = request.env[model]
         record_sudo = RecordModel.sudo().browse(res_id).exists()
         if not record_sudo:
             # record does not seem to exist -> redirect to login
-            return cls._redirect_to_messaging()
+            return cls._redirect_to_generic_fallback(
+                model, res_id, access_token=access_token, **kwargs,
+            )
 
         suggested_company = record_sudo._get_redirect_suggested_company()
         # the record has a window redirection: check access rights
         if uid is not None:
             if not RecordModel.with_user(uid).has_access('read'):
-                return cls._redirect_to_messaging()
+                return cls._redirect_to_generic_fallback(
+                    model, res_id, access_token=access_token, **kwargs,
+                )
             try:
                 # We need here to extend the "allowed_company_ids" to allow a redirection
                 # to any record that the user can access, regardless of currently visible
@@ -95,36 +123,38 @@ def _redirect_to_record(cls, model, res_id, access_token=None, **kwargs):
                     record_sudo.with_user(uid).with_context(allowed_company_ids=cids).check_access('read')
                     request.future_response.set_cookie('cids', '-'.join([str(cid) for cid in cids]))
             except AccessError:
-                return cls._redirect_to_messaging()
+                return cls._redirect_to_generic_fallback(
+                    model, res_id, access_token=access_token, **kwargs,
+                )
             else:
                 record_action = record_sudo._get_access_action(access_uid=uid)
         else:
             record_action = record_sudo._get_access_action()
-            if suggested_company:
-                cids = [suggested_company.id]
+            # we have an act_url (probably a portal link): we need to retry being logged to check access
             if record_action['type'] == 'ir.actions.act_url' and record_action.get('target_type') != 'public':
-                url_params = {
-                    'model': model,
-                    'id': res_id,
-                    'active_id': res_id,
-                    'action': record_action.get('id'),
-                }
-                if cids:
-                    request.future_response.set_cookie('cids', '-'.join([str(cid) for cid in cids]))
-                view_id = record_sudo.get_formview_id()
-                if view_id:
-                    url_params['view_id'] = view_id
-                url = '/web/login?redirect=#%s' % url_encode(url_params)
-                return request.redirect(url)
+                return cls._redirect_to_login_with_mail_view(
+                    model, res_id, access_token=access_token, **kwargs,
+                )
 
         record_action.pop('target_type', None)
         # the record has an URL redirection: use it directly
         if record_action['type'] == 'ir.actions.act_url':
             return request.redirect(record_action['url'])
-        # other choice: act_window (no support of anything else currently)
-        elif not record_action['type'] == 'ir.actions.act_window':
+        # anything else than an act_window is not supported
+        elif record_action['type'] != 'ir.actions.act_window':
             return cls._redirect_to_messaging()
 
+        # backend act_window: when not logged, unless really readable as public,
+        # user is going to be redirected to login -> keep mail/view as redirect
+        # in that case. In case of readable record, we consider this might be
+        # a customization and we do not change the behavior in stable
+        if uid is None or request.env.user._is_public():
+            has_access = record_sudo.with_user(request.env.user).has_access('read')
+            if not has_access:
+                return cls._redirect_to_login_with_mail_view(
+                    model, res_id, access_token=access_token, **kwargs,
+                )
+
         url_params = {}
         menu_id = request.env['ir.ui.menu']._get_best_backend_root_menu_id_for_model(model)
         if menu_id:
@@ -134,12 +164,12 @@ def _redirect_to_record(cls, model, res_id, access_token=None, **kwargs):
             url_params['view_id'] = view_id
         if cids:
             request.future_response.set_cookie('cids', '-'.join([str(cid) for cid in cids]))
-        
+
         # @see commit c63d14a0485a553b74a8457aee158384e9ae6d3f
         # @see router.js: heuristics to discrimate a model name from an action path
         # is the presence of dots, or the prefix m- for models
         model_in_url = model if "." in model else "m-" + model
-        url = f'/odoo/{model_in_url}/{res_id}?{url_encode(url_params)}'
+        url = f'/odoo/{model_in_url}/{res_id}?{url_encode(url_params, sort=True)}'
         return request.redirect(url)
 
     @http.route('/mail/view', type='http', auth='public')

addons/mail/models/mail_thread.py

@@ -4102,31 +4102,17 @@ def _notify_get_recipients_classify(self, message, recipients_data,
 
     def _notify_get_action_link(self, link_type, **kwargs):
         """ Prepare link to an action: view document, follow document, ... """
-        params = {
-            'model': kwargs.get('model', self._name),
-            'res_id': kwargs.get('res_id', self.ids and self.ids[0] or False),
-        }
-        # keep only accepted parameters:
-        # - action (deprecated), token (assign), access_token (view)
-        # - auth_signup: auth_signup_token and auth_login
-        # - portal: pid, hash
-        params.update(dict(
-            (key, value)
-            for key, value in kwargs.items()
-            if key in ('action', 'token', 'access_token', 'auth_signup_token',
-                       'auth_login', 'pid', 'hash')
-        ))
+        params = self._get_action_link_params(link_type, **kwargs)
 
         if link_type in ['view', 'assign', 'follow', 'unfollow']:
             base_link = '/mail/%s' % link_type
         elif link_type == 'controller':
             controller = kwargs.get('controller')
-            params.pop('model')
             base_link = '%s' % controller
         else:
             return ''
 
-        if link_type not in ['view']:
+        if link_type != 'view':
             token = self._encode_link(base_link, params)
             params['token'] = token
 
@@ -4146,6 +4132,28 @@ def _encode_link(self, base_link, params):
         hm = hmac.new(secret.encode('utf-8'), token.encode('utf-8'), hashlib.sha1).hexdigest()
         return hm
 
+    def _get_action_link_params(self, link_type, **kwargs):
+        """ Parameters management for '_notify_get_action_link' """
+        params = {
+            'model': kwargs.get('model', self._name),
+            'res_id': kwargs.get('res_id', self.ids[0] if self else False),
+        }
+        # keep only accepted parameters:
+        # - action (deprecated), token (assign), access_token (view)
+        # - auth_signup: auth_signup_token and auth_login
+        # - portal: pid, hash
+        params.update({
+            key: value
+            for key, value in kwargs.items()
+            if key in ('action', 'token', 'access_token', 'auth_signup_token',
+                       'auth_login', 'pid', 'hash')
+        })
+        if link_type == 'controller':
+            params.pop('model')
+        elif link_type not in ['view', 'assign', 'follow', 'unfollow']:
+            return {}
+        return params
+
     @api.model
     def _extract_model_and_id(self, msg_vals):
         """

addons/mail/tests/common.py

@@ -1020,7 +1020,6 @@ def _create_template(cls, model, template_values=None):
         cls.email_template = cls.env['mail.template'].create(create_values)
         return cls.email_template
 
-
     def _generate_notify_recipients(self, partners, record=None):
         """ Tool method to generate recipients data according to structure used
         in notification methods. Purpose is to allow testing of internals of
@@ -1041,6 +1040,29 @@ def _generate_notify_recipients(self, partners, record=None):
             } for partner in partners
         ]
 
+    def _get_mail_composer_web_context(self, records, add_web=True, **values):
+        """ Helper to generate composer context. Will make tests a bit less
+        verbose.
+
+        :param bool add_web: add web context, generally making noise especially in
+          mass mail mode (active_id/ids both present in context)
+        """
+        base_context = {
+            'default_model': records._name,
+            'default_res_ids': records.ids,
+        }
+        if len(records) == 1:
+            base_context['default_composition_mode'] = 'comment'
+        else:
+            base_context['default_composition_mode'] = 'mass_mail'
+        if add_web:
+            base_context['active_model'] = records._name
+            base_context['active_id'] = records[0].id
+            base_context['active_ids'] = records.ids
+        if values:
+            base_context.update(**values)
+        return base_context
+
     # ------------------------------------------------------------
     # MAIL ASSERTS WRAPPERS
     # ------------------------------------------------------------

addons/portal/controllers/mail.py

@@ -121,6 +121,13 @@ def portal_message_update_is_internal(self, message_id, is_internal):
 
 class MailController(mail.MailController):
 
+    @classmethod
+    def _redirect_to_generic_fallback(cls, model, res_id, access_token=None, **kwargs):
+        # Generic fallback for a share user is the customer portal
+        if request.session.uid and request.env.user.share:
+            return request.redirect('/my')
+        return super()._redirect_to_generic_fallback(model, res_id, access_token=access_token, **kwargs)
+
     @classmethod
     def _redirect_to_record(cls, model, res_id, access_token=None, **kwargs):
         """ If the current user doesn't have access to the document, but provided
@@ -156,7 +163,7 @@ def _redirect_to_record(cls, model, res_id, access_token=None, **kwargs):
                             url = urls.url_parse(url)
                             url_params = url.decode_query()
                             url_params.update([("pid", pid), ("hash", hash)])
-                            url = url.replace(query=urls.url_encode(url_params)).to_url()
+                            url = url.replace(query=urls.url_encode(url_params, sort=True)).to_url()
                         return request.redirect(url)
         return super(MailController, cls)._redirect_to_record(model, res_id, access_token=access_token, **kwargs)
 

addons/test_mail/tests/test_mail_composer.py

@@ -118,21 +118,7 @@ def _get_web_context(self, records, add_web=True, **values):
         :param add_web: add web context, generally making noise especially in
           mass mail mode (active_id/ids both present in context)
         """
-        base_context = {
-            'default_model': records._name,
-            'default_res_ids': records.ids,
-        }
-        if len(records) == 1:
-            base_context['default_composition_mode'] = 'comment'
-        else:
-            base_context['default_composition_mode'] = 'mass_mail'
-        if add_web:
-            base_context['active_model'] = records._name
-            base_context['active_id'] = records[0].id
-            base_context['active_ids'] = records.ids
-        if values:
-            base_context.update(**values)
-        return base_context
+        return self._get_mail_composer_web_context(records, add_web=add_web, **values)
 
 
 @tagged('mail_composer')