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?