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

Do not expose private metadata via relationfield serializer. #1635

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

maethu
Copy link

@maethu maethu commented May 1, 2023

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
Copy link

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

netlify bot commented May 1, 2023

Deploy Preview for plone-restapi canceled.

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

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

maethu commented May 1, 2023

@jenkins-plone-org please run jobs

@jensens
Copy link
Member

jensens commented May 2, 2023

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.

Comment on lines +40 to +43
if value:
return [item for item in json_compatible(value) if item]
else:
return super().__call__()
Copy link
Member

Choose a reason for hiding this comment

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

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__()

Copy link
Member

Choose a reason for hiding this comment

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

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

@erral
Copy link
Member

erral commented May 2, 2023

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

Comment on lines 20 to 22
if value.to_object:
mtool = api.portal.get_tool("portal_membership")
if value.to_object and mtool.checkPermission("View", value.to_object):
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@jensens
Copy link
Member

jensens commented May 2, 2023

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

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

maethu commented May 3, 2023

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

if value.to_object:
has_permission = api.user.has_permission(
"Access Contents Information",
obj=value.__parent__
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

tisto commented Aug 23, 2023

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

@davisagli
Copy link
Member

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

maethu commented Aug 24, 2023

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

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

Successfully merging this pull request may close these issues.

Do not expose information via relationfield serializer
6 participants