thorium-reader
thorium-reader copied to clipboard
edrlab
HTTP GET/POST/PUT (LCP/LSD), support for "Problem Details JSON" (RFC7807)
Related issue: https://github.com/readium/readium-desktop/issues/326
We need to better support schemes such as "Problem Details JSON" (RFC7807) for server "error" responses (to be precise: response bodies associated with non-2xx status codes).
The current code obfuscates the response by creating an Error object with a syntactically non-parsable string (at least, not reliably parsable).
https://github.com/readium/readium-desktop/blob/04deba11264bd82d9af90aeb58d3f0b2681e6fc1/src/common/utils/http.ts#L58-L61
Suggestion (1): throw an Error object with smarter heuristics to build the error payload, maybe modelled on the LCP/LSD return/renew/register principles (copied here):
// Both LSD "renew" and "return" interactions can return errors (i.e. Promise.reject => try/catch with async/await) // when the server responds with HTTP statusCode < 200 || >= 300. // The err object in the above code snippet can be a number (HTTP status code) when no response body is available. // Otherwise, it can be an object with the httpStatusCode property (number) and httpResponseBody (string) // when the response body cannot be parsed to JSON. // Otherwise, it can be an object with the httpStatusCode property (number) and other arbitrary JSON properties, // depending on the server response. Typically, compliant LCP/LSD servers are expected to return Problem Details JSON (RFC7807), // which provides title type and details JSON properties. // See https://readium.org/technical/readium-lsd-specification/#31-handling-errors
Note that there should be an externalized reusable helper function (to create the appropriate throw Error() payload), so that the same logic can be applied on the calling side of httpGet() (try {} catch() {} to capture promise rejection due to other errors not handled inside httpGet(), such as HTTP timeout).
Therefore, even better suggestion (2): throw the actual response body, no pre-processing. Just let the outside try/catch (or promise.then.catch callback chain) handle the error payload.
+1 to reuse the LSD error structure, which is based on RFC7807; from the LSD spec:
To provide a consistent behavior for both clients and end users, all servers must handle errors using the Problem Details JSON object as defined in [RFC7807] with the following additional requirements:
- The server MUST return a title and a type in the Problem Details JSON object
- The server SHOULD attempt to localize both title and detail based on the Accept-Language header sent by the client
Suggested approach, copy/pasted from another discussion regarding the httpGet() utility function (convenient wrapper for the request API):
https://github.com/readium/readium-desktop/pull/541#pullrequestreview-273424534
The response body for HTTP status codes <200 >=300 can contain valuable information, so we should not discard it inside the httpGet() function (we can of course discard this information on the outside / calling side).
At a UX level, we can of course decide to display only user-friendly minimal information (e.g. maybe just the HTTP status code), whilst logging the full error message somewhere else (for example a "more info" expandable area in an popup modal dialog box, or some kind of textual application log that users can easily have access to, and copy-paste from, in order to report bugs to the development team, or the publication / content providers).
My suggestion is therefore to throw an Error with a text message corresponding to the JSON.stringify() of a well-defined TypeScript data structure, such as export interface IHttpGetErrorPayload { url: string, httpStatus: number, responseBody?: JsonMap | string, responseType?: string }. The responseBody JSON property is optional and is obtained from the response.body (JsonMap is useful for properly-formatted returns, like RFC7807 "Problem Details JSON", and string accommodates text/html, or just text/plain ... in other words, no support for Buffer responses, which I think a perfectly acceptable in our context). The optional responseType JSON field corresponds to response.headers.get("content-type"). I think this is all we would need to convey in the error message. Thoughts?
Consequently, on the receiving end / API consumer side (i.e. try/catch(err) of the httpGet() promise, or .then().catch(err)), the code can do: const errJsonOrString = JSON.parse(err.message) and check with typeof errJsonOrString === "string" | "object". If object, then check for known IHttpGetErrorPaylaod data structure (i.e. by introspecting the presence of the JSON properties errJsonOrString.url + errJsonOrString.httpStatus + errJsonOrString.responseBody? + errJsonOrString.responseType?), and once again: const responseJsonOrString = JSON.parse(errJsonOrString.responseBody), check with typeof responseJsonOrString === "string" | "object" (maybe also cross-check with errJsonOrString.responseType, for example typeof responseJsonOrString === "object" to match application/json prefix (could be application/json+xxx)).
Finally, format the received data as a string suitable for displaying to the end-user (warning: in the case of typeof responseJsonOrString === "string" and errJsonOrString.responseType == "text/html", makes sure to sanitize responseJsonOrString before inserting into the UI!).
This issue can be narrowed-down to LCP/LSD, so I changed the issue title to a more explicit description
I just updated the codebase of the LCP server to generate "problem details" according to the spec (there was no param "type" in the structures). Now a problem details is of type
{ "type": "http://readium.org/license-status-document/error/notfound", "title": "Failed to find the license ID", "status": 404 }
if I ask for example to the lsd server a document status with wrong license id -> ex http://192.168.0.1:8990/licenses/id-errone/status
a "detail" appears in some cases, e.g. if I send a register request with a valid license id but an empty id:
http://192.168.0.1:8990/licenses/ae824ebe-50ac-4d00-b75b-31188faf7fcd/register?id=&name=devicename1
{ "type": "http://readium.org/license-status-document/error/registration", "title": "Bad Request", "status": 400, "detail": "device id and device name are mandatory and their maximum length is 255 bytes" }
The information we should be able to access from Thorium are title and detail. It would be useful in order to find some ugly issues LCP license providers may face.