Improve error handling

.gitignore

@@ -4,3 +4,4 @@ dist/
 .idea/
 .vscode/
 .DS_Store
+debug.js

README.md

@@ -53,7 +53,7 @@ const auth0Strategy = new auth.strategies.Auth0({
 const enforcer = new auth.Enforcer({
   strategy: auth0Strategy,
   // Pass in an asynchronous database lookup function
-  // that takes the user id as the only argument.
+  // that takes the user ID as the only argument.
   userGetter: id => User.findOne(id),
 });
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@pathcheck/safeplaces-auth",
-  "version": "1.0.0-alpha.4",
+  "version": "1.0.0-alpha.5",
   "description": "The modular authentication service for the SafePlaces backend",
   "main": "src/index.js",
   "license": "MIT",
@@ -25,7 +25,8 @@
     "ajv": "^6.12.3",
     "jsonwebtoken": "^8.5.1",
     "jwks-rsa": "^1.8.1",
-    "node-fetch": "^2.6.0"
+    "node-fetch": "^2.6.0",
+    "verror": "^1.10.0"
   },
   "devDependencies": {
     "coveralls": "^3.1.0",

src/common/generate.js

@@ -1,12 +1,13 @@
+const assert = require('assert');
 const crypto = require('crypto');
 
 function cookieString(attributes) {
-  if (!attributes) {
-    throw new Error('Cookie attributes are required');
-  }
-  if (!attributes.name || !attributes.value) {
-    throw new Error('Cookie name and value are required');
-  }
+  assert.ok(attributes, 'cookie attributes are required');
+  assert.ok(
+    attributes.name && attributes.value,
+    'cookie name and value are required',
+  );
+
   const {
     name,
     value,
@@ -44,9 +45,7 @@ function cookieString(attributes) {
 }
 
 function password(length) {
-  if (length === null || length === undefined) {
-    throw new Error('Password length is required');
-  }
+  assert.ok(length, 'password length is required');
 
   return crypto
     .randomBytes(length)

src/common/hook.js

@@ -1,44 +1,48 @@
+const assert = require('assert');
+
 class Hook {
   constructor(callbacks) {
-    if (!callbacks) {
-      throw new Error('Callbacks are required');
-    }
-    if (!callbacks.id) {
-      throw new Error('Id translation callbacks are required');
-    }
-    if (!callbacks.id.dbToIdm) {
-      throw new Error('DB to IDM id translation callback is required');
-    }
-    if (typeof callbacks.id.dbToIdm !== 'function') {
-      throw new Error('DB to IDM id translation should be a function');
-    }
-    if (!callbacks.id.idmToDb) {
-      throw new Error('IDM to DB id translation callback is required');
-    }
-    if (typeof callbacks.id.idmToDb !== 'function') {
-      throw new Error('IDM to DB id translation should be a function');
-    }
-    if (!callbacks.users) {
-      throw new Error('User modification callbacks are required');
-    }
-    if (!callbacks.users.create) {
-      throw new Error('User creation callback is required');
-    }
-    if (typeof callbacks.users.create !== 'function') {
-      throw new Error('User creation callback should be a function');
-    }
-    if (!callbacks.users.delete) {
-      throw new Error('User deletion callback is required');
-    }
-    if (typeof callbacks.users.delete !== 'function') {
-      throw new Error('User deletion callback should be a function');
-    }
-    if (!callbacks.users.getAll) {
-      throw new Error('User get all callback is required');
-    }
-    if (typeof callbacks.users.getAll !== 'function') {
-      throw new Error('User get all callback should be a function');
-    }
+    assert.ok(callbacks, 'callbacks are required');
+
+    assert.ok(callbacks.id, 'ID translation callbacks are required');
+
+    assert.ok(
+      callbacks.id.dbToIdm,
+      'DB to IDM id translation callback is required',
+    );
+    assert.ok(
+      typeof callbacks.id.dbToIdm === 'function',
+      'DB to IDM id translation should be a function',
+    );
+
+    assert.ok(
+      callbacks.id.idmToDb,
+      'IDM to DB id translation callback is required',
+    );
+    assert.ok(
+      typeof callbacks.id.idmToDb === 'function',
+      'IDM to DB id translation should be a function',
+    );
+
+    assert.ok(callbacks.users, 'user modification callbacks are required');
+
+    assert.ok(callbacks.users.create, 'user creation callback is required');
+    assert.ok(
+      typeof callbacks.users.create === 'function',
+      'user creation callback should be a function',
+    );
+
+    assert.ok(callbacks.users.delete, 'user deletion callback is required');
+    assert.ok(
+      typeof callbacks.users.delete === 'function',
+      'user deletion callback should be a function',
+    );
+
+    assert.ok(callbacks.users.getAll, 'user get all callback is required');
+    assert.ok(
+      typeof callbacks.users.getAll === 'function',
+      'user get all callback should be a function',
+    );
 
     this._callbacks = callbacks;
   }

src/common/werror.js

@@ -0,0 +1,32 @@
+/**
+ * Wrapped error object.
+ */
+class WError extends Error {
+  constructor(options) {
+    const message = options.cause
+      ? `${options.message}: ${options.cause.message}`
+      : options.message;
+    super(message);
+    this.name = options.name || 'Error';
+    this.data = options.data || {};
+    this.cause = options.cause;
+  }
+
+  static getCause(err) {
+    if (err.cause) {
+      return err.cause;
+    }
+    return null;
+  }
+
+  static hasCauseWithName(err, name) {
+    if (err.name === name) return true;
+    const cause = WError.getCause(err);
+    if (cause) {
+      return WError.hasCauseWithName(cause, name);
+    }
+    return false;
+  }
+}
+
+module.exports = WError;

src/gatekeeper/enforcer.js

@@ -1,16 +1,22 @@
+const assert = require('assert');
+const WError = require('../common/werror');
 const requtils = require('./requtils');
-const errors = require('./errors');
+const errorTranslator = require('./errortranslator');
 
 class Enforcer {
-  constructor({ strategy, userGetter, authorizer }) {
-    if (!strategy) {
-      throw new Error('Enforcer strategy is required');
-    }
-    if (!userGetter) {
-      throw new Error('Enforcer user getter is required');
-    }
-    if (authorizer && typeof authorizer !== 'function') {
-      throw new Error('Enforcer authorizer must be a function');
+  constructor(params) {
+    assert.ok(params, 'enforcer parameters are required');
+
+    const { strategy, userGetter, authorizer } = params;
+    assert.ok(strategy, 'enforcer strategy is required');
+    assert.ok(userGetter, 'enforcer user getter is required');
+
+    if (authorizer) {
+      assert.strictEqual(
+        typeof authorizer,
+        'function',
+        'enforcer authorizer must be a function',
+      );
     }
 
     this.strategy = strategy;
@@ -25,20 +31,15 @@ class Enforcer {
 
   async handleRequest(req, res, next) {
     try {
-      const note = await this.processRequest(req);
-      // Some problem was encountered, indicate this to the client via a header.
-      if (note) {
-        const errorCode = errors.lookup(note);
-        res.header(errors.getHeaderNS(), errorCode);
-      }
+      await this.processRequest(req);
     } catch (e) {
       if (this.verbose) {
         console.log(e);
       }
-      const errorCode = errors.lookup(e.name);
+      const errorCode = errorTranslator.lookup(e);
       return res
         .status(403)
-        .header(errors.getHeaderNS(), errorCode)
+        .header(errorTranslator.getHeaderNS(), errorCode)
         .send('Forbidden');
     }
     if (next) {
@@ -47,8 +48,6 @@ class Enforcer {
   }
 
   async processRequest(req) {
-    let note = null;
-
     Enforcer.checkCSRF(req);
 
     // Try to obtain the token from the header.
@@ -61,20 +60,24 @@ class Enforcer {
     // Try to validate and decode the token.
     const decoded = await strategy.validate(token);
 
+    let user;
     try {
-      const user = await this.userGetter(decoded.sub);
-      if (!user) {
-        if (this.verbose) {
-          console.error(
-            `Could not obtain user ${decoded.sub} from user getter`,
-          );
-        }
-        note = 'UserGetterNotFound';
-      }
-      req.user = user;
+      user = await this.userGetter(decoded.sub);
     } catch (e) {
-      throw errors.construct('UserGetterFailure', e.message);
+      throw new WError({
+        name: 'UserGetterError',
+        message: 'user getter threw an error',
+        cause: e,
+      });
+    }
+
+    if (!user) {
+      throw new WError({
+        name: 'UserGetterError',
+        message: 'user getter not found',
+      });
     }
+    req.user = user;
 
     if (this.authorizer) {
       try {
@@ -83,29 +86,34 @@ class Enforcer {
         if (this.verbose) {
           console.log(e);
         }
-        throw errors.construct('AuthorizerFailure', e.message);
+        throw new WError({
+          name: 'AuthorizerError',
+          message: 'authorizer threw an error',
+          cause: e,
+        });
       }
     }
-
-    return note;
   }
 
   static checkCSRF(req) {
     if (!req.headers) {
-      throw errors.construct('MissingHeaders', 'No headers found');
+      throw new WError({
+        name: 'MissingHeadersError',
+        message: 'no headers found',
+      });
     }
     const csrfHeader = req.headers['x-requested-with'];
     if (!csrfHeader) {
-      throw errors.construct(
-        'MissingCSRFHeader',
-        'x-requested-with header not found',
-      );
+      throw new WError({
+        name: 'MissingCSRFHeaderError',
+        message: 'x-requested-with header not found',
+      });
     }
     if (csrfHeader !== 'XMLHttpRequest') {
-      throw errors.construct(
-        'InvalidCSRFHeader',
-        `Invalid value in x-requested-with header: ${csrfHeader}`,
-      );
+      throw new WError({
+        name: 'InvalidCSRFHeaderError',
+        message: 'invalid value in x-requested-with header: ' + csrfHeader,
+      });
     }
   }
 }