proposal: x/oauth2: Add RFC 6749 error fields to RetrieveError

[hickford]

Motivation: make it easier for apps to show readable errors to users

OAuth spec RFC 6749 defines JSON error responses with three parameters: error, error_description and error_uri. https://datatracker.ietf.org/doc/html/rfc6749#section-5.2

However error type oauth2.RetrieveError merely holds the raw response body unparsed.

For clients to more easily handle errors and show users readable error messages, it would greatly help to parse JSON error responses and populate corresponding fields:

// RetrieveError is the error returned when the token endpoint returns a
+ // non-2XX HTTP status code or populates RFC 6749's 'error' parameter.
+ // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2
 type RetrieveError struct {
 	Response *http.Response
 	// Body is the body that was consumed by reading Response.Body.
 	// It may be truncated.
 	Body []byte
+ 	// ErrorCode is RFC 6749's 'error' parameter.
+ 	ErrorCode string
+ 	// ErrorDescription is RFC 6749's 'error_description' parameter.
+ 	ErrorDescription string
+ 	// ErrorURI is RFC 6749's 'error_uri' parameter.
+ 	ErrorURI string
}

The ErrorCode field is so named to avoid conflict with the Error() method.

Would it be acceptable to change the string returned by the Error() method to be more user friendly when these fields are set?

Here's a change including parsing logic golang/oauth2#610

[icholy]

Should be ErrorURI

[hickford]

@icholy thanks, amended.

[ianlancetaylor]

CC @golang/security

[hickford]

@ianlancetaylor is there anything I can do to speed up review? I see that there's a backlog of 300+ incoming proposals

https://github.com/golang/proposal#readme

[ianlancetaylor]

The first step here will be to get a comment from someone on @golang/security such as @rolandshoemaker .

[rolandshoemaker]

Unmarshalling the body ourselves, assuming it is standardized, which it looks like it is, seems like a reasonable convenience improvement. The Error return can also be changed without breaking compat, it seems reasonable to just make it output the ErrorDescription or something, rather than just spitting out the entire JSON body.

One question is what to do if the body cannot be unmarshalled, but in that case it is probably reasonable to just spit out the raw body and move on.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

We can't call the field Error string because of the method, but ErrorCode makes it sound like a code, like http.Response.StatusCode, which it's not. It looks to be free-form text. Is that right? Should it be ErrorText? Something else?

[hickford]

@rsc the RFC calls it an "error code" https://datatracker.ietf.org/doc/html/rfc6749#section-5.2

The authorization server responds with an HTTP 400 (Bad Request)
status code (unless specified otherwise) and includes the following
parameters with the response:

error
REQUIRED. A single ASCII [USASCII] error code from the
following:

invalid_request
The request is missing a required parameter, includes an
unsupported parameter value (other than grant type),
repeats a parameter, includes multiple credentials,
utilizes more than one mechanism for authenticating the
client, or is otherwise malformed.

invalid_client
Client authentication failed (e.g., unknown client, no
client authentication included, or unsupported
authentication method). The authorization server MAY
return an HTTP 401 (Unauthorized) status code to indicate
which HTTP authentication schemes are supported. If the
client attempted to authenticate via the "Authorization"
request header field, the authorization server MUST
respond with an HTTP 401 (Unauthorized) status code and
include the "WWW-Authenticate" response header field
matching the authentication scheme used by the client.

invalid_grant
The provided authorization grant (e.g., authorization
code, resource owner credentials) or refresh token is
invalid, expired, revoked, does not match the redirection
URI used in the authorization request, or was issued to
another client.

unauthorized_client
The authenticated client is not authorized to use this
authorization grant type.

[rsc]

ErrorCode it is. Thanks.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group