Thanks for the patches! I've pushed the first two, here are a few
comments for the third one. Overall this looks good!
On Tuesday, April 18th, 2023 at 20:20, Jamie Mansfield <jmansfield@cadixdev.org> wrote:
> +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