Support OAuth API endpoints

[aggrolite]

UPDATE

This patch set has changed quite a bit since May.

Here is a little how-to on the basic features of the OAuth code:

https://gist.github.com/aggrolite/160d5be23adb9e597553

[jzelinskie]

Thanks for the WIP. I just got back from CoreOS Fest and am quite jetlagged, so you'll have to give me a bit of time to read over everything and generate a response.

[aggrolite]

Not a problem. I will leave comments if I make further changes.

[jzelinskie]

Looks really good so far :+1:

[jzelinskie]

@aggrolite any progress here? I've also noticed there's this library which should prove extremely useful for this

[aggrolite]

@jzelinskie I will update the patch soon. Thanks for linking the oauth library.

[aggrolite]

I'm back on this. Sorry for the delay. I'm hoping to crank out some code during Xmas.

[jzelinskie]

Cool -- no rush, get it done in your leisure. Have a great holiday :snowman:

[aggrolite]

The OAuth library really simplified the code. Thanks for pointing it out.

Everything's still WIP, but I have changes to oauth_session.go pushed. For now, oauth_request.go has been removed. I might bring it back if I want to abstract any HTTP client code there. But the OAuth config really handles a lot of that already.

Next TODO's include: adding more endpoints and allowing geddit to authenticate on behalf of a user (not just for personal scripts).

We might consider scrapping the old HTTP API code. I believe it still works, but any client using it will be severely rate limited. Maybe phase it out after we're sure OAuth support is stable.

[aggrolite]

Sorry if I'm getting spammy. I will squash every thing neatly and rewrite the pull request body to explain the changes.

In short:

Most of the changes include API endpoint coverage - some "Me" endpoints, /api/submit support (for links and comments), and fetching subreddit listings.

I added just enough testing to mock JSON responses, that's about it so far.

I've also tinkered around with the constructor. This is because I hope to add support for implicit grant authorization (along with the existing login auth method). (DONE) This will let the caller decide how the token should be created (either by user/password or a auth code). Example usage for a personal script:

o, err := NewOAuthSession("client_id", "client_secret", "Descriptive UserAgent v1.0", "http://my.url")
err = o.LoginAuth("my_bot_user", "my_bot_password")
// Now we can do API calls using variable o

Notice that I've also added a parameter to the constructor which defines the use of a built-in rate limit. I was working on a bot for myself which required lots of requests, and wanted to make sure I stayed within the 60-requests-per-minute limit. I wrote a custom Roundtripper (this took a lot of Googling) which enforces the interval using a channel. Alternatively, the user can opt out and the default Transport type is used.

^ I've changed this to using a library called go-rate. Using a custom transport caused problems since the oauth2 package warns to not modify existing HTTP clients

Like I said, I'll organize everything better and do a proper write up (+docs) so that everything is crystal clear. Many of the changes come from personal taste and experimentation, so feel free to rip it all apart. I'm open to feedback always!

[aggrolite]

Ugh...a refresh token is only given when the parameter duration=permanent is provided via access token URL.

I run into these oauth2 errors after my reddit bot runs for an hour:

`Problem fetching links: Get https://oauth.reddit.com/r/hockey/hot.json?limit=100: oauth2: token expired and refresh token is not set`

This is fine (I think) for Auth Code URLs (i.e. implicit authorization) as we can pass a custom AuthCodeOption to oauth2's AuthCodeURL() function.

However, this creates problems for personal script usage. The oauth2 library does not allow the caller to define custom URL parameters. I will reach out to the Golang folks and see if they have suggestions.

:tada: :beer: Happy 2016! :beer: :tada:

[aggrolite]

^ This is no longer an issue.

Apparently reddit does not even provide a refresh token when authenticating for personal scripts. Reddit expects the script owner to check the expiration of the token each time before doing a request. When the token expires, the access_token request can be made again.

So this will just require us to expose the oauth2 token's Expiry field.

On my local branch, I am currently exporting the field TokenExpiry from our OAuthSession type. This is a time.Time type and the user can probably just check with time.Time's Before() function to check for expired tokens. Unless we want to provide a dead-simple helper function to do this.

I will push that change to my github remote for review after testing the changes on my bot.

[aggrolite]

TokenExpiry change here.

My bot is currently running code similar to the following:

// Check if new token is needed before making request.
if myBot.Client.TokenExpiry.Before(time.Now()) {
    log.Print("Token is expired! Requesting new token...")
    err = myBot.Reauthorize()
    if err != nil {
        log.Printf("Problem creating new token: %s. Will try again.\n", err)
        continue
    }
}

[jzelinskie]

Apparently reddit does not even provide a refresh token when authenticating for personal scripts.

What are you calling personal scripts? Something you haven't registered as an OAuth application in the User preferences page?

[aggrolite]

No, this is still using an OAuth app I've registered. I am just getting the access token with my own reddit account credentials (my username + my password). Trading a username/password pair for an access token is what Reddit API docs refer to as personal script usage. This is probably because any application outside of a personal script won't be asking a user for their username and password. Instead the app would generate an authentication URL for the user to visit.

[KevinMGranger]

Hi folks. I'm interested in helping out with these changes!

@aggrolite, the oauth2 package has some features for handling expiring and renewing tokens (the TokenSource interface). Perhaps we could use that?

There's also a couple of things I'm working on now:

1. Supporting key durations
2. Modifying the underlying http.Client for token exchange so we don't get rate limited (this does seem to happen. Or I messed up something while testing)

[aggrolite]

Hi @KevinMGranger thanks for the interest.

Supporting key durations

