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

x/oauth2: confusing definition of DeviceAuthResponse #63543

Open
andig opened this issue Oct 14, 2023 · 3 comments
Open

x/oauth2: confusing definition of DeviceAuthResponse #63543

andig opened this issue Oct 14, 2023 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@andig
Copy link
Contributor

andig commented Oct 14, 2023

https://go-review.googlesource.com/c/oauth2/+/450155 recently added oauth2.DeviceAuthResponse:

type DeviceAuthResponse struct {
	// DeviceCode
	DeviceCode string `json:"device_code"`
	// UserCode is the code the user should enter at the verification uri
	UserCode string `json:"user_code"`
	// VerificationURI is where user should enter the user code
	VerificationURI string `json:"verification_uri"`
	// VerificationURIComplete (if populated) includes the user code in the verification URI. This is typically shown to the user in non-textual form, such as a QR code.
	VerificationURIComplete string `json:"verification_uri_complete,omitempty"`
	// Expiry is when the device code and user code expire
	Expiry time.Time `json:"expires_in,omitempty"`
	// Interval is the duration in seconds that Poll should wait between requests
	Interval int64 `json:"interval,omitempty"`
}

According to https://datatracker.ietf.org/doc/html/rfc8628#section-3.2

expires_in: REQUIRED. The lifetime in seconds of the "device_code" and "user_code".

Since Expiry is a time.Time it should imho not marshal/unmarshal to expires_in. Also see #61417 for related discussion on how to serialise expires_in.

/cc @hickford

@gopherbot gopherbot added this to the Unreleased milestone Oct 14, 2023
@andig andig changed the title x/oauth2: invalid definition of DeviceAuthResponse x/oauth2: confusing definition of DeviceAuthResponse Oct 14, 2023
@hickford
Copy link

hickford commented Oct 14, 2023

@andig Could you give an example of the problem comparing input, output and expected output?

The current behaviour (which ought to be better documented) is:

When encoding or decoding JSON, the following relation is used: Expiry = time.Now() + expires_in

See the tests for examples https://github.com/golang/oauth2/blob/master/deviceauth_test.go

@gopherbot
Copy link

Change https://go.dev/cl/535217 mentions this issue: oauth2: document JSON encoding of DeviceAuthResponse.Expiry

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 16, 2023
@andig
Copy link
Contributor Author

andig commented Oct 25, 2023

@hickford I don't think the PR is a valid fix. Shall we follow with #61417 and then act accordingly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants