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

Cannot VHM-root site to an item within the site #253

Open
Rudd-O opened this issue Sep 7, 2022 · 4 comments
Open

Cannot VHM-root site to an item within the site #253

Rudd-O opened this issue Sep 7, 2022 · 4 comments

Comments

@Rudd-O
Copy link

Rudd-O commented Sep 7, 2022

This is functionality that used to work perfectly fine years ago (think before we had XHR folder contents browsing), but since it's a rare use case, as we moved forward with Plone 5, the newer things added to Plone (like the aforementioned folder contents browsing) never got tested to work with it.

Currently, if the admin VHMs the root of the site to an item (folder / non-folder) of the site, none of the client-side functionality provided by plone.app.content (and also by plone.app.widgets) works. User can't browse folder, can't embed content using TinyMCE. There is a workaround but it involves the ZMI.

Details

Assume site structure as follows:

/Plone
  /Folder
    /contentitem1
    /contentitem2

Now assume the following virtual hosting root: /Plone/Folder/VirtualSiteRoot.

Technically, this setup should make the user visiting http://site.com/ get onscreen the contents of /Plone/Folder. And this works perfectly well, exactly as advertised, save for the top navigation bar displaying the navigation bar generated by the /Plone site object.

The problem is when the user tries to view the folder_contents or tries to edit the item TTW using TinyMCE. Nothing works then.

The first problem is that the @@qsOptions view is only available on the ISiteNavigationRoot interface, which /Plone/Folder does not satisfy. I thought to edit configure.zcml to add that, and then @@qsOptions works well (if this PR plone/plone.base#18 is applied to fix a long-standing @@qsOptions bug). That makes @@qsOptions load fine.

But then folder contents don't work anyway. The reason they don't work is because the vocabulary view simply returns an error Vocabulary lookup not allowed (and does so returning HTTP 200 OK!!!), eventually causing an undefined error in the JS console. What is being loaded by the browser JS is the URL http://site.com/@@getVocabularies with parameters "gimme listing of /", which after VHM translates to an object traversal to /Plone/Folder:getVocabularies(...).

This error comes from the following snippet in vocabulary.py:

class VocabularyView(BaseVocabularyView):
    """Queries a named vocabulary and returns JSON-formatted results."""

    def get_context(self):
        return self.context

    def get_vocabulary(self):
        # Look up named vocabulary and check permission.

        context = self.context
        factory_name = self.request.get("name", None)
        field_name = self.request.get("field", None)
        if not factory_name:
            raise VocabLookupException("No factory provided.")
        authorized = None
        sm = getSecurityManager()
        if factory_name not in PERMISSIONS or not INavigationRoot.providedBy(context):
            # Check field specific permission
            if field_name:
                permission_checker = queryAdapter(context, IFieldPermissionChecker)
                if permission_checker is not None:
                    authorized = permission_checker.validate(field_name, factory_name)
                elif sm.checkPermission(
                    PERMISSIONS.get(factory_name, DEFAULT_PERMISSION), context
                ):
                    # If no checker, fall back to checking the global registry
                    authorized = True

            if not authorized:
                raise VocabLookupException("Vocabulary lookup not allowed") # <---HERE

        # Short circuit if we are on the site root and permission is
        # in global registry
        elif not sm.checkPermission(
            PERMISSIONS.get(factory_name, DEFAULT_PERMISSION), context
        ):
            raise VocabLookupException("Vocabulary lookup not allowed ")

        factory = queryUtility(IVocabularyFactory, factory_name)
        if not factory:
            raise VocabLookupException(
                'No factory with name "%s" exists.' % factory_name
            )

As you can see, since the folderish item does not provide INavigationRoot (which I believe is the wrong interface to check anyway), we get bypassed to if not authorized: right away.

Of course, everything works fine if I manually change the folderish item to provide INavigationRoot.

Is perhaps this permission check farcical altogether? I feel like the getVocabulary view — if the user has permission for the object — should work fine no matter where the VHM has rooted the site, and should not be a question of whether the user is looking at a Navigation Root (?!) or not. Why would it matter? My browser is querying http://site.com/@@getVocabulary for items under /, which after VHM translates clearly to /Plone/Folder:@@getVocabulary, so @@getVocabulary should just give me the folder listing for everything under /Plone/Folder, the NavigationRoot be damned. Or maybe the permission check is wrong anyway — after all I am doing all this testing as Manager.

Admittedly, this is a corner case, but in the interest of correctness, the code fundamentally does not seem to be correct here — it has rather implemented an unprincipled exception, the rationale of which escapes me. Who knows what other subtle bugs this may be causing on client-side code?

@thet
Copy link
Member

thet commented Sep 17, 2022

@Rudd-O can be closed, right?

@Rudd-O
Copy link
Author

Rudd-O commented Oct 1, 2022

I don't think so. Let me review tomorrow please.

@Rudd-O
Copy link
Author

Rudd-O commented Oct 2, 2022

OK. This should not be closed. The code should be fixed to support VHM-rooting to any content item on the site, rather than make a special case exception for IPloneSiteRoot objects. Unfortunately I have near-zero expertise with Zope security so I would probably add a gaping hole here if I were to make changes. Who would you recommend to advise me on how best to fix this code?

@spereverde
Copy link
Member

Just in case, this is still an issue. We have problems with this as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants