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

Export Auther #76

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

andrewmostello
Copy link
Contributor

Hello,

We've been using this package to authorize requests against an OAuth 1.0a API for a number of years, and it works great.

However, we're applying it to a new package, and it would be much more convenient to apply the authorization header directly when building the request. The existing usage requires creating a new http.Client with the package's http.RoundTripper.

The proposal here is to export Auther and its key fields and methods. This doesn't affect existing usage.

I've exported both Clock and Config to make it easier to test packages using Auther (e.g. using a fixed clock).

I've also replaced usage of ioutil with io, as it's deprecated, and resolved an unused value of err warning in TestBaseURI.

Thanks for writing and maintaining this package.

These functions now live in io.
Export Auther to enable package consumers to directly set the
authorization header of their requests.

There are use cases where adding authorization transparently through
http.Client is less desirable than just setting the header directly.
Export Auther to allow for this use case.
Copy link
Owner

@dghubble dghubble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the more idiomatic approach would be to chain http.RoundTripper's to compose your own http.Client. Here's someone's blog with pointers.

The oauth1.Transport is already a http.RoundTripper, so we'd just need something similar to config.NewClient, such as a NewTransport that does the same thing but doesn't wrap the RoundTripper in an http.Client.

// NewTransport returns a new RoundTripper that signs requests via OAuth1.
func NewTransport(ctx context.Context, config *Config, token *Token) http.RoundTripper {
	return &Transport{
		Base:   contextTransport(ctx),
		source: StaticTokenSource(token),
		auther: newAuther(config),
	}
}

In that sense, oauth1 would just provide a http.RoundTripper that's about the same as what NewClient does, but can be composed with any other RoundTrippers to your liking and used with your own http.Client. You should be able to try this out without any change to this library by using config.NewClient(...).Transport.

It doesn't seem right for this package to expose a Clock or to directly expose the internal auther struct. Also, it would be nice to have the cleanups as a separate change.

@andrewmostello
Copy link
Contributor Author

I agree that it's idiomatic to provide a NewTransport function and bring-your-own http.Client, but it's also common to provide a method to set the header on a request. e.g., the OAuth2 package enables you to do that (Token.SetAuthHeader), and http.Request has a method to set a basic auth header. They're both acceptable and different use cases.

In what we're working on, this is part of a request builder, so it's a bit out of left field to instead start wrapping the http.Client.

I understand though if that's not what you're going for here and it runs counter to the package.

I can pull together a separate pull request with the other two items as well.

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 this pull request may close these issues.

2 participants