Support specifying AuthCodeOption in PasswordCredentialsToken

[tiziano88]

Our use case is to pass additional URL parameters as part of the password credentials grant. Currently we need to hook into the context, create a RoundTripper that attaches extra args to the request, and inject that into the call to PasswordCredentialsToken.

If it supported options similarly to AuthCodeURL, this would be trivial to do from the client point of view instead.

I propose modifying

func (c *Config) PasswordCredentialsToken(ctx context.Context, username, password string) (*Token, error)

into

func (c *Config) PasswordCredentialsToken(ctx context.Context, username, password string, opts ...AuthCodeOption) (*Token, error)

If the AuthCodeOption naming is confusing, I imagine we could have a separate PasswordCredentialsOption type, but I'm not sure it's worth it.

[tiziano88]

Also it would be good to support adding custom headers to auth requests (useful for tracing or custom extensions). This could be modelled as an additional AuthCodeOption impl (with some changes to the interface).

We are happy to contribute those changes if you are willing to merge them, but if not just let us know and we'll create our own fork instead. Thanks!

[SimonInman]

Suggested fix: https://go-review.googlesource.com/c/oauth2/+/84295

[bradfitz]

I had some API comments on Gerrit, but I don't understand the problem yet so it's hard to find a solution.

It sounds like there are multiple use cases here. I've heard:

that attaches extra args to the request

and

adding custom headers to auth requests (useful for tracing or custom extensions)

Can we get more concrete? APIs that expose arbitrary "do whatever you want here" hooks never age well.

Which "custom extensions" or "extra args" are we talking about?

For tracing, that would likely be done at the RoundTripper/RPC layer, not something user code would concern itself with, or it might be attached to the already-existing context.Context, rather than explicitly added to the url.Values.

Talk me through some example use cases.

[tiziano88]

Thanks @bradfitz , we have two use cases we wish to support:

• (major) passing additional auth credentials to the server (e.g. OTP / 2FA); this is non-standard but we are using it in the web flow and we also need a way of emulating it in the Password Credentials Grant.
• (minor) passing additional HTTP headers, so that we can implement tracing (e.g. using http://opentracing.io/ ) and obtain distributed spans for auth operations involving the auth service. AFAICT attaching it to the context.Context would not make it propagate unless the HTTP client used is also able to unwrap it and propagate it (which looks like it may be possible, using https://github.com/golang/oauth2/blob/master/internal/transport.go#L34 , but not sure that is actually exposed)

[bradfitz]

passing additional auth credentials to the server (e.g. OTP / 2FA)

Is this something your one server is doing, or something multiple parties are doing?

If it's one-off, I'd rather clients experimenting with new things just make their own HTTP requests. I don't want to add cognitive load to all users of the oauth2 package by adding more API surface they need to read in the godoc, obscuring the parts they're probably looking for.

so that we can implement tracing

Tracing should be done independently of this bug and just be automatic. If this package received a context with trace info attached from some upper level HTTP server or gRPC server or whatever, it should just pass through in the context and the underlying HTTP client should add the trace headers. User code using oauth2 shouldn't need to think about it.

There should be existing bugs open for tracing. @rakyll, got any pointers?

[tiziano88]

This is something that our own server is doing. Note anyway that what we are suggesting is semantically equivalent to the existing https://godoc.org/golang.org/x/oauth2#SetAuthURLParam , we are only suggesting extending it to the password credentials grant; I would imagine that all the considerations that applied to adding SetAuthURLParam in the first place should also apply identically to what we are suggesting here. From that point of view, I see our proposed change as simplifying things rather than making them harder, since it makes the API more uniform, anyway I would imagine this is arguable to some extent.

Making our own HTTP request is indeed an option, but we would also need to parse the resulting token, and currently the retrieveToken method is not exposed and it relies on internal packages in turn, so we would have to reimplement / copy-paste / fork all of that.

As for tracing, I doubt that tracing could ever be fully automatic (unless you are talking about embedding an opentracing tracer within the oauth client as a special case, which would obviously only work for that particular kind of tracing). Anyway we can leave that out of the scope of this discussion for now, and I may create a separate issue for it.

[bradfitz]

/cc @zombiezen for another case of users wanting raw token retrieval.

[jmnote]

Suggested fix: https://go-review.googlesource.com/c/oauth2/+/555395