refactor: FORMS-1488 new middleware for reminder route (bcgov#1482)

* fix: FORMS-1448 new middleware for reminder route

* fix test comments

* fixed a comment that could cause confusion

app/src/forms/public/middleware/apiAccess.js

@@ -0,0 +1,35 @@
+const Problem = require('api-problem');
+
+/**
+ * Checks that the API Key in the request headers matches the API Key in the
+ * process environment variables.
+ *
+ * @param {*} req the Express object representing the HTTP request.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ */
+const checkApiKey = async (req, _res, next) => {
+  try {
+    const requestApikey = req.headers.apikey;
+    if (requestApikey === undefined || requestApikey === '') {
+      throw new Problem(401, {
+        detail: 'No API key provided',
+      });
+    }
+
+    const systemApikey = process.env.APITOKEN;
+    if (requestApikey !== systemApikey) {
+      throw new Problem(401, {
+        detail: 'Invalid API key',
+      });
+    }
+
+    next();
+  } catch (error) {
+    next(error);
+  }
+};
+
+module.exports = {
+  checkApiKey,
+};

app/src/forms/public/routes.js

@@ -1,28 +1,9 @@
 const routes = require('express').Router();
 
+const apiAccess = require('./middleware/apiAccess');
 const controller = require('./controller');
 
-routes.use('/reminder', (req, res, next) => {
-  // eslint-disable-next-line no-empty
-  try {
-    if (req.method == 'GET') {
-      const apikeyEnv = process.env.APITOKEN;
-      const apikeyIncome = req.headers.apikey;
-      if (apikeyEnv == apikeyIncome && (apikeyIncome == undefined || apikeyIncome == '')) return res.status(401).json({ message: 'No API key provided' });
-      if (apikeyIncome === apikeyEnv) {
-        next();
-      } else {
-        return res.status(401).json({ message: 'Invalid API key' });
-      }
-    } else {
-      return res.status(404).json({ message: 'Only GET request is accepted' });
-    }
-  } catch (err) {
-    return res.status(500).json({ message: err.message });
-  }
-});
-
-routes.get('/reminder', async (req, res, next) => {
+routes.get('/reminder', apiAccess.checkApiKey, async (req, res, next) => {
   await controller.sendReminderToSubmitter(req, res, next);
 });
 

app/tests/unit/forms/public/middleware/apiAccess.spec.js

@@ -0,0 +1,66 @@
+const { getMockReq, getMockRes } = require('@jest-mock/express');
+const uuid = require('uuid');
+
+const apiAccess = require('../../../../../src/forms/public/middleware/apiAccess');
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
+
+describe('checkApiKey', () => {
+  process.env.APITOKEN = uuid.v4();
+
+  describe('401 response when', () => {
+    const expectedStatus = { status: 401 };
+
+    test('there is no APITOKEN or apikey', async () => {
+      const req = getMockReq();
+      const { res, next } = getMockRes();
+
+      await apiAccess.checkApiKey(req, res, next);
+
+      expect(next).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test('the apikey is empty', async () => {
+      const req = getMockReq({
+        headers: { apikey: '' },
+      });
+      const { res, next } = getMockRes();
+
+      await apiAccess.checkApiKey(req, res, next);
+
+      expect(next).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test('the apikey exists but does not match', async () => {
+      const req = getMockReq({
+        headers: { apikey: uuid.v4() },
+      });
+      const { res, next } = getMockRes();
+
+      await apiAccess.checkApiKey(req, res, next);
+
+      expect(next).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+  });
+
+  describe('allows', () => {
+    test('matching APITOKEN and apikey', async () => {
+      const req = getMockReq({
+        headers: {
+          apikey: process.env.APITOKEN,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await apiAccess.checkApiKey(req, res, next);
+
+      expect(next).toBeCalledTimes(1);
+      expect(next).toBeCalledWith();
+    });
+  });
+});

app/tests/unit/forms/public/routes.spec.js

@@ -4,6 +4,16 @@ const uuid = require('uuid');
 const { expressHelper } = require('../../../common/helper');
 
 const controller = require('../../../../src/forms/public/controller');
+const apiAccess = require('../../../../src/forms/public/middleware/apiAccess');
+
+//
+// Mock out all the middleware - we're testing that the routes are set up
+// correctly, not the functionality of the middleware.
+//
+
+apiAccess.checkApiKey = jest.fn((_req, _res, next) => {
+  next();
+});
 
 //
 // Create the router and a simple Express server.
@@ -18,10 +28,6 @@ afterEach(() => {
   jest.clearAllMocks();
 });
 
-// TODO - move code out of the route and into middleware or the controller. The
-// tests should continue to pass, but should themselves be moved into the
-// middleware or controller tests.
-
 describe(`${basePath}/reminder`, () => {
   const path = `${basePath}/reminder`;
 
@@ -32,46 +38,9 @@ describe(`${basePath}/reminder`, () => {
   it('200s when the APITOKEN matches apiKey', async () => {
     process.env.APITOKEN = uuid.v4();
 
-    const response = await appRequest.get(path).set({ apikey: process.env.APITOKEN });
+    await appRequest.get(path).set({ apikey: process.env.APITOKEN });
 
+    expect(apiAccess.checkApiKey).toBeCalledTimes(1);
     expect(controller.sendReminderToSubmitter).toBeCalledTimes(1);
-    expect(response.status).toBe(200);
-  });
-
-  // This is testing some strange code. Refactor the code and get rid of this.
-  it('401s when there is no APITOKEN or apiKey', async () => {
-    process.env.APITOKEN = '';
-
-    const response = await appRequest.get(path).set({ apikey: process.env.APITOKEN });
-
-    expect(controller.sendReminderToSubmitter).toBeCalledTimes(0);
-    expect(response.status).toBe(401);
-  });
-
-  it('401s when the apiKey is empty', async () => {
-    process.env.APITOKEN = uuid.v4();
-
-    const response = await appRequest.get(path).set({ apikey: '' });
-
-    expect(controller.sendReminderToSubmitter).toBeCalledTimes(0);
-    expect(response.status).toBe(401);
-  });
-
-  it('401s when the apiKey exists but does not match', async () => {
-    process.env.APITOKEN = uuid.v4();
-
-    const response = await appRequest.get(path).set({ apikey: uuid.v4() });
-
-    expect(controller.sendReminderToSubmitter).toBeCalledTimes(0);
-    expect(response.status).toBe(401);
-  });
-
-  // Ideally the code should be refactored to not create the 404 and let Express
-  // do it. Then this test should still pass.
-  it('404s on a non-GET', async () => {
-    const response = await appRequest.post(path);
-
-    expect(controller.sendReminderToSubmitter).toBeCalledTimes(0);
-    expect(response.status).toBe(404);
   });
 });