Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling: Always return instance of 'AuthenticationError' #105

Merged
merged 4 commits into from
Sep 16, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
12 changes: 0 additions & 12 deletions lib/badrequesterror.js

This file was deleted.

68 changes: 68 additions & 0 deletions lib/error.js
Original file line number Diff line number Diff line change
@@ -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;

69 changes: 30 additions & 39 deletions lib/passport-local-mongoose.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -92,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));
Expand All @@ -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) {
Expand All @@ -151,15 +141,16 @@ 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, {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());
}
}
});
Expand All @@ -177,7 +168,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 {
Expand All @@ -189,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() {
Expand All @@ -202,24 +193,24 @@ 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));
}
});
}
};
};

schema.statics.serializeUser = function() {
return function(user, cb) {
cb(null, user.get(options.usernameField));
}
};
};

schema.statics.deserializeUser = function() {
var self = this;

return function(username, cb) {
self.findByUsername(username, cb);
}
};
};

schema.statics.register = function(user, password, cb) {
Expand All @@ -229,25 +220,25 @@ 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;
self.findByUsername(user.get(options.usernameField), function(err, existingUser) {
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) {
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);
Expand Down
12 changes: 6 additions & 6 deletions test/badrequesterror.js → test/error.js
Original file line number Diff line number Diff line change
@@ -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);
});
Expand Down
2 changes: 1 addition & 1 deletion test/issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
14 changes: 7 additions & 7 deletions test/passport-local-mongoose.js
Original file line number Diff line number Diff line change
@@ -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);
Expand Down Expand Up @@ -723,28 +723,28 @@ 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({});
UserSchema.plugin(passportLocalMongoose, {});
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({});
UserSchema.plugin(passportLocalMongoose, {});
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();
});
});
Expand Down