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

Port whitelist creation from zope.browserpage #643

Merged
merged 14 commits into from
Jun 18, 2019

Conversation

pbauer
Copy link
Member

@pbauer pbauer commented May 28, 2019

Together with zopefoundation/zope.browserpage#7 this fixes #397.

All Plone tests are green with that (see plone/buildout.coredev#588)

Open points:

  • Implement a pair of tests using allowed_interface.

@icemac
Copy link
Member

icemac commented May 29, 2019

In #610 @jaroel requested to be able to have a custom IPublishTraverse adapter. IMHO this is not solved by this PR. Is this request still valid?

@icemac icemac force-pushed the wilkes/397-allowed-attributes branch from 3e8664c to c9f4938 Compare May 29, 2019 06:03
@icemac
Copy link
Member

icemac commented May 29, 2019

I ported the tests from branch issue_397 here but the test for accessing the allowed attribute fails. Am I missing a part of the solution?

@MatthewWilkes
Copy link
Member

Okay, I've fixed that failing test and cleaned up the implementation a bit. Rather than relying on replacing __call__ I've moved the implementation of browserDefault from the metaconfigure directive into the simple class and added code to handle the changing attribute to avoid having to rebind the function.

I'm waiting for the Plone tests to finish, but I'm hopeful.

@MatthewWilkes
Copy link
Member

@jaroel Can you clarify how important a usecase you think the publish traverse adapter is? It's possible to add a publishTraverse method directly to a view, but if we really need to support adapter based things I think we can. We'd probably need to add an adapter lookup in the publishTraverse of the simple class that looks up the adapter on the relevant baseclass.

@jaroel
Copy link

jaroel commented May 29, 2019

@MatthewWilkes , @icemac,
I mainly said that because in Plone+Zope2 a Browserview doesn't implement IPublishTravese, which in turn make the publisher query for an adapter, or use a fallback.
The latter is why the sub path traversal worked in the first place.

I'm unaware of code that uses the IPublishTraverse-as-adapter approach, so the current approach should work just fine by the looks of it.

@icemac icemac added do not merge RELEASE BLOCK Issue blocking the release/milestone it is in labels Jun 7, 2019
@icemac
Copy link
Member

icemac commented Jun 7, 2019

This PR looks okay to me, I think a test for allowed_interface should be added for completeness. After the explanation of @jaroel I think we could drop the requirement of using a adapter. It could be added later if there is an actual demand.

@icemac icemac added this to the 4.0.1 milestone Jun 7, 2019
@icemac
Copy link
Member

icemac commented Jun 7, 2019

zopefoundation/zope.browserpage#7 currently fails.

@MatthewWilkes
Copy link
Member

@icemac Yep, it does. I've not had a chance to look at this again yet, but my instinct is that the problem is the test, not the code. It's testing that browserDefault is None as a way of identifying a misconfigured view, but I'm not convinced that is legitimate. We could likely add a special case for that, but I'd probably approach it by making the test more closely resemble the traversal/publishing flow.

@pbauer
Copy link
Member Author

pbauer commented Jun 15, 2019

I have no real idea if the code is wrong or the test. But in zopefoundation/zope.browserpage@6ab5166 I attempted to adapt the test to show what's actually going wrong with the misconfigured view.

@icemac
Copy link
Member

icemac commented Jun 17, 2019

After zopefoundation/zope.browserpage#7 is accepted, merged and released. The do not merge label can be dropped here.

@icemac icemac self-assigned this Jun 17, 2019
@icemac icemac merged commit d6e4974 into master Jun 18, 2019
@icemac icemac deleted the wilkes/397-allowed-attributes branch June 18, 2019 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RELEASE BLOCK Issue blocking the release/milestone it is in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BrowserViews implement IPublishTraverse in Zope 4 which breaks allowed_attributes and allowed_interface
4 participants