Refactor Problem handling to throw instead of sending

In order to standardize how we deal with 400 and 500 class HTTP responses,
we now always throw a Problem error instead of short circuiting with the
send method. Instead, we now depend on the top level app error handler to
capture and render the Problem. This ensures that we only have one pattern
for problem emission. Other minor various unit tests are also updated.

Signed-off-by: Jeremy Ho <jujaga@gmail.com>

app/app.js

@@ -71,11 +71,11 @@ if (config.has('server.privacyMask')) {
 }
 
 // Block requests until service is ready
-app.use((_req, res, next) => {
+app.use((_req, _res, next) => {
   if (state.shutdown) {
-    new Problem(503, { detail: 'Server is shutting down' }).send(res);
+    throw new Problem(503, { detail: 'Server is shutting down' });
   } else if (!state.ready) {
-    new Problem(503, { detail: 'Server is not ready' }).send(res);
+    throw new Problem(503, { detail: 'Server is not ready' });
   } else {
     next();
   }
@@ -107,24 +107,21 @@ apiRouter.use('/v1', v1Router);
 // Root level Router
 app.use(/(\/api)?/, apiRouter);
 
-// Handle ValidationError & 500
-// eslint-disable-next-line no-unused-vars
-app.use((err, _req, res, _next) => {
+// Handle 404
+app.use((req, _res) => { // eslint-disable-line no-unused-vars
+  throw new Problem(404, { instance: req.originalUrl });
+});
+
+// Handle Problem Responses
+app.use((err, req, res, _next) => { // eslint-disable-line no-unused-vars
   if (err instanceof Problem) {
     err.send(res);
   } else {
-    // Only log unexpected errors
-    if (err.stack) log.error(err);
-
-    new Problem(500, { detail: err.message ?? err }).send(res);
+    if (err.stack) log.error(err); // Only log unexpected errors
+    new Problem(500, { detail: err.message ?? err, instance: req.originalUrl }).send(res);
   }
 });
 
-// Handle 404
-app.use((req, res) => {
-  new Problem(404, { instance: req.originalUrl }).send(res);
-});
-
 // Ensure unhandled errors gracefully shut down the application
 process.on('unhandledRejection', err => {
   log.error(`Unhandled Rejection: ${err.message ?? err}`, { function: 'onUnhandledRejection' });

app/src/controllers/bucket.js

@@ -102,6 +102,7 @@ const controller = {
    * @param {object} res Express response object
    * @param {function} next The next callback function
    * @returns {function} Express middleware function
+   * @throws The error encountered upon failure
    */
   async createBucket(req, res, next) {
     const data = {
@@ -122,7 +123,7 @@ const controller = {
       if (e instanceof UniqueViolationError) {
         // Grant all permissions if credentials precisely match
         response = await bucketService.checkGrantPermissions(data).catch(permErr => {
-          throw new Problem(403, { detail: permErr.message }).send(res);
+          throw new Problem(403, { detail: permErr.message, instance: req.originalUrl });
         });
       } else {
         next(errorToProblem(SERVICE, e));

app/src/controllers/object.js

@@ -164,6 +164,7 @@ const controller = {
    * @param {object} res Express response object
    * @param {function} next The next callback function
    * @returns {function} Express middleware function
+   * @throws The error encountered upon failure
    */
   async addTags(req, res, next) {
     try {
@@ -190,7 +191,10 @@ const controller = {
 
       if (newSet.length > 10) {
         // 409 when total tag limit exceeded
-        return new Problem(409, { detail: 'Request exceeds maximum of 9 user-defined tag sets allowed' }).send(res);
+        throw new Problem(409, {
+          detail: 'Request exceeds maximum of 9 user-defined tag sets allowed',
+          instance: req.originalUrl
+        });
       }
 
       const data = {
@@ -233,7 +237,10 @@ const controller = {
       if (bucketId && userId) {
         const permission = await bucketPermissionService.searchPermissions({ userId: userId, bucketId: bucketId, permCode: 'CREATE' });
         if (!permission.length) {
-          throw new Problem(403, { detail: 'User lacks permission to complete this action' });
+          throw new Problem(403, {
+            detail: 'User lacks permission to complete this action',
+            instance: req.originalUrl
+          });
         }
       }
 
@@ -263,18 +270,27 @@ const controller = {
         });
 
         // Hard short circuit skip file as the object already exists on bucket
-        throw new Problem(409, { detail: 'Bucket already contains object' });
+        throw new Problem(409, {
+          detail: 'Bucket already contains object',
+          instance: req.originalUrl
+        });
       } catch (err) {
         if (err instanceof Problem) throw err; // Rethrow Problem type errors
 
         // Object is soft deleted from the bucket
         if (err.$response?.headers['x-amz-delete-marker']) {
-          throw new Problem(409, { detail: 'Bucket already contains object' });
+          throw new Problem(409, {
+            detail: 'Bucket already contains object',
+            instance: req.originalUrl
+          });
         }
 
         // Skip upload in the unlikely event we get an unexpected error from headObject
         if (err.$metadata?.httpStatusCode !== 404) {
-          throw new Problem(502, { detail: 'Bucket communication error' });
+          throw new Problem(502, {
+            detail: 'Bucket communication error',
+            instance: req.originalUrl
+          });
         }
 
         // Object does not exist on bucket
@@ -293,7 +309,10 @@ const controller = {
           });
           s3Response = await storageService.upload({ ...data, stream: req });
         } else {
-          throw new Problem(413, { detail: 'File exceeds maximum 50GB limit' });
+          throw new Problem(413, {
+            detail: 'File exceeds maximum 50GB limit',
+            instance: req.originalUrl
+          });
         }
       }
 
@@ -932,7 +951,10 @@ const controller = {
 
         // Skip upload in the unlikely event we get an unexpected response from headObject
         if (headResponse.$metadata?.httpStatusCode !== 200) {
-          throw new Problem(502, { detail: 'Bucket communication error' });
+          throw new Problem(502, {
+            detail: 'Bucket communication error',
+            instance: req.originalUrl
+          });
         }
 
         // Object exists on bucket
@@ -951,20 +973,32 @@ const controller = {
           });
           s3Response = await storageService.upload({ ...data, stream: req });
         } else {
-          throw new Problem(413, { detail: 'File exceeds maximum 50GB limit' });
+          throw new Problem(413, {
+            detail: 'File exceeds maximum 50GB limit',
+            instance: req.originalUrl
+          });
         }
       } catch (err) {
         if (err instanceof Problem) throw err; // Rethrow Problem type errors
         else if (err.$metadata?.httpStatusCode !== 404) {
           // An unexpected response from headObject
-          throw new Problem(502, { detail: 'Bucket communication error' });
+          throw new Problem(502, {
+            detail: 'Bucket communication error',
+            instance: req.originalUrl
+          });
         } else {
           if (err.$response?.headers['x-amz-delete-marker']) {
             // Object is soft deleted from the bucket
-            throw new Problem(409, { detail: 'Unable to update soft deleted object' });
+            throw new Problem(409, {
+              detail: 'Unable to update soft deleted object',
+              instance: req.originalUrl
+            });
           } else {
             // Bucket is missing the existing object
-            throw new Problem(409, { detail: 'Bucket does not contain existing object' });
+            throw new Problem(409, {
+              detail: 'Bucket does not contain existing object',
+              instance: req.originalUrl
+            });
           }
           // TODO: Add in sync operation to update object record in COMS DB?
         }

app/src/middleware/authentication.js

@@ -44,6 +44,7 @@ const _spkiWrapper = (spki) => `-----BEGIN PUBLIC KEY-----\n${spki}\n-----END PU
  * @param {object} res Express response object
  * @param {function} next The next callback function
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const currentUser = async (req, res, next) => {
   const authorization = req.get('Authorization');
@@ -84,7 +85,7 @@ const currentUser = async (req, res, next) => {
           throw new Error('Invalid authorization token');
         }
       } catch (err) {
-        return new Problem(403, { detail: err.message }).send(res);
+        throw new Problem(403, { detail: err.message, instance: req.originalUrl });
       }
     }
   }

app/src/middleware/authorization.js

@@ -49,11 +49,12 @@ const _checkPermission = async ({ currentObject, currentUser, params }, permissi
  * @function checkAppMode
  * Rejects the request if the incoming authentication mode does not match the application mode
  * @param {object} req Express request object
- * @param {object} res Express response object
+ * @param {object} _res Express response object
  * @param {function} next The next callback function
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
-const checkAppMode = (req, res, next) => {
+const checkAppMode = (req, _res, next) => {
   const authMode = getAppAuthMode();
   const authType = req.currentUser ? req.currentUser.authType : undefined;
 
@@ -65,11 +66,12 @@ const checkAppMode = (req, res, next) => {
     }
   } catch (err) {
     log.verbose(err.message, { function: 'checkAppMode', authMode: authMode, authType: authType });
-    return new Problem(501, {
+    throw new Problem(501, {
       detail: 'Current application mode does not support incoming authentication type',
+      instance: req.originalUrl,
       authMode: authMode,
       authType: authType
-    }).send(res);
+    });
   }
 
   next();
@@ -107,9 +109,10 @@ const currentObject = async (req, _res, next) => {
  * - if passed permission exists for current user on object or bucket (see: _checkPermission)
  * @param {string} permission a permission code (eg: READ)
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const hasPermission = (permission) => {
-  return async (req, res, next) => {
+  return async (req, _res, next) => {
     const authMode = getAppAuthMode();
     const authType = req.currentUser ? req.currentUser.authType : undefined;
 
@@ -133,7 +136,7 @@ const hasPermission = (permission) => {
       }
     } catch (err) {
       log.verbose(err.message, { function: 'hasPermission' });
-      return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res);
+      throw new Problem(403, { detail: 'User lacks permission to complete this action', instance: req.originalUrl });
     }
 
     next();

app/src/middleware/featureToggle.js

@@ -10,6 +10,7 @@ const { getAppAuthMode } = require('../components/utils');
  * @param {object} res Express response object
  * @param {function} next The next callback function
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const requireBasicAuth = (req, res, next) => {
   const authMode = getAppAuthMode();
@@ -18,11 +19,17 @@ const requireBasicAuth = (req, res, next) => {
   const canBasicMode = (mode) => [AuthMode.BASICAUTH, AuthMode.FULLAUTH].includes(mode);
 
   if (authMode === AuthMode.OIDCAUTH) {
-    return new Problem(501, { detail: 'This action is not supported in the current authentication mode' }).send(res);
+    throw new Problem(501, {
+      detail: 'This action is not supported in the current authentication mode',
+      instance: req.originalUrl
+    });
   }
 
   if (canBasicMode(authMode) && authType !== AuthType.BASIC) {
-    return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res);
+    throw new Problem(403, {
+      detail: 'User lacks permission to complete this action',
+      instance: req.originalUrl
+    });
   }
 
   next();
@@ -35,13 +42,17 @@ const requireBasicAuth = (req, res, next) => {
  * @param {object} res Express response object
  * @param {function} next The next callback function
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const requireSomeAuth = (req, res, next) => {
   const authMode = getAppAuthMode();
   const authType = req.currentUser ? req.currentUser.authType : undefined;
 
   if (authMode !== AuthMode.NOAUTH && (!authType || authType === AuthType.NONE)) {
-    return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res);
+    throw new Problem(403, {
+      detail: 'User lacks permission to complete this action',
+      instance: req.originalUrl
+    });
   }
 
   next();

app/src/middleware/upload.js

@@ -8,13 +8,17 @@ const contentDisposition = require('content-disposition');
  * @param {object} _res Express response object
  * @param {function} next The next callback function
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const currentUpload = (strict = false) => {
-  return (req, res, next) => {
+  return (req, _res, next) => {
     // Check Content-Length Header
     const contentLength = parseInt(req.get('Content-Length'));
     // TODO: Figure out what's killing and returning a 400 in response stack
-    if (!contentLength) return new Problem(411, { detail: 'Content-Length must be greater than 0' }).send(res);
+    if (!contentLength) throw new Problem(411, {
+      detail: 'Content-Length must be greater than 0',
+      instance: req.originalUrl
+    });
 
     // Check Content-Disposition Header
     let filename;
@@ -27,10 +31,16 @@ const currentUpload = (strict = false) => {
         filename = parameters?.filename;
       } catch (e) {
         // Ignore improperly formatted Content-Disposition when not in strict mode
-        if (strict) return new Problem(400, { detail: `Content-Disposition header error: ${e.message}` }).send(res);
+        if (strict) throw new Problem(400, {
+          detail: `Content-Disposition header error: ${e.message}`,
+          instance: req.originalUrl
+        });
       }
     } else {
-      if (strict) return new Problem(415, { detail: 'Content-Disposition header missing' }).send(res);
+      if (strict) throw new Problem(415, {
+        detail: 'Content-Disposition header missing',
+        instance: req.originalUrl
+      });
     }
 
     // Check Content-Type Header

app/src/middleware/validation.js

@@ -5,9 +5,10 @@ const Problem = require('api-problem');
  * Performs express request validation against a specified `schema`
  * @param {object} schema An object containing Joi validation schema definitions
  * @returns {function} Express middleware function
+ * @throws The error encountered upon failure
  */
 const validate = (schema) => {
-  return (req, res, next) => {
+  return (req, _res, next) => {
     const validationErrors = Object.entries(schema)
       .map(([prop, def]) => {
         const result = def.validate(req[prop], { abortEarly: false })?.error;
@@ -16,13 +17,13 @@ const validate = (schema) => {
       .filter(error => !!error);
 
     if (Object.keys(validationErrors).length) {
-      return new Problem(422, {
+      throw new Problem(422, {
         detail: validationErrors
           .flatMap(groups => groups[1]?.map(error => error?.message))
           .join('; '),
         instance: req.originalUrl,
         errors: Object.fromEntries(validationErrors)
-      }).send(res);
+      });
     }
     else next();
   };

app/src/validators/object.js

@@ -27,8 +27,6 @@ const schema = {
     }).nand('s3VersionId', 'versionId')
   },
 
-  // TODO: Make this schema definition unit-testable
-  // bucketId property was undefined in unit test
   createObject: {
     headers: type.metadata(),
     query: Joi.object({