Skip to content
This repository has been archived by the owner on Jul 15, 2020. It is now read-only.

Facebook oAuth not working properly and causing crash #22

Closed
iaincollins opened this issue May 14, 2017 · 5 comments
Closed

Facebook oAuth not working properly and causing crash #22

iaincollins opened this issue May 14, 2017 · 5 comments
Labels

Comments

@iaincollins
Copy link
Owner

iaincollins commented May 14, 2017

Somehow the oauth login is not working properly on live anymore with Facebook.

  • Facebook oauth does not seem to be supplying an email address anymore.
  • nextjs-starter is accepting a blank email address and saving the value as empty (replacing a temporary twitter oauth based email if there is one) when linking to an account that was created by signing in Twitter.
  • The blank email address value is then causing a crash somewhere else in the app when trying to link to a Google account or unlink then relink to a Twitter account (now.sh restarts the instance shortly after but the server goes down briefly).

This is a relatively new issue, previously working behaviour and may be as a result of a Facebook change or a bug introduced by refactoring.

@iaincollins
Copy link
Owner Author

Have confirmed issue triggered by email address not being returned by Facebook API. They seem to have form for changing this and not following SEMVER for API changes. Am working on tweak to use placeholder email address value (as with Twitter).

Also adding defensive coding to passport-strategies.js as the following line (142) was causing the subsequent crash due to user.email being null:

if (user.email.match(/.*@localhost.localdomain$/)) {

@iaincollins
Copy link
Owner Author

iaincollins commented May 14, 2017

Good news:

  • Have refactored how oAuth services that don't return email addresses work, which is cleaner in the code and and makes it easier to add providers that don't return them.
  • The sign-in page now shows "N/A" if we don't have a valid email address for the user (i.e. if the value is a placeholder email address ending in @localhost.localdomain).
  • Have added in the defensive checks to fix the bug which triggered the crash.
  • Also added hooks to log uncaughtException and unhandledRejection errors to make it easy to debug issues like this in production with now-logs.

Bad news:

I don't know why the Facebook API is no longer returning an email address, even though we already explicitly ask for that field. I don't know if this something specific to privacy settings on my account, or a result in the change on the API. Either way, Facebook oAuth works again, we just aren't able to grab the email address the user has given to Facebook (and we now just handle that gracefully).

It's worth noting the reason we bother with a temporary email address at all is because it makes it easier to also support email based sign in at the same time as supporting oAuth (if we only did oAuth then we wouldn't have to care about an email address field).

This update will be landing as v2.10.0 shortly.

@juanmiret
Copy link

Hey! You just have to add a profileFields property in strategyOptions to make Facebook return emails again, like this:
providers.push({ provider: 'facebook', Strategy: require('passport-facebook').Strategy, strategyOptions: { clientID: process.env.FACEBOOK_ID, clientSecret: process.env.FACEBOOK_SECRET, profileFields: [ 'id', 'displayName', 'email', 'link' ] }, scope: [ 'email', 'public_profile' ], getUserFromProfile(profile) { return { id: profile.id, name: profile.displayName, email: profile._json.email } } })

@iaincollins iaincollins reopened this Jun 1, 2017
@iaincollins
Copy link
Owner Author

@juanmiret Thanks Juan!

So I thought 'scope' was actually mapped to 'profileFields' in the Facebook Passport app and didn't understand them to be separate fields (I came across it while trying to debug the problem but gave up trying to figure out why it wasn't working once I'd refactored to make it not required).

I think I might not be passing in the profileFields option in Passport auth so might need to refactor the code, but will definitely give this a go!

@iaincollins iaincollins added the bug label Jun 1, 2017
iaincollins added a commit that referenced this issue Jun 1, 2017
A previous fix for #22 was rolled to accomodate Facebook API change which meant it no longer returned email addresses when signing in. The fix rolled out simply had the system handle not getting an email address from a provider more gracefully (i.e. not bombing out).

@juanmiret suggest a fix to the passport-facebook strategy which should resolve the issue. This further enhancement should restore the previously lost functionality - so that logging in with a Facebook account now provides the email address once again - while preserving the improving the handling for oAuth providers that don’t return an email addresses.
@iaincollins
Copy link
Owner Author

The fix I rolled out didn't work (Facebook oAuth is unique in that it can't be tested against localhost auth it seems), but I see what was doing differently and why it didn't work before either - see issue #30 for future updates to this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants