From a028d9d19887e2518a6e2ce9fb3b5194d395ed5c Mon Sep 17 00:00:00 2001 From: Ron Edgecomb Date: Thu, 18 Dec 2014 15:26:27 -0500 Subject: [PATCH] Add error code property to known error responses. Enhance the error objects with a `code` property containing a machine-readable string code describing the error, for example INVALID_TOKEN or USER_NOT_FOUND. Also improve 404 error messages to include the model name. --- common/models/access-token.js | 1 + common/models/user.js | 8 ++++++- lib/application.js | 19 +++++++++++++---- lib/model.js | 4 +++- lib/persisted-model.js | 2 ++ test/access-token.test.js | 40 +++++++++++++++++++++++++++++++---- test/model.test.js | 10 ++++++++- test/relations.integration.js | 2 ++ test/rest.middleware.test.js | 10 ++++++++- test/user.test.js | 40 +++++++++++++++++++++++++++++++++-- 10 files changed, 122 insertions(+), 14 deletions(-) diff --git a/common/models/access-token.js b/common/models/access-token.js index 9c5b3285..c8db38fd 100644 --- a/common/models/access-token.js +++ b/common/models/access-token.js @@ -108,6 +108,7 @@ module.exports = function(AccessToken) { } else { var e = new Error('Invalid Access Token'); e.status = e.statusCode = 401; + e.code = 'INVALID_TOKEN'; cb(e); } }); diff --git a/common/models/user.js b/common/models/user.js index 97ea107d..f2739bfc 100644 --- a/common/models/user.js +++ b/common/models/user.js @@ -167,17 +167,20 @@ module.exports = function(User) { if (realmRequired && !query.realm) { var err1 = new Error('realm is required'); err1.statusCode = 400; + err1.code = 'REALM_REQUIRED'; return fn(err1); } if (!query.email && !query.username) { var err2 = new Error('username or email is required'); err2.statusCode = 400; + err2.code = 'USERNAME_EMAIL_REQUIRED'; return fn(err2); } self.findOne({where: query}, function(err, user) { var defaultError = new Error('login failed'); defaultError.statusCode = 401; + defaultError.code = 'LOGIN_FAILED'; if (err) { debug('An error is reported from User.findOne: %j', err); @@ -193,6 +196,7 @@ module.exports = function(User) { debug('User email has not been verified'); err = new Error('login failed as the email has not been verified'); err.statusCode = 401; + err.code = 'LOGIN_FAILED_EMAIL_NOT_VERIFIED'; return fn(err); } else { user.createAccessToken(credentials.ttl, function(err, token) { @@ -396,9 +400,11 @@ module.exports = function(User) { if (user) { err = new Error('Invalid token: ' + token); err.statusCode = 400; + err.code = 'INVALID_TOKEN'; } else { err = new Error('User not found: ' + uid); err.statusCode = 404; + err.code = 'USER_NOT_FOUND'; } fn(err); } @@ -447,7 +453,7 @@ module.exports = function(User) { } else { var err = new Error('email is required'); err.statusCode = 400; - + err.code = 'EMAIL_REQUIRED'; cb(err); } }; diff --git a/lib/application.js b/lib/application.js index 53bfec63..8f265f91 100644 --- a/lib/application.js +++ b/lib/application.js @@ -297,6 +297,7 @@ app.enableAuth = function() { var Model = method.ctor; var modelInstance = ctx.instance; var modelId = modelInstance && modelInstance.id || req.param('id'); + var modelName = Model.modelName; var modelSettings = Model.settings || {}; var errStatusCode = modelSettings.aclErrorStatus || app.get('aclErrorStatus') || 401; @@ -319,13 +320,23 @@ app.enableAuth = function() { } else { var messages = { - 403:'Access Denied', - 404: ('could not find a model with id ' + modelId), - 401:'Authorization Required' + 403: { + message: 'Access Denied', + code: 'ACCESS_DENIED' + }, + 404: { + message: ('could not find ' + modelName + ' with id ' + modelId), + code: 'MODEL_NOT_FOUND' + }, + 401: { + message: 'Authorization Required', + code: 'AUTHORIZATION_REQUIRED' + } }; - var e = new Error(messages[errStatusCode] || messages[403]); + var e = new Error(messages[errStatusCode].message || messages[403].message); e.statusCode = errStatusCode; + e.code = messages[errStatusCode].code || messages[403].code; next(e); } } diff --git a/lib/model.js b/lib/model.js index dd76b437..1981cda9 100644 --- a/lib/model.js +++ b/lib/model.js @@ -152,7 +152,7 @@ Model.setup = function() { } else { err = new Error('could not find a model with id ' + id); err.statusCode = 404; - + err.code = 'MODEL_NOT_FOUND'; fn(err); } }); @@ -455,6 +455,7 @@ Model.hasManyRemoting = function(relationName, relation, define) { var msg = 'Unknown "' + toModelName + '" id "' + fk + '".'; var error = new Error(msg); error.statusCode = error.status = 404; + error.code = 'MODEL_NOT_FOUND'; cb(error); } @@ -552,6 +553,7 @@ Model.hasManyRemoting = function(relationName, relation, define) { var msg = 'Unknown "' + modelName + '" id "' + id + '".'; var error = new Error(msg); error.statusCode = error.status = 404; + error.code = 'MODEL_NOT_FOUND'; cb(error); } else { cb(); diff --git a/lib/persisted-model.js b/lib/persisted-model.js index a4b4a5b2..4a74cde7 100644 --- a/lib/persisted-model.js +++ b/lib/persisted-model.js @@ -73,6 +73,7 @@ function convertNullToNotFoundError(ctx, cb) { var msg = 'Unknown "' + modelName + '" id "' + id + '".'; var error = new Error(msg); error.statusCode = error.status = 404; + error.code = 'MODEL_NOT_FOUND'; cb(error); } @@ -522,6 +523,7 @@ PersistedModel.setupRemoting = function() { var msg = 'Unknown "' + modelName + '" id "' + id + '".'; var error = new Error(msg); error.statusCode = error.status = 404; + error.code = 'MODEL_NOT_FOUND'; cb(error); } else { cb(); diff --git a/test/access-token.test.js b/test/access-token.test.js index 2f3c535f..58649554 100644 --- a/test/access-token.test.js +++ b/test/access-token.test.js @@ -191,7 +191,15 @@ describe('app.enableAuth()', function() { .del('/tests/123') .expect(401) .set('authorization', this.token.id) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'AUTHORIZATION_REQUIRED'); + done(); + }); }); it('prevent remote call with app setting status on denied ACL', function(done) { @@ -199,7 +207,15 @@ describe('app.enableAuth()', function() { .del('/tests/123') .expect(403) .set('authorization', this.token.id) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'ACCESS_DENIED'); + done(); + }); }); it('prevent remote call with app setting status on denied ACL', function(done) { @@ -207,7 +223,15 @@ describe('app.enableAuth()', function() { .del('/tests/123') .expect(404) .set('authorization', this.token.id) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'MODEL_NOT_FOUND'); + done(); + }); }); it('prevent remote call if the accessToken is missing and required', function(done) { @@ -215,7 +239,15 @@ describe('app.enableAuth()', function() { .del('/tests/123') .expect(401) .set('authorization', null) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'AUTHORIZATION_REQUIRED'); + done(); + }); }); it('stores token in the context', function(done) { diff --git a/test/model.test.js b/test/model.test.js index 2f32032d..ad6c305f 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -152,7 +152,15 @@ describe.onServer('Remote Methods', function() { request(app) .get('/users/not-found') .expect(404) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'MODEL_NOT_FOUND'); + done(); + }); }); }); diff --git a/test/relations.integration.js b/test/relations.integration.js index edd90064..b268577c 100644 --- a/test/relations.integration.js +++ b/test/relations.integration.js @@ -812,6 +812,7 @@ describe('relations - integration', function() { this.get(url).expect(404, function(err, res) { expect(res.body.error.status).to.be.equal(404); expect(res.body.error.message).to.be.equal('Unknown "todoItem" id "2".'); + expect(res.body.error.code).to.be.equal('MODEL_NOT_FOUND'); done(); }); }); @@ -1273,6 +1274,7 @@ describe('relations - integration', function() { expect(res.body.error).to.be.an.object; var expected = 'could not find a model with id unknown'; expect(res.body.error.message).to.equal(expected); + expect(res.body.error.code).to.be.equal('MODEL_NOT_FOUND'); done(); }); }); diff --git a/test/rest.middleware.test.js b/test/rest.middleware.test.js index 4a8243d9..ac29e7a3 100644 --- a/test/rest.middleware.test.js +++ b/test/rest.middleware.test.js @@ -18,7 +18,15 @@ describe('loopback.rest', function() { app.use(loopback.rest()); request(app).get('/mymodels/1') .expect(404) - .end(done); + .end(function(err, res) { + if (err) { + return done(err); + } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'MODEL_NOT_FOUND'); + done(); + }); }); it('should report 404 for HEAD /:id not found', function(done) { diff --git a/test/user.test.js b/test/user.test.js index 022af58c..96fbb000 100644 --- a/test/user.test.js +++ b/test/user.test.js @@ -126,6 +126,7 @@ describe('User', function() { User.login({email: 'b@c.com'}, function(err, accessToken) { assert(!accessToken, 'should not create a accessToken without a valid password'); assert(err, 'should not login without a password'); + assert.equal(err.code, 'LOGIN_FAILED'); done(); }); }); @@ -243,11 +244,20 @@ describe('User', function() { it('Login should only allow correct credentials', function(done) { User.login(invalidCredentials, function(err, accessToken) { assert(err); + assert.equal(err.code, 'LOGIN_FAILED'); assert(!accessToken); done(); }); }); + it('Login a user providing incomplete credentials', function(done) { + User.login(incompleteCredentials, function(err, accessToken) { + assert(err); + assert.equal(err.code, 'USERNAME_EMAIL_REQUIRED'); + done(); + }); + }); + it('Login a user over REST by providing credentials', function(done) { request(app) .post('/users/login') @@ -279,6 +289,8 @@ describe('User', function() { if (err) { return done(err); } + var errorResponse = res.body.error; + assert.equal(errorResponse.code, 'LOGIN_FAILED'); done(); }); }); @@ -293,6 +305,8 @@ describe('User', function() { if (err) { return done(err); } + var errorResponse = res.body.error; + assert.equal(errorResponse.code, 'USERNAME_EMAIL_REQUIRED'); done(); }); }); @@ -308,6 +322,8 @@ describe('User', function() { if (err) { return done(err); } + var errorResponse = res.body.error; + assert.equal(errorResponse.code, 'USERNAME_EMAIL_REQUIRED'); done(); }); }); @@ -370,6 +386,7 @@ describe('User', function() { // strongloop/loopback#931 // error message should be "login failed" and not "login failed as the email has not been verified" assert(err && !/verified/.test(err.message), ('expecting "login failed" error message, received: "' + err.message + '"')); + assert.equal(err.code, 'LOGIN_FAILED'); done(); }); }); @@ -377,6 +394,7 @@ describe('User', function() { it('Login a user by without email verification', function(done) { User.login(validCredentials, function(err, accessToken) { assert(err); + assert.equal(err.code, 'LOGIN_FAILED_EMAIL_NOT_VERIFIED'); done(); }); }); @@ -414,10 +432,14 @@ describe('User', function() { .expect(401) .send({ email: validCredentialsEmail }) .end(function(err, res) { + if (err) { + return done(err); + } // strongloop/loopback#931 // error message should be "login failed" and not "login failed as the email has not been verified" var errorResponse = res.body.error; assert(errorResponse && !/verified/.test(errorResponse.message), ('expecting "login failed" error message, received: "' + errorResponse.message + '"')); + assert.equal(errorResponse.code, 'LOGIN_FAILED'); done(); }); }); @@ -432,6 +454,8 @@ describe('User', function() { if (err) { return done(err); } + var errorResponse = res.body.error; + assert.equal(errorResponse.code, 'LOGIN_FAILED_EMAIL_NOT_VERIFIED'); done(); }); }); @@ -530,6 +554,7 @@ describe('User', function() { it('rejects a user by without realm', function(done) { User.login(credentialWithoutRealm, function(err, accessToken) { assert(err); + assert.equal(err.code, 'REALM_REQUIRED'); done(); }); }); @@ -537,6 +562,7 @@ describe('User', function() { it('rejects a user by with bad realm', function(done) { User.login(credentialWithBadRealm, function(err, accessToken) { assert(err); + assert.equal(err.code, 'LOGIN_FAILED'); done(); }); }); @@ -544,6 +570,7 @@ describe('User', function() { it('rejects a user by with bad pass', function(done) { User.login(credentialWithBadPass, function(err, accessToken) { assert(err); + assert.equal(err.code, 'LOGIN_FAILED'); done(); }); }); @@ -593,6 +620,7 @@ describe('User', function() { function(done) { User.login(credentialRealmInEmail, function(err, accessToken) { assert(err); + assert.equal(err.code, 'REALM_REQUIRED'); done(); }); }); @@ -841,7 +869,9 @@ describe('User', function() { if (err) { return done(err); } - assert(res.body.error); + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'USER_NOT_FOUND'); done(); }); }, done); @@ -858,7 +888,9 @@ describe('User', function() { if (err) { return done(err); } - assert(res.body.error); + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'INVALID_TOKEN'); done(); }); }, done); @@ -873,6 +905,7 @@ describe('User', function() { it('Requires email address to reset password', function(done) { User.resetPassword({ }, function(err) { assert(err); + assert.equal(err.code, 'EMAIL_REQUIRED'); done(); }); }); @@ -909,6 +942,9 @@ describe('User', function() { if (err) { return done(err); } + var errorResponse = res.body.error; + assert(errorResponse); + assert.equal(errorResponse.code, 'EMAIL_REQUIRED'); done(); }); });