ramosbugs
Handle 'server_error' and 'temporarily_unavailable' sub error types defined in RFC 6749 (and errata)?
Looking at an errata for RFC 6749 I noticed that two error response codes server_error and temporarily_unavailable defined by RFC 6749 are missing in one case in the spec.
However, looking at the code for 4.0.0-beta.1 this crate does not seem to have specific handling for these "extra" error "codes" at all when constructing an RequestTokenError value, e.g. in BasicErrorResponseType it lists the other "codes" but not these "extra" two "codes":
impl BasicErrorResponseType {
pub(crate) fn from_str(s: &str) -> Self {
match s {
"invalid_client" => BasicErrorResponseType::InvalidClient,
"invalid_grant" => BasicErrorResponseType::InvalidGrant,
"invalid_request" => BasicErrorResponseType::InvalidRequest,
"invalid_scope" => BasicErrorResponseType::InvalidScope,
"unauthorized_client" => BasicErrorResponseType::UnauthorizedClient,
"unsupported_grant_type" => BasicErrorResponseType::UnsupportedGrantType,
ext => BasicErrorResponseType::Extension(ext.to_string()),
}
}
}
I wonder whether these two types should be detected and also result in the RequestTokenError::Request variant (documented as "An error occurred while sending the request or receiving the response (e.g., network connectivity failed)."), otherwise client code has to manually inspect the content of RequestTokenError::Other when looking to detect these cases.
hey @ximon18,
it's not clear to me what the status of the errata on that site are, and whether there's any sort of formalized approval process. I think it would be fine for the client to accept the additional error codes, but this crate can also be used by servers, and I'd be more hesitant to make it easy for servers to return error codes that standardized clients may not understand. thoughts?
separately, I think this change would have to wait for 5.x since I promoted 4.x to beta, and this would be a breaking change.
The status is that the errata has not yet been accepted. However, in this particular case the "missing" fields are actually already in the rfc elsewhere but still not, if I recall correctly, implemented by the crate.
Ah, extending the set of error codes would break existing match blocks as it was not marked non exhaustive, is that it?
The status is that the errata has not yet been accepted. However, in this particular case the "missing" fields are actually already in the rfc elsewhere but still not, if I recall correctly, implemented by the crate.
the RFC mentions these values in the context of the implicit flow, which is always handled within the browser since URL fragments aren't sent along with HTTP server requests. originally, this crate wasn't targeting browser client-side code. now that it has a WASM target, it may be worth revisiting that. still, I think in that case it would be preferable to add a brand new enum to represent errors returned by the implicit flow. this wouldn't be a breaking change because there isn't any current code for handling the response to the implicit flow (I think).
looking more closely at the errata, I think it's incorrect, and the original RFC is right. the errata references section 5.2, which is about the response to the HTTP POST request to the token endpoint. if the server experiences an error, it's supposed to follow the HTTP spec and return a 500 status code (or 503 in the case of temporarily_unavailable). the text in the errata even says this (copied from the implicit flow text):
This error code is needed because a 500 Internal Server Error HTTP status code cannot be returned to the client via an HTTP redirect.
this response isn't being sent in an HTTP redirect though; it's being returned directly to the client. that's why the implicit flow needs these error codes but the token endpoint does not.
Ah, extending the set of error codes would break existing match blocks as it was not marked non exhaustive, is that it?
precisely :-)