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

Updated Google provider so that login uses the recommended Google+ People endpoint #108

Merged
merged 6 commits into from Jul 14, 2015

Conversation

@qualquervalor
Copy link

qualquervalor commented Jun 19, 2015

Auth and Token have been updated based on values found in the OpenIDConnect discovery document:
https://accounts.google.com/.well-known/openid-configuration
Scope discussion can be found here:
https://developers.google.com/+/web/api/rest/oauth#plus.login
Goggle discovery doc for the plus service:
https://www.googleapis.com/discovery/v1/apis/plus/v1/rest
Documentation of response body:
https://developers.google.com/+/web/api/rest/latest/people#resource

Chris Henney added 2 commits Jun 19, 2015
@geek

This comment has been minimized.

Copy link
Member

geek commented Jun 22, 2015

@qualquervalor please update so that this can be merged... thanks :)

@geek geek added the bug label Jun 22, 2015
@geek geek self-assigned this Jun 22, 2015
@geek

This comment has been minimized.

Copy link
Member

geek commented Jun 22, 2015

@qualquervalor looks good. Have you tried this with the /examples/google.js yet?

@geek geek added this to the 4.0.1 milestone Jun 22, 2015
Chris Henney added 2 commits Jun 23, 2015
@qualquervalor

This comment has been minimized.

Copy link
Author

qualquervalor commented Jun 23, 2015

Yes. I hadn't planned on checking in my updates to the example, but here they go.

@fson

This comment has been minimized.

Copy link

fson commented Jun 24, 2015

What is the purpose of requesting those Google+ permissions? Note that doing this, those permissions are asked from the user:
Before:
screen shot 2015-06-24 at 21 42 00

After:
screen shot 2015-06-24 at 21 26 02

From the documentation it seems that OpenID Connect is still a recommended way to implement authentication with Google using OAuth 2.0.

For OpenID Connect, the scope should be openid,email, openid,profile or openid,email,profile. (https://developers.google.com/identity/protocols/OpenIDConnect#scope-param)

Fetching the profile seems a bit more complex with OpenID Connect however: apparently you're supposed to fetch the Discovery Document and then make a request to the userinfo_endpoint URL specified in the document.

@qualquervalor

This comment has been minimized.

Copy link
Author

qualquervalor commented Jun 25, 2015

The motivation was to use the latest and greatest as suggested in Google's migration guide https://developers.google.com/+/web/api/rest/auth-migration#oauth2login. A user has the ability to override the scopes. Simpler scopes are still supported as described here https://developers.google.com/+/web/api/rest/oauth#plus.login. I could swap out the newer scopes and replace them with ['profile', 'email'] for the default case, This would still use the newer Google+ endpoint and ask the same level of permission that were previously requested.

@fson

This comment has been minimized.

Copy link

fson commented Jun 25, 2015

Thanks for the detailed reply @qualquervalor! This all makes sense now.

I think whatever scope is sufficient for getting the very basic information (displayName, name, email) that is extracted by default in the provider code makes sense as a default scope. Users wanting to extract more information using the raw profile can then easily override the scope to include all the scopes they need.

@geek geek removed this from the 4.0.1 milestone Jul 14, 2015
@geek geek added this to the 5.0.0 milestone Jul 14, 2015
geek added a commit that referenced this pull request Jul 14, 2015
Updated Google provider so that login uses the recommended Google+ People endpoint
@geek geek merged commit 3b41529 into hapijs:master Jul 14, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.