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

Query email rather than oauth provider id on /auth/<provider> #223

Closed
jakobrosenberg opened this Issue Jun 13, 2016 · 8 comments

Comments

Projects
None yet
5 participants
@jakobrosenberg
Copy link

jakobrosenberg commented Jun 13, 2016

Currently /auth/ checks for an existing profile with the following query

      query: {
        [`${options.provider}Id`]: profile.id
      }

This breaks functionality for users who have already signed up locally or with a different provider.

Why not query the email instead? When john@doe.com signs up with a password and then later on tries to login with facebook, why not acknowledge that local john@doe.com is the same person as facebook john@doe.com.

In other words, why not check if the john@doe.com exists rather than check if john@doe.com has previously signed in with a specific service? The specificity serves no purpose.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 13, 2016

Not all OAUth providers give you the email address. Facebook for example requires a separate scope. We should probably add an easier way to customize querying the user though. The current solution is to override create in a hook, look up the user the way you want, then patch and set hook.result which will skip the original create:

const service = app.service('/users');

service.before({
  create(hook) {
    return service.find({ email: hook.data.facebook.email }).then(existingUser => {
      if(existingUser.length === 1) {
        return service.patch(existingUser.id, {
          facebook: hook.data.facebook
        }).then(updatedUser => {
          // Set `hook.result` to skip the actual `create` since we updated it already
          hook.result = updatedUser;
          return hook;
        });
      }

      // Just return the hook to continue as usual
      return hook;
    });
  }
})
@jakobrosenberg

This comment has been minimized.

Copy link
Author

jakobrosenberg commented Jun 14, 2016

Thanks for the advice daffl.

A little unrelated but, the scope in my config file works for Google, but not for facebook. Despite ["public_profile", "email"] in the scope, hook.data.facebook only returns name, id and accessToken.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 15, 2016

@jakobrosenberg that is by design on Facebook's end. You have to make a separate request to their api endpoint /me?fields=email to get it.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 15, 2016

Here's a hook I wrote a while ago to automatically add it upon user signup with Facebook.

'use strict';

const request = require('request');

// src/services/users/hooks/getEmailFromFacebook.js
//
// Use this hook to manipulate incoming or outgoing data.
// For more information on hooks see: http://docs.feathersjs.com/hooks/readme.html

const defaults = {};

module.exports = function(options) {
  options = Object.assign({}, defaults, options);

  return function(hook) {
    if (hook.data.facebook) {
      return new Promise(resolve => {
        var req = {
          url: 'https://graph.facebook.com/me?fields=name,email,first_name,last_name',
          headers: {
            'Authorization': 'OAuth ' + hook.data.facebook.accessToken
          }
        };
        request(req, function (error, response, body) {
          if (!error && response.statusCode === 200) {
            body = JSON.parse(body);
            if (body.email) {
              hook.data.email = body.email;
            }
            resolve(hook);
          } else {
            // console.log(arguments);
          }
        });
      });
    }
  };
};
@eblin

This comment has been minimized.

Copy link
Contributor

eblin commented Jun 15, 2016

"scope": ["public_profile","email"] basically tells facebook what "permissions" you'd like to use, but it does not return the email to you.

Since feathers-authentication just uses passport.js you can easily tell it what fields you want with profileFields (http://passportjs.org/docs/profile), passport.js normalizes this data so this should work for other providers as well

"auth" : {
...
"facebook": {
      "clientID": "...",
      "clientSecret": "...",
      "profileFields": ["id", "displayName", "photos", "email"],
      "permissions": {
        "scope": ["public_profile","email"]
      }
    }
}

You will still need to create the before hook, but now you have all the data you need in hook.data, here's the hook I wrote for that.

exports.facebookRegister = function (options) {
  return function (hook) {
    if (!hook.data.facebook) return hook; //bail if there's no facebook data;
    const facebookId = hook.data.facebookId;
    const { email, name} = hook.data.facebook;
    const data = {
      facebookId,
      email,
      name,
      avatar: hook.data.facebook.picture.data.url
    };
    // Find existing user and add their facebook info
    // OR just crete new account with their facebook info
    return this.find({ query: { email: hook.data.facebook.email } })
               .then(results => {
                 let existingUser = results[0] ||
                   results.data && results.data[0];
                 if (existingUser) {
                   // update
                   return this.patch(existingUser.id, data)
                              .then(updatedUser => {
                                hook.result = updatedUser;
                                return hook;
                              });
                 }
                 // create
                 return this.create(data).then(user => {
                   hook.result = user;
                   return hook;
                 }).catch(err => console.log(err));

               }).catch(err => console.log(err));
  };
}
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 15, 2016

@eblin thank you! Using profileFields is a cleaner approach. I hoped there was a way to preconfigure the fields.

@jakobrosenberg

This comment has been minimized.

Copy link
Author

jakobrosenberg commented Jun 15, 2016

Thanks guys!

I still hope the Authentication will use email as ID at some point, but this works for now.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jul 7, 2016

@jakobrosenberg it won't use email by default because that is completely up to your app. We are working on making it easier for you to "hook" into setting up the query though. Right now Auth is not that extendable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.