Lower restriction of vocabularies endpoint #1258

ksuess · 2021-10-30T18:09:09Z

No check of field permissions: that's not the responsibility of the vocabulary, but of the schema.

mister-roboto · 2021-10-30T18:09:13Z

@ksuess thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

ksuess · 2021-10-31T09:34:35Z

@jenkins-plone-org please run jobs

ksuess · 2021-10-31T16:20:22Z

@jenkins-plone-org please run jobs

jensens · 2021-11-02T09:41:57Z

src/plone/restapi/services/vocabularies/get.py

+            title="plone.restapi: Access Plone vocabularies"
+            />
+        """
+        if vocabulary_name in PERMISSIONS:


We need to always check the DEFAULT_PERMISSION in order to get the same defined behavior as here:
https://github.com/plone/plone.app.content/blob/7a1bd2b60136b1fd380d4fa04bd052498548f272/plone/app/content/browser/vocabulary.py#L304-L324

I skip the field permission check as this is not the responisbility of the vocabulary, but that of the schema and its consumer.

I agree here.

Anyway, if vocabulary_name in PERMISSION is wrong/ superfluous. DEFAULT_PERMISSION must always be checked to match the logic in the above referenced current code.

One could argue, if it needs check if it is different from the permission the view is registered with.

Thinking further, the endpoint can be registered with zope.Public in ZCML. The default goes with zope.View as is. Just, if anyone in an add-on needs it, a vocabulary could be registered with zope.Public. This way we do not disallow this (probably rare) use case by design.

I understand that DEFAULT_PERMISSION needs to be checked if the same behavior as VocabularyView.get_vocabulary of https://github.com/plone/plone.app.content/blob/7a1bd2b60136b1fd380d4fa04bd052498548f272/plone/app/content/browser/vocabulary.py#L304-L324 should be followed in vocabulary endpoint.
It's not in scope of this PR to think about why a vocabulary is a utility which cannot be registered with a permission. Which means that the DEFAULT_PERMISSION is supposed to fit for custom vocabularies.

I will take just the one step to follow the classic behavior in vocabulary endpoint and check DEFAULT_PERMISSION for all vocabularies.

jensens · 2021-11-02T09:44:10Z

src/plone/restapi/services/vocabularies/get.py

+try:
+    from plone.app.vocabularies import DEFAULT_PERMISSION
+    from plone.app.vocabularies import PERMISSIONS
+except ImportError:
+    from plone.app.content.browser.vocabulary import (
+        DEFAULT_PERMISSION,
+        PERMISSIONS,
+    )


Do you plan to create a separate PR to move the default permissions to p.a.vocabularies? It's where I would expect those, not in the depth of p.a.content?

Independent related PRs:
plone/plone.app.vocabularies#67
plone/plone.app.content#237

@jensens @ksuess if we move the permissions to p.a.vocabularies and plone.app.content, we introduce a tight coupling to those packages, right? This means we would have to backport those enhancements to Plone 5.2 compatible versions and add lots of version pins for the different Plone versions plone.restapi supports (Plone 5.2 and Plone 6). In the past (e.g. for a simple enhancement in plone.schema) this approach caused lots of trouble and confusion. Even in the best possible scenario, we might end up with a long list of KGS version pins to make Plone 5.2 work with plone.restapi. I don't have a silver bullet solution here but maybe it is better to defer those moves until Plone 6 is stable and we can start moving things around in Plone 7 anyway.

plone.app.content and plone.app.vocabularies are anyway coupled. I tend to move those basic things as an (empty) permission map for vocabularies to a plone.base package at some point and fill it with values where vocabularies are created. But this is for future. For now plone.app.content and plone.app.vocabularies are anyway coupled over CMFPlone in both directions.

I would not open that box here in plone.restapi discussion and go with pragmatic small steps.

I do not think, if done right, we run into trouble in a 5.2 usage. So, IMO @ksuess did it right by importing from the old place in plone.restapi, so no problems here. We may want to add a comment at least why the alternative import is here and when we can remove it.

Regarding the move itself, pa.vocabularies would need a breaking change/major version bump and buildout.coredev 5.2 needs to use the pa.vocabularies 4.x branch then while master is 5.

jensens · 2021-11-02T09:46:35Z

I added comments/ a review - even if this one is a draft. I hope that's OK.

ksuess · 2021-11-05T08:51:04Z

Independent related PRs:
plone/plone.app.vocabularies#67
plone/plone.app.content#237

ksuess · 2021-11-05T08:51:36Z

@jenkins-plone-org please run jobs

ksuess · 2021-11-05T09:00:28Z

@jenkins-plone-org please run jobs

jensens · 2021-11-05T17:47:49Z

src/plone/restapi/services/vocabularies/get.py

@@ -24,7 +34,24 @@ def _error(self, status, type, message):
        self.request.response.setStatus(status)
        return {"error": {"type": type, "message": message}}

+    def _has_permission_to_access_vocabulary(self, vocabulary_name):
+        """Check if user is authorized to access built-in vocabulary