Yes, the oauth2 package will handle expired tokens when a refresh token is set. Be aware that Reddit's API does not provide a refresh token when trading username/password for an auth token. It could be nice to have reauth automated. Currently my bot checks the TokenExpiration field before making requests.

Trading an auth code for a token (using a generated auth URL) does return a refresh token. In this case, the oauth2 package sets the refresh token for us and handles expired tokens on its own. It's just that easy. IIRC this should be working with my proposed changes.

Modifying the underlying http.Client for token exchange so we don't get rate limited (this does seem to happen. Or I messed up something while testing)

If you see the "Too Many Requests" error upon token retrieval, be sure your user agent is very descriptive and includes your reddit username. I still receive this error at times upon authentication and it is annoying. Perhaps if we could adjust the code to retry upon failure, warn the user if the UA looks stupid, etc, etc...I'm open to ideas.

Regarding rate limits, I have implemented the function func (o *OAuthSession) Throttle(interval time.Duration). It might be useful, or it might be garbage. I wrote this expecting to use it, but my bot makes better use of Go's tickers.

BTW you may run into problems modifying the underlying HTTP client. The oauth2 documentation advises not to do this.

Feel free to leave feedback on my code. Ping me if you have questions.

[aggrolite]

I've rebased this branch. Build fails because of existing reddit_test.go. I'm assuming the login credentials are placeholders which might return the nil pointer being used.

[KevinMGranger]

Sorry, I didn't clarify: I meant &duration=permanent for the initial auth code retrieval. That wouldn't apply for script-based flows. (The app I'm working on uses an implicit / installed flow.)

For username/password tokens without a refresh token, we just need to create a struct to hold the username, password, client ID and client secret, and implement TokenSource (Token()) on it to get the initial token with at least the AccessToken and Expiry fields filled out. Wrap that in ReuseTokenSource, and we've got a fully-functional, auto-renewing script access token source.

It does say not to modify the underlying Transport or Client, but I interpreted that as saying don't modify it after it's been created, and you're free to modify the Transport within the Context that it'll be based off of. Unfortunately, I guess rate limiting would have to be done in the Client and not the Transport.

For token retrieval, I meant during Config.Exchange, which makes use of the http client in the Context. We'd have to modify that to change the user agent. Right now the user agent is only used for requests after we get the token, unless I'm mistaken. Also, you make a good point, we should probably log a warning if people don't set the user agent.

[KevinMGranger]

I've been digging through the source and I don't think modifying the underlying client before creating it will work. But it's probably easier to modify the user-agent and keep track of rate limiting by wrapping the client anyway.

Edit: upon further reading, I see we could very easily modify the Transport for both rate limiting and user-agent insertion.

[aggrolite]

Thanks for clarifying. I agree with every thing you said.

Regarding rate limiting, I think it's best to keep it optional. I did have a version of rate limiting written on this branch using the Transport. I don't remember exactly what caused me to scrap it and start over with using go-rate, but I did have problems. Regardless, I think it would be nice to have that logic in Transport if possible.

I wasn't aware the UA is not set on token retrieval. That explains why my bot must retry due to 429 codes. Maybe this is something the oauth2 constructor should support. However, if we can set this in the transport as well, then maybe it's not a big deal.

[KevinMGranger]

I just realized we can't do it in the transport. If we block during RoundTrip to wait for the rate limiting to die down, the bearer token could lapse and we'd have no easy way of refreshing it. Maybe we should just do it above the client like before.

[arbrown]

What is the current status of this PR? I get an error when running go test on my machine and I'm not sure if that's due to some WIP code or because I'm running on Windows. Is the oauth2 implementation in a working state?

[aggrolite]

Hi @arbrown, I am successfully running these changes for a personal bot I use to repost from reddit to twitter.

Are you running go test against the master branch? Or against this one?

[arbrown]

@aggrolite I think I was running against your master branch (which I think is the same as this PR, right?) I can look into the test failure in the next couple days, but I suspect that it's some weird Windows compatibility issue. In any case, I was able to use the package with no problem. I added a bit of code to fix an oversight in the oauth Comments() implementation, and might find other little fixes as I try the code out a bit more. Would it be best to submit a PR to your repository and have you roll them into this one?

[aggrolite]

Ah yes, I usually branch off for changes but for this particular repository I've kept on master.

FYI there is known problems with the tests using deprecated cookie API. I think this is due to increased rate limits of non oauth calls.

However, I'd be interested in helping solve Windows specific issues.

If you have any oauth-specific changes you can request a patch against my remote and I will cherry pick the change on this branch. Any improvements are greatly appreciated. Feedback on my Go style is welcome as well.

Excuse any mistakes - typing on my iPhone.

[avinassh]

any update to this PR?

[aggrolite]

Unfortunately I haven't made much use of this branch as of late. However, if someone is still interested in salvaging this code I am happy to look into rebasing this branch onto upstream/master.

[aggrolite]

This branch is rebased onto upstream/master.

While I haven't used this branch in a while, I did fix the issue of receiving 429 too many requests response code when creating a new token. I had to supply my own transport type which implements a RoundTripper interface.

If anyone gets around to using this code, let me know of any issues...

[aggrolite]

Hi @jzelinskie,

Five things:

1. Debug output has been removed.
2. New endpoint wrapper added for fetching subreddits for the current user.
3. Squashing fixups gave me rebase hell. I have left the branch unsquashed.
4. I accidentally deleted my test project using this branch, but as of this morning I was able to do normal things like authenticate with user/pw, fetch subreddits, frontpage, etc.
5. Let me know if you have any other feedback. WIP tag removed from the title.

[jzelinskie]

🎉 merged 🎉

Thanks again for all your effort!

[aggrolite]

Thanks for your feedback!