From 6e89734e127b16976aea04455da36d745f712008 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 12 Jan 2017 14:08:52 -0700 Subject: [PATCH] Moved errors to errors.js, std error msgs/types --- app.js | 40 +++++---- errors.js | 126 +++++++++++++++++++++++------ middleware/authorization.js | 12 +-- models/setting.js | 2 +- routes/api/auth/index.js | 5 +- routes/api/comments/index.js | 9 ++- routes/api/stream/index.js | 6 +- tests/routes/api/comments/index.js | 4 +- 8 files changed, 140 insertions(+), 64 deletions(-) diff --git a/app.js b/app.js index b128eb283..9cdad4ea3 100644 --- a/app.js +++ b/app.js @@ -9,6 +9,7 @@ const enabled = require('debug').enabled; const RedisStore = require('connect-redis')(session); const redis = require('./services/redis'); const csrf = require('csurf'); +const errors = require('./errors'); const app = express(); @@ -109,40 +110,47 @@ app.use('/', require('./routes')); // ERROR HANDLING //============================================================================== -const ErrNotFound = new Error('Not Found'); -ErrNotFound.status = 404; - // Catch 404 and forward to error handler. app.use((req, res, next) => { - next(ErrNotFound); + next(errors.ErrNotFound); }); // General error handler. Respond with the message and error if we have it while // returning a status code that makes sense. app.use('/api', (err, req, res, next) => { - if (err !== ErrNotFound) { + if (err !== errors.ErrNotFound) { if (app.get('env') !== 'test' || enabled('talk:errors')) { console.error(err); } } - res.status(err.status || 500); - res.json({ - message: err.message, - error: app.get('env') === 'development' ? err : {} - }); + if (err instanceof errors.APIError) { + res.status(err.status).json({ + message: err.message, + error: err + }); + } else { + res.status(500).json({}); + } }); app.use('/', (err, req, res, next) => { - if (err !== ErrNotFound) { + if (err !== errors.ErrNotFound) { console.error(err); } - res.status(err.status || 500); - res.render('error', { - message: err.message, - error: app.get('env') === 'development' ? err : {} - }); + if (err instanceof errors.APIError) { + res.status(err.status); + res.render('error', { + message: err.message, + error: app.get('env') === 'development' ? err : {} + }); + } else { + res.render('error', { + message: err.message, + error: app.get('env') === 'development' ? err : {} + }); + } }); module.exports = app; diff --git a/errors.js b/errors.js index 0651b4fff..b5b217b61 100644 --- a/errors.js +++ b/errors.js @@ -1,45 +1,123 @@ +/** + * ExtendableError provides a base Error class to source off of that does not + * break the inheritence chain. + */ +class ExtendableError { + constructor(message = null) { + this.message = message; + this.stack = (new Error()).stack; + } +} + +/** + * APIError is the base error that all application issued errors originate, they + * are composed of data used by the front end and backend to handle errors + * consistently. + */ +class APIError extends ExtendableError { + constructor(message, {status = 500, translation_key = null}, metadata = {}) { + super(message); + + this.status = status; + this.translation_key = translation_key; + this.metadata = metadata; + } + + toJSON() { + return { + message: this.message, + status: this.status, + translation_key: this.translation_key, + metadata: this.metadata + }; + } +} + // ErrPasswordTooShort is returned when the password length is too short. -const ErrPasswordTooShort = new Error('password must be at least 8 characters'); -ErrPasswordTooShort.translation_key = 'PASSWORD_LENGTH'; -ErrPasswordTooShort.status = 400; +const ErrPasswordTooShort = new APIError('password must be at least 8 characters', { + status: 400, + translation_key: 'PASSWORD_LENGTH' +}); -const ErrMissingEmail = new Error('email is required'); -ErrMissingEmail.translation_key = 'EMAIL_REQUIRED'; -ErrMissingEmail.status = 400; +const ErrMissingEmail = new APIError('email is required', { + translation_key: 'EMAIL_REQUIRED', + status: 400 +}); -const ErrMissingPassword = new Error('password is required'); -ErrMissingPassword.translation_key = 'PASSWORD_REQUIRED'; -ErrMissingPassword.status = 400; +const ErrMissingPassword = new APIError('password is required', { + translation_key: 'PASSWORD_REQUIRED', + status: 400 +}); -const ErrEmailTaken = new Error('Email address already in use'); -ErrEmailTaken.translation_key = 'EMAIL_IN_USE'; -ErrEmailTaken.status = 400; +const ErrEmailTaken = new APIError('Email address already in use', { + translation_key: 'EMAIL_IN_USE', + status: 400 +}); -const ErrSpecialChars = new Error('No special characters are allowed in a display name'); -ErrSpecialChars.translation_key = 'NO_SPECIAL_CHARACTERS'; -ErrSpecialChars.status = 400; +const ErrSpecialChars = new APIError('No special characters are allowed in a display name', { + translation_key: 'NO_SPECIAL_CHARACTERS', + status: 400 +}); -const ErrMissingDisplay = new Error('A display name is required to create a user'); -ErrMissingDisplay.translation_key = 'DISPLAY_NAME_REQUIRED'; -ErrMissingDisplay.status = 400; +const ErrMissingDisplay = new APIError('A display name is required to create a user', { + translation_key: 'DISPLAY_NAME_REQUIRED', + status: 400 +}); // ErrMissingToken is returned in the event that the password reset is requested // without a token. -const ErrMissingToken = new Error('token is required'); -ErrMissingToken.status = 400; +const ErrMissingToken = new APIError('token is required', { + status: 400 +}); + +// ErrAssetCommentingClosed is returned when a comment or action is attempted on +// a stream where commenting has been closed. +class ErrAssetCommentingClosed extends APIError { + constructor(closedMessage = null) { + super('asset commenting is closed', { + status: 400 + }, { + + // Include the closedMessage in the metadata piece of the error. + closedMessage + }); + } +} // ErrContainsProfanity is returned in the event that the middleware detects // profanity/wordlisted words in the payload. -const ErrContainsProfanity = new Error('Suspected profanity. If you think this in error, please let us know!'); -ErrContainsProfanity.translation_key = 'PROFANITY_ERROR'; -ErrContainsProfanity.status = 400; +const ErrContainsProfanity = new APIError('Suspected profanity. If you think this in error, please let us know!', { + translation_key: 'PROFANITY_ERROR', + status: 400 +}); + +const ErrNotFound = new APIError('not found', { + status: 404 +}); + +const ErrInvalidAssetURL = new APIError('asset_url is invalid', { + status: 400 +}); + +// ErrNotAuthorized is an error that is returned in the event an operation is +// deemed not authorized. +const ErrNotAuthorized = new APIError('not authorized', { + status: 401 +}); module.exports = { + ExtendableError, + APIError, ErrPasswordTooShort, ErrMissingEmail, ErrMissingPassword, + ErrMissingToken, ErrEmailTaken, ErrSpecialChars, ErrMissingDisplay, - ErrContainsProfanity + ErrContainsProfanity, + ErrAssetCommentingClosed, + ErrNotFound, + ErrInvalidAssetURL, + ErrNotAuthorized }; diff --git a/middleware/authorization.js b/middleware/authorization.js index 65f77b753..efe4b1034 100644 --- a/middleware/authorization.js +++ b/middleware/authorization.js @@ -7,17 +7,7 @@ const authorization = module.exports = { }; const debug = require('debug')('talk:middleware:authorization'); - -/** - * ErrNotAuthorized is an error that is returned in the event an operation is - * deemed not authorized. - * @type {Error} - */ -const ErrNotAuthorized = new Error('not authorized'); -ErrNotAuthorized.status = 401; - -// Add the ErrNotAuthorized error to the authorization object to be exported. -authorization.ErrNotAuthorized = ErrNotAuthorized; +const ErrNotAuthorized = require('../errors').ErrNotAuthorized; /** * has returns true if the user has all the roles specified, otherwise it will diff --git a/models/setting.js b/models/setting.js index ad9fa6599..ed41acfa6 100644 --- a/models/setting.js +++ b/models/setting.js @@ -42,7 +42,7 @@ const SettingSchema = new Schema({ }, closedMessage: { type: String, - default: '' + default: 'Expired' }, wordlist: WordlistSchema, charCount: { diff --git a/routes/api/auth/index.js b/routes/api/auth/index.js index 6a84c6049..050aa1135 100644 --- a/routes/api/auth/index.js +++ b/routes/api/auth/index.js @@ -1,6 +1,7 @@ const express = require('express'); const passport = require('../../../services/passport'); const authorization = require('../../../middleware/authorization'); +const errors = require('../../../errors'); const router = express.Router(); @@ -42,7 +43,7 @@ const HandleAuthCallback = (req, res, next) => (err, user) => { } if (!user) { - return next(authorization.ErrNotAuthorized); + return next(errors.ErrNotAuthorized); } // Perform the login of the user! @@ -65,7 +66,7 @@ const HandleAuthPopupCallback = (req, res, next) => (err, user) => { } if (!user) { - return res.render('auth-callback', {err: JSON.stringify(authorization.ErrNotAuthorized), data: null}); + return res.render('auth-callback', {err: JSON.stringify(errors.ErrNotAuthorized), data: null}); } // Perform the login of the user! diff --git a/routes/api/comments/index.js b/routes/api/comments/index.js index 21426d217..226f772be 100644 --- a/routes/api/comments/index.js +++ b/routes/api/comments/index.js @@ -1,4 +1,5 @@ const express = require('express'); +const errors = require('../../../errors'); const Comment = require('../../../models/comment'); const Asset = require('../../../models/asset'); const User = require('../../../models/user'); @@ -20,13 +21,13 @@ router.get('/', (req, res, next) => { // everything on this route requires admin privileges besides listing comments for owner of said comments if (!authorization.has(req.user, 'admin') && !user_id) { - next(authorization.ErrNotAuthorized); + next(errors.ErrNotAuthorized); return; } // if the user is not an admin, only return comment list for the owner of the comments if (req.user.id !== user_id && !authorization.has(req.user, 'admin')) { - next(authorization.ErrNotAuthorized); + next(errors.ErrNotAuthorized); return; } @@ -102,14 +103,14 @@ router.post('/', wordlist.filter('body'), (req, res, next) => { status = Asset .rectifySettings(Asset.findById(asset_id).then((asset) => { if (!asset) { - return Promise.reject(new Error('asset referenced is not found')); + return Promise.reject(errors.ErrNotFound); } // Check to see if the asset has closed commenting... if (asset.isClosed) { // They have, ensure that we send back an error. - return Promise.reject(new Error(`asset has commenting closed because: ${asset.closedMessage}`)); + return Promise.reject(new errors.ErrAssetCommentingClosed(asset.closedMessage)); } return asset; diff --git a/routes/api/stream/index.js b/routes/api/stream/index.js index 68d0f0ad9..540e36e52 100644 --- a/routes/api/stream/index.js +++ b/routes/api/stream/index.js @@ -1,6 +1,7 @@ const express = require('express'); const _ = require('lodash'); const scraper = require('../../../services/scraper'); +const errors = require('../../../errors'); const url = require('url'); const Comment = require('../../../models/comment'); @@ -9,9 +10,6 @@ const Action = require('../../../models/action'); const Asset = require('../../../models/asset'); const Setting = require('../../../models/setting'); -const ErrInvalidAssetURL = new Error('asset_url is invalid'); -ErrInvalidAssetURL.status = 400; - const router = express.Router(); router.get('/', (req, res, next) => { @@ -21,7 +19,7 @@ router.get('/', (req, res, next) => { // Verify that the asset_url is parsable. let parsed_asset_url = url.parse(asset_url); if (!parsed_asset_url.protocol) { - return next(ErrInvalidAssetURL); + return next(errors.ErrInvalidAssetURL); } // Get the asset_id for this url (or create it if it doesn't exist) diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index 360daa5dc..246da24a7 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -101,7 +101,7 @@ describe('/api/v1/comments', () => { expect(res).to.be.empty; }) .catch((err) => { - expect(err).to.have.property('status', 401); + expect(err).to.have.status(401); }); }); @@ -287,7 +287,7 @@ describe('/api/v1/comments', () => { .catch((err) => { expect(err.response.body).to.not.be.null; expect(err.response.body).to.have.property('message'); - expect(err.response.body.message).to.contain('tests said expired!'); + expect(err.response.body.error.metadata.closedMessage).to.be.equal('tests said expired!'); }); });