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
proposal: x/oauth2: add Token.ExpiresIn struct field #61417
Comments
This proposal has been added to the active column of the proposals project |
Is the specific proposal here to change Token.UnmarshalJSON to set the Expiry field based on expires_in? |
Yes. |
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? |
I'm sorry for the confusion. The request here is to allow unmarshaling an |
What is the specific API that we should add? |
@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:
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? |
@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:
and similarly for oauth2.TokenFromJSON. |
@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
Let's discuss |
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
If we add this, then json.Unmarshal will preserve the information, and applications can use it themselves. |
The current 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 I'd further suggest to add:
|
expires_in is the field that gets sent on the wire, so it seems wrong to omit it from JSON export. Assuming we add ExpiresIn to resolve this proposal, have all remaining concerns been addressed? |
Thank you, yes 👍🏻 |
Based on the discussion above, this proposal seems like a likely accept. |
I have added https://go-review.googlesource.com/c/oauth2/+/534835 |
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 |
See @rsc's comment above:
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). |
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. |
Why include Any data structure with both expiry and expires_in can only be correct for an instant. |
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. |
This proposal has been added to the active column of the proposals project |
The discussion has then evolved to also include marshaling of
I would suggest to:
|
Any objections to the updated proposal above? @rsc would you want this proposal to include the resulting follow-up for |
@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? |
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 |
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. |
@seankhliao @rsc could you comment? |
1 similar comment
@seankhliao @rsc could you comment? |
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. |
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. |
@rsc not sure I follow what you're proposing. Might be a language issue on my side.
What do you mean by time-based and mistaken? Adding
The behaviour above is never cleaner, it is simply always wrong when
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
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?). |
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:
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. |
@rsc that is NOT the proposal and
not the desired result :( |
To repeat: request is to be able to unmarshal |
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add MarshalJSON and UnmarshalJSON methods to Token, similar to the ones in DeviceAuthResponse. Specifically:
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. |
This is exactly NOT what we want. Clarified once more in #61417 (comment). @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 Since this would make it worse for |
@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:
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? |
We are in an awkward spot, but perhaps one way to move forward would be to add an |
@ianlancetaylor that is exactly what was concluded above (and the CL does). That does not make |
This proposal has been added to the active column of the proposals project |
@rsc proposal has once more escaped the proposal process and is not mentioned in the proposal groups's notes. |
@andig not sure what you mean, it is listed near the bottom of #33502 (comment). |
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'sExpiry
field. Hence, the token structure needs be duplicated/embedded to provide this logic as it currently lives inoauth2/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) addingToken.MarshalJSON
may not make sense given the nature ofexpires_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:
internal/RetrieveToken
to avoid new methods onToken
like Make RetrieveToken usable by client packages oauth2#354oauth2.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#483The text was updated successfully, but these errors were encountered: