search

plone / plone.restapi

RESTful API for Plone.
http://plonerestapi.readthedocs.org/
84 stars 73 forks source link

Do not expose private metadata via relationfield serializer. #1635

Open maethu opened 1 year ago

maethu commented 1 year ago

Fixes #1634

This PR implements two changes:

  1. Only return summary if the user has View permission on target object.
  2. Filter None values in RelationListFieldSerializer, which result from not having a summary for inaccessible content.
mister-roboto commented 1 year ago

@maethu 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!

netlify[bot] commented 1 year ago

Deploy Preview for plone-restapi canceled.

Name Link
Latest commit bc3b71eacc494a124386f232f199344c2412126c
Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/6452a5cb29a6bf00083a7433
maethu commented 1 year ago

@jenkins-plone-org please run jobs

jensens commented 1 year ago

The permission to check if metadata access is allowed is "Access Contents Information"

Access Contents Information

This permission allows access to an object, without necessarily viewing the object. For example, a user may want to see the object’s title in a list of results, even though the user can’t view the contents of that file.

erral commented 1 year ago

If the user hasn't the View permission on the target object, should it be returned as a related object at all?

jensens commented 1 year ago

If the user hasn't the View permission on the target object, should it be returned as a related object at all?

In Plone we have these two permissions for a reason. You might want or not want to show an object in listing. You might want to tell, look there is a relation to X even if you're not allowed to view X details. Using these two permissions this is possible. Usually both are mapped the same in workflow permission mappings. But, depends on the customization, not always.

davisagli commented 1 year ago

Also consider: The serialization is also used when loading the item for editing. What should happen if a user has permission to edit an item that has references to items that they do not have permission to view? I am not sure what the right answer is, but I am sure that it is not silently removing the reference.

maethu commented 1 year ago

Yes my bad... I should know this 🙈 The issue is in my workflow definition.

But to clarify: In the context if a relation, or multiple relations, the parent of a relation is the current object.

So if doc1 has doc2 as related item. And the user does not have "Access Contents Information" on doc1. Does this mean relatedItems should be empty?

I totally doe see the issue regarding: Also consider: The serialization is also used when loading the item for editing. What should happen if a user has permission to edit an item that has references to items that they do not have permission to view?

This is no longer related to my issue, but I'm a bit confused what make sense and what not.

I adapted the code to check for "Access content information" on the parent of the relation.

But I'm not sure if the helps a lot... or if it makes things more confusing. Feel free to close this PR, it's not really a bug in the end, more a integration issue depending on your application.

I implemented some custom relation serializer on my end, which filters relations for anonymous users depending on the View permission. Because for my use case this makes the most sense.

tisto commented 11 months ago

@maethu @davisagli @jensens what's the status of this PR?

davisagli commented 11 months ago

@tisto From my point of view, I am waiting for a proposal for how to resolve the problem I mentioned in https://github.com/plone/plone.restapi/pull/1635#issuecomment-1531626897

I think we need something like a way to include UIDs for items that the current user does not have permission to view. Probably this should be a querystring option. Then the edit view can ask for them and make sure they are not removed, but other use cases can leave them out.

maethu commented 10 months ago

@maethu @davisagli @jensens what's the status of this PR?

Sorry got a bit lost on my end. I'm happy to change the permission check if that make sense. But I'm not sure. As of now it might cause more harm than it helps.