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

adding identity auth checking for a reddit user #30

Merged
merged 3 commits into from Apr 8, 2020

Conversation

stedn
Copy link
Contributor

@stedn stedn commented Apr 6, 2020

Adding a method to allow authentication of a reddit user as part of the goal of adding reddit to bridgy. Not sure if this is everything that's needed in this part of the project, but I figured I'd work on one thing at a time.

For implementation, it seems that reddit's Oauth confirmation method is a little different from the normal flow. See comments in the reddit.py for refs on why it's done this way.

(PS this took me way longer than I thought it would, but i now somewhat understand how oauth works, which is cool)

Copy link
Owner

@snarfed snarfed left a comment

Choose a reason for hiding this comment

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

awesome! this is a great first pass. if you want, we can probably merge with pretty minimal changes, and then you can add access token and user info fetching in another PR, or you can do them directly in this PR. up to you!

setup.py Outdated Show resolved Hide resolved
oauth_dropins/reddit.py Show resolved Hide resolved
requests to the Tumblr API. Stores OAuth credentials in the datastore. See
models.BaseAuth for usage details.

reddit-specific details: implements "access_token," which is really a refresh_token
Copy link
Owner

Choose a reason for hiding this comment

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

oh ugh i see, their access tokens only last for 60m, so you have to refresh after that. ok, understood. https://github.com/reddit-archive/reddit/wiki/OAuth2#refreshing-the-token

except then the access_token() method should probably actually do the refresh and return a real access token, right? feel free to punt that to another PR if you want!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conceptually returning the access_token makes sense, but after spending a while on it, I'm pretty sure praw doesn't give access to that directly. instead they've written it so that you pass the refresh_token to the api every time. do you know how i can change things to accommodate that? otherwise i guess i could write the authorize http request myself, but I'm not super familiar with how to do that.

Copy link
Owner

@snarfed snarfed Apr 7, 2020

Choose a reason for hiding this comment

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

ah, got it! totally fine. exposing refresh token directly makes sense then, just name the method refresh_token(), and omit an access_token() method entirely. you can briefly mention which methods are available and not like this:

GitHub-specific details: implements get() but not urlopen(), or api().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so i tried this originally, but it seems that access_token() is required by oauth_dropins/models.py. I got a NotImplementedError when i remove that function. Also it seems I can't have both a refresh_token attribute and a refresh_token method in the same class, should i just omit the method?

and did you mean to link to that table in praw? or were you trying to link to an example from this repo?

Copy link
Owner

Choose a reason for hiding this comment

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

hah sorry, yeah, misfired with the link.

ohhh is it this call that dies?

token = auth_entity.access_token()

it's not strictly necessary. maybe we wrapp it in a try/catch NotImplementedError and just omit the token in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, try catch on the NotImplementedError is in place. Hopefully this wraps it up? Now onto the actual hard stuff 😅

Copy link
Owner

Choose a reason for hiding this comment

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

looks like you maybe still need to git add models.py?

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 added the try except to the handlers.py. can you see that?

Copy link
Owner

Choose a reason for hiding this comment

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

oh! sorry! yes, got it now.

oauth_dropins/reddit.py Outdated Show resolved Hide resolved
oauth_dropins/reddit.py Show resolved Hide resolved
oauth_dropins/reddit.py Show resolved Hide resolved
oauth_dropins/reddit.py Show resolved Hide resolved
@stedn
Copy link
Contributor Author

stedn commented Apr 7, 2020

i think i took care of most of the stuff. let me know if you know any way to get the actual access token.

oauth_dropins/reddit.py Show resolved Hide resolved
requests to the Tumblr API. Stores OAuth credentials in the datastore. See
models.BaseAuth for usage details.

reddit-specific details: implements "access_token," which is really a refresh_token
Copy link
Owner

@snarfed snarfed Apr 7, 2020

Choose a reason for hiding this comment

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

ah, got it! totally fine. exposing refresh token directly makes sense then, just name the method refresh_token(), and omit an access_token() method entirely. you can briefly mention which methods are available and not like this:

GitHub-specific details: implements get() but not urlopen(), or api().

@snarfed
Copy link
Owner

snarfed commented Apr 8, 2020

oh, also, are you able to add me to your reddit API app? my username is schnarfed.

if not, no worries, i can just make my own app.

@stedn
Copy link
Contributor Author

stedn commented Apr 8, 2020

i think you've been added to the API. let me know if you want me to send you the app keys and such.

requests to the Tumblr API. Stores OAuth credentials in the datastore. See
models.BaseAuth for usage details.

reddit-specific details: implements "access_token," which is really a refresh_token
Copy link
Owner

Choose a reason for hiding this comment

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

oh! sorry! yes, got it now.

@snarfed
Copy link
Owner

snarfed commented Apr 8, 2020

woo, congrats! i'm off to create some more dedicated API keys for oauth-dropins and granary and try it myself.

@snarfed snarfed merged commit 5a9736d into snarfed:master Apr 8, 2020
@stedn stedn deleted the reddit_identity branch April 8, 2020 05:38
@stedn
Copy link
Contributor Author

stedn commented Apr 8, 2020

sweet! i plan to start looking over granary tomorrow night, though i expect that'll take longer to figure out.

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.

None yet

2 participants