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

why does the RelationValue serializer not add the UID of the item? #1585

Open
fredvd opened this issue Feb 15, 2023 · 4 comments · May be fixed by #1586
Open

why does the RelationValue serializer not add the UID of the item? #1585

fredvd opened this issue Feb 15, 2023 · 4 comments · May be fixed by #1586
Assignees

Comments

@fredvd
Copy link
Member

fredvd commented Feb 15, 2023

in tnis part of the Serializer ISerializeToJsonSummary is looked up. the problem for me with the default implementation is that it only serializes the absolute_url in the json response, not the UID()

This is probably fine for the volto frontend.. But on deserialisation it has to do a restrictedtraverse to the target item, and it has the full url with the website domain embedded in the id. I am trying to add support for restoring relationValues better in collecfive. importexport and the UID is much better suited there.

The funny thing is that the deserializer already supports just uids and relative paths from the site root in the id key.

(value.to_object, getRequest()), ISerializeToJsonSummary

@sneridagh @avoinea may I ask your advice? just add it to the ISerializeToJsonSummary ?

@mauritsvanrees
Copy link
Member

This seems like a logical change to me. But I would be happy to read a different view point.

I have a PR #1586 that would fix this, and it is just one line. TODO: obviously some test fixes are needed. But if others do not like the approach, I would rather not spend the time.

Alternative is to explicitly request the UID as additional metadata. In a curl command:

curl -H "Accept: application/json" http://localhost:8080/Plone/page?metadata_fields=UID

@davisagli
Copy link
Member

I think the philosophical justification for not including the UID in the REST API is that the REST API aims to always use an external URL as a resource identifier, rather than the UID which is internal.

But, URLs change when an item is moved or renamed, so I can see the benefit of being able to access the UID which is stable. I think we should include it.

Any objection, @tisto?

@tisto
Copy link
Member

tisto commented Aug 25, 2023

@fredvd @davisagli @mauritsvanrees I understand the value of having the UID for exportimport. Though, in general, I am wondering if we are comfortable exposing an internal database ID to anonymous users. I could imagine that this is something a security audit might complain about. What if we would just expose the UID attribute when the user has the manager role? This would solve the problem for exportimport and would not expose the UID for anonymous users.

@davisagli
Copy link
Member

@tisto The thing is, I don't really consider it an internal database ID. Yes, we hide it from end users, but it also has a real purpose which is to be a stable UID that does not change when an item is moved around. That is not only useful inside Plone. If I want to put a stable reference to a Plone item in an external database, the UID would be the best choice.

In security audits at my last job the only concern about exposing internal ids was if they could be used to predict other ids. Completely random uuid4 ids like we use in Plone are not a security problem.

I would be fine with attaching it to a new "View UIDs" permission though. (Always protect actions with a permission, not a specific role. Then the policy for who gets the permission can be adjusted when necessary.)

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 a pull request may close this issue.

6 participants