-
Notifications
You must be signed in to change notification settings - Fork 3
Use Bad Request for invalid inputs in /chares #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| message EvidenceOut { | ||
| Status status = 1; | ||
| bytes evidence = 2; | ||
| uint32 statusCode = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always supposed to carry an HTTP status code?
If so, I suggest we make it self-evident in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call it httpStatusCode? I believe the usage is mainly to tell what type of HTTP error is encountered by the leaf attesters, though this patch sets the statusCode to be 200 when the API call is successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coupling the REST API with the backend protocol may or may not be a good idea 😄
This kind of coupling can be problematic if we want to reuse the backend protocol with another API (e.g., CoAP), for example.
A better, albeit clunkier, approach would be to define the backend protocol and its own codes, and have a mapping function defined by each frontend protocol (HTTP, CoAP, other).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over RFC 7252 and it seems CoAP also uses 400 for Bad Requests, 500 for internal server errors. Now, all errors reported by the leaf attesters are treated as 500 Internal Server Error even the error is caused by the client (e.g. invalid nonce), and we want to categorize the errors returned by the leaf attesters. To make the naming of statusCode more self-evident, we could call it subattesterResponseCode, or just respondCode.
Given ratsd core is still HTTP-based, i.e. writing to the httpResponseWriter, should we adopt 400 Bad Request, 500 Internal Server error from HTTP/CoAP as the statusCode between the lead attester and the leafs, and defer the decoupling until we refactor the ratsd core to support other protocols using the same backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the two categories you have highlighted are good:
- caller input errors (that map to HTTP 4xx and CoAP 4.xx)
- business logic error (that map to HTTP 5xx, CoAP 5.xx)
If we also add a "frontend protocol” error mapper, e.g.:
func responseCodeToHTTP(uint32 responseCode) (code int) {
// switch ...
}we are good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the mapper in the server code.
Use HTTP 400 Bad Request when the subattester fail to parse the input JSON tsructure, or the expcted length of the nonce doesn't match. Signed-off-by: Ian Chin Wang <[email protected]>
5bf4868 to
04891f8
Compare
Use HTTP 400 Bad Request when the subattester fail to parse the input JSON tsructure, or the expcted length of the nonce doesn't match.