Do not expose private metadata via relationfield serializer. #1635

maethu · 2023-05-01T18:02:33Z

This PR implements two changes:

Only return summary if the user has View permission on target object.
Filter None values in RelationListFieldSerializer, which result from not having a summary for inaccessible content.

mister-roboto · 2023-05-01T18:02:38Z

@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 · 2023-05-01T18:02:41Z

✅ Deploy Preview for plone-restapi canceled.

Name	Link
🔨 Latest commit	`bc3b71e`
🔍 Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/6452a5cb29a6bf00083a7433

maethu · 2023-05-01T20:08:19Z

@jenkins-plone-org please run jobs

jensens · 2023-05-02T08:32:51Z

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.

jensens · 2023-05-02T08:34:04Z

src/plone/restapi/serializer/relationfield.py

+        if value:
+            return [item for item in json_compatible(value) if item]
+        else:
+            return super().__call__()


Suggested change

if value:

return [item for item in json_compatible(value) if item]

else:

return super().__call__()

if value:

return [item for item in json_compatible(value) if item]

return super().__call__()

For the sake of readability: No else needed when the if returns.

erral · 2023-05-02T08:39:06Z

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

jensens · 2023-05-02T08:41:56Z

src/plone/restapi/serializer/relationfield.py

-    if value.to_object:
+    mtool = api.portal.get_tool("portal_membership")
+    if value.to_object and mtool.checkPermission("View", value.to_object):


this is too much overhead and probably slow. Use the Zope basics, those are perfectly fine here:

from AccessControl.SecurityManagement import getSecurityManager getSecurityManager().checkPermission(permission, obj)

btw, if you anyway use plone.api, there is api.user.check_permission too as an alternative.

If I am not wrong, plone.api may only be used in plone.restapi tests right? @plone/restapi-team

I am not sure what the rules are, but if there are those rules, they must be documented somewhere.

jensens · 2023-05-02T14:53:38Z

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 · 2023-05-02T14:55:40Z

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.

…w permission on the current object.

maethu · 2023-05-03T18:26:37Z

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.

jensens · 2023-05-05T11:36:31Z

src/plone/restapi/serializer/relationfield.py

@@ -17,7 +18,12 @@
 @adapter(IRelationValue)
 @implementer(IJsonCompatible)
 def relationvalue_converter(value):
-    if value.to_object:
+    has_permission = api.user.has_permission(


@plone/restapi-team In a comment above @erral wrote plone.api usage is allowed in this package in tests only and asked if this is right. May one you confirm this?

We agreed that we can use plone.api in tests. Though, if I am not mistaken we also decided lately that plone.restapi can use plone.api everywhere, correct? Sorry for the late reply here. Just want to get this straight.

jensens · 2023-05-05T11:38:29Z

src/plone/restapi/serializer/relationfield.py

-    if value.to_object:
+    has_permission = api.user.has_permission(
+        "Access Contents Information",
+        obj=value.__parent__


I do not get why we now check on parent? This is is probably a misunderstanding and wrong.

@jensens

Probably another miss-understanding on my end.

I used the parent, since @davisagli wrote in the issue #1634
@maethu In general, permission to view an object's metadata in listings is controlled by the "Access contents information" permission on its container, not the View permission.

I'm happy to change this!

tisto · 2023-08-23T08:57:49Z

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

davisagli · 2023-08-23T15:01:17Z

@tisto From my point of view, I am waiting for a proposal for how to resolve the problem I mentioned in #1635 (comment)

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 · 2023-08-24T12:31:13Z

@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.

maethu force-pushed the mle-fix-relationfield-permission-issue branch from 66027a5 to bceee96 Compare May 1, 2023 18:06

Do not expose private metadata via relationfield serializer.

b5124fa

maethu force-pushed the mle-fix-relationfield-permission-issue branch from bceee96 to b5124fa Compare May 1, 2023 18:12

jensens reviewed May 2, 2023

View reviewed changes

Check for Access contents information on parent object instead of vie…

bc3b71e

…w permission on the current object.

maethu mentioned this pull request May 3, 2023

Do not expose information via relationfield serializer #1634

Open

jensens reviewed May 5, 2023

View reviewed changes

jensens requested changes May 5, 2023

View reviewed changes

tisto added the 22 status: in-progress label Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not expose private metadata via relationfield serializer. #1635

Do not expose private metadata via relationfield serializer. #1635

maethu commented May 1, 2023

mister-roboto commented May 1, 2023

netlify bot commented May 1, 2023 •

edited

Loading

maethu commented May 1, 2023

jensens commented May 2, 2023

jensens May 2, 2023

jensens May 5, 2023

erral commented May 2, 2023

jensens May 2, 2023

jensens May 2, 2023

erral May 2, 2023

jensens May 2, 2023

jensens commented May 2, 2023

davisagli commented May 2, 2023

maethu commented May 3, 2023

jensens May 5, 2023

tisto Aug 23, 2023

jensens May 5, 2023

maethu May 9, 2023

tisto commented Aug 23, 2023

davisagli commented Aug 23, 2023

maethu commented Aug 24, 2023

Do not expose private metadata via relationfield serializer. #1635

Are you sure you want to change the base?

Do not expose private metadata via relationfield serializer. #1635

Conversation

maethu commented May 1, 2023

mister-roboto commented May 1, 2023

netlify bot commented May 1, 2023 • edited Loading

✅ Deploy Preview for plone-restapi canceled.

maethu commented May 1, 2023

jensens commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erral commented May 2, 2023

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 May 2, 2023

davisagli commented May 2, 2023

maethu commented May 3, 2023

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

tisto commented Aug 23, 2023

davisagli commented Aug 23, 2023

maethu commented Aug 24, 2023

netlify bot commented May 1, 2023 •

edited

Loading