Jamie Mansfield: 3 Fix IntrospectionEndpointAuthSigningAlgValuesSupported annotation Set Accept header when expecting a JSON response Support RFC 8628 (device authorisation grant) 4 files changed, 108 insertions(+), 1 deletions(-)
Thanks for the patches! I've pushed the first two, here are a few comments for the third one. Overall this looks good!
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~emersion/public-inbox/patches/40465/mbox | git am -3Learn more about email & git
--- oauth2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth2.go b/oauth2.go index c2c2599..c97a48d 100644 --- a/oauth2.go +++ b/oauth2.go @@ -34,7 +34,7 @@ type ServerMetadata struct { IntrospectionEndpoint string `json:"introspection_endpoint,omitempty"` IntrospectionEndpointAuthMethodsSupported []AuthMethod `json:"introspection_endpoint_auth_methods_supported,omitempty"` - IntrospectionEndpointAuthSigningAlgValuesSupported []string `json"introspection_endpoint_auth_signing_alg_values_supported,omitempty"` + IntrospectionEndpointAuthSigningAlgValuesSupported []string `json:"introspection_endpoint_auth_signing_alg_values_supported,omitempty"` CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported,omitempty"` } -- 2.40.0
Unfortunately some OAuth providers give non-standard responses, such as GitHub (which defaults to 'application/x-www-form-urlencoded'). --- client.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client.go b/client.go index ea0950d..3257646 100644 --- a/client.go +++ b/client.go @@ -94,6 +94,8 @@ func (c *Client) newFormURLEncodedRequest(endpoint string, params url.Values) (* } func (c *Client) doJSON(req *http.Request, data interface{}) error { + req.Header.Set("Accept", "application/json") + resp, err := c.httpClient().Do(req) if err != nil { return err -- 2.40.0
--- device.go | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ oauth2.go | 3 ++ 2 files changed, 105 insertions(+) create mode 100644 device.go diff --git a/device.go b/device.go new file mode 100644 index 0000000..39adc87 --- /dev/null +++ b/device.go @@ -0,0 +1,102 @@ +package oauth2 + +import ( + "errors" + "net/url" + "time" +) + +const ( + // RFC 8628 - Section 4 + GrantTypeDeviceCode GrantType = "urn:ietf:params:oauth:grant-type:device_code" + + // RFC 8628 - Section 3.5 + ErrorCodeAuthorisationPending ErrorCode = "authorization_pending" + ErrorCodeSlowDown ErrorCode = "slow_down" + ErrorCodeExpiredToken ErrorCode = "expired_token" +)
Can we move these constants over to the existing const blocks in oauth2.go? This reads better on the GoDoc page: the definitions are all grouped together instead of being scattered.
+ +// DeviceAuthOptions are optional parameters for the device authorisation endpoint. +type DeviceAuthOptions struct { + Scope string +} + +// DeviceAuthResp contains the data returned by the device authorisation endpoint. +type DeviceAuthResp struct { + DeviceCode string `json:"device_code,omitempty"` + UserCode string `json:"user_code,omitempty"` + VerificationURI string `json:"verification_uri,omitempty"`
These three fields above are mandatory, can we drop the "omitempty" option to reflect this?
+ ExpiresAt time.Time `json:"-"` + Interval int64 `json:"interval,omitempty"`
Maybe it makes more sense to expose this as a time.Duration in our API instead of a bare int64?
+}
Do we want to add verification_uri_complete? Or was this omitted on purpose?
+ +// DeviceAuth performs the device authorisation request. +// +// See RFC 8628. +func (c *Client) DeviceAuth(options *DeviceAuthOptions) (*DeviceAuthResp, error) { + q := make(url.Values) + q.Set("client_id", c.ClientID) + if options.Scope != "" { + q.Set("scope", options.Scope) + } + + req, err := c.newFormURLEncodedRequest(c.Server.DeviceAuthorizationEndpoint, q) + if err != nil { + return nil, err + } + + var data struct { + DeviceAuthResp + ExpiresIn int64 `json:"expires_in"` + } + if err := c.doJSON(req, &data); err != nil { + return nil, err + } + + if data.ExpiresIn > 0 { + data.ExpiresAt = time.Now().Add(time.Duration(data.ExpiresIn) * time.Second) + } + if data.Interval == 0 { + data.Interval = 5 + } + + return &data.DeviceAuthResp, nil +} + +// PollAccessToken performs the device authorisation request, polling the endpoint +// until such time that either the device code has timed out or the server responds +// with an error.
… or the server responds with a success?
+// +// See RFC 8628. +func (c *Client) PollAccessToken(auth *DeviceAuthResp) (*TokenResp, error) { + interval := time.Duration(auth.Interval) * time.Second + + for { + time.Sleep(interval) + if time.Now().After(auth.ExpiresAt) { + return nil, errors.New("oauth2: timeout occurred while polling for access token") + } + + q := make(url.Values) + q.Set("grant_type", "urn:ietf:params:oauth:grant-type:device_code") + q.Set("device_code", auth.DeviceCode) + q.Set("client_id", c.ClientID) + + resp, err := c.doToken(q) + if err == nil { + return resp, nil + } + + if err, ok := err.(*Error); ok { + if err.Code == ErrorCodeSlowDown { + interval += time.Duration(5) * time.Second
We need to continue here, or else we will return the slow_down error below. Nit: no need to explicitly convert "5" to time.Duration here.
+ } else if err.Code == ErrorCodeAuthorisationPending { + continue + } + + return nil, err + } else { + return nil, err + }
Style nit: since the contents of both the if and else branches are the same, the return statement can be taken out of the if block.
+ } +} diff --git a/oauth2.go b/oauth2.go index c97a48d..2e8725f 100644 --- a/oauth2.go +++ b/oauth2.go @@ -37,6 +37,9 @@ type ServerMetadata struct { IntrospectionEndpointAuthSigningAlgValuesSupported []string `json:"introspection_endpoint_auth_signing_alg_values_supported,omitempty"` CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported,omitempty"` + + // RFC 8628 - Section 4
Nit: when using the notation "RFC 8628 section 4" then GoDoc will automatically create a link to the section of the RFC, which is nice for users. (IOW, it's better to drop the "-".)
+ DeviceAuthorizationEndpoint string `json:"device_authorization_endpoint,omitempty"` } // ClientMetadata contains registered client metadata defined in RFC 7591. -- 2.40.0
Thanks for the patches! I've pushed the first two, here are a few comments for the third one. Overall this looks good!