Merge branch 'main' into feat/forms-1331-event-stream

app/package.json

@@ -51,8 +51,8 @@
     "@json2csv/transforms": "^6.1.3",
     "api-problem": "^9.0.2",
     "aws-sdk": "^2.1376.0",
-    "axios": "^0.28.1",
-    "axios-oauth-client": "^1.5.0",
+    "axios": "^1.7.4",
+    "axios-oauth-client": "^2.2.0",
     "axios-token-interceptor": "^0.2.0",
     "bytes": "^3.1.2",
     "compression": "^1.7.4",

app/src/components/clientConnection.js

@@ -4,6 +4,22 @@ const tokenProvider = require('axios-token-interceptor');
 
 const log = require('./log')(module.filename);
 
+// axios-oauth-client removed the "interceptor" in v2.2.0. Replicate it here.
+
+const getMaxAge = (res) => {
+  return res.expires_in * 1e3;
+};
+
+const headerFormatter = (res) => {
+  return 'Bearer ' + res.access_token;
+};
+
+const interceptor = (tokenProvider, authenticate) => {
+  const getToken = tokenProvider.tokenCache(authenticate, { getMaxAge });
+
+  return tokenProvider({ getToken, headerFormatter });
+};
+
 class ClientConnection {
   constructor({ tokenUrl, clientId, clientSecret }) {
     log.verbose(`Constructed with ${tokenUrl}, ${clientId}, clientSecret`, { function: 'constructor' });
@@ -19,15 +35,7 @@ class ClientConnection {
       // Wraps axios-token-interceptor with oauth-specific configuration,
       // fetches the token using the desired claim method, and caches
       // until the token expires
-      oauth.interceptor(
-        tokenProvider,
-        oauth.client(axios.create(), {
-          url: this.tokenUrl,
-          grant_type: 'client_credentials',
-          client_id: clientId,
-          client_secret: clientSecret,
-        })
-      )
+      interceptor(tokenProvider, oauth.clientCredentials(axios.create(), tokenUrl, clientId, clientSecret))
     );
   }
 }

app/tests/unit/README.md

@@ -53,3 +53,36 @@ describe('my tests', () => {
 ```
 
 Similar to `.only` is the `.skip` modifier to skip a test or group of tests.
+
+## Testing Strategy
+
+The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests we should:
+
+- ensure that the tests are consistent
+- ensure that we have 100% test coverage
+- ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage
+- test the interface, not the implementation
+- test the unit under test, not its dependencies
+
+### Middleware Testing
+
+The tests for the middleware files should:
+
+- mock all services calls used by the middleware, including both exception and minimal valid results
+- test all response codes produced by the middleware
+
+### Route Testing
+
+The tests for the `route.js` files should:
+
+- mock all middleware used by the file
+- each route test should check that every middleware is called the proper number of times
+- each route test should mock the controller function that it calls
+- mock controller functions with `res.sendStatus(200)`, as doing something like `next()` will call multiple controller functions when route paths are overloaded
+- check that the mocked controller function is called - this will catch when a new route path accidentally overloads an old one
+- for consistency and ease of comparison, alphabetize the expect clauses ("alphabetize when possible")
+
+Note:
+
+- Some middleware is called when the `routes.js` is loaded, not when the route is called. For example, the parameters to `userAccess.hasFormPermissions` cannot be tested by calling the route using it. Even if we reload the `routes.js` for each route test, it would be hard to tell which call to `hasFormPermissions` was the call for the route under test
+- Maybe we should refactor and create a set of standard middleware mocks that live outside the `routes.spec.js` files

app/tests/unit/forms/form/externalApi/routes.spec.js

@@ -83,6 +83,7 @@ describe(`${basePath}/:formId/externalAPIs`, () => {
     expect(controller.listExternalAPIs).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -98,6 +99,7 @@ describe(`${basePath}/:formId/externalAPIs`, () => {
     expect(controller.createExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -125,6 +127,7 @@ describe(`${basePath}/:formId/externalAPIs/:externalAPIId`, () => {
     expect(controller.deleteExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(1);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -152,6 +155,7 @@ describe(`${basePath}/:formId/externalAPIs/:externalAPIId`, () => {
     expect(controller.updateExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(1);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -219,6 +223,7 @@ describe(`${basePath}/:formId/externalAPIs/statusCodes`, () => {
     expect(controller.listExternalAPIStatusCodes).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });