11
\$\begingroup\$

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' }),
                }
            ];
}
\$\endgroup\$
6
\$\begingroup\$

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..
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.