Skip to content
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

Support specifying AuthCodeOption in PasswordCredentialsToken #259

Open
tiziano88 opened this issue Nov 30, 2017 · 8 comments · May be fixed by #694
Open

Support specifying AuthCodeOption in PasswordCredentialsToken #259

tiziano88 opened this issue Nov 30, 2017 · 8 comments · May be fixed by #694

Comments

@tiziano88
Copy link

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
Copy link
Author

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 pushed a commit to SimonInman/oauth2 that referenced this issue Dec 11, 2017
Allow clients to pass additional URL parameters as part of the
PasswordCredentialsToken grant.

Fixes golang#259
@SimonInman
Copy link

@bradfitz
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

bradfitz commented Jan 4, 2018

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

@jmnote jmnote linked a pull request Jan 11, 2024 that will close this issue
@jmnote
Copy link

jmnote commented Jan 31, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants