x/oauth2: add Token.ExpiresIn struct field

[andig]

Migrated from https://github.com/golang/oauth2/issues/484, refs https://github.com/golang/go/issues/56402#issuecomment-1639698959

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 https://github.com/golang/oauth2/issues/354
• providing other means of obtaining tokens in a configurable way from oauth2.Config like https://github.com/golang/oauth2/issues/533, https://github.com/golang/oauth2/issues/521 or https://github.com/golang/oauth2/issues/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]

@hickford Plus Expiry should get lower case json tag expiry. Should we continue in #63543 once this proposal has been accepted/implemented?

[hickford]

Why include expiry in JSON? If you are implementing an OAuth client, you receive JSON expires_in then store expiry time. If you are implementing an OAuth server, you respond with JSON expires_in and store token expiry time.

Any data structure with both expiry and expires_in can only be correct for an instant.

[rsc]

Thanks for pointing out DeviceAuthResponse. It does seem like we should take time to decide which of these two approaches should be used and then do that consistently. The fact that DeviceAuthResponse already does this makes it somewhat difficult to change, but not impossible.

[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

[andig]

The fact that DeviceAuthResponse already does this makes it somewhat difficult to change, but not impossible.

DeviceAuthResponse already unmarshals expires_in. Token does not. Doing so was the proposal of this PR.

The discussion has then evolved to also include marshaling of expires_in in addition to Expiry since only the latter guarantees a stable validity date. That's what https://go-review.googlesource.com/c/oauth2/+/534835 implements.

DeviceAuthResponse does not marshal Expiry, or rather only does so as the relative expires_in, hence #63543 was opened.

I would suggest to:

• complete this proposal as discussed and already approved
• use #63543 to give DeviceAuthResponse the same additional capability

[andig]

Any objections to the updated proposal above?

@rsc would you want this proposal to include the resulting follow-up for DeviceAuth?

[andig]

@rsc already tentatively accepted and no new comments. Could we proceed with this? Noticed that its no longer tracked in https://github.com/golang/go/issues/33502.

[andig]

@rsc 3 weeks in final comment period. No updates at all begins to feel a little frustrating. There are no open objections/comments and CL is done. Is there anything I could do to move this forward?

[andig]

@rsc moved this from Likely Accept to Active in Proposals on Oct 27

Doesn't feel very active ;)

[andig]

Sorry to say this, but more than 6 weeks have passed now. This feels absolutely unappreciated. More so since there's apparently no way to make it back into the proposal process. I've really liked working with and on Go, but not so this time.

/cc @rsc

[gophun]

Maybe it was removed from the review meeting minutes template by accident. All other active proposals at least have a bullet point stating 'discussion ongoing' or something similar. This one appears to be the odd exception. Perhaps you could raise it on the golang-dev mailing list.

[andig]

@seankhliao @rsc could you comment?

[andig]

@seankhliao @rsc could you comment?

[rsc]

Apologies for dropping this. We switched tracking systems in early November and for whatever reason we lost this one. I am going to add some extra checks to make sure we don't lose any in the future. This is back in the process now.

[rsc]

Given that DeviceAuthResponse already does the (arguably mistaken) time-based JSON marshal/unmarshal, it seems like doing the same for Token is at least consistently arguably mistaken. If we do the thing we were discussing before DeviceAuthResponse was pointed out, maybe it would be cleaner in isolation but then there would be two inconsistent and both weird behaviors in the package. Better to have one?

It sounds like we should go back to just using the existing fields and adding the time-based marshal/unmarshal to Token.

Maybe this can be cleaned up in an eventual v2, but for now it is what it is.

[andig]

@rsc not sure I follow what you're proposing. Might be a language issue on my side.

Given that DeviceAuthResponse already does the (arguably mistaken) time-based JSON marshal/unmarshal

What do you mean by time-based and mistaken? Adding expires_in to time.Now()? That is indeed wrong, to be corrected by https://github.com/golang/go/issues/63543.

If we do the thing we were discussing before DeviceAuthResponse was pointed out, maybe it would be cleaner in isolation but then there would be two inconsistent and both weird behaviors in the package.

The behaviour above is never cleaner, it is simply always wrong when Expiry is set. For Token that is not an issue since Expiry is already being (un)marshaled.

It sounds like we should go back to just using the existing fields and adding the time-based marshal/unmarshal to Token.

In the understanding above- yes. That's what this issue proposes: add additional(!) time-based marshal to Token. Since it is additional it is only used when Expiry is not set, such as when receiving token from wire.

Maybe this can be cleaned up in an eventual v2, but for now it is what it is.

I don't follow. What should be cleaned up after this proposal is accepted?

The longer the discussion goes, the less I understand what is being discussed here and what the objections are (are there any?). To summarise: Token lacks the ability ot unmarshal expires_in. It's only ever used if Expiry is empty. Everything else about Token is fine. Lets add that capability. Separate PR: plus make sure DeviceAuth adds capability for Expiry as Token did for years.

[rsc]

Have all remaining concerns about this proposal been addressed?

The proposal is to add MarshalJSON and UnmarshalJSON methods to Token, similar to the ones in DeviceAuthResponse. Specifically:

func (Token) MarshalJSON() ([]byte, error)
func (*Token) UnmarshalJSON([]byte) error

and these would use the current time to marshal or unmarshal the Expiry field. This makes JSON operations time-dependent, but DeviceAuthResponse already did that.

[andig]

@rsc that is NOT the proposal and

This makes JSON operations time-dependent, but DeviceAuthResponse already did that.

not the desired result :(

[andig]

To repeat: request is to be able to unmarshal ExpiresIn. Approach is to add (Un)MarshalJSON and use ExpiresIn for setting Expiry when unmarshaling if and only when Expiry is empty. Doing so does not make JSON operations more time-dependent than today. Instead it adds a use case that is not possible today at all. Thank you!

[rsc]

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

The proposal is to add MarshalJSON and UnmarshalJSON methods to Token, similar to the ones in DeviceAuthResponse. Specifically:

func (Token) MarshalJSON() ([]byte, error)
func (*Token) UnmarshalJSON([]byte) error

and these would use the current time to marshal or unmarshal the Expiry field. This makes JSON operations time-dependent, but DeviceAuthResponse already did that.

[andig]

This makes JSON operations time-dependent, but DeviceAuthResponse already did that.

This is exactly NOT what we want. Clarified once more in https://github.com/golang/go/issues/61417#issuecomment-1975889369.

@rsc I'm not sure why it seems as if you're pushing the proposal in that direction. It feels almost as if the remaining comments are not being read. Maybe mentioning the issues with DeviceAuthResponse was a mistake on my side as it apparently made the discussion more complex (then, DeviceAuthResponse was 2 weeks old...).

Since this would make it worse for oauth2.Token instead of fixing it (and then fixing DeviceAuthResponse) I'm closing this proposal. The process has been a frustrating experience :(

[rsc]

@andig Apologies for the confusion and frustrating process. I am not 100% sure we are talking about the same thing. This issue started with golang/oauth2#484, where your top comment ended with:

It would be nice if oauth2.Token.UnmarshalJSON populated the Expiry field when expires_in is populated.

And I think that, after probably too long a discussion, that's where we ended up. The early comments here seemed to suggest that too. Are you saying that you don't think we should take that approach anymore?

Or is the objection about MarshalJSON also doing something with the field?

[ianlancetaylor]

We are in an awkward spot, but perhaps one way to move forward would be to add an ExpiresIn field to both DeviceAuthResponse and Token. When unmarshaling, if expires-in appears and expiry does not, we can both ExpiresIn and Expiry based on expires-in.

[andig]

@ianlancetaylor that is exactly what was concluded above (and the CL does). That does not make Token time-dependent which is important. Seems that got lost in the sparse updates. Imho ExpiresIn should not be marshaled by Token and neither by DeviceAuthResponse, but it seems the latter ship has sailed.

[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

[andig]

@rsc proposal has once more escaped the proposal process and is not mentioned in the proposal groups's notes.

[rsc]

@andig not sure what you mean, it is listed near the bottom of https://github.com/golang/go/issues/33502#issuecomment-2038280866.

[andig]

@rsc still "discussion ongoing". What's open to discuss? Can I contribute anything?

[andig]

@rsc still "discussion ongoing". What's open to discuss? Can I contribute anything?

[andig]

@rsc still "discussion ongoing" in proposal meeting. What's open to discuss? Can I contribute anything?

[rsc]

"Discussion ongoing" means the discussion is ongoing on the issue tracker, not that proposal review is discussing it actively. In this case, I think we have been going in circles in tiny messages, and I have not had time to try to write and post a standalone summary of the possible solutions and the arguments that have been made in favor and against each one. You are welcome to do that if you want.

[andig]

In this case, I think we have been going in circles in tiny messages

@rsc that is unfortunately very true. Imho part of the issue is that- it seems to me- the updates in this issue are not really being read. It seems we share the diagnosis:

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?

and I have not had time to try to write and post a standalone summary

That has made it hard for me to understand what is missing

You are welcome to do that if you want.

Let's restart, hope this helps.

Problem

Allow unmarshaling an oauth2.Token from its usual json representation, i.e. containing the expires_in field when not going through any of the oauth2 token processes.

Minimal solution

Add https://go-review.googlesource.com/c/oauth2/+/534835

// 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"`

Alternative solution

Add

// 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"`

func (Token) MarshalJSON() ([]byte, error)
func (*Token) UnmarshalJSON([]byte) error

UnmarshalJSON must be implemented so that it uses ExpiresIn for setting Expiry if and only if Expiry is otherwise empty. This alternative solution make's implementer's life easier by switching the task of balancing Expiry vs. ExpiresIn to UnmarshalJSON.

Summary

Contrary to https://github.com/golang/go/issues/61417#issuecomment-1984999987, this does NOT makes JSON operations time-dependent.

What about DeviceAuthResponse?

DeviceAuthResponse has the extremely unfortunate property of of round-tripping Expiry through expires_in. That does make it time-dependent which was identified in https://github.com/golang/go/issues/63543. It would be preferable for DeviceAuthResponse to behave like the alternative solution above, but it seems that's too late now. In any case out of scope for this proposal.