owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.4k stars 182 forks source link

Ability to return error messages in Webdav response bodies #1293

Open PVince81 opened 4 years ago

PVince81 commented 4 years ago

In OC 10, the response body for errors/exceptions in Webdav would contain an XML body generated by Sabre DAV. In OCIS the body is empty in such cases.

Example Sabre response:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>No public access to this resource., Username or password was incorrect, No 'Authorization: Bearer' header found. Either the client didn't send one, or the server is mis-configured, Username or password was incorrect</s:message>
</d:error>

TODOs:

Related tickets:

As discussed with @butonic we might still want a mechanism for returning more information about the error, but it's currently unclear what the format would be.

PVince81 commented 4 years ago

I suspect that the desktop client at least is already parsing some error messages as we have some status codes like 423 Locked where the message is important.

So we'll still want to return a message there. If we change the format from Sabre XML to something else, it would require changes on the clients. For now I'd tend to want to use the same format as before, even if it says "Sabre" when it's not Sabre...

@michaelstingl @TheOneRing @felix-schwarz

TheOneRing commented 4 years ago

Yes https://github.com/owncloud/client/blob/master/src/libsync/abstractnetworkjob.cpp#L405

PVince81 commented 4 years ago

Interesting, it seems the client code doesn't rely on namespaces, so we could almost rename it to an OC specific namespace ? @TheOneRing

PVince81 commented 4 years ago

at this point if we want to keep compatibility we could simply return the same responses that Sabre XML had.

an alternative would be to examine and/or extend the precondition responses in the Webdav spec, which comes with its own elements depending on the error cases: https://tools.ietf.org/html/rfc4918#section-16

PVince81 commented 4 years ago

as discussed with @butonic, as a first step we'll return the exact same responses, even if they include the Sabre namespace. This will keep compatibility.

in the future we can think of adding error codes

felix-schwarz commented 4 years ago

The iOS SDK is currently looking for a d:error node and then at s:exception and s:message inside it.

It has specific support for converting particular s:exception values like Sabre\DAV\Exception\ServiceUnavailable and Sabre\DAV\Exception\NotFound into SDK-internal error codes to allow more targeted error handling.

Messages returned via s:message are used or passed through to the user as additional or only information in several places.

In the past @michaelstingl had also aimed for error messages returned by the server to be localized and ready for presentation to the user (IIRC to unify messages and simplify client implementations).

Here's the code in the SDK: https://github.com/owncloud/ios-sdk/blob/develop/ownCloudSDK/Errors/NSError+OCDAVError.m

Adapting it to different XML elements to provide an error description shouldn't be a problem.

RFC 4918 section 16 looks good conceptually, but I believe should be extended with additional elements similar to s:exception and s:message that clients can fall back to if they don't support or understand the returned error-specific tags.

@PVince81 @TheOneRing

felix-schwarz commented 4 years ago

Same format for compatibility + possible evolution of the format in the future sounds good. 👍

refs commented 3 years ago

we did introduce a package for this on Reva; we need to revamp this issue.

individual-it commented 2 years ago

I've tried to access webdav with other tools like the file-manager and LibreOffice and they don't ask for a username/password. I believe this issue could be the reason.

fschade commented 5 months ago

Please open again if the ticket is still relevant