x/oauth2: add Token.ExpiresIn struct field

[andig]

Migrated from golang/oauth2#484, refs #56402 (comment)

There are a number of OAuth2 token uses outside of the oauth2 library with the token structure being the common denominator. Unfortunately, unmarshaling JSON into an oauth2.Token does not populate it's Expiry field. Hence, the token structure needs be duplicated/embedded to provide this logic as it currently lives in oauth2/internal.

Proposed solution:

Allow unmarshaling an oauth2.Token from its usual json representation, i.e. containing the expires_in field.

An implementation choice might be to add a Token.UnmarshalJSON method though that might imply that (later) adding Token.MarshalJSON may not make sense given the nature of expires_in being relative to the current moment.

Consequence of not implementing:

Duplicated code like https://cs.github.com/?scopeName=All+repos&scope=&q=language%3Agolang+ExpiresIn+int#.

Alternatives:

• Duplicated code like https://github.com/evcc-io/evcc/blob/master/util/oauth/token.go#L13
• expose internal/RetrieveToken to avoid new methods on Token like Make RetrieveToken usable by client packages oauth2#354
• providing other means of obtaining tokens in a configurable way from oauth2.Config like custom exchange request attribute/header oauth2#533, Config: Refresh Token Request with Custom Parameters oauth2#521 or Add additional headers during token refresh oauth2#483

[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]

Is the specific proposal here to change Token.UnmarshalJSON to set the Expiry field based on expires_in?

[andig]

Yes. Token.UnmarshalJSON is not exposed today. If it gets exposed it should set the Expiry field based on expires_in. If exposing/adding Token.UnmarshalJSON is not feasible the proposal would be to provide another method to the same result. Thank you!

[rsc]

I'm very confused, since Token.UnmarshalJSON does not exist and yet the top proposal above mentions it. What is the specific API change we are discussing? Or do you have a link to an implementation of the change?

[andig]

I'm sorry for the confusion. The request here is to allow unmarshaling an oauth2.Token from its usual json representation, i.e. containing the expires_in field. Doing so by adding UnmarshalJson or exposing an existing internal function is irrelevant. I've updated the proposal.

[rsc]

What is the specific API that we should add?

[andig]

@rsc I'm proposing to add the ability of unmarshaling tokens from JSON. I'm in no position to propose a specific api. As written before options I see are:

1. adding Token.UnmarshalJSON
2. add a new oauth2.TokenFromJSON method based on internal.tokenJSON

I feel the weekly round trips on this proposal do not really help to move it forward as apparently I cannot make the required contribution. Is there any chance one of the oauth2 owners joins here to improve the contribution?

[rsc]

@andig, I apologize for the unclear questions. When I ask what is the specific API, I mean what is the full godoc output for the API being added? For a function, what is the signature and the doc comment? For a struct field in an existing type, what is the name, type, and doc comment? And so on.

It sounds like Token.UnmarshalJSON is a method, so the question is how this gets filled out:

// UnmarshalJSON does ???
func (t *Token) UnmarshalJSON(???) (???)

and similarly for oauth2.TokenFromJSON.

[andig]

@rsc I'm still slightly puzzled why we are discussing the signature instead of the best approach, but let me try!

Signature should be normal json.Unmarshaler, maintaining the same properties as oauth2 does today for compatibility, especially regarding raw values. It would be this if UnmarshalJSON is acceptable:

// UnmarshalJSON unmarshals an OAuth2 token from its JSON representation.
// The `expires_in` attribute is converted to a timestamp.
// The raw json attributes are preserved in `Raw`.
// If data contains `error` it will be converted into an error, containing `error_description` and `error_uri` if present.
func (t *Token) UnmarshalJSON(data []byte) error

Let's discuss TokenFromJSON if this is not acceptable to narrow the discussion?

[rsc]

Thanks for the clarification. It sounds like perhaps instead we should add an ExpiresIn int64 field to the Token so that people who want to unmarshal can do that. This would require them to consult ExpiresIn or to switch to Expiry by calling time.Now.Add themselves. It seems like a mistake to call time.Now during json.Unmarshal, since that will make json.Unmarshal produce non-repeatable results.

So what do people think of adding

// ExpiresIn is the OAuth2 wire format "expires_in" field,
// which specifies how many seconds later the token expires,
// relative to an unknown time base approximately around "now".
ExpiresIn int64`json:"expires_in,omitempty"`

If we add this, then json.Unmarshal will preserve the information, and applications can use it themselves.

[andig]

The current Token has a special property of preserving the original JSON keys as part of Raw. It seems as if there are various deviations of token structure which makes this a useful feature. You would lose that but I don't know how important that is (and has not been requested here).

I like the idea- sweet and simple- but I'm wondering how many bugs the will create when one uses such an unmarshaled token and it is immediately expired due to Expiry not being populated. On the other hand, unmarshaling a persisted token would still restore Expiry which is nice. Should ExpiresIn be omitted from JSON export to keep the current structure? It's not useful without further context anyway.

I'd further suggest to add:

// It is application responsibility to populate `Expiry` from `ExpiresIn` when needed

[rsc]

Should ExpiresIn be omitted from JSON export

expires_in is the field that gets sent on the wire, so it seems wrong to omit it from JSON export.
It sounds like documentation is the best we can do about avoiding confusion.

Assuming we add ExpiresIn to resolve this proposal, have all remaining concerns been addressed?

[andig]

Thank you, yes 👍🏻

[rsc]

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

[andig]

I have added https://go-review.googlesource.com/c/oauth2/+/534835

[hickford]

call time.Now during json.Unmarshal

oauth2.DeviceAuthResponse has the convenient property that its JSON encoding is consistent with OAuth RFC 8628 Device Authorization Response https://datatracker.ietf.org/doc/html/rfc8628#section-3.2

// DeviceAuthResponse describes a successful RFC 8628 Device Authorization Response 
type DeviceAuthResponse struct {
    // Expiry is when the device code and user code expire. When encoding or
    // decoding JSON, the following relation is used: Expiry = time.Now() + expires_in
    Expiry time.Time `json:"expires_in,omitempty"`

It would be neat if oauth2.Token and OAuth access token response JSON were consistent in the same way https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2

[andig]

See @rsc's comment above:

It sounds like perhaps instead we should add an ExpiresIn int64 field to the Token so that people who want to unmarshal can do that. This would require them to consult ExpiresIn or to switch to Expiry by calling time.Now.Add themselves. It seems like a mistake to call time.Now during json.Unmarshal, since that will make json.Unmarshal produce non-repeatable results.

I agree that consistency in the codebase is a high value which might make it desirable to use the same approach for both tokens (and actually was what triggered me to ask #63543 before I discovered the Unmarshal implementation).

[hickford]

So concretely?

type Token struct {
-    Expiry time.Time `json:"expiry,omitempty"`
+    Expiry time.Time
+    ExpiresIn int64 `json:"expires_in,omitempty"`
     ...
}

type DeviceAuthResponse struct {
-    Expiry time.Time `json:"expires_in,omitempty"`
+    Expiry time.Time
+    ExpiresIn int64 `json:"expires_in,omitempty"`
     ...
}

Then the JSON simply agrees with OAuth RFCs without custom MarshalJSON or UnmarshalJSON. That's appealing.

Functions Config.Exchange(...) and Config.DeviceAuth(...) must continue to populate Expiry based on time.Now + ExpiresIn.

[andig]

@rsc with this outcome- do you want to revive #63543 outside of this proposal?

[ianlancetaylor]

Is there a way to change that without breaking backward compatibility?

[andig]

Adding Expiry to DeviceAuth would require changing the Marshal/Unmarshal. This is breaking. I'd argue that the current behaviour is broken and entirely useless. It's hard to imagine that anybody uses it. Happy to continue in #63543.

[rsc]

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

The proposal is to add one field to the Token type:

// ExpiresIn is the OAuth2 wire format "expires_in" field,
// which specifies how many seconds later the token expires,
// relative to an unknown time base approximately around "now".
// It is application responsibility to populate
// `Expiry` from `ExpiresIn` when required.
ExpiresIn int64 `json:"expires_in,omitempty"`

This field is automatically handled by json.Marshal and json.Unmarshal; there are no custom marshalers or unmarshalers. Code can do what it needs to with the field.

[rsc]

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

The proposal is to add one field to the Token type:

// ExpiresIn is the OAuth2 wire format "expires_in" field,
// which specifies how many seconds later the token expires,
// relative to an unknown time base approximately around "now".
// It is application responsibility to populate
// `Expiry` from `ExpiresIn` when required.
ExpiresIn int64 `json:"expires_in,omitempty"`

This field is automatically handled by json.Marshal and json.Unmarshal; there are no custom marshalers or unmarshalers. Code can do what it needs to with the field.

[andig]

Note sure how to link here in https://go-review.googlesource.com/c/oauth2/+/605955. Now points to https://github.com/golang/oauth2/issues/61417.

[magical]

@andig You can use a cross-repo issue reference, like so: Fixes golang/go#61417.

[gopherbot]

Change https://go.dev/cl/605955 mentions this issue: x/oauth2: add Token.ExpiresIn

[andig]

Sorry, I don't get it right:

Fixes golang/go#61417

still points to oauth2.

[magical]

Odd. That's the right syntax; i'm not sure why gerrit's autolinker isn't parsing it correctly. Maybe something misconfigured? Here's another x/oauth2 CL showing the same problem (https://golang.org/cl/450155), whereas a recent x/tools CL shows a correct link (https://golang.org/cl/605015).

In any case, it should work correctly when the commit is eventually merged.

[mvdan]

Thank you all for this proposal! Even in its minimal form, it's already saving me some amount of copy pasting: https://review.gerrithub.io/c/cue-lang/cue/+/1201519

[gopherbot]

Change https://go.dev/cl/621215 mentions this issue: x/oauth2: apply the expires_in value returned by the server to Token.ExpiresIn