Work on fixing allowed_attributes and allowed_interface on pages
#610
Conversation
* `simple` no longer provides `IPublishTraverse`. * Accessing an attribute the user is not allowed to now leads to HTTP-401 instead of HTTP-404 (404 came from `zope.browserpage.metaconfigure.simple`) * In Zope 2 views did not have the possibility to call `publishTraverse` so I removed the test.
|
@dataflake What do you think about the implementation I did together with @sallner? |
|
@icemac See that lone failure on Python 2 because it renders as @pbauer Can you run the whole Plone unittest battery against this branch? I am sure Plone would exercise all of this a lot more. I'm fine with any solution that makes all tests succeed. I do want to fix up that non-running doctest that uses PythonScripts, though. |
|
After a lengthy discussion over a good lunch, we decided that a 404 should be the correct response in case an attribute is not allowed. This will be resolved after we decided on a good implementation in an upcoming bugfix release. |
|
I'm fine with pushing this out a little bit, but I'm sure the Plone guys will be unhappy that they basically cannot use 4.0 until that bugfix is out, right? |
|
@jensens assured that there are workarounds for all relevant parts and therefore no blocker. |
|
Plone itself does not use this feature a lot but there are some use-cases that are broken. Among them is enabling ical-upload to a folder: There is a action that enables this with the url: Here ist the registration: <browser:page
for="plone.folder.interfaces.IFolder"
name="ical_import_settings"
class=".importer.IcalendarImportSettingsFormView"
allowed_attributes="enable
disable"
permission="plone.app.event.ImportIcal"
layer="..interfaces.IBrowserLayer"
/>In 5.2 this feature is broken so far (it has no tests that use the action...). |
|
So: There are cases without workarounds. There are a lot more cases where |
|
Jens Vagelpohl wrote at 2019-5-10 04:26 -0700:
@icemac See that lone failure on Python 2 because it renders as `u'The eagle has landed'`? Is there some doctest way of fixing it? The only reason I did that as doctests, which I otherwise hate, was because all other examples there use it.
My question about Python2+3 doctest support was closed, because
Zope plans to drop Python 2 support in the near future.
Thus, maybe, you would be content with a workaround.
For ASCII strings (as in your case), you can replace
the output via an equality test, e.g. instead of
```
>> f()
"<some ASCII text>"
```
you can use in your doctest the equivalent
```
>> f() == "<some ASCII text>"
True
```
This uses that Python2 automatically converts `str` to `unicode`
when the two come together.
Alternatively, you could set up a "custom checker". You find
details in @ale-rt 's comment to #463.
|
|
With plone/plone.app.event#306 this still results in a 404: |
* Require a permission on the view so security has to do its work * add a test for an attribute which is not allowed explicitly The first test currently fails because `guarded_getattr` requires an authorized user. Which is not the case during traversal.
|
The problem is that authentication is done after traversal. So I also tried to move the Currently I am low on ideas and out of time. The next step could be to find out, how it ever worked before the changes in 9bc2e7f...da088f0. I adapted the test so it fails quite the same way as it does in |
|
@icemac I think that the way it worked in Zope 2.13 is that DefaultPublishTraverse would find the attribute via getattr, but it would fail access control validation so return a 401. Why are you intent on returning a 404 rather than a 401 when an attribute is not allowed? That's not consistent with what the Zope publisher does in other cases when an object is traversable but not authorized. And as you've found, the way that the publisher is structured makes it impossible to customize traversal based on the user's access. If the 404 is critical, you might be able to achieve it by making traversal return a DeferredAttrAuthProxy like this: Make sure that DeferredAttrAuthProxy is declared as public (I forget the best way to do this.) Then make SimplePublishTraverse.publishTraverse do |
|
@davisagli Thank you for investigating this issue. I implemented your suggestion in this PR. |
|
Will do so asap |
|
This breaks a lot of tests and features. Enabling ical-support now works but the a CSRF-check does not kick where it should. Among the broken features with tests are:
See https://jenkins.plone.org/job/pull-request-5.2/239/ (python2.7) and https://jenkins.plone.org/job/pull-request-5.2-3.7/213/ (python 3.7) for the test-results. |
|
David's approach looks sensible to me, but I very much agree with his point that it's not normal for traversing to a view that you fail a permissions check for to issue a 404. David's approach is very much like my one in #397 except that he creates a wrapper object where I mutate the I've pushed my changes to |
|
Okay, I worked out how to build it. Sorry, I'm out of practice with the coredev buildout. I've got a branch of Plone building at plone/buildout.coredev#588 that includes @pbauer's plone.app.event and my changes to zope.browserpage and Zope core. I don't know if this will work, or if it's the route you'd want to take, but hopefully it's useful information. |
|
Matthew Wilkes wrote at 2019-5-27 11:57 -0700:
David's approach looks sensible to me, but I very much agree with his point that it's not normal for traversing to a view that you fail a permissions check for to issue a 404.
I agree.
However, it is exactly this what security experts recommend:
let a resource inaccessible with the current proviledges behave
exactly like a non existing resource -- in order to
give a potential attacker the least information possible.
On the other hand, Zope does not follow this recommendation:
One of its operation principles is to allow a reauthentication
when the user tries to access a resource with insufficient
priviledges. Therefore, there is no need to implement the
recommendation at an isolated place.
|
|
In #642 I created a variant which does not insist to have a HTTP-404 for view attributes which are not in |
|
@MatthewWilkes wrote:
Cool to see an approach which seems to work. I am not sure whether I like the necessary changes in |
|
#642 has the same problems as this one with portlets, content-rules and changing templates. The approach by @MatthewWilkes (ab64f98 plus zopefoundation/zope.browserpage@f8dedc9) seems to fix all features and tests in Python 3. In Python 2.7 there is the issue that enabling ical-support raises Forgive me for not having a opinion regarding 404 vs. 401 or implementation-details at this point. I only focus on finishing Plone 5.2 |
|
I lost a bit track here about implementation details.
my 0.02€ |
|
@icemac The reason I was keen to do the bulk of this in I can see why you'd be a bit concerned about the |
|
@MatthewWilkes If @jensens The approach in #643 seems to implement the behaviour you are expecting. |
|
Superseded by #643. |
Fixes #397
This is work in progress so this is a "draft" PR that cannot be merged.
Open issues:
Move added implementation ofdoes not help, the problem is that authentication is done after traversalpublishTraverseto an adapter (as requested by @jaroel)mouseinstead ofeagle).allowed_interface.