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

Add check for protected vocabulary in endpoint 'vocabularies'. #1287

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ksuess
Copy link
Member

@ksuess ksuess commented Nov 30, 2021

Fix 1286

@mister-roboto
Copy link

@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
Copy link
Member Author

ksuess commented Nov 30, 2021

@jenkins-plone-org please run jobs

@ksuess ksuess marked this pull request as ready for review November 30, 2021 19:12
@jensens
Copy link
Sponsor Member

jensens commented Nov 30, 2021

@ksuess Genius! That's how it should work. I love it.

I would just merge this!

Then we can go further:

  • We need the same over in plone.app.content.browser.vocabulary
  • then protect all the plone.app.vocabularies utility registrations with permissions
  • import the PERMISSION_MAP conditional for restapi (to not break older Plones), see below
  • drop PERMISSION_MAP in Plone 6 at all, including its permission check.
  • document it (which is just pointing to utility basically).

Finally a clean solution.

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Awesome!

Sorry for being picky, I checked quickly and in the cell phone, but shouldn't we also take care of the 401 use case from a formal point of view?

@ksuess
Copy link
Member Author

ksuess commented Dec 11, 2021

May I propose to close this PR.
A registration like below can fool developers into believe that this vocabulary is protected.
But the vocabulary is protected by requesting via REST API endpoint, but not in general.

  <utility
      provides="zope.schema.interfaces.IVocabularyFactory"
      name="plone.restapi.testing.protected_vocabulary"
      component=".tests.test_services_vocabularies.test_vocabulary_factory"
      permission="cmf.ManagePortal"
      />

Thanks for the kind feedback and sorry for the noise.

@ksuess
Copy link
Member Author

ksuess commented Dec 11, 2021

In midterm we can maybe think of a Plone Vocabulary Registry that can deal with permission protection.

A first try: plone/plone.app.vocabularies#69

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

Successfully merging this pull request may close these issues.

Add check for protected vocabularies in endpoint 'vocabularies'.
4 participants