Skip to content
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

Jump to bottom

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

Closed
hickford opened this issue Jan 28, 2023 · 12 comments
Closed

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

hickford opened this issue Jan 28, 2023 · 12 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Unreleased

Comments

@hickford
Copy link

hickford commented Jan 28, 2023 •
edited

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
@gopherbot gopherbot added this to the Proposal milestone Jan 28, 2023
@icholy
Copy link

icholy commented Jan 29, 2023

Should be ErrorURI

@hickford
Copy link
Author

@icholy thanks, amended.

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 1, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@hickford
Copy link
Author

@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
Copy link
Contributor

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

@hickford hickford mentioned this issue Feb 27, 2023
Closed
@rolandshoemaker
Copy link
Member

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
Copy link
Contributor

rsc commented Mar 1, 2023

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
Copy link
Contributor

rsc commented Mar 15, 2023

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
Copy link
Author

@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
Copy link
Contributor

rsc commented Mar 29, 2023

ErrorCode it is. Thanks.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

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

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

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

@rsc rsc changed the title proposal: x/oauth2: Add RFC 6749 error fields to RetrieveError x/oauth2: Add RFC 6749 error fields to RetrieveError Apr 6, 2023
@rsc rsc modified the milestones: Proposal, Backlog Apr 6, 2023
gopherbot pushed a commit to golang/oauth2 that referenced this issue Apr 11, 2023
@hickford @neild
oauth2: parse RFC 6749 error response
cfe200d 
Parse error response described in https://datatracker.ietf.org/doc/html/rfc6749#section-5.2

Handle unorthodox servers responding 200 in error case.

Implements API changes in accepted proposal golang/go#58125

Fixes #441
Fixes #274
Updates #173

Change-Id: If9399c3f952ac0501edbeefeb3a71ed057ca8d37
GitHub-Last-Rev: 0030e27
GitHub-Pull-Request: #610
Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/451076
Run-TryBot: Matt Hickford <matt.hickford@gmail.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Matt Hickford <matt.hickford@gmail.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Cody Oss <codyoss@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Apr 21, 2023
@hickford hickford changed the title x/oauth2: Add RFC 6749 error fields to RetrieveError proposal: x/oauth2: Add RFC 6749 error fields to RetrieveError Apr 25, 2023
@bobheadxi bobheadxi mentioned this issue May 8, 2023
Merged
bobheadxi added a commit to sourcegraph/sourcegraph that referenced this issue May 9, 2023
@bobheadxi
deps: revert to golang.org/x/oauth2 upstream (#51495)
44e0db3 
The issue this fork was meant to address
(golang/oauth2#441) has been fixed upstream:

- [proposed fix that was part of
fork](golang/oauth2#442)
- [upstream
fix](golang/oauth2@cfe200d)
(golang/go#58125)

Notably, both implementations change the error returned to render more
verbose details on `.Error()` if the response is a status 200 _but_ an
error (see above issues)

A diff against upstream indicates that there are no other changes:
sourcegraph/oauth2#3

## Test plan

Test suites pass
VolhaBakanouskaya pushed a commit to VolhaBakanouskaya/sourcegraph-public that referenced this issue Jun 30, 2023
@bobheadxi @ErikaRS
deps: revert to golang.org/x/oauth2 upstream (#51495)
e5f0c18 
The issue this fork was meant to address
(golang/oauth2#441) has been fixed upstream:

- [proposed fix that was part of
fork](golang/oauth2#442)
- [upstream
fix](golang/oauth2@cfe200d)
(golang/go#58125)

Notably, both implementations change the error returned to render more
verbose details on `.Error()` if the response is a status 200 _but_ an
error (see above issues)

A diff against upstream indicates that there are no other changes:
sourcegraph/oauth2#3

## Test plan

Test suites pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals
Status: Accepted
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@rsc @hickford @icholy @dmitshur @ianlancetaylor @rolandshoemaker @gopherbot