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 OAuth API endpoints #12

Merged
merged 20 commits into from
Mar 9, 2017
Merged

Conversation

aggrolite
Copy link
Contributor

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

oauth_session.go Outdated
return session, nil
}

func (s *OAuthSession) newToken(postValues *url.Values) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was originally broken up into a separate method with the intent of it being used by whatever does re-authorization of tokens. Or however we decide on implementing implicit grant authorization.

@jzelinskie
Copy link
Owner

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

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

oauth_request.go Outdated
@@ -0,0 +1,53 @@
package geddit
Copy link
Owner

Choose a reason for hiding this comment

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

copyright

@jzelinskie
Copy link
Owner

Looks really good so far 👍

@jzelinskie
Copy link
Owner

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

@aggrolite
Copy link
Contributor Author

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

@aggrolite
Copy link
Contributor Author

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

@jzelinskie
Copy link
Owner

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

@aggrolite
Copy link
Contributor Author

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.

oauth_session.go Outdated

// NewLoginSession creates a new session for those who want to log into a
// reddit account via OAuth.
func NewOAuthSession(username, password, useragent, clientID, clientSecret string) (*OAuthSession, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't necessarily like passing more than a few args to functions (it's hard to remember the order without looking at documentation).

I was thinking maybe we could pass a struct or something instead.

However, I was also thinking we have two constructor functions for OAuthSession types: One for user/password tokens, and another for auth code tokens (asking for a user's permission).

edit: and currently useragent is not being used. I will fix that.

@aggrolite aggrolite changed the title First stab at OAuth [WIP] First stab at OAuth Dec 19, 2015
@aggrolite
Copy link
Contributor Author

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 and others added 13 commits March 5, 2017 10:58
After a new OAuthSession type is created with NewOAuthSession(),
the caller is given the choice of how the auth token should be
created. The auth token is required to define the HTTP client used
for API requests.

Currently, LoginAuth() is the only method of token exchange. Other
methods should be implemented in the future, which is why LoginAuth()
does not happen by default within the constructor of OAuthSession.
Disabled by default. Accepts a time.Duration and each HTTP request
must wait on interval to elapse.

User can disable any existing throttling by passing 0.
When creating a new OAuthSession type, the caller can now choose
between authenticating for personal use or on behalf of a user.
Personal script authentication (password credential exchange) is
not issued a refresh token by Reddit's API. The caller must request
a new access token after each one expires. The TokenExpiry field
indicates the lifetime of a token and will help the user determine
when a new access token must be created.
* Adding sort to request for comments
* Minor (automatic) changes by gofmt
@aggrolite
Copy link
Contributor Author

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...

Copy link
Owner

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

This change looks awesome! Just one minor nitpack about printing to stdout.

oauth_session.go Outdated
o.throttle.Wait()
}

fmt.Println("Performing request...")
Copy link
Owner

Choose a reason for hiding this comment

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

IMO, if there's going to be any output, it should be to the standard logger and only if debug is enabled.

@aggrolite aggrolite changed the title [WIP] First stab at OAuth First stab at OAuth Mar 7, 2017
@aggrolite aggrolite changed the title First stab at OAuth Support OAuth API endpoints Mar 7, 2017
@aggrolite
Copy link
Contributor Author

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 jzelinskie merged commit eb88cc7 into jzelinskie:master Mar 9, 2017
@jzelinskie
Copy link
Owner

jzelinskie commented Mar 9, 2017

🎉 merged 🎉

Thanks again for all your effort!

@aggrolite
Copy link
Contributor Author

Thanks for your feedback!

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.

6 participants