Merge branch 'main' into fix/forms_1410_ext-api-no-ministry

usingtechnology · Aug 6, 2024 · e26167d · e26167d
2 parents 25b6c19 + 9e08f10
commit e26167d
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 38 deletions.
diff --git a/app/src/forms/auth/middleware/userAccess.js b/app/src/forms/auth/middleware/userAccess.js
@@ -356,15 +356,9 @@ const hasRoleModifyPermissions = async (req, _res, next) => {
 
       if (userRoles.includes(Roles.OWNER)) {
         // Can't remove a different user's owner role unless you are an owner.
-        //
-        // TODO: Remove this if statement and just throw the exception. It's not
-        // possible for userId === currentUser.id since we're in an if that we
-        // are !isOwner but also that userRoles.includes(Roles.OWNER).
-        if (userId !== currentUser.id) {
-          throw new Problem(401, {
-            detail: "You can't update an owner's roles.",
-          });
-        }
+        throw new Problem(401, {
+          detail: "You can't update an owner's roles.",
+        });
       } else if (futureRoles.includes(Roles.OWNER)) {
         // Can't add an owner role unless you are an owner.
         throw new Problem(401, {

diff --git a/app/src/forms/common/middleware/validateParameter.js b/app/src/forms/common/middleware/validateParameter.js
@@ -1,9 +1,9 @@
 const Problem = require('api-problem');
 const uuid = require('uuid');
 
+const externalApiService = require('../../form/externalApi/service');
 const formService = require('../../form/service');
 const submissionService = require('../../submission/service');
-const { ExternalAPI } = require('../models');
 
 /**
  * Throws a 400 problem if the parameter is not a valid UUID.
@@ -60,6 +60,32 @@ const validateDocumentTemplateId = async (req, _res, next, documentTemplateId) =
   }
 };
 
+/**
+ * Validates that the :externalApiId route parameter exists and is a UUID. This
+ * validator requires that the :formId route parameter also exists.
+ *
+ * @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.
+ * @param {*} externalAPIId the :externalAPIId value from the route.
+ */
+const validateExternalAPIId = async (req, _res, next, externalAPIId) => {
+  try {
+    _validateUuid(externalAPIId, 'externalAPIId');
+
+    const externalApi = await externalApiService.readExternalAPI(externalAPIId);
+    if (!externalApi || externalApi.formId !== req.params.formId) {
+      throw new Problem(404, {
+        detail: 'externalAPIId does not exist on this form',
+      });
+    }
+
+    next();
+  } catch (error) {
+    next(error);
+  }
+};
+
 /**
  * Validates that the :fileId route parameter exists and is a UUID.
  *
@@ -148,37 +174,11 @@ const validateFormVersionId = async (req, _res, next, formVersionId) => {
   }
 };
 
-/**
- * Validates that the :externalApiId route parameter exists and is a UUID. This
- * validator requires that the :formId route parameter also exists.
- *
- * @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.
- * @param {*} externalAPIId the :externalAPIId value from the route.
- */
-const validateExternalAPIId = async (req, _res, next, externalAPIId) => {
-  try {
-    _validateUuid(externalAPIId, 'externalAPIId');
-
-    const externalApi = await ExternalAPI.query().findById(externalAPIId);
-    if (!externalApi || externalApi.formId !== req.params.formId) {
-      throw new Problem(404, {
-        detail: 'externalAPIId does not exist on this form',
-      });
-    }
-
-    next();
-  } catch (error) {
-    next(error);
-  }
-};
-
 module.exports = {
   validateDocumentTemplateId,
+  validateExternalAPIId,
   validateFileId,
   validateFormId,
-  validateFormVersionId,
   validateFormVersionDraftId,
-  validateExternalAPIId,
+  validateFormVersionId,
 };
diff --git a/app/src/forms/form/externalApi/service.js b/app/src/forms/form/externalApi/service.js
@@ -99,6 +99,10 @@ const service = {
     await ExternalAPI.query().modify('findByIdAndFormId', externalAPIId, formId).first().throwIfNotFound();
     await ExternalAPI.query().deleteById(externalAPIId);
   },
+
+  readExternalAPI: (externalAPIId) => {
+    return ExternalAPI.query().findById(externalAPIId);
+  },
 };
 
 module.exports = service;
diff --git a/app/tests/unit/forms/common/middleware/validateParameter.spec.js b/app/tests/unit/forms/common/middleware/validateParameter.spec.js
@@ -2,6 +2,7 @@ const { getMockReq, getMockRes } = require('@jest-mock/express');
 const { v4: uuidv4 } = require('uuid');
 
 const validateParameter = require('../../../../../src/forms/common/middleware/validateParameter');
+const externalApiService = require('../../../../../src/forms/form/externalApi/service');
 const formService = require('../../../../../src/forms/form/service');
 const submissionService = require('../../../../../src/forms/submission/service');
 
@@ -198,6 +199,123 @@ describe('validateDocumentTemplateId', () => {
   });
 });
 
+describe('validateExternalApiId', () => {
+  const externalApiId = uuidv4();
+
+  const mockReadExternalApiResponse = {
+    formId: formId,
+    id: externalApiId,
+  };
+
+  externalApiService.readExternalAPI = jest.fn().mockReturnValue(mockReadExternalApiResponse);
+
+  describe('400 response when', () => {
+    const expectedStatus = { status: 400 };
+
+    test('externalAPIId is missing', async () => {
+      const req = getMockReq({
+        params: {
+          formId: formId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(0);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test.each(invalidUuids)('externalAPIId is "%s"', async (eachExternalApiId) => {
+      const req = getMockReq({
+        params: {
+          formId: formId,
+          externalAPIId: eachExternalApiId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next, eachExternalApiId);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(0);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+  });
+
+  describe('404 response when', () => {
+    const expectedStatus = { status: 404 };
+
+    test('formId is missing', async () => {
+      const req = getMockReq({
+        params: {
+          externalAPIId: externalApiId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next, externalApiId);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test('formId does not match', async () => {
+      externalApiService.readExternalAPI.mockReturnValueOnce({
+        formId: uuidv4(),
+        id: externalApiId,
+      });
+      const req = getMockReq({
+        params: {
+          externalAPIId: externalApiId,
+          formId: formId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next, externalApiId);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+  });
+
+  describe('handles error thrown by', () => {
+    test('readVersion', async () => {
+      const error = new Error();
+      externalApiService.readExternalAPI.mockRejectedValueOnce(error);
+      const req = getMockReq({
+        params: {
+          externalAPIId: externalApiId,
+          formId: formId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next, externalApiId);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(1);
+      expect(next).toBeCalledWith(error);
+    });
+  });
+
+  describe('allows', () => {
+    test('external api with matching form id', async () => {
+      const req = getMockReq({
+        params: {
+          externalAPIId: externalApiId,
+          formId: formId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateExternalAPIId(req, res, next, externalApiId);
+
+      expect(externalApiService.readExternalAPI).toBeCalledTimes(1);
+      expect(next).toBeCalledWith();
+    });
+  });
+});
+
 describe('validateFileId', () => {
   describe('400 response when', () => {
     const expectedStatus = { status: 400 };