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

fix: handle new userinfo endpoint #51

Closed
wants to merge 5 commits into from

Conversation

MarshallOfSound
Copy link

@MarshallOfSound MarshallOfSound commented Dec 21, 2018

Google is deprecating and completely removing the Google Plus APIs early next year. The easiest migration path for users of this module is to provide userProfileURL: 'https://www.googleapis.com/oauth2/v3/userinfo', in the strategy options (use the oauth userinfo endpoint instead of G+). This mostly works in its current state.

However there are some issues with the openid parser on this endpoint, this PR fixes several of those.

  • email_verified can now be verified_email
  • sub is now id
  • I have read the CONTRIBUTING guidelines.
  • I have added test cases which verify the correct operation of this feature or patch.
  • I have added documentation pertaining to this feature or patch.
  • The automated test suite ($ make test) executes successfully.
  • The automated code linting ($ make lint) executes successfully.

* `email_verified` can now be `verified_email`
* `sub` is now `id`
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

6 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b3f943 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@coveralls
Copy link

coveralls commented Dec 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling bf7b576 on MarshallOfSound:patch-1 into 96a25f6 on jaredhanson:master.

@ergdyne
Copy link

ergdyne commented Dec 21, 2018

Seems like the URL should be "https://www.googleapis.com/oauth2/v3/userinfo" to be slightly more up-to-date over "v2"

@MarshallOfSound
Copy link
Author

Seems like the URL should be "https://www.googleapis.com/oauth2/v3/userinfo" to be slightly more up-to-date over "v2"

Good catch, for some reason Google's docs still reference the v2 API everywhere

@regevbr
Copy link

regevbr commented Dec 21, 2018

Thanks for the PR! when can we expect a new version to be released?

@skogsmaskin
Copy link

Thank you @MarshallOfSound!

Would be great if we could have this merged before the holidays :)

@fdebef
Copy link

fdebef commented Dec 22, 2018

Wouldn't it be logical to change directly the default userProfileURL in strategy.js?
Line 55:
this._userProfileURL = options.userProfileURL || 'https://www.googleapis.com/plus/v1/people/me'

If I understand it, this URL will be depracated and have it as default does not make sense any more.

@ergdyne
Copy link

ergdyne commented Dec 22, 2018

Yes, that does make sense.

@MarshallOfSound
Copy link
Author

@fdebef Changing the default will be a breaking change, my intention was to land these fixes and docs and then follow up with a PR to change the default in a breaking release.

Just relies on @jaredhanson being around to review. This might have to wait till after the New Year (everyone needs a holiday)

@fdebef
Copy link

fdebef commented Dec 22, 2018

Definitelly no hurry. I'm happy I have this solution, that I have already implemented. Big THANKS, Samuel!

@regevbr
Copy link

regevbr commented Dec 23, 2018

@jaredhanson can you please push this up the queue?

@eponymz
Copy link

eponymz commented Dec 28, 2018

Awesome. Added the userProfileURL param to the strategy and poof. Thanks for the googling on this one!! Confirmed by disabling the G+ API on my dev server. You deserve all the SO and reddit karma.

@ginovski
Copy link

ginovski commented Feb 1, 2019

@jaredhanson When are you going to review and probably approve this PR?

cdunford added a commit to cdunford/chickentendermarketplace that referenced this pull request Feb 3, 2019
- The passport module used for Google authentication leveraged
  legacy Google+ APIs; have now switched to a new library which
  uses the recommended/supported oauth endpoints for authentication.

See: jaredhanson/passport-google-oauth2#51
     https://github.com/passport-next/passport-google-oauth2
@dhans
Copy link

dhans commented Feb 4, 2019

a) Google has announced that these requests will start failing by end of the week, and there's been a pretty long time frame for announcing this breaking change. What's the reason for delaying these fixes? It seems odd considering the popularity of this package and users will end up with a broken log-in experience.

b) From stepping through the code, the updates to json.id and json.email_verified don't seem needed - am I missing something?

c) Why not just update the default userProfileURL? There's no reason to keep a reference to an API that will stop working.

@TobyEalden
Copy link

TobyEalden commented Feb 4, 2019

I don't understand all the fuss - I simply added the userProfileURL: "https://www.googleapis.com/oauth2/v3/userinfo", to the strategy options and it works with no other changes required and the Google+ API disabled.

@dhans
Copy link

dhans commented Feb 4, 2019

Correct - that wasn't obvious from this thread. Still seems appropriate to get in a fix to the library well before that deadline.

@raxityo
Copy link

raxityo commented Feb 5, 2019

The profile information returned from the OpenID endpoint and G+ sign in has two fields that need attention.

Field OpenID G+ sign-in Change needed in the lib?
User Identifier sub id No
Has the email been verified? email_verified Not needed. It only returns verified emails and user's Google Account email No

So it appears that this library needs only one change :

Ninja Edit: Sorry, that change is also not needed for the openid parser.
as @TobyEalden mentioned, using userProfileURL: "https://www.googleapis.com/oauth2/v3/userinfo" is all you need.

@MarshallOfSound:

email_verified can now be verified_email

I couldn't find a documentation referring to verified_email. Can you point to the correct docs?

References:

@bitkidd bitkidd mentioned this pull request Feb 6, 2019
7 tasks
@adamreisnz
Copy link

adamreisnz commented Feb 9, 2019

This fork has this merged https://github.com/passport-next/passport-google-oauth2

@rwky could you tell me a bit more about this passport-next project? Have you created forks of all passport modules and updated them? Are they safe to use in production?

Edit: never mind found it here: https://github.com/passport-next/passport
I have successfully replaced all passports modules with @passport-next equivalents and it's working like a charm 👍

chasenlehara pushed a commit to LBH3/lbh3.org that referenced this pull request Feb 9, 2019
This should prevent the plus.people.get API from being used.

jaredhanson/passport-google-oauth2#51 (comment)
@rwky
Copy link

rwky commented Feb 11, 2019

Glad to hear it's working @adamreisnz I always appreciate feedback. FYI all of the forked repos are used in production so should be safe to use.

brandon-pereira added a commit to brandon-pereira/betterdo-api that referenced this pull request Feb 14, 2019
mwbrooks added a commit to mwbrooks/passport-google-oauth that referenced this pull request Feb 14, 2019
TheOptimisticFactory pushed a commit to sportheroes/passport-google-oauth2 that referenced this pull request Feb 19, 2019
@MathRobin
Copy link

Have tried a poke on twitter to @jaredhanson and to @auth0. Hope they will do something https://twitter.com/mathrobin/status/1102527314720624640

@springuper
Copy link

springuper commented Mar 6, 2019

as I tested today, the profile in google oauth response is compatible with previous, which means, when I added userProfileURL: 'https://www.googleapis.com/oauth2/v3/userinfo' option, the email_verified and sub fields have correct values:

qq20190306-174319 2x

@twelve17
Copy link

twelve17 commented Mar 6, 2019

I don't understand all the fuss - I simply added the userProfileURL: "https://www.googleapis.com/oauth2/v3/userinfo", to the strategy options and it works with no other changes required and the Google+ API disabled.

@TobyEalden I tried this, but eventually I noticed that the NPM of this module points to this fork, which has the profile URL hardcoded:

Strategy.prototype.userProfile = function(accessToken, done) {
  this._oauth2.get('https://www.googleapis.com/plus/v1/people/me', accessToken, function (err, body, res) {
    if (err) { return done(new InternalOAuthError('failed to fetch user profile', err)); }

So setting that override does nothing in that version.

@TobyEalden
Copy link

@TobyEalden I tried this, but eventually I noticed that the NPM of this module points to this fork, which has the profile URL hardcoded:

Not sure I follow - the dependency is "passport-oauth2": "1.x.x", which seems to me to be https://www.npmjs.com/package/passport-oauth2 and repo is https://github.com/jaredhanson/passport-oauth2

@twelve17
Copy link

twelve17 commented Mar 6, 2019

@TobyEalden is this issue not for passport-google-oauth2, not passport-oauth2? The former overrides the latter's userProfile function, so the profileURL parameter is essentially ignored.

@TobyEalden
Copy link

@TobyEalden is this issue not for passport-google-oauth2, not passport-oauth2?

It is confusing.

This repo (https://github.com/jaredhanson/passport-google-oauth2) is published on npm as passport-google-oauth20.

And this repo has a dependency on passport-oauth2, to which it then passes the url from options if given.

@twelve17
Copy link

twelve17 commented Mar 6, 2019

@TobyEalden is this issue not for passport-google-oauth2, not passport-oauth2?

It is confusing.

This repo (https://github.com/jaredhanson/passport-google-oauth2) is published on npm as passport-google-oauth20.

Ah, I see! I missed the "20" vs "2". Thanks for pointing that out.

@pigeonman99
Copy link

pigeonman99 commented Mar 6, 2019 via email

@Martii
Copy link

Martii commented Mar 6, 2019

@pigeonman99

Re:

we have created a new implementation of several people.get and people.getOpenIdConnect APIs that will only return basic fields necessary for sign-in functionality such as name and email address, if authorized by the user.

Nothing like band-aids. The issue still remains here and over on the base issue. We can always add the new user endpoint but this PR also has other conditional test fixes that we would rather have fixed here instead of patches on our end. I've been following this since the PR creation, with all of it's comments including the ones about the alleged not needing the other conditional fixes, and it's beyond confusing. Having goo add aliases is going to confuse everyone even more. Clearly the conditional fixes were put in for a reason by @MarshallOfSound . Also what happens in "version 3.0 of the deprecation" down the road when they realize it's consuming resources for doing an alias?! Mistakes happen whether any one admits it or not. Having the code default to something that is clearly active is preferred instead of some hacky work-around on their end... maybe I'm asking too much from google.

I do appreciate everyone's input but I think google needs to step up to the plate and do the right thing now instead of the thing right now.

@jaredhanson
Copy link
Owner

I've merged PR #53 and published passport-google-oauth20@2.0.0 to npm. More information here: https://medium.com/passportjs/google-api-shutdown-330c3b47e3df

The only other changes introduced by this PR involve parsing id and verified_email field. However, these fields are not returned by the userinfo endpoint, and so the parsing doesn't belong in the OpenID profile parser.

Rather, these fields are returned by the legacy profile.get endpoints (including the new implementations introduced to mitigate migration timelines). The existing Google+ profile parser already handles the id field. I would accept a PR that added support for the verified_email field. However, I suspect its unlikely anyone would deliberately set the userProfileURL option to revert back to using the legacy endpoints.

Closing.

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.