Suggested change

"""Check if user is authorized to access built-in vocabulary

"""Check if user is authorized to access the vocabulary.

The endpoint using this method is supposed to have no further protection (`zope.2Public` permission).

A vocabulary with no further protection follows the `plone.app.vocabularies.DEFAULT_PERMISSION` (usually `zope2.View`).

For further protection the dictionary `plone.app.vocabularies.PERMISSION` is used.

It is a mapping from vocabulary name to permission.

If a vocabulary is mapped there, the permission from the map is taken.

Thus vocabularies can be protected stronger or weaker than the default.

src/plone/restapi/services/vocabularies/configure.zcml

jensens · 2021-11-05T18:06:25Z

The permission here is now no longer needed, right?

plone.restapi/src/plone/restapi/permissions.zcml

Lines 11 to 14 in 9487658

    
           <permission 
        
               id="plone.restapi.vocabularies" 
        
               title="plone.restapi: Access Plone vocabularies" 
        
               />

jensens · 2021-11-05T18:09:50Z

Forget my above comment, the @sources endpoint uses it. TBH, I guess It needs some love too.

Check if user is authorized to access vocabulary default permission for endpoint for all vocabularies, built-in and others, was <permission id="plone.restapi.vocabularies" title="plone.restapi: Access Plone vocabularies" /> permission for built-in vocabularies in Classic is PERMISSIONS = { "plone.app.vocabularies.Catalog": "View", "plone.app.vocabularies.Keywords": "Modify portal content", "plone.app.vocabularies.SyndicatableFeedItems": "Modify portal content", "plone.app.vocabularies.Users": "Modify portal content", "plone.app.multilingual.RootCatalog": "View", }

See plone.app.vocabularies.PERMISSIONS

…abularies

ksuess · 2021-11-24T12:37:42Z

@jenkins-plone-org please run jobs

sneridagh · 2021-11-25T09:07:49Z

@jensens So we have a messy situation here. This is what happened:

Yesterday in the same moment I was cutting a release of p.restapi, you merged this PR. So, when I pushed the result, then it conflicted, then the history got messed up. So to solve it without rewriting the history, then I re-released. So we have 8.15.1 and 8.15.2.

I tested it then in our project, and it's throwing errors in some tests that we have in custom vocabularies.

So questions:

Is this PR a breaking change?
Could we drop a minimum amount of documentation about how they work now?
I see from the comments that this relies in other PRs in other packages, that are still not merged, let alone released.
Do the latter change the local dependencies of p.restapi, and if so, should we pin some minimum versions?

If after answering this, we conclude it's a brown bag release, we should revert the changes, then re-release again.

Please I need a swift answer on this one.

ksuess · 2021-11-25T11:41:02Z

So questions:

1. Is this PR a breaking change?

It's a bug fix.
IMHO it's not a breaking change, as it

decreases the protection
but does not open a security hole (DEFAULT_PERMISSION zope2.view is checked)

The former endpoints permission="plone.restapi.vocabularies" with

  <permission name="plone.restapi: Access Plone vocabularies" acquire="True">
      <role name="Manager"/>
      <role name="Site Administrator"/>
    </permission>

led to missing options in select fields for Editors, e.a.. This is patched in multiple forms out there.

2. Could we drop a minimum amount of documentation about how they work now?

No patch for users without 'Manager' and 'Site Administrator' role needed anymore.

'_has_permission_to_access_vocabulary' got a sum up of what it does in docstring

plone.restapi/src/plone/restapi/services/vocabularies/get.py

Lines 33 to 42 in 8f1d624

    
               def _has_permission_to_access_vocabulary(self, vocabulary_name): 
        
                   """Check if user is authorized to access the vocabulary. 
        
                   The endpoint using this method is supposed to have no further protection (`zope.View` permission). 
        
                   A vocabulary with no further protection follows the `plone.app.vocabularies.DEFAULT_PERMISSION` (usually `zope2.View`). 
        
                   For further protection the dictionary `plone.app.vocabularies.PERMISSION` is used. 
        
                   It is a mapping from vocabulary name to permission. 
        
                   If a vocabulary is mapped there, the permission from the map is taken. 
        
                   Thus vocabularies can be protected stronger than the default. 
        
                   """

As this is a bug fix, I do not see the urge to add an explanation to an upgrade guide.

3. I see from the comments that this relies in other PRs in other packages, that are still not merged, let alone released.

This PR is independent.
It does not rely on the mentioned PRs on moving permissions for the built-in vocabularies
PERMISSIONS = {
}
"plone.app.vocabularies.Catalog": "View",
"plone.app.vocabularies.Keywords": "Modify portal content",
"plone.app.vocabularies.SyndicatableFeedItems": "Modify portal content",
"plone.app.vocabularies.Users": "Modify portal content",
"plone.app.multilingual.RootCatalog": "View",
}
We even removed the alternative import (p.a.vocabulary instead of p.a.content) and postponed it to a following PR that is explicitly addressed to the move of built-in vocabulary permissions.

4. Do the latter change the local dependencies of p.restapi, and if so, should we pin some minimum versions?

No, there is no dependency.

jensens · 2021-11-25T11:42:07Z

Is this PR a breaking change?

No. It is a bufix.

Could we drop a minimum amount of documentation about how they work now?

At the moment there is not any word about access permission of the endpoints here https://plonerestapi.readthedocs.io/en/latest/vocabularies.html
As a bugfix, vocabularies are working now the same way as they used to before plone.restapi emerged.
But overall we should document the PERMISSION global of (currently) plone.app.content (see below), which is valid for restapi and classic..

I see from the comments that this relies in other PRs in other packages, that are still not merged, let alone released.
Do the latter change the local dependencies of p.restapi, and if so, should we pin some minimum versions?

No, in a call @ksuess and I decided to not cover the changes over there in this PR. It would enhance the scope too much. So there is no dependency to the mentioned PRs anymore in the merged code.

sneridagh · 2021-11-25T12:00:34Z

Thanks a lot to both of you @ksuess and @jensens for the clarification!

I will try to debug what's going on in my project and what went wrong. If you could also test 8.15.2 in your projects it would be great!

sneridagh · 2021-11-26T15:21:59Z

Hey @ksuess @jensens I have to protect one vocabulary, how do you protect just one in a sane way?

patch the p.a.content.PERMISSIONS doesn't seem an option to me. How about if your code and several add-ons want to do it at the same time?

jensens · 2021-11-27T13:42:08Z

Patching is the way it was in the getVocabulary endpoint of plone.app.content all the time. Its not perfect, but that's how it works and now concequently restapi follow the same permissions. I am open for ideas how to improve here. One idea I have is to facilitate the existing permission attribute of the <utility ../> directive if possible (overides.zcmlfor overriding) or another one to create an own directive<vocabulary ../>` with permissions.

Given your case of patching it now: I would not expect several add-ons to patch a vocabulary permission. In fact, in past it was rarely done.

from plone.app.content.browser.vocabulary import PERMISSIONS

# patch an existing
PERMISSIONS["plone.app.vocabularies.Users"] = "Manage Users"

# add custom
PERMISSIONS["myproject.productcategeories"] = "Myproject: Mypermission"

ksuess · 2021-11-30T15:15:56Z

zope.component is kind of complex. I think there is a permission checker missing in zope.component.zcml.utility

        if component:
            # checker = Checker({
            #     '__call__': permission
            # })
            checker = _checker(component, permission, None, None)
            component = proxify(
                component,
                checker,
                provides,
                permission
            )

https://github.com/zopefoundation/zope.component/blob/5bd246ed532d8ace5034bb09777b9f5b8eb87150/src/zope/component/zcml.py#L398-L401

Even with the checker its not obvious for me why the permission is not regarded.

So I propose to add a check in endpoint 'vocabularies'.
#1287

cekk · 2021-12-01T15:16:51Z

I really like this implementation!

Just one thing: these changes basically are exposing all vocabularies to everyone that can View the context where the endpoint is called.

@vocabularies will expose all the available vocabularies to anonymous users and then when we want to get one of them, there is the permission check (now with the mapping with PERMISSIONS variable and later with the #1287).

Is it ok to expose to everyone some vocabularies like the ones for permissions, roles, groups, etc. ? Only Users are protected somehow because are defined in PERMISSIONS mapping.

jensens · 2021-12-01T15:37:00Z

Is it ok to expose to everyone some vocabularies like the ones for permissions, roles, groups, etc. ?

Good question, but that's how its been in the past with the old @@getVocabulary endpoint. If we want to change this it is probably a new PLIP to discuss.

cekk · 2021-12-01T15:48:35Z

you're right but i didn't care on old plone versions because you needed to know the view and the vocabulary name. With restapi, the endpoint returns all the infos and a list of vocabularies.

Where is the right place to open that PLIP? Here or in a different repo? For me this could be only a restapi-related problem

jensens · 2021-12-01T16:06:01Z

PLIPs are always going to Products.CMFPlone - theres a template for this.

ksuess linked an issue Oct 30, 2021 that may be closed by this pull request

"plone.restapi: Access Plone vocabularies" permission is granted too strict by default #1174

Closed

ksuess force-pushed the restriction-vocabularies branch from 4147426 to 6353561 Compare October 31, 2021 09:17

ksuess requested review from tisto and jensens October 31, 2021 09:39

ksuess force-pushed the restriction-vocabularies branch from 6353561 to 708de0d Compare October 31, 2021 15:23

jensens requested changes Nov 2, 2021

View reviewed changes

ksuess force-pushed the restriction-vocabularies branch from 9773c65 to 4dee9ac Compare November 5, 2021 08:52

ksuess marked this pull request as ready for review November 5, 2021 10:00

jensens reviewed Nov 5, 2021

View reviewed changes

src/plone/restapi/services/vocabularies/configure.zcml Show resolved Hide resolved

jensens reviewed Nov 5, 2021

View reviewed changes

src/plone/restapi/services/vocabularies/configure.zcml Show resolved Hide resolved

ksuess added 6 commits November 24, 2021 10:35

Test authorization for accessing special built-in vocabularies

5a66ad7

See plone.app.vocabularies.PERMISSIONS

black, flake8, isort

3a2b68b

Create 1258.bugfix

4647945

Import from plone.app.vocabularies.security, instead of plone.app.voc…

bc583b1

…abularies

Check DEFAULT_PERMISSION for all vocabularies (built-in and custom)

f4ad5a9

ksuess force-pushed the restriction-vocabularies branch from 68397cd to f4ad5a9 Compare November 24, 2021 09:37

ksuess added 2 commits November 24, 2021 12:43

Modify docstring

05a7f28

Switch back to former import of PERMISSIONS for vocabulary

264c9ea

jensens approved these changes Nov 24, 2021

View reviewed changes

jensens merged commit 8f1d624 into master Nov 24, 2021

jensens deleted the restriction-vocabularies branch November 24, 2021 14:35

davisagli mentioned this pull request Oct 15, 2024

Vocabularies endpoint: Use p.a.content permission lookup #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower restriction of vocabularies endpoint #1258

Lower restriction of vocabularies endpoint #1258

ksuess commented Oct 30, 2021 •

edited

Loading

mister-roboto commented Oct 30, 2021

ksuess commented Oct 31, 2021

ksuess commented Oct 31, 2021

jensens Nov 2, 2021

ksuess Nov 5, 2021

jensens Nov 5, 2021

jensens Nov 5, 2021

ksuess Nov 24, 2021

jensens Nov 2, 2021

ksuess Nov 5, 2021

tisto Nov 23, 2021

jensens Nov 23, 2021

jensens commented Nov 2, 2021 •

edited

Loading

ksuess commented Nov 5, 2021

ksuess commented Nov 5, 2021

ksuess commented Nov 5, 2021

jensens Nov 5, 2021 •

edited

Loading

jensens commented Nov 5, 2021

jensens commented Nov 5, 2021

ksuess commented Nov 24, 2021

sneridagh commented Nov 25, 2021

ksuess commented Nov 25, 2021

jensens commented Nov 25, 2021

sneridagh commented Nov 25, 2021

sneridagh commented Nov 26, 2021 •

edited

Loading

jensens commented Nov 27, 2021 •

edited

Loading

ksuess commented Nov 30, 2021 •

edited

Loading

cekk commented Dec 1, 2021

jensens commented Dec 1, 2021

cekk commented Dec 1, 2021

jensens commented Dec 1, 2021 •

edited

Loading

Lower restriction of vocabularies endpoint #1258

Lower restriction of vocabularies endpoint #1258

Conversation

ksuess commented Oct 30, 2021 • edited Loading

mister-roboto commented Oct 30, 2021

ksuess commented Oct 31, 2021

ksuess commented Oct 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensens commented Nov 2, 2021 • edited Loading

ksuess commented Nov 5, 2021

ksuess commented Nov 5, 2021

ksuess commented Nov 5, 2021

jensens Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

jensens commented Nov 5, 2021

jensens commented Nov 5, 2021

ksuess commented Nov 24, 2021

sneridagh commented Nov 25, 2021

ksuess commented Nov 25, 2021

jensens commented Nov 25, 2021

sneridagh commented Nov 25, 2021

sneridagh commented Nov 26, 2021 • edited Loading

jensens commented Nov 27, 2021 • edited Loading

ksuess commented Nov 30, 2021 • edited Loading

cekk commented Dec 1, 2021

jensens commented Dec 1, 2021

cekk commented Dec 1, 2021

jensens commented Dec 1, 2021 • edited Loading

ksuess commented Oct 30, 2021 •

edited

Loading

jensens commented Nov 2, 2021 •

edited

Loading

jensens Nov 5, 2021 •

edited

Loading

sneridagh commented Nov 26, 2021 •

edited

Loading

jensens commented Nov 27, 2021 •

edited

Loading

ksuess commented Nov 30, 2021 •

edited

Loading

jensens commented Dec 1, 2021 •

edited

Loading