plone / plone.base

Plone Interface contracts, plus basic features and utilities
https://pypi.org/project/plone.base
2 stars 0 forks source link

Fix permissions during traversal executed from `get_top_site_from_url()`. #19

Closed Rudd-O closed 2 years ago

Rudd-O commented 2 years ago

My previous fix broke several tests. This appears to be the result of trying to traverse iteratively through the structure of a site (which my code does and the previous code does not do in the same way), and encountering Unauthorized exceptions during traversal (which is a legitimate thing to get, as /a/b/c may be accessible to user XXX, but /a/b may not necessarily be).

The fix is to use unrestricted traversal (bypassing security), and then to return the selected object by using restricted traversal in that case.

I am confident this should not result in a security issue — however I ask the true expert Plonistas to verify this confidence is warranted. Thanks.

And sorry for the breakage. We need CI tests in plone.base.

mister-roboto commented 2 years ago

@Rudd-O thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

Rudd-O commented 2 years ago

@jenkins-plone-org please run jobs

Rudd-O commented 2 years ago

@mauritsvanrees' opinion would be great too.

Once we hit the second LGTM, please just merge. I may not be at my desk for the next day or so, but this should go in ASAP to fix all the tests everywhere else.

jensens commented 2 years ago

Oh, yeah, unrestricted is needed obviously.

mauritsvanrees commented 2 years ago

For clarity: today (well, tonight) I have already released Plone 6.0.0b2 without this. Thanks for working on this. Will definitely make it into the next Plone release. I hope to make the first release candidate within the next few weeks, shortly before the Plone conference.

mauritsvanrees commented 2 years ago

O, and the fix looks good to me. :-)

mauritsvanrees commented 2 years ago

Still something is wrong though. There are three Jenkins failures. Trying it locally in buildout.coredev:

$ bin/test -s plone.app.dexterity -t editing.rst
...
File "/Users/maurits/shared-eggs/cp310/plone.app.dexterity-3.0.0b1-py3.10.egg/plone/app/dexterity/tests/editing.rst", line 120, in editing.rst
Failed example:
    for name, factory in sorted(component.getUtilitiesFor(
        interfaces.IFieldFactory)):
        if hasattr(factory, 'protected') and factory.protected(None):
            continue
        browser.open(schemaeditor_url)
        # If two changes happen in the same moment, the schema lookup will find an old schema,
        # so we sleep shortly.
        now = time.time()
        time.sleep(0.1)
        browser.getLink('Add new field').click()
        browser.getControl('Title').value = name
        field_id = normalizer.normalize(name).replace('-', '_')
        browser.getControl('Short Name').value = field_id
        browser.getControl('Field type').getControl(
            value=factory.title).selected = True
        browser.getControl('Add').click()
        schema = plonista.lookupSchema()
        assert browser.url == "http://nohost/plone/dexterity-types/plonista/@@add-field", (
            "Couldn't successfully add %r" % name)
        assert field_id in schema, '%r not in %r' % (
            field_id, schema)
        assert factory.fieldcls._type is None or isinstance(
            schema[field_id], factory.fieldcls
            ), '%r is not an instance of %r' % (
                schema[field_id], factory.fieldcls)
        browser.open(schemaeditor_url)
        browser.getLink(url=field_id).click()
        browser.getControl('Save').click()
Exception raised:
    Traceback (most recent call last):
...
      File "/Users/maurits/community/plone-coredev/6.0/src/Products.CMFPlone/Products/CMFPlone/patterns/settings.py", line 140, in tinymce
        related_items_config = get_relateditems_options(
      File "/Users/maurits/shared-eggs/cp310/plone.app.widgets-4.0.0b1-py3.10.egg/plone/app/widgets/utils.py", line 103, in get_relateditems_options
        site = get_top_site_from_url(context, request)
      File "/Users/maurits/community/plone-coredev/6.0/src/plone.base/src/plone/base/utils.py", line 319, in get_top_site_from_url
        _site = context.unrestrictedTraverse(site_path)
      File "/Users/maurits/shared-eggs/cp310/Zope-5.6-py3.10.egg/OFS/Traversable.py", line 347, in unrestrictedTraverse
        raise e
       - __traceback_info__: ([], 'plonista')
      File "/Users/maurits/shared-eggs/cp310/Zope-5.6-py3.10.egg/OFS/Traversable.py", line 313, in unrestrictedTraverse
        raise NotFound(name)
       - __traceback_info__: ([], 'plonista')
    zExceptions.NotFound: zExceptions.NotFound: plonista

This should not have been merged. The PR job already failed. Irritating that the result is somehow not getting through correctly to GitHub.

@Rudd-O Not sure why it fails now. You seem to be on the right path. Please try again. Meanwhile I will revert the checkout in the buildout, so the build is fixed.

Rudd-O commented 2 years ago

I will try again, testing plone.app.dexterity too. I am very sorry for the troubles caused.

Rudd-O commented 2 years ago

Need help here. (EDIT: solved below.)

find-links +=
    https://dist.plone.org/release/6.0.0b2/

extends =
    https://dist.plone.org/release/6.0.0b2/versions.cfg

->

PATH=~/.local/bin:$PATH venv/bin/buildout || { ret=$? ; rm -rf bin ; exit $ret ; }
While:
  Initializing.

An internal error occurred due to a bug in either zc.buildout or in a
recipe being used:
Traceback (most recent call last):
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 2180, in main
    buildout = Buildout(config_file, options,
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 377, in __init__
    _update(data, _open(os.path.dirname(config_file), config_file, [],
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 1830, in _open
    eresult = _open(base, extends.pop(0), seen, dl_options, override,
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 1830, in _open
    eresult = _open(base, extends.pop(0), seen, dl_options, override,
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 1830, in _open
    eresult = _open(base, extends.pop(0), seen, dl_options, override,
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/buildout.py", line 1809, in _open
    result = zc.buildout.configparser.parse(
  File "/home/user/Projects/Rudd-O.com/venv/lib/python3.10/site-packages/zc/buildout/configparser.py", line 188, in parse
    section_condition = eval(expr, context)[0]
  File "<string>", line 1, in <module>
NameError: name 'python310' is not defined
Rudd-O commented 2 years ago

This does not happen with beta 1 versions.cfg.

Rudd-O commented 2 years ago

Yah 3.0.0rc3 buildout is needed to build it, but that is not in the notes, and it is not downloaded by default when pip install zc.buildout which is what my Makefile does.

Rudd-O commented 2 years ago

Today I will be running the battery of tests that failed, all against Plone 6.0.0b2, but with my fixes in branch. I will report results soon — buildout is buildouting right now.

Rudd-O commented 2 years ago

Confirming that the test fails with Plone 6.0.0b2 buildout (not with Plone 6.0.0b1's). Digging further.

Rudd-O commented 2 years ago

Pull request here: https://github.com/plone/plone.base/pull/20/files .

Dispatching the tests now: https://jenkins.plone.org/job/pull-request-6.0-3.10/32/console .

Rudd-O commented 2 years ago

Tests work. #20 is ready for review.