Products.cmfplone: Template customization in portal_view_customizations breaks login

[davilima6]

Tested on 5.0.4. Steps to reproduce:

1. Go to ZMI > portal_view_customizations.
2. Locate the "search" template (I tried with the one from Products.CMFPlone, not plone.app.portlets) and customize it.
3. Try to login from another browser.

After posting the login form (via overlay or in-page at /login), I get redirected to homepage. Nothing in the error_log, even if I clear Ignored exception types. Besides if I go to /search I get a login page because of Insufficient Privileges, failing with:

Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module five.customerize.zpt, line 83, in __call__
  Module Products.PageTemplates.ZopePageTemplate, line 338, in _exec
  Module Products.PageTemplates.ZopePageTemplate, line 435, in pt_render
  Module Products.PageTemplates.PageTemplate, line 87, in pt_render
  Module zope.pagetemplate.pagetemplate, line 132, in pt_render
  Module five.pt.engine, line 93, in __call__
  Module z3c.pt.pagetemplate, line 163, in render
  Module chameleon.zpt.template, line 261, in render
  Module chameleon.template, line 191, in render
  Module chameleon.template, line 171, in render
  Module fc98a57cfc93f51888d55f04a5915d09.py, line 3329, in render
  Module 9b4dbc68d707d8294cd5b2eeaf8d4141.py, line 1223, in render_master
  Module 9b4dbc68d707d8294cd5b2eeaf8d4141.py, line 458, in render_content
  Module fc98a57cfc93f51888d55f04a5915d09.py, line 3232, in __fill_main
  Module fc98a57cfc93f51888d55f04a5915d09.py, line 381, in render_search_results
  Module fc98a57cfc93f51888d55f04a5915d09.py, line 189, in render_sorting
Unauthorized: You are not allowed to access 'sortkey' in this context

 - Expression: "python:item.sortkey and item.sortkey or None"
 - Filename:   ... .app.layout.navigation.interfaces.inavigationroot-search
 - Location:   (line 207: col 56)
 - Arguments:  repeat: {...} (0)
               template: <ImplicitAcquisitionWrapper plone.app.layout.navigation.interfaces.inavigationroot-search at 0x7f5ae15da820>
               modules: <instance - at 0xfd6ab8>
               here: <ImplicitAcquisitionWrapper plone5 at 0x7f5ad94386e0>
               user: <ImplicitAcquisitionWrapper - at 0x7f5ae15da9b0>
               nothing: <NoneType - at 0x7f5b00ff1f00>
               target_language: en
               container: <ImplicitAcquisitionWrapper portal_view_customizations at 0x7f5ad10265f0>
               default: <object - at 0x7f5b011bb520>
               request: <instance - at 0x7f5ad102a5f0>
               wrapped_repeat: <SafeMapping - at 0x7f5ad0aa1d60>
               loop: {...} (3)
               context: <ImplicitAcquisitionWrapper plone5 at 0x7f5ad94386e0>
               translate: <function translate at 0x7f5ada3f86e0>
               root: <Application Zope at 0x8870c80>
               options: {...} (1)
               view: <TTWView None at 0x7f5ada3eb090>

If I delete the customization, login is possible again.

Finally, if I fail login I do see an error portal status message.

[tkimnguyen]

This is the usual problem you run into when you customize a viewlet TTW, because Python code in TTW customized viewlets runs in restricted Python.

[davilima6]

From @tkimnguyen on IRC: "sometimes even if the template calls something that breaks under restricted python you can "fix" that by hardcoding things or removing expressions and tags you don't really need".

So I checked and with rare exceptions such as accessibilty and colophon, the majority of objects in portal_view_customizations rely on python: expressions. We should either document this and put a warning on ZMI or I think we'd be better off deprecating/removing portal_view_customizations and focusing at integrating collective.jbot into core or just improving it (e.g. make it work w/ P5).

Note this is a regression from Plone 4.3.9 where we could customize search template even though it had lots of python: expressions in it.

[mauritsvanrees]

The problem is not the python: expression itself. The problem is what the expression contains. Try it: customize the search template, throw everything away except the html boilerplate, and add something like: <p tal:content="python:6*7" />.

The problem usually is that the template accesses a method or property on the view that is not in allowed_attributes or allowed_interface in zcml. I think I have changed a template or zcml in core code once or twice explicitly to make it customizable.

Technically, browser view templates are Zope 3 templates, and templates in portal_view_customization are Zope 2 templates. there is a different security model underlying the two implementations.

[tkimnguyen]

Maybe a warning could appear if the template contains "python:" ?

[davilima6]

We could try to render the template in background and only display the warning if Zope raises any Unauthorized errors. However what is allowed in one context may not be on another (e.g. allowed_attributs may vary between events and news types), so maybe checking for "python:" is the best we can have.

[mauritsvanrees]

As I said, python: itself is fine. I customized search.pt and ended up with this diff that works. I took the easy way out near the top by simply removing stuff, but the other items seem equivalent:

--- a/Products/CMFPlone/browser/templates/search.pt
+++ b/Products/CMFPlone/browser/templates/search.pt
@@ -204,9 +204,9 @@
                           <tal:block repeat="item view/sort_options">
                             <a data-sort="" tal:content="item/title"
                                tal:attributes="href item/url;
-                                               data-sort python:item.sortkey and item.sortkey or None;
-                                               data-order python: item.reverse and 'reverse' or '';
-                                               class python: item.selected and 'active' or ''"></a>
+                                               data-sort python:None;
+                                               data-order python:'';
+                                               class python:''"></a>
                           </tal:block>
                         </metal:sorting>
                       </span>
@@ -233,21 +233,22 @@
                                      tal:attributes="src string:${item/getURL}/@@images/image/icon;">
                                 <a href="#"
                                    tal:define="item_url item/getURL;
-                                               item_type item/PortalType"
+                                               item_type item/PortalType;
+dataorigin item/getDataOrigin"
                                    tal:attributes="href python:item_type in use_view_action and (item_url + '/view') or item_url;
                                                    class string:state-${item/review_state}"
-                                   tal:content="python:item.getDataOrigin().pretty_title_or_id()" />
+                                   tal:content="python:dataorigin.pretty_title_or_id()" />
                               </span>
                               <span class="discreet" i18n:domain="plone"
                                     tal:condition="show_about">
                                 <span class="documentAuthor"
                                       i18n:translate="label_by_author">
                                   by
-                                  <tal:block tal:condition="item/Creator"
-                                             tal:define="author python:context.portal_membership.getMemberInfo(item.Creator())">
+                                  <tal:block tal:condition="creator"
+                                             tal:define="creator item/Creator; author python:context.portal_membership.getMemberInfo(creator)">
                                     <a href="#"
                                        tal:attributes="href string:$navigation_root_url/author/${item/Creator}"
-                                       tal:content="python:author and author['fullname'] or item.Creator()"
+                                       tal:content="python:author and author['fullname'] or creator"
                                        tal:omit-tag="not:item/Creator"
                                        i18n:name="author">
                                         Bob Dobalina

It does complain more than I would expect, and you cannot solve this with an allowed_attributes in zcml.

Rendering the template in background is not going to work I think: you would need to render it in the correct context. And if the half of the template is covered by a tal:condition for anonymous and the other half for authenticated, then you will only be testing half of the template.

[djay]

please don't try to put c.jbot into the core. We should also get rid of portal_customisations. The idea of exposing plones internals as a method of customisation and therefore having no clear api division between internal and external is just always going to result in messes like this. Not just security problems, but upgrade issues and a steeper learning curve.
Instead we should be looking at a combination of diazo, rapido and mosaic to cover online customisation.

[djay]

@davilima6 what customisation are you trying to make? perhaps I can help suggest a better way. I really don't think this is a bug btw. The bug is we need to remove portal_customisations.

[davilima6]

Hi, @djay. I agree on removing portal_view_customizations but until then we should try to fix it. _Silently_ breaking login or anything else on the site when user makes basic use of it is buggy UX from my viewpoint.

Mostly for these reasons (and because FS) I almost don't use it but I appreciate your offer. I was trying to debug #1631 by removing one single line from search.pt, the one that contains the <script> tag for search.js.

All in all I'd be glad to close this issue with consensus on at least a ZMI warning for new devs that this tool will be deprecated and they shouldn't be using it, specially if there are python expressions involved. We could extract that messsage from @mauritsvanrees explanations above. For instance:

This tool will be deprecated so beware using it. Besides there may be a problem with the python: expressions used in this template, making it impossible to customize it by using this tool. Technically, browser view templates are Zope 3 templates while items in portal_view_customization are Zope 2 templates. The different security model underlying the two implementations may break rendering of some parts of the site. If that happens just delete the custom copy by using the Contents tab above.

[djay]

to remove a tag, diazo is by the far the easiest and cleanest way to do that.

It's bad UX but its not really fixable. The only way to fix it is to test and constrain all templates to only use expressions that work TTW as well as on the fs, ie portal_customise every pt in plone and then rerun all the functional tests. Even then it's still a bad UX.

[davilima6]

Closed by plone/plone.app.customerize#11.