From 6e0388bcb749ed434ad5fe247d2ec64181e57711 Mon Sep 17 00:00:00 2001 From: Frederic Hemberger Date: Mon, 14 Sep 2015 16:18:39 +0200 Subject: [PATCH 1/4] Error handling: Always return instance of 'AuthenticationError' --- README.md | 26 +++++----- lib/badrequesterror.js | 12 ----- lib/error.js | 68 +++++++++++++++++++++++++++ lib/passport-local-mongoose.js | 34 +++++--------- test/{badrequesterror.js => error.js} | 12 ++--- test/issues.js | 2 +- test/passport-local-mongoose.js | 14 +++--- 7 files changed, 109 insertions(+), 59 deletions(-) delete mode 100644 lib/badrequesterror.js create mode 100644 lib/error.js rename test/{badrequesterror.js => error.js} (60%) diff --git a/README.md b/README.md index 2bb18ed..a93feaf 100644 --- a/README.md +++ b/README.md @@ -111,16 +111,6 @@ field to hold the username for example "email". * passwordValidator: specifies your custom validation function for the password in the form 'function(password,cb)'. Default: validates non-empty passwords. * usernameQueryFields: specifies alternative fields of the model for identifying a user (e.g. email). -__Error Message Options__ - -* incorrectPasswordError: specifies the error message returned when the password is incorrect. Defaults to 'Incorrect password'. -* incorrectUsernameError: specifies the error message returned when the username is incorrect. Defaults to 'Incorrect username'. -* missingUsernameError: specifies the error message returned when the username has not been set during registration. Defaults to 'Field %s is not set'. -* missingPasswordError: specifies the error message returned when the password has not been set during registration. Defaults to 'Password argument not set!'. -* userExistsError: specifies the error message returned when the user already exists during registration. Defaults to 'User already exists with name %s'. -* noSaltValueStored: specifies the error message returned in case no salt value is stored in the mongodb collection. Defaults to 'Authentication not possible. No salt value stored in mongodb collection!' -* tooManyAttemptsError: specifies the error message returned when the user's account is locked due to too many failed login attempts. Defaults to 'Account locked due to too many failed login attempts'. - *Attention!* Changing any of the hashing options (saltlen, iterations or keylen) in a production environment will prevent that existing users to authenticate! ### Hash Algorithm @@ -151,11 +141,25 @@ asynchronous method to reset a user's number of failed password attempts (only d - thisModel - the model getting authenticated *if* authentication was successful otherwise false - passwordErr - - the reason the password failed, else undefined ex. `{message: "Incorrect password"} + - an instance of `AuthenticationError` describing the reason the password failed, else undefined. Using `setPassword()` will only update the document's password fields, but will not save the document. To commit the changed document, remember to use Mongoose's `document.save()` after using `setPassword()`. +**Error Handling** + +* `IncorrectPasswordError`: specifies the error message returned when the password is incorrect. Defaults to 'Incorrect password'. +* `IncorrectUsernameError`: specifies the error message returned when the username is incorrect. Defaults to 'Incorrect username'. +* `MissingUsernameError`: specifies the error message returned when the username has not been set during registration. Defaults to 'Field %s is not set'. +* `MissingPasswordError`: specifies the error message returned when the password has not been set during registration. Defaults to 'Password argument not set!'. +* `UserExistsError`: specifies the error message returned when the user already exists during registration. Defaults to 'User already exists with name %s'. +* `NoSaltValueStored`: Occurs in case no salt value is stored in the MongoDB collection. +* `AttemptTooSoonError`: Occurs if the option `limitAttempts` is set to true and a login attept occures while the user is still penalized. +* `TooManyAttemptsError`: Returned when the user's account is locked due to too many failed login attempts. + +All those errors inherit from `AuthenticationError`, if you need a more general error class for checking. + + ### Static methods Static methods are exposed on the model constructor. For example to use createStrategy function use diff --git a/lib/badrequesterror.js b/lib/badrequesterror.js deleted file mode 100644 index 00acd63..0000000 --- a/lib/badrequesterror.js +++ /dev/null @@ -1,12 +0,0 @@ -var util = require('util'); - -function BadRequestError(message) { - Error.call(this); - Error.captureStackTrace(this, arguments.callee); - this.name = 'BadRequestError'; - this.message = message || null; -} - -util.inherits(BadRequestError, Error); - -module.exports = BadRequestError; diff --git a/lib/error.js b/lib/error.js new file mode 100644 index 0000000..c778b7f --- /dev/null +++ b/lib/error.js @@ -0,0 +1,68 @@ +var util = require('util'); + +function AuthenticationError(message) { + this.name = 'AuthenticationError'; + this.message = message || null; + this.stack = (new Error()).stack; +} + +function IncorrectUsernameError(usernameField) { + this.name = 'IncorrectUsernameError'; + this.message = util.format('Incorrect %s', usernameField); +} + +function IncorrectPasswordError() { + this.name = 'IncorrectPasswordError'; + this.message = 'Incorrect password'; +} + +function MissingUsernameError(usernameField) { + this.name = 'MissingUsernameError'; + this.message = util.format('Field %s is not set', usernameField); +} + +function MissingPasswordError() { + this.name = 'MissingPasswordError'; + this.message = util.format('Password argument not set'); +} + +function UserExistsError(usernameField, usernameValue) { + this.name = 'UserExistsError'; + this.message = util.format('User already exists with %s %s', usernameField, usernameValue); +} + +function NoSaltValueStoredError() { + this.name = 'NoSaltValueStoredError'; + this.message = util.format('Authentication not possible. No salt value stored in MongoDB collection'); +} + +function AttemptTooSoonError() { + this.name = 'AttemptTooSoonError'; + this.message = util.format('Login attempted too soon after previous attempt'); +} + +function TooManyAttemptsError() { + this.name = 'TooManyAttemptsError'; + this.message = util.format('Account locked due to too many failed login attempts'); +} + +util.inherits(AuthenticationError, Error); +util.inherits(IncorrectUsernameError, AuthenticationError); +util.inherits(IncorrectPasswordError, AuthenticationError); +util.inherits(MissingUsernameError, AuthenticationError); +util.inherits(MissingPasswordError, AuthenticationError); +util.inherits(UserExistsError, AuthenticationError); +util.inherits(NoSaltValueStoredError, AuthenticationError); +util.inherits(AttemptTooSoonError, AuthenticationError); +util.inherits(TooManyAttemptsError, AuthenticationError); + +module.exports.AuthenticationError = AuthenticationError; +module.exports.IncorrectUsernameError = IncorrectUsernameError; +module.exports.IncorrectPasswordError = IncorrectPasswordError; +module.exports.MissingUsernameError = MissingUsernameError; +module.exports.MissingPasswordError = MissingPasswordError; +module.exports.UserExistsError = UserExistsError; +module.exports.NoSaltValueStoredError = NoSaltValueStoredError; +module.exports.AttemptTooSoonError = AttemptTooSoonError; +module.exports.TooManyAttemptsError = TooManyAttemptsError; + diff --git a/lib/passport-local-mongoose.js b/lib/passport-local-mongoose.js index 84f2d15..12d7f58 100644 --- a/lib/passport-local-mongoose.js +++ b/lib/passport-local-mongoose.js @@ -1,7 +1,6 @@ -var util = require('util'); var crypto = require('crypto'); var LocalStrategy = require('passport-local').Strategy; -var BadRequestError = require('./badrequesterror'); +var Err = require('./error.js'); var scmp = require('scmp'); module.exports = function(schema, options) { @@ -39,15 +38,6 @@ module.exports = function(schema, options) { options.maxAttempts = options.maxAttempts || Infinity; } - options.incorrectPasswordError = options.incorrectPasswordError || 'Incorrect password'; - options.incorrectUsernameError = options.incorrectUsernameError || 'Incorrect %s'; - options.missingUsernameError = options.missingUsernameError || 'Field %s is not set'; - options.missingPasswordError = options.missingPasswordError || 'Password argument not set!'; - options.userExistsError = options.userExistsError || 'User already exists with %s %s'; - options.noSaltValueStoredError = options.noSaltValueStoredError || 'Authentication not possible. No salt value stored in mongodb collection!'; - options.attemptTooSoonError = options.attemptTooSoonError || 'Login attempted too soon after previous attempt'; - options.tooManyAttemptsError = options.tooManyAttemptsError || 'Account locked due to too many failed login attempts'; - var pbkdf2 = function(password, salt, callback) { if (crypto.pbkdf2.length >= 6) { crypto.pbkdf2(password, salt, options.iterations, options.keylen, options.digestAlgorithm, callback); @@ -82,7 +72,7 @@ module.exports = function(schema, options) { schema.methods.setPassword = function(password, cb) { if (!password) { - return cb(new BadRequestError(options.missingPasswordError)); + return cb(new Err.MissingPasswordError()); } var self = this; @@ -121,16 +111,16 @@ module.exports = function(schema, options) { if (Date.now() - user.get(options.lastLoginField) < calculatedInterval) { user.set(options.lastLoginField, Date.now()); user.save(); - return cb(null, false, {message: options.attemptTooSoonError}); + return cb(null, false, new Err.AttemptTooSoonError()); } if (user.get(options.attemptsField) >= options.maxAttempts) { - return cb(null, false, {message: options.tooManyAttemptsError}); + return cb(null, false, new Err.TooManyAttemptsError()); } } if (!user.get(options.saltField)) { - return cb(null, false, {message: options.noSaltValueStoredError}); + return cb(null, false, new Err.NoSaltValueStoredError()); } pbkdf2(password, user.get(options.saltField), function(err, hashRaw) { @@ -153,13 +143,13 @@ module.exports = function(schema, options) { user.set(options.attemptsField, user.get(options.attemptsField) + 1); user.save(function(err) { if (user.get(options.attemptsField) >= options.maxAttempts) { - return cb(null, false, {message: options.tooManyAttemptsError}); + return cb(null, false, new Err.TooManyAttemptsError()); } else { - return cb(null, false, {message: options.incorrectPasswordError}); + return cb(null, false, new Err.IncorrectPasswordError()); } }); } else { - return cb(null, false, {message: options.incorrectPasswordError}); + return cb(null, false, new Err.IncorrectPasswordError()); } } }); @@ -177,7 +167,7 @@ module.exports = function(schema, options) { if (user) { return authenticate(user, password, cb); } else { - return cb(null, false, {message: util.format(options.incorrectUsernameError, options.usernameField)}) + return cb(null, false, new Err.IncorrectUsernameError(options.usernameField)); } }); } else { @@ -202,7 +192,7 @@ module.exports = function(schema, options) { if (user) { return user.authenticate(password, cb); } else { - return cb(null, false, {message: util.format(options.incorrectUsernameError, options.usernameField)}) + return cb(null, false, new Err.IncorrectUsernameError(options.usernameField)); } }); } @@ -229,7 +219,7 @@ module.exports = function(schema, options) { } if (!user.get(options.usernameField)) { - return cb(new BadRequestError(util.format(options.missingUsernameError, options.usernameField))); + return cb(new Err.MissingUsernameError(options.usernameField)); } var self = this; @@ -237,7 +227,7 @@ module.exports = function(schema, options) { if (err) { return cb(err); } if (existingUser) { - return cb(new BadRequestError(util.format(options.userExistsError, options.usernameField, user.get(options.usernameField)))); + return cb(new Err.UserExistsError(options.usernameField, user.get(options.usernameField))); } user.setPassword(password, function(err, user) { diff --git a/test/badrequesterror.js b/test/error.js similarity index 60% rename from test/badrequesterror.js rename to test/error.js index 9514fd2..c9a4b34 100644 --- a/test/badrequesterror.js +++ b/test/error.js @@ -1,22 +1,22 @@ -var BadRequestError = require('../lib/badrequesterror'); +var Err = require('../lib/error.js'); var expect = require('chai').expect; -describe('BadRequestError', function() { +describe('AuthenticationError', function() { it('should construct a valid error with stack trace and name', function() { - var error = new BadRequestError(); + var error = new Err.AuthenticationError(); expect(error.stack).to.exist; - expect(error.name).to.equal('BadRequestError'); + expect(error.name).to.equal('AuthenticationError'); }); it('should construct a bad request error with message passed', function() { - var error = new BadRequestError('Test'); + var error = new Err.AuthenticationError('Test'); expect(error.message).to.equal('Test'); }); it('should construct a bad request error with null message if no message was passed', function() { - var error = new BadRequestError(); + var error = new Err.AuthenticationError(); expect(error.message).to.equal(null); }); diff --git a/test/issues.js b/test/issues.js index 2de6c85..ed768e6 100644 --- a/test/issues.js +++ b/test/issues.js @@ -55,7 +55,7 @@ describe('issues', function() { assert.equal(false, auth); assert.ok(reason); - assert.equal('Authentication not possible. No salt value stored in mongodb collection!', reason.message); + assert.equal('Authentication not possible. No salt value stored in MongoDB collection', reason.message); done(); }); diff --git a/test/passport-local-mongoose.js b/test/passport-local-mongoose.js index fa653ca..5f8007e 100644 --- a/test/passport-local-mongoose.js +++ b/test/passport-local-mongoose.js @@ -1,10 +1,10 @@ var mongoose = require('mongoose'); var Schema = mongoose.Schema; -var BadRequestError = require('../lib/badrequesterror'); -var passportLocalMongoose = require('../lib/passport-local-mongoose'); +var Err = require('../lib/error.js'); +var passportLocalMongoose = require('../lib/passport-local-mongoose.js'); var assert = require('assert'); var expect = require('chai').expect; -var mongotest = require('./mongotest'); +var mongotest = require('./mongotest.js'); var DefaultUserSchema = new Schema(); DefaultUserSchema.plugin(passportLocalMongoose); @@ -723,7 +723,7 @@ describe('passportLocalMongoose', function() { }); }); - it('should result in BadRequest error in case no username was given', function(done) { + it('should result in AuthenticationError error in case no username was given', function(done) { this.timeout(5000); // Five seconds - mongo db access needed var UserSchema = new Schema({}); @@ -731,12 +731,12 @@ describe('passportLocalMongoose', function() { var User = mongoose.model('RegisterUserWithoutUsername', UserSchema); User.register({}, 'password', function(err) { - expect(err).to.be.instanceof(BadRequestError); + expect(err).to.be.instanceof(Err.AuthenticationError); done(); }); }); - it('should result in BadRequest error in case no password was given', function(done) { + it('should result in AuthenticationError error in case no password was given', function(done) { this.timeout(5000); // Five seconds - mongo db access needed var UserSchema = new Schema({}); @@ -744,7 +744,7 @@ describe('passportLocalMongoose', function() { var User = mongoose.model('RegisterUserWithoutPassword', UserSchema); User.register({username: 'hugo'}, undefined, function(err) { - expect(err).to.be.instanceof(BadRequestError); + expect(err).to.be.instanceof(Err.AuthenticationError); done(); }); }); From 651089933c3a1b869049a8d2fdd8f7c3dc69cde5 Mon Sep 17 00:00:00 2001 From: Frederic Hemberger Date: Mon, 14 Sep 2015 16:22:25 +0200 Subject: [PATCH 2/4] Lint: Don't shadow variable names --- lib/passport-local-mongoose.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/passport-local-mongoose.js b/lib/passport-local-mongoose.js index 12d7f58..ef067f3 100644 --- a/lib/passport-local-mongoose.js +++ b/lib/passport-local-mongoose.js @@ -82,16 +82,16 @@ module.exports = function(schema, options) { return cb(err); } - crypto.randomBytes(options.saltlen, function(err, buf) { - if (err) { - return cb(err); + crypto.randomBytes(options.saltlen, function(randomBytesErr, buf) { + if (randomBytesErr) { + return cb(randomBytesErr); } var salt = buf.toString(options.encoding); - pbkdf2(password, salt, function(err, hashRaw) { - if (err) { - return cb(err); + pbkdf2(password, salt, function(pbkdf2Err, hashRaw) { + if (pbkdf2Err) { + return cb(pbkdf2Err); } self.set(options.hashField, new Buffer(hashRaw, 'binary').toString(options.encoding)); @@ -230,14 +230,14 @@ module.exports = function(schema, options) { return cb(new Err.UserExistsError(options.usernameField, user.get(options.usernameField))); } - user.setPassword(password, function(err, user) { - if (err) { - return cb(err); + user.setPassword(password, function(setPasswordErr, user) { + if (setPasswordErr) { + return cb(setPasswordErr); } - user.save(function(err) { - if (err) { - return cb(err); + user.save(function(saveErr) { + if (saveErr) { + return cb(saveErr); } cb(null, user); From fcf4b41ccfac4b6b45f3282a14205b85a1c2df30 Mon Sep 17 00:00:00 2001 From: Frederic Hemberger Date: Mon, 14 Sep 2015 16:22:43 +0200 Subject: [PATCH 3/4] Lint: Handle error case --- lib/passport-local-mongoose.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/passport-local-mongoose.js b/lib/passport-local-mongoose.js index ef067f3..87ce020 100644 --- a/lib/passport-local-mongoose.js +++ b/lib/passport-local-mongoose.js @@ -141,7 +141,8 @@ module.exports = function(schema, options) { if (options.limitAttempts) { user.set(options.lastLoginField, Date.now()); user.set(options.attemptsField, user.get(options.attemptsField) + 1); - user.save(function(err) { + user.save(function(saveErr) { + if (saveErr) { return cb(saveErr); } if (user.get(options.attemptsField) >= options.maxAttempts) { return cb(null, false, new Err.TooManyAttemptsError()); } else { From a29f04da8b648986418c9f7f4e586afd12a71a34 Mon Sep 17 00:00:00 2001 From: Frederic Hemberger Date: Mon, 14 Sep 2015 16:23:06 +0200 Subject: [PATCH 4/4] Lint: Add some semicolons --- lib/passport-local-mongoose.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/passport-local-mongoose.js b/lib/passport-local-mongoose.js index 87ce020..ef0d485 100644 --- a/lib/passport-local-mongoose.js +++ b/lib/passport-local-mongoose.js @@ -180,7 +180,7 @@ module.exports = function(schema, options) { schema.methods.resetAttempts = function(cb) { this.set(options.attemptsField, 0); this.save(cb); - } + }; } schema.statics.authenticate = function() { @@ -196,13 +196,13 @@ module.exports = function(schema, options) { return cb(null, false, new Err.IncorrectUsernameError(options.usernameField)); } }); - } + }; }; schema.statics.serializeUser = function() { return function(user, cb) { cb(null, user.get(options.usernameField)); - } + }; }; schema.statics.deserializeUser = function() { @@ -210,7 +210,7 @@ module.exports = function(schema, options) { return function(username, cb) { self.findByUsername(username, cb); - } + }; }; schema.statics.register = function(user, password, cb) {