Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BrowserViews implement IPublishTraverse in Zope 4 which breaks allowed_attributes and allowed_interface #397

Closed
pbauer opened this issue Nov 11, 2018 · 20 comments

Comments

@pbauer
Copy link
Contributor

commented Nov 11, 2018

In Zope 4 a BrowserView always implements IPublishTraverse which it did not in Zope 2.
This breaks the way to make methods traveseable using allowed_attributes and allowed_interface in zcml.

Example:

  <browser:page
      name="foo"
      for="*"
      class=".dummy.Foo"
      permission="zope2.View"
      allowed_attributes="bar"
      />

dummy.py:

from Products.Five import BrowserView

class Foo(BrowserView):

    def __call__(self):
        return 'foo'

    def bar(self):
        """bar"""
        return 'foobar'

In Zope 2 (Plone 5.1) http://0.0.0.0:8080/Plone/foo/bar returned foobar. In Zope 4 (Plone 5.2) that throws a 404.

The relavant code is in BaseRequest.traverseName:

if IPublishTraverse.providedBy(ob):
    ob2 = ob.publishTraverse(self, name)
else:
    adapter = queryMultiAdapter((ob, self), IPublishTraverse)
    if adapter is None:
        # Zope2 doesn't set up its own adapters in a lot of cases
        # so we will just use a default adapter.
        adapter = DefaultPublishTraverse(ob, self)

    ob2 = adapter.publishTraverse(self, name)

In Zope 4 IPublishTraverse.providedBy(ob) is true for all BrowserViews because in https://github.com/zopefoundation/Zope/blob/master/src/Products/Five/browser/metaconfigure.py#L145 the class zope.browserpage.metaconfigure.simple is injected in the bases of each BrowserView. Since Foo has no publishTraverse of its own the one from simple is called which then raises NotFound.

The mro looks like this before the injection:

(Pdb++) pp ob.__class__.__mro__
(<class 'Products.CMFPlone.browser.dummy.Foo'>,
 <class 'Products.Five.browser.BrowserView'>,
 <class 'zope.publisher.browser.BrowserView'>,
 <class 'zope.location.location.Location'>,
 <class 'object'>)

And like this after it:

(Pdb++) pp ob.__class__.__mro__
(<class 'Products.Five.browser.metaconfigure.Foo'>,
 <class 'Products.CMFPlone.browser.dummy.Foo'>,
 <class 'Products.Five.browser.BrowserView'>,
 <class 'Products.Five.browser.metaconfigure.simple'>,
 <class 'zope.browserpage.metaconfigure.simple'>,
 <class 'zope.publisher.browser.BrowserView'>,
 <class 'zope.location.location.Location'>,
 <class 'object'>)

@jaroel and me were trying to figure out a solution but we require help.

This issue is a replacement for the original ticket plone/Products.CMFPlone#2624 which I moved to Zope since it is not directly related to Plone.

@pbauer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

Anyone have a idea how to deal with this? The commits that refactorded metaconfigure.py in Products.Five in 2012 are all anonymous so I have no idea who to ask.
@hannosch @davisagli @icemac maybe?

@pbauer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

I found out that Yuppie was the author of the changes in metaconfigure.py in Juli 2012. The changes can be seen here: 9bc2e7f...da088f0 and also here: https://github.com/zopefoundation/Zope/commits/master/src/Products/Five/browser/metaconfigure.py

The changenotes make me guess these might be responsible for the issue:

- Five: Removed obsolete metaclass.

- Five: Refactored ``browser:view`` and ``browser:page`` directives.
  This makes their implementation more similar to that in ``zope.browserpage``
  and adds allowed_interface support for the ``browser:view`` directive.
  By default the `aq_*` attributes are no longer available on those
  views/pages. If you still use them, you have to mix in Five's BrowserView.

@icemac icemac added this to To do in Zope 4 final release via automation Nov 29, 2018

jaroel added a commit to plone/Products.CMFPlone that referenced this issue Mar 5, 2019

jaroel added a commit to plone/Products.CMFPlone that referenced this issue Mar 5, 2019

jaroel added a commit to plone/Products.CMFPlone that referenced this issue Mar 5, 2019

Revert "Add workaround for zopefoundation/Zope#397 / IBrowserPublishe…
…r on BrowserView"

no bueno

This reverts commit c897100.

@icemac icemac added the RELEASE BLOCK label May 6, 2019

@icemac

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@pbauer Is this issue still a problem for Plone or did you manage to find a proper workaround? (At least in union.cms it did not bite us, probably because we do not use this feature.)

@icemac icemac assigned icemac and pbauer and unassigned icemac May 6, 2019

@sallner

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@jensens Do you have any info about this topic?

@jensens

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Yes, this is still an issue.

@dataflake

This comment has been minimized.

Copy link
Member

commented May 10, 2019

First of all, I don't know much about the internals here, but bear with me.

I've tried to look at Yuppie's checkins from that period and it's clear that there's so much changed it's not going to be possible to unravel that and make sense of it.

I'm looking at that mixed in class Products.Five.browser.metaconfigure.simple and it doesn't have its own publishTraverse method. It gets it from the single parent class zope.browserpage.metaconfigure.simple. The implementation is that class does nothing and just raises NotFound.

IMHO implementing a publishTraverse method on Products.Five.browser.metaconfigure.simple that does something intelligent may be the way to go. That's hoping the class isn't used elsewhere and some code is relying on the NotFound.

@dataflake

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@icemac @pbauer @jensens is there test code you can point me to that registers a browser page like the ZCML at the top here does? That way I could write a basic test as a starting point.

@dataflake

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I have found code in Products.Five.browser.tests.test_scriptsecurity that exercises an allowed_attributes view method and would have pointed out this failure much earlier. If it had run. But it never did because it has a dependency on Products.PythonScripts and checked for its existence before giving up silently.🤬

In PR #610 I have done the first step and added a failing test. For comparison, it exercises the code path that would have been taken if the view did not implement IPublishTraverse first, which is the detour through ZPublisher.BaseRequest.DefaultPublishTraverse.

Now we should think of a sensible publishTraverse implementation to add into the class Products.Five.browser.metaconfigure.simple, if that is indeed the right way to go.

@jaroel

This comment has been minimized.

Copy link

commented May 10, 2019

Maybe the IPublishTraverse should be removed from https://github.com/zopefoundation/zope.browserpage/blob/0478e6b8d1f1583101edf43f07aeda7c06b27496/src/zope/browserpage/metaconfigure.py#L410 ?
That way the publisher will try to adapt in BaseRequest.traverseName and we should get the correct behaviour.

Another option would be to have Products.five.browser.metaconfigure.simple extend zope.publisher.browser.BrowserView directly, and keep the rest as it is.
Products.five.browser.metaconfigure.simple already overrides __call__, so we won't actually miss anything as far as I can tell.

Note: I don't think anything in Plone world is relying on the NotFound exception, as we only now got around to upgrading to where this exists.

@icemac icemac modified the milestones: 4.0 final, 4.0.1 May 10, 2019

@icemac icemac removed this from To do in Zope 4 final release May 10, 2019

@icemac icemac added this to To do in Zope 4 bugfix via automation May 10, 2019

@vincg

This comment has been minimized.

Copy link

commented May 16, 2019

I've openend an issue for this and provided a workaround,
I don't know the package well enough to call it a fix, but it works for me.
zopefoundation/zope.browserpage#6

@icemac

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@vincg The fix in #610 takes another approach by not extending from zope.browserpage.metaconfigure.simple. But it still has the problem that It would not work for you because it defines a publishTraverse method. The idea to solve this issue is to add an adapter.

pbauer added a commit to plone/plone.app.event that referenced this issue May 18, 2019

@pbauer

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

I added a regression-test for this in plone/plone.app.event#306. With the first implementation (7febc46) that worked, with the current implementation (3d47465) it does not.

I also found another feature in Plone that is broken because of this: Unlocking locked objects. That's almost as easy to test as enabling ical-upload to a folder and much more important.

@MatthewWilkes

This comment has been minimized.

Copy link
Member

commented May 26, 2019

I've had a look a this today, following a request from @pbauer and I've got to say, I'm a bit confused.

Firstly, it looks like the zope.browserpage.metaconfigure.page_ handler isn't actually used, but there's an (almost?) identical one at Products.Five.browser.metaconfigure.page that is used. That's what injects the simple baseclass.

What's confusing me is why permissions are a problem at all. I'm sure I'm just misunderstanding, but the following appears to work for me:

diff --git a/src/zope/browserpage/metaconfigure.py b/src/zope/browserpage/metaconfigure.py
index c9d7d8a..e2e228e 100644
--- a/src/zope/browserpage/metaconfigure.py
+++ b/src/zope/browserpage/metaconfigure.py
@@ -168,6 +168,7 @@ def page(_context, name, permission, for_=Interface,
                                required)

    _handle_for(_context, for_)
+    new_class._simple_whitelist = set(required) - set([attribute, 'browserDefault', '__call__', 'publishTraverse'])

    defineChecker(new_class, Checker(required))

@@ -411,7 +412,12 @@ def _handle_for(_context, for_):
class simple(BrowserView):

    def publishTraverse(self, request, name):
-        raise NotFound(self, name, request)
+        if name in getattr(self, "_simple_whitelist", []):
+            self.__page_attribute__ = name
+            self.__call__ = simple.__call__
+            return self
+        else:
+            raise NotFound(self, name, request)

    def __call__(self, *a, **k):
        # If a class doesn't provide it's own call, then get the attribute

(With an equivalent line that adds the whitelist in Products.Five, of course)

That causes the ical view to work for me TTW (albeit with a CSRF error that I assume is unrelated) and causes @pbauer's test in plone.app.event to pass.

It seems to me that if the simple view is causing the problem by being too simple, it should be expanded slightly to work. This sets up a whitelist on the derived class created in the metaconfigure of all attributes other than the standard ones that were added for permission checks. Then, in the publishTraverse, if the name matches one of those whitelists the instance of the class is mutated in place to disregard the __call__ of the implementing class and change the __page_attribute__ to point to the selected attribute.

I'm sure that there are other related approaches, like mutating __call__ to point to the attribute directly, but this seems to work for me with the expected permissions.

Can someone tell me what I've missed? In the unlikely case that this is useful code, I've got some tests I can share too.

@d-maurer

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@icemac

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@MatthewWilkes There is a suggestion of @davisagli at the PR for this issue (#610) which I used as it seemed simpler to me. Could you please have a look at it?

pbauer added a commit to plone/buildout.coredev that referenced this issue May 27, 2019

@MatthewWilkes

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@d-maurer The correct permissions don't need to be set up for traversal, though. The allow_attributes parameter to the metadirective ensures that the permissions are set appropriately for the attributes, it's not the job of the traverser to implement permission checks so it shouldn't matter that the user isn't set up yet.

MatthewWilkes added a commit to plone/buildout.coredev that referenced this issue May 27, 2019

@d-maurer

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@pbauer

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

#643 and zopefoundation/zope.browserpage#7 seem fix this issue if you are ok with the implementation.

@icemac icemac pinned this issue Jun 13, 2019

pbauer added a commit to plone/buildout.coredev that referenced this issue Jun 15, 2019

pbauer added a commit to plone/buildout.coredev that referenced this issue Jun 15, 2019

@tomgross tomgross unpinned this issue Jun 17, 2019

@tomgross tomgross pinned this issue Jun 17, 2019

@icemac icemac closed this in #643 Jun 18, 2019

Zope 4 bugfix automation moved this from To do to Done Jun 18, 2019

pbauer added a commit to plone/buildout.coredev that referenced this issue Jun 18, 2019

@pbauer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

🎉

@jaroel

This comment has been minimized.

Copy link

commented Jun 18, 2019

Thank you all!

@dataflake dataflake unpinned this issue Jun 18, 2019

mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jun 21, 2019

[fc] Repository: plone.app.event
Branch: refs/heads/master
Date: 2019-05-18T13:49:44+02:00
Author: Philip Bauer (pbauer) <bauer@starzel.de>
Commit: plone/plone.app.event@3d466a9

Add regression-test for zopefoundation/Zope#397

Files changed:
A news/306.bugfix
A plone/app/event/tests/test_ical_import.py
Repository: plone.app.event

Branch: refs/heads/master
Date: 2019-06-21T11:37:50+02:00
Author: Philip Bauer (pbauer) <bauer@starzel.de>
Commit: plone/plone.app.event@727fb23

Merge pull request #306 from plone/add_regression_test_for_allowed_attributes

Add regression-test for allowed_attributes

Files changed:
A news/306.bugfix
A plone/app/event/tests/test_ical_import.py

mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jun 21, 2019

[fc] Repository: plone.app.event
Branch: refs/heads/master
Date: 2019-05-18T13:49:44+02:00
Author: Philip Bauer (pbauer) <bauer@starzel.de>
Commit: plone/plone.app.event@3d466a9

Add regression-test for zopefoundation/Zope#397

Files changed:
A news/306.bugfix
A plone/app/event/tests/test_ical_import.py
Repository: plone.app.event

Branch: refs/heads/master
Date: 2019-06-21T11:37:50+02:00
Author: Philip Bauer (pbauer) <bauer@starzel.de>
Commit: plone/plone.app.event@727fb23

Merge pull request #306 from plone/add_regression_test_for_allowed_attributes

Add regression-test for allowed_attributes

Files changed:
A news/306.bugfix
A plone/app/event/tests/test_ical_import.py

reinhardt added a commit to collective/collective.solr that referenced this issue Jul 29, 2019

Use final release of Plone.
Pulls in fix for maintenance view not being traversable.
See zopefoundation/Zope#397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.