Commit c484f8aa authored by Georgios Dagkakis's avatar Georgios Dagkakis

erp5_xhtml_style Base_cancel and logged_in: do not allow redirection outside ERP5 site.

parent 2677f46c
...@@ -52,6 +52,12 @@ ...@@ -52,6 +52,12 @@
<key> <string>_body</string> </key> <key> <string>_body</string> </key>
<value> <string encoding="cdata"><![CDATA[ <value> <string encoding="cdata"><![CDATA[
topmost_url_document = context.Base_getURLTopmostDocumentValue()\n
if not topmost_url_document.isURLAncestorOf(cancel_url):\n
return context.ERP5Site_redirect(topmost_url_document.absolute_url(),\n
keep_items={\'portal_status_message\': \'Redirection to an external site prevented.\'},\n
**kw)\n
\n
if \'?selection_name=\' in cancel_url or \'&selection_name=\' in cancel_url:\n if \'?selection_name=\' in cancel_url or \'&selection_name=\' in cancel_url:\n
# if selection_name is already present in the cancel URL, we do not\n # if selection_name is already present in the cancel URL, we do not\n
# use erp5_xhtml_style script that would add it again.\n # use erp5_xhtml_style script that would add it again.\n
......
...@@ -64,6 +64,10 @@ if portal.portal_membership.isAnonymousUser():\n ...@@ -64,6 +64,10 @@ if portal.portal_membership.isAnonymousUser():\n
+ ("&amp;came_from=" + url if url else ""))\n + ("&amp;came_from=" + url if url else ""))\n
elif not url:\n elif not url:\n
url = context.absolute_url()\n url = context.absolute_url()\n
topmost_url_document = context.Base_getURLTopmostDocumentValue()\n
if not topmost_url_document.isURLAncestorOf(url):\n
return context.ERP5Site_redirect(topmost_url_document.absolute_url(),\n
keep_items={\'portal_status_message\': \'Redirection to an external site prevented.\'})\n
return RESPONSE.redirect(url)\n return RESPONSE.redirect(url)\n
......
  • mentioned in merge request !89 (merged)

    Toggle commit list
  • @georgios.dagkakis in most cases, we use erp5_authentication_policy which comes with it's own logged_in script and we don't have such a check in erp5_authentication_policy. Is it OK to add such a check in erp5_authentication_policy as well ?

    /cc @gabriel

  • Yes, I think it would be good to add it in the logged_in page template of erp5_authentication_policy. We would care for this only in the cases of successful login, right?

    Something like the below should be enough maybe?

        ...
        <tal:block tal:condition="not: isAnon"
                   tal:define="is_user_account_password_expired_expire_date python:request.get('is_user_account_password_expired_expire_date', 0);
                               topmost_url_document python: here.Base_getURLTopmostDocumentValue();
                               came_from python: request.get('came_from');
                               came_from python: came_from if topmost_url_document.isURLAncestorOf(came_from) else None;
                               ...
  • Thanks for feedback. Probably we want to check redirections also for all cases. I'm looking at this now and I'm really wondering why this is implemented as a page template. Probably the extra logic of erp5_authentication_policy should be merged in erp5_xhtml_style logged_in (and ERP5JS equivalent)

  • mentioned in commit c0db5542

    Toggle commit list
  • mentioned in merge request !1330 (merged)

    Toggle commit list
  • I'm looking at this now and I'm really wondering why this is implemented as a page template.

    +1, I do not really understand either

    Probably the extra logic of erp5_authentication_policy should be merged in erp5_xhtml_style logged_in (and ERP5JS equivalent)

    Not sure myself, it is the dilemma between:

    • Override code in specific higher level BT
    • Add code to lower level BT, that normally would not be used, unless higher level BT is installed (otherwise this code should be dead code)

    I really am not sure which is the best approach in this case

  • There are other places where erp5_authentication_policy is implemented "outside of erp5_authentication_policy", for example the authentication system (lower level code) calls login.isLoginBlocked or login.isPasswordExpired that have a default "do nothing" implementation in lower level code but are implemented in erp5_authentication_policy, so I thought also handling logged_in with the "add code to lower level BT" approach was OK here (that's the approach taken in !1330 (merged) )

    The "override in higher level BT" is sometimes better, but it was not really good in this case, because we implemented this by duplicating code from lower level BT and when we improved the lower level BT we did not update the higher level.

  • There are other places where erp5_authentication_policy is implemented "outside of erp5_authentication_policy", for example ...

    ok, then indeed it makes sense to use same approach here

    because we implemented this by duplicating code from lower level BT and when we improved the lower level BT we did not update the higher level

    happens a lot. In theory we could override only the parts we want and use skinSuper. But this is not always possible

  • mentioned in commit b9097101

    Toggle commit list
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment