Ntrnl 295 add validation mw

package.json

@@ -44,6 +44,7 @@
     "cors": "^2.8.5",
     "crypto": "^1.0.1",
     "express": "^4.18.2",
+    "express-validator": "^7.0.1",
     "govuk-frontend": "^4.7.0",
     "helmet": "^7.0.0",
     "nunjucks": "^3.2.4"

src/middleware/validation.middleware.ts

@@ -0,0 +1,34 @@
+import { NextFunction, Request, Response } from 'express';
+import { validationResult, FieldValidationError } from 'express-validator';
+
+import { log } from '../utils/logger';
+import { FormattedValidationErrors } from '../model/validation.model';
+
+export const checkValidations = (req: Request, res: Response, next: NextFunction) => {
+    try {
+        const errorList = validationResult(req);
+
+        if (!errorList.isEmpty()) {
+            const template_path = req.path.substring(1);
+            const errors = formatValidationError(errorList.array() as FieldValidationError[]);
+
+            log.info(`Validation error on ${template_path} page`);
+
+            return res.render(template_path, { ...req.body, errors });
+        }
+
+        return next();
+    } catch (err: any) {
+        log.error(err.message);
+        next(err);
+    }
+};
+
+const formatValidationError = (errorList: FieldValidationError[]) => {
+    const errors = { 'errorList': [] } as FormattedValidationErrors;
+    errorList.forEach( (e: FieldValidationError) => {
+        errors.errorList.push({ href: `#${e.path}`, text: e.msg });
+        errors[e.path] = { text: e.msg };
+    });
+    return errors;
+};

src/model/validation.model.ts

@@ -0,0 +1,9 @@
+import { ErrorMessages } from '../validation/error.messages';
+
+export interface FormattedValidationErrors {
+    [key: string]: any,
+    errorList: {
+        href: string,
+        text: ErrorMessages
+    }[]
+}

src/routes/remove-member.ts

@@ -1,13 +1,15 @@
 import { Router } from 'express';
 
 import { authentication } from '../middleware/authentication.middleware';
-
+import { checkValidations } from '../middleware/validation.middleware';
+import { removeMember } from '../validation/remove-member.validation';
 import { get, post } from '../controller/remove-member.controller';
+
 import * as config from '../config';
 
 const removeMemberRouter = Router();
 
 removeMemberRouter.get(config.REMOVE_MEMBER_URL, authentication, get);
-removeMemberRouter.post(config.REMOVE_MEMBER_URL, authentication, post);
+removeMemberRouter.post(config.REMOVE_MEMBER_URL, authentication, ...removeMember, checkValidations, post);
 
 export default removeMemberRouter;

src/validation/error.messages.ts

@@ -0,0 +1,4 @@
+export enum ErrorMessages {
+  GIT_HANDLE = 'Enter the username of your GitHub handle (aka GitHub account)',
+  DESCRIPTION_LENGTH = 'Description must be 1000 characters or less',
+}

src/validation/remove-member.validation.ts

@@ -0,0 +1,8 @@
+import { body } from 'express-validator';
+
+import { ErrorMessages } from './error.messages';
+
+export const removeMember = [
+    body('github_handle').not().isEmpty({ ignore_whitespace: true }).withMessage(ErrorMessages.GIT_HANDLE),
+    body('description').isLength({ max: 1000 }).withMessage(ErrorMessages.DESCRIPTION_LENGTH)
+];

src/views/include/back-link.html

@@ -0,0 +1,4 @@
+{{ govukBackLink({
+  text: "Back",
+  href: "/landing-page"
+}) }}

src/views/include/error-list.html

@@ -0,0 +1,9 @@
+{% if errors and errors.errorList and errors.errorList.length > 0 %}
+  {{ govukErrorSummary({
+    titleText: "There is a problem",
+    errorList: errors.errorList if errors,
+    attributes: {
+      "tabindex": "0"
+    }
+  }) }}
+{% endif %}

src/views/include/save-button.html

@@ -0,0 +1,3 @@
+{{ govukButton({
+    text: "Save"
+}) }}

src/views/layout.html

@@ -2,6 +2,7 @@
 
 {% from "govuk/components/footer/macro.njk"           import govukFooter %}
 {% from "govuk/components/header/macro.njk"           import govukHeader %}
+{% from "govuk/components/error-summary/macro.njk"    import govukErrorSummary %}
 {% from "govuk/components/panel/macro.njk"            import govukPanel %}
 {% from "govuk/components/input/macro.njk"            import govukInput %}
 {% from "govuk/components/fieldset/macro.njk"         import govukFieldset %}

src/views/remove-member.html

@@ -1,10 +1,7 @@
 {% extends "layout.html" %}
 
 {% block beforeContent %}
-  {{ govukBackLink({
-    text: "Back",
-    href: "/landing-page"
-  }) }}
+  {% include "include/back-link.html" %}
 {% endblock %}
 
 {% block content %}
@@ -16,19 +13,25 @@ <h1 class="govuk-heading-l">Remove a GitHub Member</h1>
         When a user no longer requires access to our GitHub organisations(e.g they have left The Cabinet Office) they should be removed from the Cabinet Office GitHub organisation.
       </p>
 
-      <form method="post" novalidate>
+      {% include "include/error-list.html" %}
+
+      <form method="post">
 
         {{ govukInput({
+          errorMessage: errors.github_handle if errors,
           label: {
             text: "GitHub Handle",
             classes: "govuk-label--m"
           },
           classes: "govuk-input--width-10",
-          id: "github-handle",
-          name: "github_handle"
+          id: "github_handle",
+          name: "github_handle",
+          value: github_handle
         }) }}
 
         {{ govukTextarea({
+          errorMessage: errors.description if errors,
+          value: description,
           name: "description",
           id: "description",
           label: {
@@ -41,10 +44,8 @@ <h1 class="govuk-heading-l">Remove a GitHub Member</h1>
           }
         }) }}
 
-        {{ govukButton({
-          text: "Save"
-        }) }}
-
+        {% include "include/save-button.html" %}
+
       </form>
     </div>
   </div>

test/integration/routes/remove-member.spec.ts

@@ -14,6 +14,7 @@ import { authentication } from '../../../src/middleware/authentication.middlewar
 
 import { MOCK_REDIRECT_MESSAGE, MOCK_GET_REMOVE_MEMBER_RESPONSE, MOCK_POST_REMOVE_MEMBER_RESPONSE } from '../../mock/text.mock';
 import { MOCK_POST_REMOVE_MEMBER } from '../../mock/data';
+import { ErrorMessages } from '../../../src/validation/error.messages';
 
 const mockedLogger = logger as jest.Mock<typeof logger>;
 mockedLogger.mockImplementation((req: Request, res: Response, next: NextFunction) => next());
@@ -35,6 +36,7 @@ describe('Remove-member endpoint integration tests', () => {
             expect(mockedAuth).toHaveBeenCalledTimes(1);
         });
     });
+
     describe('POST tests', () => {
         test('Should redirect to landing page after POST request', async () => {
             const res = await request(app).post(config.REMOVE_MEMBER_URL).send(MOCK_POST_REMOVE_MEMBER);
@@ -44,6 +46,21 @@ describe('Remove-member endpoint integration tests', () => {
             expect(mockedLogger).toHaveBeenCalledTimes(1);
             expect(mockedAuth).toHaveBeenCalledTimes(1);
         });
+
+        test('Should render the same page with error messages after POST request', async () => {
+            const res = await request(app).post(config.REMOVE_MEMBER_URL).send({
+                github_handle: '',
+                description: '1000chars.'.repeat(100) + ':)'
+            });
+
+            expect(res.status).toEqual(200);
+            expect(res.text).toContain(ErrorMessages.GIT_HANDLE);
+            expect(res.text).toContain(ErrorMessages.DESCRIPTION_LENGTH);
+            expect(res.text).toContain(MOCK_GET_REMOVE_MEMBER_RESPONSE);
+            expect(mockedLogger).toHaveBeenCalledTimes(1);
+            expect(mockedAuth).toHaveBeenCalledTimes(1);
+        });
+
         test('Should log the GitHub Handle and More Details on POST request.', async () => {
             const res = await request(app).post(config.REMOVE_MEMBER_URL).send(MOCK_POST_REMOVE_MEMBER);
 

test/unit/middleware/validation.middleware.spec.ts

@@ -0,0 +1,85 @@
+jest.mock('express-validator');
+jest.mock('../../../src/utils/logger');
+
+import { describe, expect, test, jest, afterEach, beforeEach } from '@jest/globals';
+import { Request, Response, NextFunction } from 'express';
+import { validationResult } from 'express-validator';
+
+import * as config from '../../../src/config';
+
+import { checkValidations } from '../../../src/middleware/validation.middleware';
+import { ErrorMessages } from '../../../src/validation/error.messages';
+import { MOCK_ERROR, MOCK_POST_REMOVE_MEMBER } from '../../mock/data';
+import { log } from '../../../src/utils/logger';
+
+const validationResultMock = validationResult as unknown as jest.Mock;
+const logInfoMock = log.info as jest.Mock;
+const logErrorMock = log.error as jest.Mock;
+
+const mockResponse = () => {
+    const res = {} as Response;
+    res.render = jest.fn() as any;
+    return res;
+};
+const mockRequest = () => {
+    const req = {} as Request;
+    req.path = config.REMOVE_MEMBER_URL;
+    req.body = MOCK_POST_REMOVE_MEMBER;
+    return req;
+};
+const next = jest.fn() as NextFunction;
+
+describe('Validation Middleware test suites', () => {
+
+    let res: any, req: any;
+
+    beforeEach(() => {
+        res = mockResponse();
+        req = mockRequest();
+    });
+
+    afterEach(() => {
+        jest.resetAllMocks();
+    });
+
+    test('Should call next if errorList is empty', () => {
+        validationResultMock.mockImplementationOnce( () => { return { isEmpty: () => true };});
+        checkValidations(req, res, next);
+
+        expect(logInfoMock).toBeCalledTimes(0);
+        expect(logErrorMock).toBeCalledTimes(0);
+        expect(next).toHaveBeenCalledTimes(1);
+    });
+
+    test(`should call res.render with ${config.REMOVE_MEMBER} view if errorList is not empty`, () => {
+        const fieldKey = 'github_handle';
+        validationResultMock.mockImplementationOnce( () => {
+            return {
+                isEmpty: () => false,
+                array: () => [ { path: fieldKey, msg: ErrorMessages.GIT_HANDLE } ]
+            };
+        });
+        req.body[fieldKey] = '';
+        checkValidations(req, res, next);
+
+        expect(logInfoMock).toHaveBeenCalledTimes(1);
+        expect(logInfoMock).toHaveBeenCalledWith(`Validation error on ${config.REMOVE_MEMBER} page`);
+        expect(res.render).toHaveBeenCalledTimes(1);
+        expect(res.render).toHaveBeenCalledWith(config.REMOVE_MEMBER, {
+            ...req.body,
+            errors: {
+                errorList: [{ 'href': `#${fieldKey}`, 'text': ErrorMessages.GIT_HANDLE }],
+                [fieldKey]: { 'text': ErrorMessages.GIT_HANDLE }
+            }
+        });
+    });
+
+    test('should catch the error log error message and call next(err)', () => {
+        validationResultMock.mockImplementationOnce( () => { throw new Error(MOCK_ERROR.message); });
+        checkValidations(req, res, next);
+
+        expect(next).toHaveBeenCalledTimes(1);
+        expect(logErrorMock).toHaveBeenCalledTimes(1);
+        expect(logErrorMock).toHaveBeenCalledWith(MOCK_ERROR.message);
+    });
+});