plone.rest breaks theme file uploads

[displacedaussie]

Problem

In the Plone theme editor it's possible to upload files.

Via the upload pattern, this ends up attempting to POST the file to something like:

http://localhost:8080/Plone/portal_resources/theme/test-theme/themeFileUpload

However this request gets flagged as an IAPIRequest by plone.rest and is therefore handled by RESTTraverse:

https://github.com/plone/plone.rest/blob/master/src/plone/rest/traverse.py#L17

(Pdb) self
<plone.rest.traverse.RESTTraverse object at 0x113613810>
(Pdb) request
<HTTPRequest, URL=http://localhost:8080/Plone/portal_resources>
(Pdb) name
'portal_resources'

During traversal it gets the portal_resources object, but this isn't IContentish or an IService and so it raises a KeyError rather than returning the object.

(Pdb) obj
<BTreeFolder2 at /Plone/portal_resources>
(Pdb) IContentish.providedBy(obj)
False
(Pdb) IService.providedBy(obj)
False

https://github.com/plone/plone.rest/blob/master/src/plone/rest/traverse.py#L25

When handling the Exception, there are no valid view lookups so it raises the Error. This then results in a 404 to the themeFileUpload URL and a "File transfer failed" in the theme file upload UI.

Options

• Check for more than IContentish and IService during traversal so that objects like this are returned correctly?
• Don't flag this request as an IAPIRequest so it's handled by normal traversal?
• Configure something elsewhere (in plone.resource?) so that traversal works as expected?

[tisto]

@displacedaussie thank you for your detailed bugreport! Would you mind providing us with the full details of the POST request (e.g. headers, payload)? This makes it easier for us to understand why this request ends up in plone.rest. If you want to help us to fix this problem even quicker, you could write a failing unit/integration test with that particular request.

[tisto]

@displacedaussie can you please provide us with the full POST request? I still don't fully understand what goes on.

[djay]

@tisto Here is sample request from the theme editor. I've also put a failing test in #72

Haven't created a test yet but here is what the request looks like

General:
Request URL:http://localhost:8080/Plone/portal_resources/theme/asdsd/themeFileUpload?_authenticator=cdabc0f0a58dd5f5b910d36c2d91bd2452a94479
Request Method:POST
Status Code:200 OK
Remote Address:127.0.0.1:8080
Referrer Policy:no-referrer-when-downgrade

Response headers:
Content-Length:21
Content-Type:application/json
Date:Fri, 16 Feb 2018 08:11:20 GMT
Server:Zope/(2.13.26, python 2.7.14, darwin) ZServer/1.1
X-Frame-Options:SAMEORIGIN

Request headers:
Accept:application/json
Accept-Encoding:gzip, deflate, br
Accept-Language:en-US,en;q=0.9
Authorization:Basic YWRtaW46YWRtaW4=
Cache-Control:no-cache
Connection:keep-alive
Content-Length:2253194
Content-Type:multipart/form-data; boundary=----WebKitFormBoundarymOeiMFE1V2Acr015
Cookie:_pk_id.1.1fff=f9e2a914c3bc1362.1507226287.1.1507226468.1507226287.; __qca=P0-866781254-1511770439608; _ga=GA1.1.1561024462.1511770438
Host:localhost:8080
Origin:http://localhost:8080
Referer:http://localhost:8080/Plone/++theme++asdsd/@@theming-controlpanel-mapper
User-Agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36
X-Requested-With:XMLHttpRequest

Query string paramaters:
_authenticator:cdabc0f0a58dd5f5b910d36c2d91bd2452a94479

Request payload:
------WebKitFormBoundarymOeiMFE1V2Acr015
Content-Disposition: form-data; name="file"; filename="Screen Shot 2018-02-16 at 3.08.15 pm.png"
Content-Type: image/png


------WebKitFormBoundarymOeiMFE1V2Acr015--

[djay]

@tisto possibly there should also be a test in https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/tests/test_controlpanel.py. Seems like there are no tests for the theme editor end points.
But that would only pickup an error during 5.1 when plone.rest is merged. Seems like plone.rest should test it doesn't get in the way of stuff thats already in plone.

[djay]

@tisto the code that is supposed to be called is https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/browser/themefile.py#L25

[tisto]

@djay if that code sets the Accept header to application/json it expects a JSON structure as a response. I'd check first if this is the case and then if it is necessary. I don't see why standard Plone code would need to do something like this. plone.rest is the wrong place to fix this IMHO.

[djay]

@tisto plone code does expect and get a JSON structure in response (I pointed you to the code where that JSON is given) The problem is that plone.rest is blocking the normal plone code from giving that response. Assuming that plone.rest is the only thing in plone that uses rest is a failure in plone.rest. plone.rest already has exceptions to where it lets the normal plone code return a json response. This is adding one more. What is the problem with that?

[tisto]

@djay I would like to see and understand why this exception is necessary before we hard-code an exception into plone.rest. I see from your POST example that the request sends an Accept header application/json and Content-Type multipart. What I don't see is the response in JSON and how this is actually used in the code.

[djay]

@tisto I think the log above is what happens when plone.rest gets in the way.
The request is generated from dropzone.js which is used as part of the thememapper plugin.
The endpoint and the response JSON is https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/browser/themefile.py#L29

[tisto]

@djay ok, this was the piece of code I was looking for. :)

So what this view does is return a very minimal JSON structure such as:

{'success':'create'}

or

{'failure':'error'}

This is not the accurate behavior from the REST architectural point of view. We are using the body of the response to indicate a success or failure. This is what HTTP status codes have been invented for. We could do a workaround in plone.rest. Though, I think it is the wrong approach to build a workaround for badly written code. In addition, fixing this in plone.app.theming is a lot easier that writing a workaround in plone.rest.