TokenSource.Token method should take in a Context

[zombiezen]

TokenSource.Token can make an HTTP request to obtain a new token. However, it does not accept a Context. This makes it difficult to pass through tracing information (mentioned in #259) or cancel requests. While 626d87b addressed the concern in some cases, it requires that a TokenSource be created per Context.

Relation to other issues:

• all: find a better way to set a custom HTTP client #223 is for not taking in *http.Client through Context, a different concern.
• This would help oauth2: automatically retry token refreshes #226 and google: retry transient errors in DefaultTokenSource #203, since the Context would have a deadline for the retries.

[taralx]

How would we make that change? We obviously can't change TokenSource itself. We could introduce ContextTokenSource or something, but that seems super ugly. Is there a deprecation path available?

[zombiezen]

https://golang.org/cl/85937 is my WIP CL to add this, which includes a gradual migration strategy, allowing both the new and the old interfaces to coexist.

[Bo0mer]

Hello,

Any plans to continue work on this?

Thanks,
Ivan

[zombiezen]

I'm still working on this, but am waiting for code review.

[jfcote87]

I have implemented a version of this idea at github.com/jfcote87/oauth2,

[leth]

Has there been any progress towards enabling this kind of API-breaking improvement in this project? e.g. adoption of semver etc

[tam7t]

When using GCP instance metadata based tokens these requests to will not include context deadlines and cannot be instrumented for tracing as the compute/metadata package is used which creates its own http client internally.

Being able to inject an HTTP client and propagate a context would improve tracing of API requests that result in token fetches.

[jens1205]

Having a golang oauth2 library which does not respect the deadlines set in the context for a request if a new token must be retrieved is simply sad...

Why not simply introduce a TokenSourceWithContext interface?

[jfcote87]

I wrote this fork to handle that problem, github.com/jfcote87/oauth2. It is in production.

…

On Tue, Jun 21, 2022 at 9:45 AM jens1205 ***@***.***> wrote: Having a golang oauth2 library which does not respect the deadlines set in the context for a request if a new token must be retrieved is simply sad... Why not simply introduce a TokenSourceWithContext interface? — Reply to this email directly, view it on GitHub <#262 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM5474BPHJMLKOGG3HWT73VQHBQDANCNFSM4EJXQK7Q> . You are receiving this because you commented.Message ID: ***@***.***>

[dnesting]

Resurrecting this issue. I was going to open a new golang proposal, but now that I found this issue I'll start here and see if people have comments.

I believe we can get the problem solved more simply than the proposed change above by introducing an optional ContextTokenSource interface that can be implemented by TokenSource implementations. Callers needing to call Token() with an available context can check that this interface is implemented and use it if it is. The existing TokenSource implementations in the oauth2 package seem like they can easily implement this interface while maintaining backward compatibility.

// A ContextTokenSource is anything that can return a token.  All
// TokenSource implementations in this package implement ContextTokenSource.
// This allows propagation of a context, such as a caller's request context,
// to the token refresh request.  It is used by the HTTP clients returned
// by NewClient and config.Client to propagate the request context to
// token refreshes.  It should generally be used like this:
//
//	// All TokenSource implementations in this package implement
// 	// ContextTokenSource.
//	ts := config.TokenSource(ctx, token).(oauth2.ContextTokenSource)
//	ts.TokenContext(ctx)
//
//	// otherwise, check for the interface:
//	if cts, ok := ts.(oauth2.ContextTokenSource); ok {
//	    token, err := cts.TokenContext(ctx)
//	} else {
//	    token, err := cts.Token()
//	}
//
// If an HTTP client is provided in the context, it will be ignored.
type ContextTokenSource interface {
	TokenSource

	// Token returns a token or an error.
	// Token must be safe for concurrent use by multiple goroutines.
	// The returned Token must not be modified.
	// If a context is not available, prefer Token() rather than
	// introduce a new context such as context.Background()
	// since implementations may have different behavior.
	TokenContext(context.Context) (*Token, error)
}

No further API changes should be needed. It looks like these oauth2 types and functions will need to be updated to support this:

• tokenRefresher.Token() must explicitly pass its t.ctx value to TokenContext to ensure backward compatibility.
• same with clientcredentials.tokenSource
• reuseTokenSource.Token() must take care to explicitly call s.new.Token() even if s.new implements ContextTokenSource, to avoid introducing a nonsense context when the underlying TokenSource (eg. tokenRefresher, previous bullet) has different behavior between Token and TokenContext.
• staticTokenSource has a trivial implementation
• retrieveToken, internal.RetrieveToken, doTokenRoundTrip need the context plumbed
• doTokenRoundTrip should be modified to use ContextClient(originalCtx).Do(req.WithContext(ctxFromTokenContext))
• Transport will need to supply the request context to t.Source if it implements ContextTokenSource

Concerns I already see:

• This is a behavior change. Previously the token refresh would have re-used the TokenSource context as its request context and now it will use the actual request context. There is no way to "opt in" or out using this proposal, short of wrapping the return from config.TokenSource() to hide its TokenContext method.
• It's unclear if we would want an HTTP client provided in the context (via the HTTPClient variable) to be used for token refreshes or not. If we do, which should get precedence between the context passed to TokenSource and the request context? Are there are enough surprises lurking here that we'd want to explicitly recommend against this practice in the interface?

Thoughts?

[dnesting]

I've implemented the above at master...dnesting:oauth2:dnesting/token-context if anyone wants to experiment.

[alexrudd]

This module has not yet reached v1 and is hosted under the golang.org/x/ path, which contains external modules that "are developed under looser compatibility requirements than the Go core."

Can we just make this a breaking change?

[adg]

@dnesting I also independently wrote a a similar change w/ ContextTokenSource and TokenContext. I made different choices, however:

• My implementation always prefers TokenContext over Token where available. The rationale here is that a context should not influence the result of a token refresh beyond deadlines and cancelations. If the context causes the returned token to be different somehow then that's on the caller. (Are there real world examples of this? I saw it mentioned as a possibility in oauth2.Config captures ctx and re-uses it later #388 (comment))
• "There is no way to "opt in" or out using this proposal" - we could initially permit opt-in with an environment variable or global variable in the oauth2 package, and then switch it to opt-out after some extensive testing by volunteers.
• "It's unclear if we would want an HTTP client provided in the context (via the HTTPClient variable) to be used for token refreshes or not." We only document it as being used for PasswordCredentialsToken, Exchange in oauth2.Config, and for clientcredentials - maybe it should only be honored in those contexts, and not for general token refreshes? That would simplify things.

I think the value of doing this outweighs the potential pitfalls. The status quo means general flakiness when people are doing perfectly reasonable things; most notably, it is very weird that in-request token refreshes don't honor request deadlines and cancelation. (now that I think about it, perhaps another approach is to have the ContextTokenSource but only attach the cancelation and deadline of the request token, but use the create-time token for all token values?)

@alexrudd the oauth2 package is way too widely-used to make a breaking change. It may not have a v1 but it has been stable for many, many years. That's an implicit expectation of stability.

[dnesting]

My implementation always prefers TokenContext over Token where available.

Yeah, not opposed to any of this. I do think documentation is either wrong (oauth2.NewClient), or ambiguous/misleading (config.Client, and config.TokenSource), and people may be more reliant on HTTPClient values today via these functions, though I have no data to support that.

The only real functional difference I see is that this no longer works:

ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient)
cl := config.Client(ctx, tok)
cl.Do(...)

When refreshing tokens, this now calls TokenContext(requestContext) and ignores ctx, meaning httpClient doesn't get used. But as you say, this technically wasn't documented.

[thestephenstanton]

Just adding my two cents here. I recently ran into this problem with the Databricks SDK. We had a server where we passed the HTTP request context to the SDK; they use TokenSource, which caches the context for token retrieval. After the token expires, it tries to refresh it using the same cached context, which has been canceled.

IMO, I'd say more people are likely to do what I did by passing in the request context vs using context to pass the HTTP client around.

[dnesting]

Also just wanted to point out that https://go-review.googlesource.com/c/oauth2/+/493335, which updates documentation to match current behavior, is currently seeing some activity. If anyone wants to rely on our ability to change undocumented behavior, I recommend weighing in on that CL before it becomes documented.