Implementing Passport authentication with promises

Question

Here is code implementing Passport authentication with a Google Strategy. It uses Mongoose to store and retrieve user data.

You might need to be familiar with both of these technologies to review this code if not check out the two links I provided.

I have included only the Passport file. I was told I am not using promises correctly and would like to rectify this if it is in fact true.

The code is full of comments below. Please let me know if more clarification or context is needed.

Thanks for any feedback.

Passport

const passport = require('passport');
const GoogleStrategy = require('passport-google-oauth').OAuth2Strategy;
const DBM = require('../../database/mongoose');
const helper = require('../../config/helper');


// review this post
// https://stackoverflow.com/questions/50339887/can-i-reduce-this-passport-code

// configure Passport to use Google Auth
passport.use('google', new GoogleStrategy(helper.getPassport(), getOrCreateUser));

// passed to passport, this is the starting point for Google Auth
function getOrCreateUser (accessToken, refreshToken, profile, done) {

  // abstracts out relevant information from what Google returns
  const user_props = obtainProps(profile);
  DBM.getUser(user_props.id_google).then( (res) => {

    // res[0] will be "true" if the user exists
    return res[0] ? done(null, user_props) : createUser(done, user_props);
  }).catch( error => {
    return done(error, null);
  });
}

// the user was not found, create the user
function createUser (done, user_props) {
  DBM.createUser(user_props).then(() => {
    return done(null, user_props);
  }).catch( error => {
    return done(error, null);
  });
}

// serializeUser required by passport to identify the user uniquely on the client
passport.serializeUser( (profile, done) => {
  done(null, profile.id_google);
});

// deserializeUser required by passport to retrieve data on the server using unique
// identifier created by serializeUser
passport.deserializeUser( (id_google, done) => {
  DBM.getUser(id_google).then((res) => {
    done(null, res[0]);
  }).catch( (error) => {
    console.error('DBM.getUser() Error: ', error);
  });
});

// takes a profile from Google and extracts parameters to save
function obtainProps (profile) {
  let props = {};
  props.id_google = profile.id;
  props.email = profile.emails[0].value;
  props.name = profile.displayName;
  props.pic_url = profile.photos[0].value;
  props.type = profile._json.objectType;
  return props;
}

module.exports = passport;

Answer 1

1. When writing your custom functions that operate on promises, a good habit is to return resulting promises in order to have ability to chain them. For instance:

   return DBM.getUser(user_props.id_google).then( (res) => { (from getOrCreateUser)
   or:
   return DBM.createUser(user_props).then(() => {

It will ensure that getOrCreateUser and createUser operations will be chained together properly.

2. The passportjs uses different API. It does not use promises but callbacks. As I see, you mixed them together by mistake! For instance in DBM.createUser from inside your promise you returned an object returned just by done callback when resulting value/other promise is expected. Same in DBM.getUser from passport.deserializeUser

3. It might be only mine (a little bit nerdy) custom, but I have a habit to wrap 3rd party non promise APIs into promise adapters.

For instance (an easy one):

function serializeUser(idProviderPromise) {
    let resultDeffered = Promise.defer();
    passport.serializeUser((profile, done) => {
        idProviderPromise(profile).then(function (profileId) {
            done(null, profileId);
            resultDeffered.resolve();
        });
    });
    return resultDeffered;
}

Usage:

passportPromiseApi.serializeUser(new Promise(profile => profile.id_google));

It can help make us sure not to mix different APIs together.

At the first glance it can look a little bit over complicated, but please take a look how it simplifies more complex cases:

function configure(getOrCreateUserPromise) {
    let resultDeffered = Promise.defer();
    function getOrCreateUser(accessToken, refreshToken, profile, done) {
        getOrCreateUserPromise(accessToken, refreshToken, profile).then(function (userProps) {
            done(null, userProps);
            resultDeffered.resolve(userProps);
        }).catch(function (error) {
            done(error, null);
            resultDeffered.fail(error);
        });
    }
    passport.use('google', new GoogleStrategy(helper.getPassport(), getOrCreateUser));
    return resultDeffered;
}

Usage:

passportPromiseApi.configure(new Promise(function (accessToken, refreshToken, profile) {
    const userProps = obtainProps(profile);
    return DBM.getUser(userProps.id_google).then(res => {
        return res[0] ? Promise.resolve(userProps) : createUser(userProps);
        //Instead Promise.resolve(userProps) simply userProps should work as well
        //But it is also a good practice for return type in any JS function to be consistent.
    });
}));

function createUser (userProps) {
    return DBM.createUser(userProps).then(() => userProps); 
    //Error handling is already done by our adapter.
    //We can focus only on business logic here!
}

4. Last less important thing: let assume some consistent naming convention for function parameters and local variables respectively. You have to decide whether to use xxx_yyy or xxxYyy (camel case). 3-rd party usages doesn't count as it must be used exactly.