HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

node.js passport wrapper

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nodewrapperpassport

Problem

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=
* /auth/callback?type=
*
* 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);

Solution

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..

Context

StackExchange Code Review Q#36938, answer score: 6

Revisions (0)

No revisions yet.