Allow HTTP 200 response for HTTP file upload PUT request (#586)
Unknown opened 1 year ago

Is your feature request related to a problem? Please describe. Allow Siskin to use HTTP file upload with backend services returning HTTP 200 for new files. For example all object stores based on the widely-used Amazon S3 API do this.

Describe the solution you'd like Accept HTTP 200 as a valid response for the HTTP PUT request.

Something like this should take care of it as all non 200/201 status codes are already filtered out.

- if code == 200 {
-   completionHandler(.failure(.invalidResponseCode(url: slot.getUri)));
- } else {
  completionHandler(.success(slot.getUri));
- }

Describe alternatives you've considered Otherwise it's not possible to directly use object stores and all PUTs need to be forcibly redirected through a proxy which is annoying.

Additional context Most other clients (Conversations, Dino, ...) are fine with this, just Siskin doesn't like it.

Unknown commented 1 year ago

Isn't this like a server feature? Like ejabberd has a s3 contrib module, maybe that module says 201 instead?

How are you storing the files? Which xmpp server software?

Unknown commented 1 year ago

Yes, this is a server feature. But the thing is that the thing that's serving the HTTP upload requests is not the XMPP server, but instead an object storage directly. The "files" are objects in a Minio S3 cluster and the upload goes straight to that (the XMPP server only provides the presigned URL serving as an authorization). This is with Prosody and mod_http_upload_s3. ejabberd with S3 should IMO have the same problem, do we know that that works?

Unknown commented 1 year ago

Siskin is closely following specification of XEP-0363: HTTP File Upload, see https://github.com/tigase/siskin-im/issues/37#issuecomment-624294330 for the details as to why Siskin shows warning dialog.

Unknown commented 1 year ago

Yes, I have read the XEP and I know that it wants a 201. I would also fix it if that would be something that's easily doable but as that endpoint is served by a S3-style API if I were to just patch Minio to return 201 other applications would be unhappy. IMO the XEP should be fixed, but even Conversations written by the author of that XEP does support 200 since its introduction [1]. I just think that it is counterproductive to show that message when the operation clearly worked because of a technicality in the XEP, especially when there seems to be consensus among all other clients that this is OK and this not really carrying any risk of introducing ambiguities or incompatibilities.

I'm going to contact Daniel Gultsch to see if anything can be done on the standards side.

[1] https://codeberg.org/iNPUTmice/Conversations/commit/e48788e821c1c2cdab3647a0f4cce197ea626fe9

Unknown commented 1 year ago

@lorenz the best and the correct way to ensure all software works together and is compatible, is when everybody follows the spec. Suggesting to deviate from the spec to be compatible with others who deviated from the spec is counterproductive. If we follow this path, we end up with chaos.

If the spec is wrong, change the spec and let everybody adjust software to adhere to the spec.

Unknown commented 1 year ago

@arturhefczyc I generally agree, I contacted the XEP author about this issue, hopefully we can get an errata or similar legitimizing using both 200 as well as 201.

Unknown commented 1 year ago

+1 for supporting 200. The spec shouldn't even require 201 specifically, but leave that to the HTTP spec IMO. 200 is just as good as 201 across almost all of the Web. It's an easy fix here, and we have the same issue with the ejabberd module indeed (i.e. not with the module, but with Garage/S3). Just switched to it a couple of days ago, and all clients I tested work fine, except for only the Apple ones (Beagle, Siskin, Snikket, Monal).

Unknown commented 1 year ago

@raucao Out of curiosity, I've checked HTTP 1.1 RFC 2616 for a response on a PUT request and according to the RFC servers after creating a new object due to a PUT request should respond with201:

If a new resource is created, the origin server MUST inform the user agent via the 201 (Created) response. If an existing resource is modified, either the 200 (OK) or 204 (No Content) response codes SHOULD be sent to indicate successful completion of the request

and checking HTTP/2 RFC 9110 we have:

If the target resource does not have a current representation and the PUT successfully creates one, then the origin server MUST inform the user agent by sending a 201 (Created) response. If the target resource does have a current representation and that representation is successfully modified in accordance with the state of the enclosed representation, then the origin server MUST send either a 200 (OK) or a 204 (No Content) response to indicate successful completion of the request.

In the case of HTTP File Upload, we are always creating a new resource after the upload (as there is no support for modification), so requirement of 201 response code is OK in this case (XEP matches expected result from the HTTP specification). However, following just HTTP specification would allow us to accept also 200 and 204, but not responding with 201 on HTTP PUT creating a new resource is not complying with HTTP specification (but could even be considered a violation of HTTP protocol).

Due to that even leaving response code just to HTTP, we should be expecting 201 response but we could accept 200 or 204.

Unknown commented 1 year ago

I would indeed prefer if S3 returned 201 (for POST this is even configurable with success_action_status) as that would be the more correct response. But that ship has sailed more than 10 years ago, S3 is THE object storage API (pretty much any object storage system implements it). It is basically impossible to get AWS to change their implementation (as it's a breaking change) and thus all third-party implementations won't either.

I contacted the author of the XEP to maybe update it or issue an errata, but haven't heard back.

Unknown commented 1 year ago

@hantu85 Yeah, I just meant the spec doesn't have to include that, since it's already in the HTTP spec. But even then, half the Web is violating that spec for PUTs anyway, and as @lorenz said, we won't get the S3 API to change either.

Btw, here's how I fixed it for our API, using the Nginx Lua Module.

issue 1 of 1
Type
Improvement
Issue Votes (0)
Watchers (0)
Reference
tigase/_clients/siskin-im#586
Please wait...
Page is in error, reload to recover