It's possible to set an invalid short name when adding content

[davisagli]

If an id is specified while creating content via the FolderPost endpoint, it is used without validating that it meets the criteria for a valid id. For example, it's possible to add an item with an id containing spaces.

What I would expect: either the id should be normalized (this is what already happens if it is set while editing the item later), or trying to add an item with an invalid id should return a 400 response with a helpful error message.

Note: This might be considered a backwards incompatible change, since someone calling the API may be assuming that the item has the id that was specified, and not a normalized form of it. But, it could also be considered a bugfix, because it never should have been possible to add items with an invalid id.

Thoughts on how to handle this, @tisto?

[erral]

Some time ago we faced an issue adding a content with layout as an id, and found that Plone was doing some id checks, somewhere, sometimes. See https://github.com/plone/Products.CMFPlone/issues/1931 and https://github.com/plone/Products.CMFPlone/pull/1932.

I think we should have one single place in CMFPlone where the id validation is done, and then use that validator here in p.restapi too.

[davisagli]

@erral There is already a way to get a valid id that takes into account those checks, which is an INameChooser adapter on the container. The restapi endpoint for adding content already uses it when the API client did not explicitly specify an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/utils.py#L93

But, that is getting skipped when the client specified an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/add.py#L93

It's an easy fix to use the name chooser there too. The question is, will the change in behavior cause any problems?

[tisto]

@davisagli if I recall correctly we added the id option mainly for migrations, so that a migration algorithm is able to set an ID. You are correct that we should validate this id no matter what.

Adding a check for a valid ID on the passed id param is not a breaking change IMO. We are just preventing the system from ending up with an invalid id/path.

[erral]

I agree with Timo, I think we should do the same as Classic UI does. If we don't allow spaces in id in Classic we should neither allow that in p.restapi

[sneridagh]

+1 for Timo's pov.

[mauritsvanrees]

When I see David's PR, I have a doubt. If the id in itself is perfectly valid, say queen, but the id is already taken, it will be normalised to queen-1. What if someone relies on the id either really staying the same or throwing an error? That is why plone.api.content.create has a safe_id option:

safe_id (boolean) – When False, the given id will be enforced. If the id is conflicting with another object in the target container, raise an InvalidParameterError. When True, choose a new, non-conflicting id.

I have never found the name of this option very intuitive, I always have to read the documentation twice to understand if I should use True or False in my use case. But I think plone.restapi should have such an option too: fail if the id is invalid for whatever reason.

The default should probably be to not fail. But I wonder if it is actually common to pass the id here, or if usually you would only pass a title, and Plone figures out a nice id for you.

[jensens]

@mauritsvanrees I agree to improve here, but first @davisagli fix solves the immediate problem. We may want to create a new issue for safe_id

[davisagli]

@mauritsvanrees You have a good point that it could be useful to be able to use this endpoint in a mode that throws an error rather than normalizing the id, if a specific id was requested but is not available.

I wonder if it is actually common to pass the id here, or if usually you would only pass a title, and Plone figures out a nice id for you.

I can think of a few different use cases for passing an id:

1. It's a site migration that is trying to keep the same id as in the source system.
2. Someone is scripting creation of content and knows what id they want to end up with.
3. Someone is creating content with Volto, and entered a value in the short name field. (This is the use case where we found this bug; a user was trying to set a short name containing spaces.)

In all these cases, I can imagine sometimes it is more helpful to succeed with a modified id and sometimes it is more helpful to get a clear error response, so that the human or code controlling the process can apply a special policy for falling back to something else. That supports the idea of adding an option for it.

I'm even leaning a little bit toward returning an error response as the default mode, because then it's more obvious that the requested id isn't being used. I think I'll revise my implementation in #1488 to do that, and wait to add an option for normalizing the id until someone needs it.

[davisagli]

My fix for this was too aggressive, and prevents setting ids that are supposed to be allowed (for example, including capital letters or periods)