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

OAuth2 #1212

Closed
wants to merge 6 commits into from
Closed

OAuth2 #1212

wants to merge 6 commits into from

Conversation

FanFani4
Copy link

Hello,
this fixes the GoogleOAuth2Mixin.get_authenticated_user, in docstring it said that its returning a user object but really it returns the access token.
and i added some popular russian socials as mail.ru, vk.com, yandex.ru.

@bdarnell
Copy link
Member

bdarnell commented Oct 1, 2014

The addition of google_request seems like a good idea (can the implementation be shared with FacebookGraphMixin's facebook_request?).

The change to get_authenticated_user is backwards-incompatible, right? I think it's probably better to change the documentation to match what it already returns than to change the implementation at this point.

I don't want to add any more specific auth providers to this module; it's better to let those live outside of the main Tornado repo. (see #236)

@FanFani4
Copy link
Author

Hello,
can you please see if i understand you correctly?
in docstring of get_authenticated_user already is said that:
Handles the login for the Google user, returning a user object

@bdarnell
Copy link
Member

Yes, if get_authenticated_user returns an access token instead of a user object, then the docs should be updated to say that.

It looks like you've replaced facebook_request with a new auth_request method; there needs to be a facebook_request method for backwards-compatibility even if it's just a wrapper around auth_request.

This functionality should also have tests.

@bdarnell bdarnell added the auth label Jan 12, 2015
@kippandrew
Copy link
Contributor

@bdarnell I noticed the GoogleOAuth2Mixin.get_authenticated_user method is still broken, or at the very least is incorrectly documented (it returns an access_token instead of the user's profile). I'm happy to fix this, but I am wondering what your preferred approach is? I can open another PR (or fix this one) with a factored out auth_request. Or did you have something else in mind? Like are you planning on removing all the OAuth stuff all-together?

bdarnell added a commit that referenced this pull request Jun 28, 2015
This method can serve as a general replacement for methods like
FacebookGraphMixin.facebook_request.

Update docs for GoogleOAuth2Mixin.get_authenticated_user to indicate
that only the tokens are returned, not other user information.

Add some basic tests for GoogleOAuth2Mixin.

This commit incorporates code from FanFani4 in #1212 and
kippandrew in #1454.
@bdarnell
Copy link
Member

I've committed a change to update the docs for get_authenticated_user (and incorporated some of the other changes in this branch).

@bdarnell bdarnell closed this Jun 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants