Add CustomTokenSource for custom token validation #396
Conversation
|
This PR (HEAD: 387e226) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/oauth2/+/192798 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
This commit adds CustomTokenSource which allows the user to specify a function for determining whether a token is valid. This also changes the internal implementation of ReuseTokenSource to use CustomTokenSource under the hood while keeping the same external API.
|
This PR (HEAD: 6d45fe3) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/oauth2/+/192798 to see it. Tip: You can toggle comments from me using the |
|
Message from Frank Harding: Patch Set 8: Adding Brad Fitzpatrick as a reviewer since it looks like he deals with a lot of the golang/oauth2 CLs, let me know if I should have added someone else. Thank you! Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Brad Fitzpatrick: Patch Set 8: Sorry, this is more API than I want to add to this package. Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want). Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Brad Fitzpatrick: Patch Set 8:
Sorry, I don't understand the problem enough probably. But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource). So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource. It's okay to compose a TokenSource out of other TokenSources. Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
This is my understanding, it could be wrong: If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client. Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc. Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Brad Fitzpatrick: Patch Set 8:
That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines. The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.
Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something? Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
And to be clear I totally agree that I would love if this was possible without changing this library, I just don't think it is without re-implementing the behavior of the unexported tokenRefresher. Would love to be wrong about that. Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Brad Fitzpatrick: Patch Set 8:
If you want to get a fresh un-cached token, call: func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource { ... with a nil Token. Then call Token() on the returned TokenSource? Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource. If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned). Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Brad Fitzpatrick: Patch Set 8:
Gotcha. Sorry, I didn't exactly follow earlier. But I don't think it's as bad as you made it sound... you said that you "have to keep the config around", but that's not any different from today. The returned TokenSource today already retains a pointer to the *Config. Any TokenSource you implement would also do so. But downstream users of either API don't need to care about that detail... they'd just use a TokenSource (or Transport/Client using said TokenSource). Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
That's true. You would just have to fetch a new TokenSource from the config whenever you want to refresh the token rather than using the underlying unexported tokenRefresher and telling that to refresh. I just think that's a bit weird, but you're right that that's doable with the current API. I think I was just a little hung up on the fact that the way the API works right now already implements token refreshing, and it seems a bit weird to have to create new token sources every time you want to refresh the token rather than just being able to tell the underlying tokenRefresher to return a new token. For example, if there was a way to get the tokenSource created by oauth2.Config.TokenSource before it's wrapped with ReuseTokenSource, this would be a lot easier. Or, if oauth2.Config had a Token method that always returned a new token (so oauth2.Config itself was a TokenSource without refreshiing). Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
And in that case you would still have to keep the oauth2.Config around, but you wouldn't be creating new TokenSources every time you want to refresh the token. That's not too much better, so I think you're right that it's probably not worth changing the API to enable doing that. I'll close this PR/CL. Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
|
Message from Frank Harding: Patch Set 8:
... and thank you for having the patience to talk through this :) Please don’t reply on this GitHub thread. Visit golang.org/cl/192798. |
This PR adds the ability to create a custom token source that takes a token validation function. This will accommodate use cases where the token should be refreshed regardless of whether the token is considered valid or not. Currently implementing this behavior is extremely awkward. For example, if you're using
oauth2.Config.Clientyou will need to create a new HTTP client if you wish to force the token to refresh. You can't even just refresh the underlying Transport TokenSource, because ofThe returned client and its Transport should not be modified.You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start. A couple use cases where this might be relevant:The internal implementation of ReuseTokenSource has also been updated to use CustomTokenSource with just returning whether the passed token is valid. The changes to the API should be backwards-compatible.