From 6c46b532c9831782c413425b23fbee4ed34d2b03 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Fri, 5 Apr 2019 17:11:44 -0400
Subject: [PATCH 1/8] [SER-279] Entire implementation.

---
 .env.example                              |   1 +
 .env.test                                 |   1 +
 CHANGELOG.md                              |   2 +-
 README.md                                 |   6 +
 src/config/config.js                      |   7 +-
 src/config/express.js                     |  17 ++-
 src/config/passport.js                    |   2 +-
 src/controllers/auth.controller.js        |  10 +-
 src/controllers/user.controller.js        |  10 +-
 src/helpers/APIError.js                   | 110 ++++++++++++++----
 src/helpers/jwt.js                        |   2 +-
 src/helpers/logLevelGte.js                |  23 ++++
 src/helpers/owasp.js                      |   2 +-
 test/helpers/APIError.spec.js             | 132 ++++++++++++++++++++++
 test/helpers/logLevelGte.spec.js          |  26 +++++
 test/integration/auth.integration.spec.js |   6 +-
 16 files changed, 308 insertions(+), 49 deletions(-)
 create mode 100644 src/helpers/logLevelGte.js
 create mode 100644 test/helpers/APIError.spec.js
 create mode 100644 test/helpers/logLevelGte.spec.js

diff --git a/.env.example b/.env.example
index a02f6dc..359d3d8 100644
--- a/.env.example
+++ b/.env.example
@@ -1,6 +1,7 @@
 ## Shared ##
 NODE_ENV=development
 LOG_LEVEL=info
+ALWAYS_INCLUDE_ERROR_STACKS=false
 
 ## Auth Microservice ##
 JWT_SECRET=0a6b944d-d2fb-46fc-a85e-0295c986cd9f
diff --git a/.env.test b/.env.test
index 74ebcc3..8ceb1fb 100644
--- a/.env.test
+++ b/.env.test
@@ -1,5 +1,6 @@
 NODE_ENV=test
 LOG_LEVEL=debug
+ALWAYS_INCLUDE_ERROR_STACKS=false
 
 JWT_MODE=hmac
 JWT_SECRET=0a6b944d-d2fb-46fc-a85e-0295c986cd9f
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 62e661e..05f750e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,7 @@
 - `auth.controller.js:login()` no longer throws duplicate errors for incorrect username or password
 - Use error handling middlware `winstonInstance.info(err)`
   * Previously, `winstonInstance.errorLogger()` always threw a `TypeError`, meaning the underlying error didn't actually show in the logs
-
+- APIError now supports all 10 features in https://jira.amida.com/browse/SER-279
 
 ## [2.7.0] -- 2019-02-04
 ### Added
diff --git a/README.md b/README.md
index 9980b4b..cf49b2a 100644
--- a/README.md
+++ b/README.md
@@ -334,6 +334,12 @@ A description of what the variable is or does.
 
 - Valid values are [winston](https://github.com/winstonjs/winston) logging levels (`error`, `warn`, etc.).
 
+##### `ALWAYS_INCLUDE_ERROR_STACKS` [`false`]
+
+The `APIError` class acts like a call-stackless "operational error" (https://www.joyent.com/node-js/production/design/errors) when called without a causal error as the first argument.
+- When false, `APIError`s without a cuasal error will not have call stacks.
+- When true, all `APIError`s will have call stacks.
+
 ##### `JWT_SECRET` (Required)
 
 First, see description of `AUTH_SERVICE_JWT_MODE`. When `AUTH_SERVICE_JWT_MODE=hmac`, this is the shared secret between this service an all services using this service for authentication. Therefore, all other such service must set their `JWT_SECRET` to match this value.
diff --git a/src/config/config.js b/src/config/config.js
index 9d3bbd0..24f6d44 100644
--- a/src/config/config.js
+++ b/src/config/config.js
@@ -16,6 +16,8 @@ const envVarsSchema = Joi.object({
         .default('production'),
     LOG_LEVEL: Joi.string()
         .default('info'),
+    ALWAYS_INCLUDE_ERROR_STACKS: Joi.bool()
+        .default(false),
     AUTH_SERVICE_PORT: Joi.number()
         .default(4000),
     AUTH_SERVICE_PUBLIC_REGISTRATION: Joi.bool()
@@ -127,9 +129,10 @@ if (error) {
     throw new Error(`Config validation error: ${error.message}`);
 }
 
-module.exports = {
+const config = {
     env: envVars.NODE_ENV,
     logLevel: envVars.LOG_LEVEL,
+    alwaysIncludeErrorStacks: envVars.ALWAYS_INCLUDE_ERROR_STACKS,
     port: envVars.AUTH_SERVICE_PORT,
     publicRegistration: envVars.AUTH_SERVICE_PUBLIC_REGISTRATION,
     registrarScopes: envVars.AUTH_SERVICE_REGISTRAR_SCOPES,
@@ -169,3 +172,5 @@ module.exports = {
         scopes: ['admin'],
     },
 };
+
+module.exports = config;
diff --git a/src/config/express.js b/src/config/express.js
index 3861c5f..5ace0b4 100644
--- a/src/config/express.js
+++ b/src/config/express.js
@@ -68,13 +68,18 @@ app.use('/api', routes);
 app.use((err, req, res, next) => {
     if (err instanceof Sequelize.ValidationError) {
         const status = err.errors[0].type === 'unique violation' ? httpStatus.CONFLICT : httpStatus.BAD_REQUEST;
-        const unifiedErrors = JSON.stringify(err.errors);
-        const error = new APIError(unifiedErrors, 'GENERIC_ERROR', status, true);
+        // err.errors[0].message has the most useful information from Sequelize, so we want it to
+        // end up in the nested message of the APIError. However, it could have PHI or PII, so it
+        // must be within the causal error message and must not be passed directly into APIError's
+        // message argument.
+        // eslint-disable-next-line no-param-reassign
+        err.message = `${err.message}: ${err.errors[0].message}`;
+        const error = new APIError(err, 'Sequelize Validation Error', 'GENERIC_ERROR', status);
         return next(error);
     } else if (err instanceof expressValidation.ValidationError) {
         // validation error contains errors which is an array of error each containing message[]
         const unifiedErrorMessage = err.errors.map(error => error.messages.join('. ')).join(' and ');
-        const error = new APIError(unifiedErrorMessage, 'GENERIC_ERROR', err.status, true);
+        const error = new APIError(unifiedErrorMessage, 'GENERIC_ERROR', err.status);
         return next(error);
     }
     return next(err);
@@ -82,7 +87,7 @@ app.use((err, req, res, next) => {
 
 // catch 404 and forward to error handler
 app.use((req, res, next) => {
-    const err = new APIError('API not found', 'UNKNOWN_API', httpStatus.NOT_FOUND);
+    const err = new APIError('API not found', 'UNKNOWN_API', httpStatus.NOT_FOUND, { isPublic: false });
     return next(err);
 });
 
@@ -97,9 +102,9 @@ if (config.env !== 'test') {
 // error handler, send stacktrace only during development
 app.use((err, req, res, next) => // eslint-disable-line no-unused-vars
     res.status(err.status || 500).json({
-        code: err.isPublic ? err.message.code : 'UNKNOWN_ERROR',
+        code: (err.options && err.isPublic) ? err.code : 'UNKNOWN_ERROR',
         status: 'ERROR',
-        message: err.isPublic ? err.message.message : httpStatus[err.status],
+        message: (err.options && err.isPublic) ? err.message : httpStatus[err.status],
         stack: config.env === 'development' ? err.stack : {},
     })
 );
diff --git a/src/config/passport.js b/src/config/passport.js
index 79f9635..b7dcb99 100644
--- a/src/config/passport.js
+++ b/src/config/passport.js
@@ -47,7 +47,7 @@ module.exports = (passport) => {
             } })
             .spread((user) => {
                 if (user !== null) return done(null, user);
-                const err = new APIError('New facebook user not created', 'FACEBOOK_CREATION_ERROR', httpStatus.INTERNAL_SERVER_ERROR, true);
+                const err = new APIError('New facebook user not created', 'FACEBOOK_CREATION_ERROR', httpStatus.INTERNAL_SERVER_ERROR);
                 return done(err);
             });
         });
diff --git a/src/controllers/auth.controller.js b/src/controllers/auth.controller.js
index d01c4c9..ebedaa5 100644
--- a/src/controllers/auth.controller.js
+++ b/src/controllers/auth.controller.js
@@ -28,7 +28,7 @@ function login(req, res, next) {
         const validUserAndPassword = !_.isNull(user) && user.testPassword(params.password);
 
         if (!validUserAndPassword) {
-            const err = new APIError('Incorrect username or password', 'INCORRECT_USERNAME_OR_PASSWORD', httpStatus.NOT_FOUND, true);
+            const err = new APIError('Incorrect username or password', 'INCORRECT_USERNAME_OR_PASSWORD', httpStatus.UNAUTHORIZED);
             return next(err);
         }
 
@@ -82,7 +82,7 @@ function submitRefreshToken(req, res, next) {
         .findOne({ where: { token: params.refreshToken, userId: userResult.id } })
         .then((tokenResult) => {
             if (_.isNull(tokenResult)) {
-                const err = new APIError('Refresh token not found', 'MISSING_REFRESH_TOKEN', httpStatus.NOT_FOUND, true);
+                const err = new APIError('Refresh token not found', 'MISSING_REFRESH_TOKEN', httpStatus.NOT_FOUND);
                 return next(err);
             }
             const userInfo = {
@@ -122,7 +122,7 @@ function rejectRefreshToken(req, res, next) {
     return RefreshToken.findOne({ where: { token: params.refreshToken } })
     .then((tokenResult) => {
         if (_.isNull(tokenResult)) {
-            const err = new APIError('Refresh token not found', 'MISSING_REFRESH_TOKEN', httpStatus.NOT_FOUND, true);
+            const err = new APIError('Refresh token not found', 'MISSING_REFRESH_TOKEN', httpStatus.NOT_FOUND);
             return next(err);
         }
         // delete the refresh token
@@ -146,7 +146,7 @@ function updatePassword(req, res, next) {
     const params = _.pick(req.body, 'oldPassword', 'password');
 
     if (!user.testPassword(params.oldPassword)) {
-        const err = new APIError('Incorrect password', 'INCORRECT_PASSWORD', httpStatus.FORBIDDEN, true);
+        const err = new APIError('Incorrect password', 'INCORRECT_PASSWORD', httpStatus.FORBIDDEN);
         return next(err);
     }
 
@@ -171,7 +171,7 @@ function resetToken(req, res, next) {
     const email = _.get(req, 'body.email');
     const resetPageUrl = _.get(req, 'body.resetPageUrl');
     if (!email) {
-        const err = new APIError('Invalid email', 'INVALID_EMAIL', httpStatus.BAD_REQUEST, true);
+        const err = new APIError('Invalid email', 'INVALID_EMAIL', httpStatus.BAD_REQUEST);
         return next(err);
     }
     return User.resetPasswordToken(email, 3600)
diff --git a/src/controllers/user.controller.js b/src/controllers/user.controller.js
index 4561a0a..61f85e1 100644
--- a/src/controllers/user.controller.js
+++ b/src/controllers/user.controller.js
@@ -22,7 +22,7 @@ function load(req, res, next, id) {
     User.findById(id)
         .then((user) => {
             if (!user) {
-                const e = new APIError('User does not exist', 'UNKNOWN_USERNAME', httpStatus.NOT_FOUND, true);
+                const e = new APIError('User does not exist', 'UNKNOWN_USERNAME', httpStatus.NOT_FOUND);
                 return next(e);
             }
             req.user = user; // eslint-disable-line no-param-reassign
@@ -44,7 +44,7 @@ function get(req, res, next) {
     User.findById(req.params.userId)
         .then((user) => {
             if (req.user.username !== user.username && !req.user.isAdmin()) {
-                const e = new APIError('Cannot get another user\'s information', 'UNAUTHORIZED_REQUEST', httpStatus.FORBIDDEN, true);
+                const e = new APIError('Cannot get another user\'s information', 'UNAUTHORIZED_REQUEST', httpStatus.FORBIDDEN);
                 return next(e);
             }
             return res.json(user.getBasicUserInfo());
@@ -70,7 +70,7 @@ function getByEmail(req, res, next) {
                 return next(e);
             }
             if (req.user.username !== user.username && !req.user.isAdmin()) {
-                const e = new APIError('Cannot get another user\'s information', 'UNAUTHORIZED_REQUEST', httpStatus.FORBIDDEN, true);
+                const e = new APIError('Cannot get another user\'s information', 'UNAUTHORIZED_REQUEST', httpStatus.FORBIDDEN);
                 return next(e);
             }
             return res.json(user.getBasicUserInfo());
@@ -104,7 +104,7 @@ function create(req, res, next) {
             // we are not admin but are registrar
             // if trying to create new registrar or admin, return forbidden
             if (potentialScopes.some(scope => ['admin', ...config.registrarScopes].includes(scope))) {
-                const e = new APIError('Registrar not allowed to create new admin or registrar users', 'CANNOT_CREATE_ADMIN', httpStatus.FORBIDDEN, true);
+                const e = new APIError('Registrar not allowed to create new admin or registrar users', 'CANNOT_CREATE_ADMIN', httpStatus.FORBIDDEN);
                 return next(e);
             }
             // else go ahead
@@ -123,7 +123,7 @@ function update(req, res, next) {
     User.findById(req.params.userId)
         .then((user) => {
             if (req.user.username !== user.username && !req.user.isAdmin()) {
-                const e = new APIError('User not allowed to update email', 'CANNOT_UPDATE_EMAIL', httpStatus.FORBIDDEN, true);
+                const e = new APIError('User not allowed to update email', 'CANNOT_UPDATE_EMAIL', httpStatus.FORBIDDEN);
                 return next(e);
             }
             return user.update({ email: req.body.email });
diff --git a/src/helpers/APIError.js b/src/helpers/APIError.js
index 987ad61..c6f2fb1 100644
--- a/src/helpers/APIError.js
+++ b/src/helpers/APIError.js
@@ -1,35 +1,95 @@
-import httpStatus from 'http-status';
+import logLevelGte from './logLevelGte';
+import config from '../config/config';
+
+const includeCausalError = logLevelGte('debug');
+const alwaysIncludeErrorStacks = config.alwaysIncludeErrorStacks;
 
 /**
  * @extends Error
  */
-class ExtendableError extends Error {
-    constructor(message, code, status, isPublic) {
-        super(message);
-        this.name = this.constructor.name;
-        this.message = { status: 'ERROR', message, code };
+class APIError extends Error {
+    constructor(arg1, arg2, arg3, arg4, arg5) {
+        if (!(typeof arg1 === 'string' || arg1 instanceof String || arg1 instanceof Error)) {
+            // eslint-disable-next-line max-len
+            throw new Error('Attempting to run APIError constructor failed: The first arg to the constructor must be a string or object that is instanceof Error.');
+        }
+
+        // eslint-disable-next-line one-var,one-var-declaration-per-line
+        let causalError, message, code, status, options;
+
+        if (typeof arg1 === 'string' || arg1 instanceof String) {
+            causalError = undefined;
+            message = arg1;
+            code = arg2;
+            status = arg3;
+            options = arg4;
+        } else {
+            causalError = arg1;
+            message = arg2;
+            code = arg3;
+            status = arg4;
+            options = arg5;
+        }
+
+        // Errors thrown by libraries we use might accidentally log data we pass into them,
+        // such as PHI or PII. Therefore, when the log level is less than debug, don't
+        // prepend this error's message with the causal/root error's message.
+        // eslint-disable-next-line no-underscore-dangle
+        let _message;
+        if (includeCausalError && causalError && causalError.message) {
+            _message = `${message}: ${causalError.message}`;
+        } else {
+            _message = message;
+        }
+
+        super(_message);
+
+        this.message = _message;
         this.code = code;
         this.status = status;
-        this.isPublic = isPublic;
-        this.isOperational = true; // This is required since bluebird 4 doesn't append it anymore.
-        Error.captureStackTrace(this, this.constructor.name);
-    }
-}
 
-/**
- * Class representing an API error.
- * @extends ExtendableError
- */
-class APIError extends ExtendableError {
-
-    /**
-     * Creates an API error.
-     * @param {string} message - Error message.
-     * @param {number} status - HTTP status code of error.
-     * @param {boolean} isPublic - Whether the message should be visible to user or not.
-     */
-    constructor(message, code, status = httpStatus.INTERNAL_SERVER_ERROR, isPublic = false) {
-        super(message, code, status, isPublic);
+        // Default to empty object in order to prevent "Cannot access X of undefined" errors.
+        if (!options) {
+            options = {};
+        }
+
+        // This class assumes that IF it was instantiated without a causal error, THEN this class is
+        // being used to define an "operational error", and there is no actual "programmer error",
+        // AKA bug (https://www.joyent.com/node-js/production/design/errors).
+        this.isOperational = !causalError;
+        if (options.isOperational !== undefined) {
+            this.isOperational = options.isOperational;
+        }
+
+        // When this.isPublic is truthy, the server's response body will include the error "code"
+        // and "message".
+        // We want isPublic to default to true. The reason is that most of the time we do want the
+        // code and message to be returned to the client rather than hidden, and we don't want to
+        // have to specify isPublic every time we create a new error. In fact, the primary purpose
+        // of the "code" is to support the front end's needs.
+        if (options.isPublic !== false) {
+            options.isPublic = true;
+        }
+        this.isPublic = options.isPublic;
+        this.options = options;
+
+        // Errors thrown by various libraries we use might accidentally log data we pass into them
+        // when we call them, such as PHI or PII. Therefore, for example, you can set the value of
+        // includeCausalError based on log level. For example, when log level is less than debug,
+        // don't nest the causal error inside this error.
+        if (this.isOperational || includeCausalError) {
+            this.cause = causalError;
+        }
+
+        // This class extends Error, so this.stack gets automatically generated; we don't always
+        // want a stack, so we have to do something with it.
+        // We don't want a stack trace when there is an operational error because we don't want to
+        // mislead future developer maintainers into thinking there's a bug, when there isn't.
+        // We only want this.stack when this class is being used for a "programmer error" OR if we
+        // override this behavior and always have stacks.
+        if (!(!this.isOperational || alwaysIncludeErrorStacks || options.includeStack)) {
+            delete this.stack;
+        }
     }
 }
 
diff --git a/src/helpers/jwt.js b/src/helpers/jwt.js
index dfe96a6..f3f7471 100644
--- a/src/helpers/jwt.js
+++ b/src/helpers/jwt.js
@@ -19,7 +19,7 @@ module.exports = {
      */
     checkExternalProvider(req, res, next) {
         if (req.user.provider !== null) {
-            const err = new APIError('Cannot call this endpoint for user managed with external auth', 'EXTERNAL_AUTH_USED', httpStatus.FORBIDDEN, true);
+            const err = new APIError('Cannot call this endpoint for user managed with external auth', 'EXTERNAL_AUTH_USED', httpStatus.FORBIDDEN);
             return next(err);
         }
         return next();
diff --git a/src/helpers/logLevelGte.js b/src/helpers/logLevelGte.js
new file mode 100644
index 0000000..23b4303
--- /dev/null
+++ b/src/helpers/logLevelGte.js
@@ -0,0 +1,23 @@
+import config from '../config/config';
+
+// Like JSON.parse(), this function and has the ability to throw errors.
+// Therefore calls to it must be wrapped in try/catch.
+const logLevelGte = (checkLogLevel) => {
+    // TODO: hard code this list of levels somewhere in the logger, and make the list used here the
+    // exact same as the one used there.
+    const validLogLevels = ['error', 'warn', 'info', 'verbose', 'debug', 'silly'];
+
+    // TODO: Improve checking via using some sort of typing instead??
+    if (validLogLevels.indexOf(checkLogLevel) === -1) {
+        // eslint-disable-next-line max-len
+        throw new Error(`logLevelGte(): 'checkLogLevel' is ${checkLogLevel}, but it must be one of the valid levels ('error', 'warn', 'info', 'verbose', 'debug', 'silly').`);
+    }
+
+    if (validLogLevels.indexOf(config.logLevel) >= validLogLevels.indexOf(checkLogLevel)) {
+        return true;
+    }
+
+    return false;
+};
+
+export default logLevelGte;
diff --git a/src/helpers/owasp.js b/src/helpers/owasp.js
index a2be8ba..6f82ae9 100644
--- a/src/helpers/owasp.js
+++ b/src/helpers/owasp.js
@@ -13,7 +13,7 @@ module.exports = {
     checkPassword(req, res, next) {
         const result = owasp.test(req.body.password);
         if (!result.strong) {
-            const err = new APIError(result.errors.join('\r\n'), 'STRONG_PASS_REQUIRED', httpStatus.BAD_REQUEST, true);
+            const err = new APIError(result.errors.join('\r\n'), 'STRONG_PASS_REQUIRED', httpStatus.BAD_REQUEST);
             return next(err);
         }
         return next();
diff --git a/test/helpers/APIError.spec.js b/test/helpers/APIError.spec.js
new file mode 100644
index 0000000..7776cfe
--- /dev/null
+++ b/test/helpers/APIError.spec.js
@@ -0,0 +1,132 @@
+/* eslint-env mocha */
+
+import { fail } from 'assert';
+import { expect } from 'chai';
+import httpStatus from 'http-status';
+
+import APIError from '../../src/helpers/APIError';
+
+const causalErrorMessage = 'Causal error message';
+const apiErrorMessage = 'APIError message';
+const errorCode = 'ERROR_CODE';
+
+describe('APIError:', () => {
+    describe('Default options.isPublic should be true...', () => {
+        it('When options argument is not specified', () => {
+            const errWithoutOptions = new APIError('a', 'b', 'c');
+            expect(errWithoutOptions.options).to.deep.equal({ isPublic: true });
+        });
+
+        it('When options argument is specified, but no isPublic option specified', () => {
+            const errWithOptionsButLackingIsPublic = new APIError('a', 'b', 'c', { optionA: 'A' });
+            expect(errWithOptionsButLackingIsPublic.options).to.deep.equal({ optionA: 'A', isPublic: true });
+        });
+    });
+
+    describe('Causal Error Handling:', () => {
+        describe('When includeCausalError is truthy...', () => {
+            it('The causal error is set to myAPIError.cause', () => {
+                const causalError = new Error(causalErrorMessage);
+                const err = new APIError(causalError, apiErrorMessage);
+
+                expect(err.cause).to.deep.equal(causalError);
+            });
+
+            it('"Concatonate" messages of APIError and the causal errror', () => {
+                const causalError = new Error(causalErrorMessage);
+                const err = new APIError(causalError, apiErrorMessage);
+
+                expect(err.message).to.equal(`${apiErrorMessage}: ${causalErrorMessage}`);
+            });
+        });
+
+        // // TODO: This requires mocking `logLevelGte()` because APIError.js has
+        // // `const includeCausalError = logLevelGte('debug')`.
+        // describe('When includeCausalError is falsy', () => {
+        //     it('myAPIError.cause is undefined', () => {
+        //         const causalError = new Error(causalErrorMessage);
+        //         const err = new APIError(causalError, apiErrorMessage);
+
+        //         expect(err.cause).to.be.undefined; // eslint-disable-line no-unused-expressions
+        //     });
+
+        //     it('Don\'t "concatonate", messages of causal error and APIError', () => {
+        //         const causalError = new Error(causalErrorMessage);
+        //         const err = new APIError(causalError, apiErrorMessage);
+
+        //         expect(err.message).to.equal(apiErrorMessage);
+        //     });
+        // });
+    });
+
+    describe('Supports argument shifting based on first argument being a causal error, or not:', () => {
+        it('Fist argument must be either an error or string', () => {
+            try {
+                new APIError(new Error('something')); // eslint-disable-line no-new
+                new APIError('this can be any string literal'); // eslint-disable-line no-new
+                new APIError(new String('something')); // eslint-disable-line no-new,no-new-wrappers
+            } catch (err) {
+                fail();
+            }
+        });
+
+        it('Throws error when first arg is not an error or string', () => {
+            try {
+                new APIError(); // eslint-disable-line no-new
+                // The error should get thrown, so this line should never run.
+                fail();
+            } catch (err) {
+                expect(err.message).to.equal('Attempting to run APIError constructor failed: The first arg to the constructor must be a string or object that is instanceof Error.');
+            }
+        });
+
+        it('Supports calling with causal error', () => {
+            const causalError = new Error(causalErrorMessage);
+            const err = new APIError(causalError, apiErrorMessage, errorCode, httpStatus.NOT_FOUND, { optionA: 'A', isPublic: false });
+
+            expect(err.cause).to.deep.equal(causalError);
+            expect(err.message).to.equal(`${apiErrorMessage}: ${causalErrorMessage}`);
+            expect(err.code).to.equal(errorCode);
+            expect(err.status).to.equal(httpStatus.NOT_FOUND);
+            expect(err.options).to.deep.equal({ optionA: 'A', isPublic: false });
+        });
+
+        it('Supports calling without causal error', () => {
+            const err = new APIError(apiErrorMessage, errorCode, httpStatus.NOT_FOUND, { optionA: 'A', isPublic: false });
+
+            expect(err.cause).to.be.undefined; // eslint-disable-line no-unused-expressions
+            expect(err.message).to.equal(apiErrorMessage);
+            expect(err.code).to.equal(errorCode);
+            expect(err.status).to.equal(httpStatus.NOT_FOUND);
+            expect(err.options).to.deep.equal({ optionA: 'A', isPublic: false });
+        });
+    });
+
+    describe('Error stacks:', () => {
+        describe('There should be a stack when...', () => {
+            it('There is a causal error', () => {
+                const causalError = new Error();
+                const err = new APIError(causalError);
+                expect(err.stack).to.exist; // eslint-disable-line no-unused-expressions
+            });
+
+            // // TODO: Mock set config.alwaysIncludeErrorStacks before importing APIError.js
+            // it('alwaysIncludeErrorStacks is truthy', () => {
+            //     const err = new APIError('some string');
+            //     expect(err.stack).to.exist; // eslint-disable-line no-unused-expressions
+            // });
+
+            it('options.includeStack is truthy', () => {
+                const err = new APIError('message', 'CODE', 'http status', { includeStack: true });
+                expect(err.stack).to.exist; // eslint-disable-line no-unused-expressions
+            });
+        });
+
+        describe('There should not be a stack...', () => {
+            it('In any other case', () => {
+                const err = new APIError('some string');
+                expect(err.stack).to.be.undefined; // eslint-disable-line no-unused-expressions
+            });
+        });
+    });
+});
diff --git a/test/helpers/logLevelGte.spec.js b/test/helpers/logLevelGte.spec.js
new file mode 100644
index 0000000..b189afd
--- /dev/null
+++ b/test/helpers/logLevelGte.spec.js
@@ -0,0 +1,26 @@
+/* eslint-env mocha */
+
+import { expect } from 'chai';
+import { fail } from 'assert';
+
+import logLevelGte from '../../src/helpers/logLevelGte';
+
+describe('#logLevelGte()', () => {
+    it('Throws error if called with invalid log level', () => {
+        try {
+            logLevelGte('not_a_real_log_level');
+            fail();
+        } catch (err) {
+            expect(err.message).to.equal("logLevelGte(): 'checkLogLevel' is not_a_real_log_level, but it must be one of the valid levels ('error', 'warn', 'info', 'verbose', 'debug', 'silly').");
+        }
+    });
+
+    it('Works when config.logLevel=debug', () => {
+        expect(logLevelGte('error')).to.be.true; // eslint-disable-line no-unused-expressions
+        expect(logLevelGte('warn')).to.be.true; // eslint-disable-line no-unused-expressions
+        expect(logLevelGte('info')).to.be.true; // eslint-disable-line no-unused-expressions
+        expect(logLevelGte('verbose')).to.be.true; // eslint-disable-line no-unused-expressions
+        expect(logLevelGte('debug')).to.be.true; // eslint-disable-line no-unused-expressions
+        expect(logLevelGte('silly')).to.be.false; // eslint-disable-line no-unused-expressions
+    });
+});
diff --git a/test/integration/auth.integration.spec.js b/test/integration/auth.integration.spec.js
index a25345f..a5118cd 100644
--- a/test/integration/auth.integration.spec.js
+++ b/test/integration/auth.integration.spec.js
@@ -54,7 +54,7 @@ describe('Auth API:', () => {
         it('should return 401 error with bad password', () =>
             request(app).post(`${common.baseURL}/auth/login`)
                 .send(common.badPassword)
-                .expect(httpStatus.NOT_FOUND)
+                .expect(httpStatus.UNAUTHORIZED)
                 .then((res) => {
                     expect(res.body.message).to.equal('Incorrect username or password');
                     expect(res.body.code).to.equal('INCORRECT_USERNAME_OR_PASSWORD');
@@ -62,10 +62,10 @@ describe('Auth API:', () => {
                 })
         );
 
-        it('should return 404 when there is no username found', () =>
+        it('should return 401 when there is no username found', () =>
             request(app).post(`${common.baseURL}/auth/login`)
                 .send(common.missingUsername)
-                .expect(httpStatus.NOT_FOUND)
+                .expect(httpStatus.UNAUTHORIZED)
                 .then((res) => {
                     expect(res.body.message).to.equal('Incorrect username or password');
                     expect(res.body.code).to.equal('INCORRECT_USERNAME_OR_PASSWORD');

From 04edc55c9125c78e1392ca13dd2b828bfd361048 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Thu, 18 Apr 2019 15:16:39 -0400
Subject: [PATCH 2/8] [SER-279] remove local logLevelGte implementation; import
 it from winston-json-formatter

---
 package.json                     |  4 ++--
 src/helpers/APIError.js          |  9 +++++++--
 src/helpers/logLevelGte.js       | 23 -----------------------
 test/helpers/APIError.spec.js    |  4 ++--
 test/helpers/logLevelGte.spec.js | 26 --------------------------
 yarn.lock                        | 12 ++++++------
 6 files changed, 17 insertions(+), 61 deletions(-)
 delete mode 100644 src/helpers/logLevelGte.js
 delete mode 100644 test/helpers/logLevelGte.spec.js

diff --git a/package.json b/package.json
index 2a0c258..4ccbf34 100644
--- a/package.json
+++ b/package.json
@@ -63,8 +63,8 @@
     "sequelize": "^4.40.0",
     "swagger-stats": "^0.95.6",
     "uuid": "3.1.0",
-    "winston": "^3.0.0",
-    "winston-json-formatter": "^0.9.2"
+    "winston": "^3.2.1",
+    "winston-json-formatter": "^0.10.0"
   },
   "devDependencies": {
     "@babel/cli": "^7.1.5",
diff --git a/src/helpers/APIError.js b/src/helpers/APIError.js
index c6f2fb1..116fdce 100644
--- a/src/helpers/APIError.js
+++ b/src/helpers/APIError.js
@@ -1,9 +1,14 @@
-import logLevelGte from './logLevelGte';
+import { initLogLevelGte } from 'winston-json-formatter';
 import config from '../config/config';
 
-const includeCausalError = logLevelGte('debug');
 const alwaysIncludeErrorStacks = config.alwaysIncludeErrorStacks;
 
+// Errors thrown by 3rd party libraries might log data that we pass into them, such as PHI or PII.
+// And, devs might pass those errors as the causalError arg into APIError. So, to prevent accidental
+// logging of PHI/PII, only include and thus log causal errors if your config.logLevel is >= debug.
+const logLevelGte = initLogLevelGte(config.logLevel);
+const includeCausalError = logLevelGte('debug');
+
 /**
  * @extends Error
  */
diff --git a/src/helpers/logLevelGte.js b/src/helpers/logLevelGte.js
deleted file mode 100644
index 23b4303..0000000
--- a/src/helpers/logLevelGte.js
+++ /dev/null
@@ -1,23 +0,0 @@
-import config from '../config/config';
-
-// Like JSON.parse(), this function and has the ability to throw errors.
-// Therefore calls to it must be wrapped in try/catch.
-const logLevelGte = (checkLogLevel) => {
-    // TODO: hard code this list of levels somewhere in the logger, and make the list used here the
-    // exact same as the one used there.
-    const validLogLevels = ['error', 'warn', 'info', 'verbose', 'debug', 'silly'];
-
-    // TODO: Improve checking via using some sort of typing instead??
-    if (validLogLevels.indexOf(checkLogLevel) === -1) {
-        // eslint-disable-next-line max-len
-        throw new Error(`logLevelGte(): 'checkLogLevel' is ${checkLogLevel}, but it must be one of the valid levels ('error', 'warn', 'info', 'verbose', 'debug', 'silly').`);
-    }
-
-    if (validLogLevels.indexOf(config.logLevel) >= validLogLevels.indexOf(checkLogLevel)) {
-        return true;
-    }
-
-    return false;
-};
-
-export default logLevelGte;
diff --git a/test/helpers/APIError.spec.js b/test/helpers/APIError.spec.js
index 7776cfe..af174b8 100644
--- a/test/helpers/APIError.spec.js
+++ b/test/helpers/APIError.spec.js
@@ -40,8 +40,8 @@ describe('APIError:', () => {
             });
         });
 
-        // // TODO: This requires mocking `logLevelGte()` because APIError.js has
-        // // `const includeCausalError = logLevelGte('debug')`.
+        // // TODO: This requires mocking `initLogLevelGte()` because APIError.js has
+        // const logLevelGte = initLogLevelGte(config.logLevel);
         // describe('When includeCausalError is falsy', () => {
         //     it('myAPIError.cause is undefined', () => {
         //         const causalError = new Error(causalErrorMessage);
diff --git a/test/helpers/logLevelGte.spec.js b/test/helpers/logLevelGte.spec.js
deleted file mode 100644
index b189afd..0000000
--- a/test/helpers/logLevelGte.spec.js
+++ /dev/null
@@ -1,26 +0,0 @@
-/* eslint-env mocha */
-
-import { expect } from 'chai';
-import { fail } from 'assert';
-
-import logLevelGte from '../../src/helpers/logLevelGte';
-
-describe('#logLevelGte()', () => {
-    it('Throws error if called with invalid log level', () => {
-        try {
-            logLevelGte('not_a_real_log_level');
-            fail();
-        } catch (err) {
-            expect(err.message).to.equal("logLevelGte(): 'checkLogLevel' is not_a_real_log_level, but it must be one of the valid levels ('error', 'warn', 'info', 'verbose', 'debug', 'silly').");
-        }
-    });
-
-    it('Works when config.logLevel=debug', () => {
-        expect(logLevelGte('error')).to.be.true; // eslint-disable-line no-unused-expressions
-        expect(logLevelGte('warn')).to.be.true; // eslint-disable-line no-unused-expressions
-        expect(logLevelGte('info')).to.be.true; // eslint-disable-line no-unused-expressions
-        expect(logLevelGte('verbose')).to.be.true; // eslint-disable-line no-unused-expressions
-        expect(logLevelGte('debug')).to.be.true; // eslint-disable-line no-unused-expressions
-        expect(logLevelGte('silly')).to.be.false; // eslint-disable-line no-unused-expressions
-    });
-});
diff --git a/yarn.lock b/yarn.lock
index c8d6863..f512269 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -7337,14 +7337,14 @@ windows-release@^3.1.0:
   dependencies:
     execa "^0.10.0"
 
-winston-json-formatter@^0.9.2:
-  version "0.9.2"
-  resolved "https://registry.yarnpkg.com/winston-json-formatter/-/winston-json-formatter-0.9.2.tgz#cb2adad2596a11c19d3ac5a454756712a2aacf98"
-  integrity sha512-IeWNsy/VzsZDXjXNrdxvw9MvCQ28QBSw6zgCkTLS2av28wJnScGDpq97ys7X9ZWygpEGZrsGXJxyWk3bBsqhZg==
+winston-json-formatter@^0.10.0:
+  version "0.10.0"
+  resolved "https://registry.yarnpkg.com/winston-json-formatter/-/winston-json-formatter-0.10.0.tgz#4e777023e4416e16740734e1b7b50c5e72dfd4fd"
+  integrity sha512-4K9rFgawAOBsKbC5D5QvxiZccbuBR+xvPY+PAm6rTDtfaA5bwUjpFKW0YRexcrFlJzV/6ExtfUBgYpDXAm4NSA==
   dependencies:
     lodash "^4.17.11"
     os "^0.1.1"
-    winston "^3.1.0"
+    winston "^3.2.1"
 
 winston-transport@^4.3.0:
   version "4.3.0"
@@ -7354,7 +7354,7 @@ winston-transport@^4.3.0:
     readable-stream "^2.3.6"
     triple-beam "^1.2.0"
 
-winston@^3.0.0, winston@^3.1.0:
+winston@^3.2.1:
   version "3.2.1"
   resolved "https://registry.yarnpkg.com/winston/-/winston-3.2.1.tgz#63061377976c73584028be2490a1846055f77f07"
   integrity sha512-zU6vgnS9dAWCEKg/QYigd6cgMVVNwyTzKs81XZtTFuRwJOcDdBg7AU0mXVyNbs7O5RH2zdv+BdNZUlx7mXPuOw==

From 841730f154f292849516d9e309d2f740c01232c4 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Fri, 19 Apr 2019 10:10:45 -0400
Subject: [PATCH 3/8] [SER-279] Improve isOperational, isPublic,
 includeCausalError, and includeStack handling

---
 src/config/express.js         |  2 +-
 src/helpers/APIError.js       | 65 +++++++++++++++--------------
 test/helpers/APIError.spec.js | 77 +++++++++++++++++++++++------------
 3 files changed, 85 insertions(+), 59 deletions(-)

diff --git a/src/config/express.js b/src/config/express.js
index 5ace0b4..c458706 100644
--- a/src/config/express.js
+++ b/src/config/express.js
@@ -74,7 +74,7 @@ app.use((err, req, res, next) => {
         // message argument.
         // eslint-disable-next-line no-param-reassign
         err.message = `${err.message}: ${err.errors[0].message}`;
-        const error = new APIError(err, 'Sequelize Validation Error', 'GENERIC_ERROR', status);
+        const error = new APIError(err, 'Sequelize Validation Error', 'GENERIC_ERROR', status, { isOperational: true });
         return next(error);
     } else if (err instanceof expressValidation.ValidationError) {
         // validation error contains errors which is an array of error each containing message[]
diff --git a/src/helpers/APIError.js b/src/helpers/APIError.js
index 116fdce..172d50b 100644
--- a/src/helpers/APIError.js
+++ b/src/helpers/APIError.js
@@ -1,6 +1,12 @@
 import { initLogLevelGte } from 'winston-json-formatter';
 import config from '../config/config';
 
+// TODO: Document this with the auto-documentation tool properly.
+// Read https://www.joyent.com/node-js/production/design/errors to understand the difference between
+// "programmer errors" (i.e. bugs) and "operational errors" (i.e. not actual bugs).
+// The APIError class assumes that if it was instantiated without a causal error, then it is being
+// used to define an "operational error", and there is no actual "programmer error"/bug.
+
 const alwaysIncludeErrorStacks = config.alwaysIncludeErrorStacks;
 
 // Errors thrown by 3rd party libraries might log data that we pass into them, such as PHI or PII.
@@ -36,12 +42,27 @@ class APIError extends Error {
             options = arg5;
         }
 
-        // Errors thrown by libraries we use might accidentally log data we pass into them,
-        // such as PHI or PII. Therefore, when the log level is less than debug, don't
-        // prepend this error's message with the causal/root error's message.
+        // Default to empty object to prevent "Cannot read property 'X' of undefined" errors.
+        if (!options) {
+            options = {};
+        }
+
+        // If there is no causal error, assume this is an "operational error" (see comment above).
+        // (Define isOperational here because it must be used before the constructor, and therefore
+        // before this.isOperational is available for use.)
+        let isOperational = !causalError;
+
+        // `options.isOperational` can override the default isOperational behavior.
+        if (options.isOperational !== undefined) {
+            isOperational = options.isOperational;
+        }
+
+        // eslint-disable-next-line no-underscore-dangle
+        const _includeCausalError = includeCausalError || options.includeCausalError;
+
         // eslint-disable-next-line no-underscore-dangle
         let _message;
-        if (includeCausalError && causalError && causalError.message) {
+        if (_includeCausalError && causalError && causalError.message) {
             _message = `${message}: ${causalError.message}`;
         } else {
             _message = message;
@@ -52,19 +73,8 @@ class APIError extends Error {
         this.message = _message;
         this.code = code;
         this.status = status;
-
-        // Default to empty object in order to prevent "Cannot access X of undefined" errors.
-        if (!options) {
-            options = {};
-        }
-
-        // This class assumes that IF it was instantiated without a causal error, THEN this class is
-        // being used to define an "operational error", and there is no actual "programmer error",
-        // AKA bug (https://www.joyent.com/node-js/production/design/errors).
-        this.isOperational = !causalError;
-        if (options.isOperational !== undefined) {
-            this.isOperational = options.isOperational;
-        }
+        this.options = options;
+        this.isOperational = isOperational;
 
         // When this.isPublic is truthy, the server's response body will include the error "code"
         // and "message".
@@ -72,27 +82,20 @@ class APIError extends Error {
         // code and message to be returned to the client rather than hidden, and we don't want to
         // have to specify isPublic every time we create a new error. In fact, the primary purpose
         // of the "code" is to support the front end's needs.
-        if (options.isPublic !== false) {
-            options.isPublic = true;
-        }
-        this.isPublic = options.isPublic;
-        this.options = options;
+        this.isPublic = (options.isPublic === undefined ? true : options.isPublic);
 
-        // Errors thrown by various libraries we use might accidentally log data we pass into them
-        // when we call them, such as PHI or PII. Therefore, for example, you can set the value of
-        // includeCausalError based on log level. For example, when log level is less than debug,
-        // don't nest the causal error inside this error.
-        if (this.isOperational || includeCausalError) {
+        // See comments at top of file about logging of causal errors.
+        if (_includeCausalError) {
             this.cause = causalError;
         }
 
-        // This class extends Error, so this.stack gets automatically generated; we don't always
-        // want a stack, so we have to do something with it.
+        // This class extends Error, so this.stack gets automatically generated; however we don't
+        // always want a stack, so we have to do something with it.
         // We don't want a stack trace when there is an operational error because we don't want to
-        // mislead future developer maintainers into thinking there's a bug, when there isn't.
+        // mislead future developer maintainers into thinking there's a bug when there isn't.
         // We only want this.stack when this class is being used for a "programmer error" OR if we
         // override this behavior and always have stacks.
-        if (!(!this.isOperational || alwaysIncludeErrorStacks || options.includeStack)) {
+        if (!(alwaysIncludeErrorStacks || options.includeStack || !this.isOperational)) {
             delete this.stack;
         }
     }
diff --git a/test/helpers/APIError.spec.js b/test/helpers/APIError.spec.js
index af174b8..1570ab9 100644
--- a/test/helpers/APIError.spec.js
+++ b/test/helpers/APIError.spec.js
@@ -11,28 +11,59 @@ const apiErrorMessage = 'APIError message';
 const errorCode = 'ERROR_CODE';
 
 describe('APIError:', () => {
-    describe('Default options.isPublic should be true...', () => {
-        it('When options argument is not specified', () => {
+    describe('isPublic:', () => {
+        it('should default to true...', () => {
             const errWithoutOptions = new APIError('a', 'b', 'c');
-            expect(errWithoutOptions.options).to.deep.equal({ isPublic: true });
-        });
+            expect(errWithoutOptions.options).to.deep.equal({});
+            expect(errWithoutOptions.isPublic).to.equal(true);
 
-        it('When options argument is specified, but no isPublic option specified', () => {
             const errWithOptionsButLackingIsPublic = new APIError('a', 'b', 'c', { optionA: 'A' });
-            expect(errWithOptionsButLackingIsPublic.options).to.deep.equal({ optionA: 'A', isPublic: true });
+            expect(errWithOptionsButLackingIsPublic.options).to.deep.equal({ optionA: 'A' });
+            expect(errWithOptionsButLackingIsPublic.isPublic).to.equal(true);
+        });
+
+        it('should be settable via options.isPublic', () => {
+            const errWithOptionsWithIsPublic = new APIError('a', 'b', 'c', { optionA: 'A', isPublic: false });
+            expect(errWithOptionsWithIsPublic.options).to.deep.equal({ optionA: 'A', isPublic: false });
+            expect(errWithOptionsWithIsPublic.isPublic).to.equal(false);
         });
     });
 
-    describe('Causal Error Handling:', () => {
+    describe('Causal Error Handling and isOperational:', () => {
+        it('When a causal error is passed in, default isOperational to false', () => {
+            const causalError = new Error(causalErrorMessage);
+            const err = new APIError(causalError, apiErrorMessage);
+
+            expect(err.isOperational).to.equal(false);
+        });
+
+        it('When a causal error is NOT passed in, default isOperational to true', () => {
+            const err = new APIError(apiErrorMessage);
+
+            expect(err.isOperational).to.equal(true);
+        });
+
+        it('`options.isOperational` overrides the default behavior', () => {
+            const causalError = new Error(causalErrorMessage);
+            const err1 = new APIError(causalError, apiErrorMessage, errorCode, httpStatus.NOT_FOUND, { isOperational: 'SOMETHING' });
+            const err2 = new APIError(apiErrorMessage, errorCode, httpStatus.NOT_FOUND, { isOperational: 'SOMETHING' });
+
+            expect(err1.isOperational).to.equal('SOMETHING');
+            expect(err1.isOperational).to.equal(err1.options.isOperational);
+            expect(err2.isOperational).to.equal('SOMETHING');
+            expect(err2.isOperational).to.equal(err2.options.isOperational);
+        });
+
+        // Note: includeCausalError is true because .env.test sets LOG_LEVEL=debug
         describe('When includeCausalError is truthy...', () => {
-            it('The causal error is set to myAPIError.cause', () => {
+            it('the causal error is set to myAPIError.cause', () => {
                 const causalError = new Error(causalErrorMessage);
                 const err = new APIError(causalError, apiErrorMessage);
 
                 expect(err.cause).to.deep.equal(causalError);
             });
 
-            it('"Concatonate" messages of APIError and the causal errror', () => {
+            it('"concatonate" messages of APIError and the causal errror', () => {
                 const causalError = new Error(causalErrorMessage);
                 const err = new APIError(causalError, apiErrorMessage);
 
@@ -40,23 +71,15 @@ describe('APIError:', () => {
             });
         });
 
-        // // TODO: This requires mocking `initLogLevelGte()` because APIError.js has
+        // TODO: Mock APIError.js's call to `initLogLevelGte()` to force initialize it to a level
+        // below 'debug' because .env.test has LOG_LEVEL=deug and APIError.js has:
+        // ```
         // const logLevelGte = initLogLevelGte(config.logLevel);
-        // describe('When includeCausalError is falsy', () => {
-        //     it('myAPIError.cause is undefined', () => {
-        //         const causalError = new Error(causalErrorMessage);
-        //         const err = new APIError(causalError, apiErrorMessage);
-
-        //         expect(err.cause).to.be.undefined; // eslint-disable-line no-unused-expressions
-        //     });
-
-        //     it('Don\'t "concatonate", messages of causal error and APIError', () => {
-        //         const causalError = new Error(causalErrorMessage);
-        //         const err = new APIError(causalError, apiErrorMessage);
-
-        //         expect(err.message).to.equal(apiErrorMessage);
-        //     });
-        // });
+        // const includeCausalError = logLevelGte('debug');
+        // ```
+        // Then, write tests for the following cases:
+        // 1. Cases for: describe('When includeCausalError is falsy', () => {
+        // 2. Cases related to options.includeCausalError
     });
 
     describe('Supports argument shifting based on first argument being a causal error, or not:', () => {
@@ -94,7 +117,7 @@ describe('APIError:', () => {
         it('Supports calling without causal error', () => {
             const err = new APIError(apiErrorMessage, errorCode, httpStatus.NOT_FOUND, { optionA: 'A', isPublic: false });
 
-            expect(err.cause).to.be.undefined; // eslint-disable-line no-unused-expressions
+            expect(err.cause).to.equal(undefined);
             expect(err.message).to.equal(apiErrorMessage);
             expect(err.code).to.equal(errorCode);
             expect(err.status).to.equal(httpStatus.NOT_FOUND);
@@ -125,7 +148,7 @@ describe('APIError:', () => {
         describe('There should not be a stack...', () => {
             it('In any other case', () => {
                 const err = new APIError('some string');
-                expect(err.stack).to.be.undefined; // eslint-disable-line no-unused-expressions
+                expect(err.stack).to.equal(undefined);
             });
         });
     });

From 23a7a57032e26fd8a4976e427a31f96fe34753d1 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Fri, 19 Apr 2019 10:11:42 -0400
Subject: [PATCH 4/8] [SER-279] log operational errors at warn level,
 programmer errors at error level.

---
 src/config/express.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/config/express.js b/src/config/express.js
index c458706..60522f5 100644
--- a/src/config/express.js
+++ b/src/config/express.js
@@ -94,7 +94,12 @@ app.use((req, res, next) => {
 // Log errors.
 if (config.env !== 'test') {
     app.use((err, req, res, next) => {
-        winstonInstance.warn(err);
+        if (err.isOperational) {
+            winstonInstance.warn(err);
+        } else {
+            winstonInstance.error(err);
+        }
+
         return next(err);
     });
 }

From 2b63485ab7a49b3740addd58adedcb6fce5090b0 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Mon, 22 Apr 2019 17:50:46 -0400
Subject: [PATCH 5/8] [SER-279] Add JSDoc to APIError, PR #90

---
 src/helpers/APIError.js | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/helpers/APIError.js b/src/helpers/APIError.js
index 172d50b..bfbfed1 100644
--- a/src/helpers/APIError.js
+++ b/src/helpers/APIError.js
@@ -1,7 +1,6 @@
 import { initLogLevelGte } from 'winston-json-formatter';
 import config from '../config/config';
 
-// TODO: Document this with the auto-documentation tool properly.
 // Read https://www.joyent.com/node-js/production/design/errors to understand the difference between
 // "programmer errors" (i.e. bugs) and "operational errors" (i.e. not actual bugs).
 // The APIError class assumes that if it was instantiated without a causal error, then it is being
@@ -19,6 +18,19 @@ const includeCausalError = logLevelGte('debug');
  * @extends Error
  */
 class APIError extends Error {
+    /* eslint-disable max-len */
+    /**
+     * Create an APIError.
+     * @param {Error | string} arg1 - Either JS Error (the causal error) (optional), or error message (required).
+     * @param {string} arg2 - Either error message (required) or error code (required).
+     * @param {string | Number} arg3 - Either error code (required) or HTTP status (required)
+     * @param {Number | Object} arg4 - Either HTTP status (required) or options object (optional)
+     * @param {Object} arg5 - Options object (optional)
+     * @returns APIError
+     * @example new APIError('Create user failed. Please provide a username.', 'CREATE_USER_FAILED', 400, { includeStack: true })
+     * @example if (err) { new APIError(err, 'Create user failed. User already exists.', 'CREATE_USER_FAILED', 409, { isPublic: false }) }
+     */
+    /* eslint-enable max-len */
     constructor(arg1, arg2, arg3, arg4, arg5) {
         if (!(typeof arg1 === 'string' || arg1 instanceof String || arg1 instanceof Error)) {
             // eslint-disable-next-line max-len

From 314cbae462c72006d5094ed7513920aa48e62394 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Mon, 22 Apr 2019 19:42:17 -0400
Subject: [PATCH 6/8] [SER-279] Bug fix: isPublic handling in express
 middleware. PR #90

---
 src/config/express.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/config/express.js b/src/config/express.js
index 60522f5..5194b50 100644
--- a/src/config/express.js
+++ b/src/config/express.js
@@ -107,9 +107,9 @@ if (config.env !== 'test') {
 // error handler, send stacktrace only during development
 app.use((err, req, res, next) => // eslint-disable-line no-unused-vars
     res.status(err.status || 500).json({
-        code: (err.options && err.isPublic) ? err.code : 'UNKNOWN_ERROR',
+        code: err.isPublic ? err.code : 'UNKNOWN_ERROR',
         status: 'ERROR',
-        message: (err.options && err.isPublic) ? err.message : httpStatus[err.status],
+        message: err.isPublic ? err.message : httpStatus[err.status],
         stack: config.env === 'development' ? err.stack : {},
     })
 );

From 0c663bc3a0672de1e988aafc8f82816047b805a2 Mon Sep 17 00:00:00 2001
From: Aaron Houlihan <aaron@amida.com>
Date: Mon, 22 Apr 2019 20:46:23 -0400
Subject: [PATCH 7/8] [SER-279] README and CHANGELOG updates. PR #90

---
 CHANGELOG.md | 5 ++++-
 README.md    | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 05f750e..4233708 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,10 @@
 - `auth.controller.js:login()` no longer throws duplicate errors for incorrect username or password
 - Use error handling middlware `winstonInstance.info(err)`
   * Previously, `winstonInstance.errorLogger()` always threw a `TypeError`, meaning the underlying error didn't actually show in the logs
-- APIError now supports all 10 features in https://jira.amida.com/browse/SER-279
+- [SER-279]
+  - APIError now supports all 10 features in https://jira.amida.com/browse/SER-279
+  - Updates `winston` to `^3.2.1` and `winston-json-formatter` to `^0.10.0` to get great new logging bug fixes and formatting features (see https://github.com/amida-tech/winston-json-formatter/pull/5).
+  - Logs "operational errors" at the `warn` level and "programmer errors" at the `error` level.
 
 ## [2.7.0] -- 2019-02-04
 ### Added
diff --git a/README.md b/README.md
index cf49b2a..d3a602c 100644
--- a/README.md
+++ b/README.md
@@ -337,8 +337,8 @@ A description of what the variable is or does.
 ##### `ALWAYS_INCLUDE_ERROR_STACKS` [`false`]
 
 The `APIError` class acts like a call-stackless "operational error" (https://www.joyent.com/node-js/production/design/errors) when called without a causal error as the first argument.
-- When false, `APIError`s without a cuasal error will not have call stacks.
-- When true, all `APIError`s will have call stacks.
+- When `false`, `APIError`s without a causal error will not have call stacks.
+- When `true`, all `APIError`s will have call stacks.
 
 ##### `JWT_SECRET` (Required)
 

From 32ca6e16ceacc75e2ebc900c6a717e4796aae04d Mon Sep 17 00:00:00 2001
From: Ryan M Harrison <rmharrison@users.noreply.github.com>
Date: Tue, 30 Apr 2019 00:58:06 -0400
Subject: [PATCH 8/8] Minor fixes for PR #90

- Fixed issues flagged by @orndorffgrant (thanks for the detailed/careful review)
- Minor CHANGELOG style fix
  * Unpacked knapsack of reference to [SER-279] _in CHANGELOG_, because JIRA issues aren't public to non-Amida users.
  * Amida users will still be able to see SER-279 reference for PR and link to commits.
---
 CHANGELOG.md                  | 11 +++++++----
 src/config/express.js         |  2 +-
 src/helpers/APIError.js       |  8 ++++----
 test/helpers/APIError.spec.js |  2 +-
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4233708..452e522 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,14 +1,17 @@
 # Changelog
 
 ## [Unreleased]
+### Added
+- ENV `ALWAYS_INCLUDE_ERROR_STACKS` to include full error stack, including any causal errors.
+  * Use with caution in production, because the full stack could leak sensitive information.
+
 ### Fixed
 - `auth.controller.js:login()` no longer throws duplicate errors for incorrect username or password
 - Use error handling middlware `winstonInstance.info(err)`
   * Previously, `winstonInstance.errorLogger()` always threw a `TypeError`, meaning the underlying error didn't actually show in the logs
-- [SER-279]
-  - APIError now supports all 10 features in https://jira.amida.com/browse/SER-279
-  - Updates `winston` to `^3.2.1` and `winston-json-formatter` to `^0.10.0` to get great new logging bug fixes and formatting features (see https://github.com/amida-tech/winston-json-formatter/pull/5).
-  - Logs "operational errors" at the `warn` level and "programmer errors" at the `error` level.
+- `class:APIError` supports full stack traces, including causal errors.
+- Update `npm:winston` to `^3.2.1` and `npm:winston-json-formatter` to `^0.10.0` for logging bug fixes and formatting features (see https://github.com/amida-tech/winston-json-formatter/pull/5).
+  * Logs "operational errors" at the `warn` level and "programmer errors" at the `error` level.
 
 ## [2.7.0] -- 2019-02-04
 ### Added
diff --git a/src/config/express.js b/src/config/express.js
index 5194b50..ebf1d56 100644
--- a/src/config/express.js
+++ b/src/config/express.js
@@ -87,7 +87,7 @@ app.use((err, req, res, next) => {
 
 // catch 404 and forward to error handler
 app.use((req, res, next) => {
-    const err = new APIError('API not found', 'UNKNOWN_API', httpStatus.NOT_FOUND, { isPublic: false });
+    const err = new APIError('Not Found', 'NOT_FOUND', httpStatus.NOT_FOUND, { isPublic: true });
     return next(err);
 });
 
diff --git a/src/helpers/APIError.js b/src/helpers/APIError.js
index bfbfed1..ef1e8c5 100644
--- a/src/helpers/APIError.js
+++ b/src/helpers/APIError.js
@@ -10,9 +10,9 @@ const alwaysIncludeErrorStacks = config.alwaysIncludeErrorStacks;
 
 // Errors thrown by 3rd party libraries might log data that we pass into them, such as PHI or PII.
 // And, devs might pass those errors as the causalError arg into APIError. So, to prevent accidental
-// logging of PHI/PII, only include and thus log causal errors if your config.logLevel is >= debug.
-const logLevelGte = initLogLevelGte(config.logLevel);
-const includeCausalError = logLevelGte('debug');
+// logging of PHI/PII, only include and thus log causal errors if your config.logLevel is >= debug,
+// i.e. silly or debug.
+const includeCausalError = initLogLevelGte(config.logLevel)('debug');
 
 /**
  * @extends Error
@@ -28,7 +28,7 @@ class APIError extends Error {
      * @param {Object} arg5 - Options object (optional)
      * @returns APIError
      * @example new APIError('Create user failed. Please provide a username.', 'CREATE_USER_FAILED', 400, { includeStack: true })
-     * @example if (err) { new APIError(err, 'Create user failed. User already exists.', 'CREATE_USER_FAILED', 409, { isPublic: false }) }
+     * @example if (err) { throw new APIError(err, 'Create user failed. User already exists.', 'CREATE_USER_FAILED', 409, { isPublic: false }) }
      */
     /* eslint-enable max-len */
     constructor(arg1, arg2, arg3, arg4, arg5) {
diff --git a/test/helpers/APIError.spec.js b/test/helpers/APIError.spec.js
index 1570ab9..e1648e9 100644
--- a/test/helpers/APIError.spec.js
+++ b/test/helpers/APIError.spec.js
@@ -63,7 +63,7 @@ describe('APIError:', () => {
                 expect(err.cause).to.deep.equal(causalError);
             });
 
-            it('"concatonate" messages of APIError and the causal errror', () => {
+            it('Concatenate messages of APIError and the causal errror', () => {
                 const causalError = new Error(causalErrorMessage);
                 const err = new APIError(causalError, apiErrorMessage);