node.js passport wrapper

Question

I have started some experiments with node.js.
Any comment on style (especially the callbacks) and best practice is appreciated:

PassportControl.js

/*
 * Export single function that creates the passportControl object
 *
 *      The passport control object is supposed to be a wrapper for
 *      nodejs/express/passport authentication.
 *
 *      When the object is created it adds two end points to the server
 *      /auth?type=<AuthenticationType>
 *      /auth/callback?type=<AuthenticationType>
 *
 *      Where AuthenticationType is the service doing the authentication.
 *          Eg Facebook/Twitter/Amazon etc
 *
 * This object has two public methods:
 *      checkPassport(req, res)
 *      registerGuest(req, res)
 *
 *          req:        http request received from node.
 *          res:        response object we use to reply to the request.
 *
 * These are automatically hooked up to the exposed endpoints.
 * To extend this for any particular service just add the appropriate
 * objects to the array built with buildData()
 *
 */
module.exports = function(app, register) {

    // App:         Application object
    // register:    The user registration service
    //              This has been abstracted from the passport authentication code.
    //              I will document this interface separately.


    // Get the passport object we reap
    // Correctly initialize and turn on sessions.
    var passport            = require('passport');
    app.use(passport.initialize());
    app.use(passport.session());

    // Set passport to only save the user ID to the session
    passport.serializeUser(function(localUser, done) {
                done(null, localUser.id);
            });

    // Set passport to retrieve the user object using the
    // saved id (see serializeUser).
    passport.deserializeUser(function(localUserId, done) {
                register.getSavedUser(localUserId,  function(localUser) {
                                                        done(null, localUser);
                                                    });
            });

    // Create the passport control object
    passportControl =  {
                data:       buildData(passport, register),
                checkPassport: function(req, res) {
                    return this.getItem(this.data[0], req, res);
                },
                registerGuest: function(req, res) {
                    return this.getItem(this.data[1], req, res);
                },

                getItem: function getItem(dataItem, req, res) {
                    item = dataItem[req.query.type];
                    if (item == null) {
                        item = dataItem['default'];
                    }
                    return item(req, res);
                }
            };
    // register the service endpoints
    // This will control all authentication.
    app.get('/auth',          function(req, res) { passportControl.checkPassport(req, res);});
    app.get('/auth/callback', function(req, res) { passportControl.registerGuest(req, res);});

    return passportControl;
}

// Global object for correctly escaping URL
var querystring = require('querystring');

// Private Method
// This builds the data object central to "passportControl"
// It is basically a two element array containing two maps.
// The Key: Is the name of the "AuthenticationType" the value is the passport object that does the authentication.
// The first object handles the initial authentication request.
// The second object handles the callback from the authentication service
function buildData(passport, register) {

    // Set Up facebook authentication strategy.
    var FacebookStrategy    = require('passport-facebook').Strategy;
    passport.use(new FacebookStrategy({
                    clientID:       "XXXXXX",
                    clientSecret:   "YYYYYY",
                    callbackURL:    "http://iPubCrawlMaps.com:3000/auth/callback?type=facebook"
        },
        function(accessToken, refreshToken, profile, done) {
                register.updateUser({   provider:   profile.provider,
                                        providerId: profile.id,
                                        displayName:profile.displayName},
                                    function(localUser) {
                                        done(null, localUser);
                                    });
            })
    );
    // Add more strategies as required here.
    return [
                {
                    default:    function(req, res) {res.redirect('/login?'+querystring.stringify({reg_error:'Invalid Authentication Type (attempt)'}));},
                    facebook:   passport.authenticate('facebook'),
                },
                {
                    default:    function(req, res) {res.redirect('/login?'+querystring.stringify({reg_error:'Invalid Authentication Type (callback)'}));},
                    facebook:   passport.authenticate('facebook', { successRedirect: '/', failureRedirect: '/login' }),
                }
            ];
}

Answer 1

From a once over:

• Your indenting is a mix of 4 space and 8 space tabbing, please stick at all times to either 4 or 2 spaces. Looking at deserializeUser is aneurysm inducing.
• You have a ton of comments, which I like, authentication can always use tons of comment
• data[0] and data[1] both use a crucial magic constant, how about data[AUTHENTICATION] and data[CALLBACK] or some such ?
• I do not like the registerGuest name, registerUser maybe?
• I also do not like getItem, this function is not getting an item..
• I am not a big fan of putting clientSecret in your code, it should get this from an environment variable really, otherwise anybody with access to your version control system knows your clientSecret..