patternjavascriptMinor
node.js passport wrapper
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);
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
deserializeUseris aneurysm inducing.
- You have a ton of comments, which I like, authentication can always use tons of comment
data[0]anddata[1]both use a crucial magic constant, how aboutdata[AUTHENTICATION]anddata[CALLBACK]or some such ?
- I do not like the
registerGuestname,registerUsermaybe?
- 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.