From 1b0a60dd5bf44c3d5eab5f415ad798672491bd93 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 19 Nov 2020 17:05:12 +0800
Subject: [PATCH 1/7] feat(adminFormUtils): add methods to assert user
 permissions on forms

---
 .../form/admin-form/admin-form.types.ts       |  9 ++
 .../form/admin-form/admin-form.utils.ts       | 84 +++++++++++++++----
 2 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/src/app/modules/form/admin-form/admin-form.types.ts b/src/app/modules/form/admin-form/admin-form.types.ts
index 8d0e0c0201..2af602d89e 100644
--- a/src/app/modules/form/admin-form/admin-form.types.ts
+++ b/src/app/modules/form/admin-form/admin-form.types.ts
@@ -1,5 +1,14 @@
+import { Result } from 'neverthrow'
+
+import { ForbiddenFormError, FormDeletedError } from '../form.errors'
+
 export enum PermissionLevel {
   Read = 'read',
   Write = 'write',
   Delete = 'delete',
 }
+
+export type FormPermissionResult = Result<
+  true,
+  ForbiddenFormError | FormDeletedError
+>
diff --git a/src/app/modules/form/admin-form/admin-form.utils.ts b/src/app/modules/form/admin-form/admin-form.utils.ts
index 780bb6aa0c..b2cfb2026b 100644
--- a/src/app/modules/form/admin-form/admin-form.utils.ts
+++ b/src/app/modules/form/admin-form/admin-form.utils.ts
@@ -20,6 +20,7 @@ import {
   CreatePresignedUrlError,
   InvalidFileTypeError,
 } from './admin-form.errors'
+import { FormPermissionResult } from './admin-form.types'
 
 const logger = createLoggerWithLabel(module)
 
@@ -87,23 +88,21 @@ export const mapRouteError = (
 }
 
 const isFormActive = (form: IPopulatedForm): Result<true, FormDeletedError> => {
-  if (form.status === Status.Archived) {
-    return err(new FormDeletedError('Form has been archived'))
-  }
-
-  return ok(true)
+  return form.status === Status.Archived
+    ? err(new FormDeletedError('Form has been archived'))
+    : ok(true)
 }
 
 /**
- * Asserts that the given user has read access to the form.
- * @returns ok(true) if given user has read permissions to form
+ * Asserts that the given user has read access for the form.
+ * @returns ok(true) if given user has read permissions
  * @returns err(FormDeletedError) if form has already been archived
- * @returns err(ForbiddenFormError) if user does not have read permissions to form
+ * @returns err(ForbiddenFormError) if user does not have read permissions
  */
 export const assertHasReadPermissions = (
   user: IUserSchema,
   form: IPopulatedForm,
-): Result<true, ForbiddenFormError | FormDeletedError> => {
+): FormPermissionResult => {
   return isFormActive(form).andThen(() => {
     // Is form admin. Automatically has permissions.
     if (String(user._id) === String(form.admin._id)) {
@@ -115,14 +114,67 @@ export const assertHasReadPermissions = (
       (allowedUser) => allowedUser.email === user.email,
     )
 
-    if (!hasReadPermissions) {
-      return err(
-        new ForbiddenFormError(
-          `User ${user.email} not authorized to perform read operation on Form ${form._id} with title: ${form.title}.`,
-        ),
-      )
+    return hasReadPermissions
+      ? ok(true)
+      : err(
+          new ForbiddenFormError(
+            `User ${user.email} not authorized to perform read operation on Form ${form._id} with title: ${form.title}.`,
+          ),
+        )
+  })
+}
+
+/**
+ * Asserts that the given user has delete permissions for the form.
+ * @returns ok(true) if given user has delete permissions
+ * @returns err(FormDeletedError) if form has already been archived
+ * @returns err(ForbiddenFormError) if user does not have delete permissions
+ */
+export const assertHasDeletePermissions = (
+  user: IUserSchema,
+  form: IPopulatedForm,
+): FormPermissionResult => {
+  return isFormActive(form).andThen(() => {
+    const isFormAdmin = String(user._id) === String(form.admin._id)
+    // If form admin
+    return isFormAdmin
+      ? ok(true)
+      : err(
+          new ForbiddenFormError(
+            `User ${user.email} not authorized to perform delete operation on Form ${form._id} with title: ${form.title}.`,
+          ),
+        )
+  })
+}
+
+/**
+ * Asserts that the given user has write permissions for the form.
+ * @returns ok(true) if given user has write permissions
+ * @returns err(FormDeletedError) if form has already been archived
+ * @returns err(ForbiddenFormError) if user does not have write permissions
+ */
+export const assertHasWritePermissions = (
+  user: IUserSchema,
+  form: IPopulatedForm,
+): FormPermissionResult => {
+  return isFormActive(form).andThen(() => {
+    // Is form admin. Automatically has permissions.
+    if (String(user._id) === String(form.admin._id)) {
+      return ok(true)
     }
 
-    return ok(true)
+    // Check if user email is currently in form's allowed list, and has write
+    // permissions.
+    const hasWritePermissions = !!form.permissionList?.find(
+      (allowedUser) => allowedUser.email === user.email && allowedUser.write,
+    )
+
+    return hasWritePermissions
+      ? ok(true)
+      : err(
+          new ForbiddenFormError(
+            `User ${user.email} not authorized to perform write operation on Form ${form._id} with title: ${form.title}.`,
+          ),
+        )
   })
 }

From b11c46c1a85e4a84d15b25c21a5d6388362030e0 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 19 Nov 2020 17:24:40 +0800
Subject: [PATCH 2/7] test(adminFormUtils): add remaining tests for assert
 permissions fns

---
 .../__tests__/admin-form.utils.spec.ts        | 238 +++++++++++++++++-
 1 file changed, 227 insertions(+), 11 deletions(-)

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
index 05af592455..dc435c8c5d 100644
--- a/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
+++ b/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
@@ -3,18 +3,22 @@ import { ObjectId } from 'bson-ext'
 import { IPopulatedForm, IPopulatedUser, Permission, Status } from 'src/types'
 
 import { ForbiddenFormError, FormDeletedError } from '../../form.errors'
-import { assertHasReadPermissions } from '../admin-form.utils'
+import {
+  assertHasDeletePermissions,
+  assertHasReadPermissions,
+  assertHasWritePermissions,
+} from '../admin-form.utils'
 
 describe('admin-form.utils', () => {
+  const MOCK_VALID_ADMIN_ID = new ObjectId()
+  const MOCK_VALID_ADMIN_EMAIL = 'test@example.com'
+  const MOCK_USER = {
+    _id: MOCK_VALID_ADMIN_ID,
+    email: MOCK_VALID_ADMIN_EMAIL,
+  } as IPopulatedUser
+
   describe('assertHasReadPermissions', () => {
-    const MOCK_VALID_ADMIN_ID = new ObjectId()
-    const MOCK_VALID_ADMIN_EMAIL = 'test@example.com'
-    const MOCK_USER = {
-      _id: MOCK_VALID_ADMIN_ID,
-      email: MOCK_VALID_ADMIN_EMAIL,
-    } as IPopulatedUser
-
-    it('should return true if user is form admin', async () => {
+    it('should return true when user is form admin', async () => {
       // Arrange
       const mockForm = {
         title: 'mockForm',
@@ -32,7 +36,7 @@ describe('admin-form.utils', () => {
       expect(actualResult._unsafeUnwrap()).toEqual(true)
     })
 
-    it('should return true if user has read permissions', async () => {
+    it('should return true when user has read permissions', async () => {
       // Arrange
       // Form is owned by another admin, but MOCK_USER has permissions.
       const mockForm = {
@@ -75,7 +79,7 @@ describe('admin-form.utils', () => {
       )
     })
 
-    it('should return ForbiddenFormError if user does not have read permissions', async () => {
+    it('should return ForbiddenFormError when user does not have read permissions', async () => {
       // Arrange
       // Form is owned by another admin, and MOCK_USER does not have
       // permissions.
@@ -102,4 +106,216 @@ describe('admin-form.utils', () => {
       )
     })
   })
+
+  describe('assertHasWritePermissions', () => {
+    it('should return true when user is form admin', async () => {
+      // Arrange
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Private,
+        _id: new ObjectId(),
+        // User is form admin.
+        admin: MOCK_USER,
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isOk()).toEqual(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(true)
+    })
+
+    it('should return true when user has write permissions', async () => {
+      // Arrange
+      // Form is owned by another admin, but MOCK_USER has permissions.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Public,
+        _id: new ObjectId(),
+        // New admin.
+        admin: {
+          _id: new ObjectId(),
+        } as IPopulatedUser,
+        // But MOCK_USER has write permissions.
+        permissionList: [{ email: MOCK_USER.email, write: true }],
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isOk()).toEqual(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(true)
+    })
+
+    it('should return FormDeletedError when given form is archived', async () => {
+      // Arrange
+      // Form is archived.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Archived,
+        _id: new ObjectId(),
+        admin: MOCK_USER,
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new FormDeletedError('Form has been archived'),
+      )
+    })
+
+    it('should return ForbiddenFormError when user has read but not write permissions', async () => {
+      // Arrange
+      // Form is owned by another admin, and MOCK_USER does not have
+      // permissions.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Public,
+        _id: new ObjectId(),
+        // New admin, no permissionsList.
+        admin: {
+          _id: new ObjectId(),
+        } as IPopulatedUser,
+        // Only read permissions.
+        permissionList: [{ email: MOCK_USER.email }],
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new ForbiddenFormError(
+          `User ${MOCK_USER.email} not authorized to perform write operation on Form ${mockForm._id} with title: ${mockForm.title}.`,
+        ),
+      )
+    })
+
+    it('should return ForbiddenFormError when user does not exist in permission list', async () => {
+      // Arrange
+      // Form is owned by another admin, and MOCK_USER does not have
+      // permissions.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Public,
+        _id: new ObjectId(),
+        // New admin, no permissionsList.
+        admin: {
+          _id: new ObjectId(),
+        } as IPopulatedUser,
+        permissionList: [] as Permission[],
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new ForbiddenFormError(
+          `User ${MOCK_USER.email} not authorized to perform write operation on Form ${mockForm._id} with title: ${mockForm.title}.`,
+        ),
+      )
+    })
+  })
+
+  describe('assertHasDeletePermissions', () => {
+    it('should return true when user is form admin', async () => {
+      // Arrange
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Private,
+        _id: new ObjectId(),
+        // User is form admin.
+        admin: MOCK_USER,
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasDeletePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isOk()).toEqual(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(true)
+    })
+
+    it('should return FormDeletedError when given form is archived', async () => {
+      // Arrange
+      // Form is archived.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Archived,
+        _id: new ObjectId(),
+        admin: MOCK_USER,
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasDeletePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new FormDeletedError('Form has been archived'),
+      )
+    })
+
+    it('should return ForbiddenFormError when user is not admin even with read permissions', async () => {
+      // Arrange
+      // Form is owned by another admin, but MOCK_USER has permissions.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Public,
+        _id: new ObjectId(),
+        // New admin.
+        admin: {
+          _id: new ObjectId(),
+        } as IPopulatedUser,
+        // But MOCK_USER has permissions..
+        permissionList: [{ email: MOCK_USER.email }],
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasDeletePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new ForbiddenFormError(
+          `User ${MOCK_USER.email} not authorized to perform delete operation on Form ${mockForm._id} with title: ${mockForm.title}.`,
+        ),
+      )
+    })
+
+    it('should return ForbiddenFormError when user is not admin even with write permissions', async () => {
+      // Arrange
+      // Form is owned by another admin, but MOCK_USER has permissions.
+      const mockForm = {
+        title: 'mockForm',
+        status: Status.Public,
+        _id: new ObjectId(),
+        // New admin.
+        admin: {
+          _id: new ObjectId(),
+        } as IPopulatedUser,
+        // But MOCK_USER has permissions.
+        permissionList: [{ email: MOCK_USER.email, write: true }],
+      } as IPopulatedForm
+
+      // Act
+      const actualResult = assertHasDeletePermissions(MOCK_USER, mockForm)
+
+      // Assert
+      expect(actualResult.isErr()).toEqual(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(
+        new ForbiddenFormError(
+          `User ${MOCK_USER.email} not authorized to perform delete operation on Form ${mockForm._id} with title: ${mockForm.title}.`,
+        ),
+      )
+    })
+  })
 })

From 6d377051d0218fe6a04bd73450bb0a2369c89a2a Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 19 Nov 2020 17:56:45 +0800
Subject: [PATCH 3/7] ref: use adminForm permission util fns when verifying
 form permissions

---
 .../authentication.server.controller.js       | 83 +++++++------------
 .../authentication.server.controller.spec.js  |  6 +-
 2 files changed, 35 insertions(+), 54 deletions(-)

diff --git a/src/app/controllers/authentication.server.controller.js b/src/app/controllers/authentication.server.controller.js
index 1c30963798..42c4993b4f 100755
--- a/src/app/controllers/authentication.server.controller.js
+++ b/src/app/controllers/authentication.server.controller.js
@@ -7,19 +7,14 @@ const { StatusCodes } = require('http-status-codes')
 const {
   PermissionLevel,
 } = require('../modules/form/admin-form/admin-form.types')
+const {
+  assertHasReadPermissions,
+  assertHasWritePermissions,
+  assertHasDeletePermissions,
+} = require('../modules/form/admin-form/admin-form.utils')
 const { createReqMeta } = require('../utils/request')
 const logger = require('../../config/logger').createLoggerWithLabel(module)
 
-/**
- * Returns the error message when a user cannot perform an action on a form
- * @param {String} user -  user email
- * @param {String} title -  form title
- * @returns {String} - the error message
- */
-const makeUnauthorizedMessage = (user, title) => {
-  return `User ${user} is not authorized to perform this operation on Form: ${title}.`
-}
-
 /**
  * Logs an error message when a user cannot perform an action on a form
  * @param {String} user -  user email
@@ -57,52 +52,38 @@ exports.verifyPermission = (requiredPermission) =>
    * @param {function} next - Next middleware function
    */
   (req, res, next) => {
-    const isFormAdmin =
-      String(req.form.admin.id) === String(req.session.user._id)
-
-    // Forbidden if requiredPersmission is admin but user is not
-    if (!isFormAdmin && requiredPermission === PermissionLevel.Delete) {
-      logUnauthorizedAccess(req, 'verifyPermission', requiredPermission)
-      return res.status(StatusCodes.FORBIDDEN).json({
-        message: makeUnauthorizedMessage(
-          req.session.user.email,
-          req.form.title,
-        ),
-      })
-    }
-
-    // Admins always have sufficient permission
-    let hasSufficientPermission = isFormAdmin
-
-    // Write users can access forms that require write/read
-    if (
-      requiredPermission === PermissionLevel.Write ||
-      requiredPermission === PermissionLevel.Read
-    ) {
-      hasSufficientPermission =
-        hasSufficientPermission ||
-        req.form.permissionList.find(
-          (userObj) =>
-            userObj.email === req.session.user.email && userObj.write,
-        )
-    }
-    // Read users can access forms that require read permissions
-    if (requiredPermission === PermissionLevel.Read) {
-      hasSufficientPermission =
-        hasSufficientPermission ||
-        req.form.permissionList.find(
-          (userObj) => userObj.email === req.session.user.email,
-        )
+    let result
+    switch (requiredPermission) {
+      case PermissionLevel.Read:
+        result = assertHasReadPermissions(req.session.user, req.form)
+        break
+      case PermissionLevel.Write:
+        result = assertHasWritePermissions(req.session.user, req.form)
+        break
+      case PermissionLevel.Delete:
+        result = assertHasDeletePermissions(req.session.user, req.form)
+        break
+      default:
+        logger.error({
+          message:
+            'Unknown permission type encountered when verifying permissions',
+          meta: {
+            action: 'verifyPermission',
+            ...createReqMeta(req),
+          },
+        })
+        return res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({
+          message: 'Unknown permission type',
+        })
     }
 
-    if (!hasSufficientPermission) {
+    if (result.isErr()) {
       logUnauthorizedAccess(req, 'verifyPermission', requiredPermission)
       return res.status(StatusCodes.FORBIDDEN).json({
-        message: makeUnauthorizedMessage(
-          req.session.user.email,
-          req.form.title,
-        ),
+        message: result.error.message,
       })
     }
+
+    // No error, pass to next function.
     return next()
   }
diff --git a/tests/unit/backend/controllers/authentication.server.controller.spec.js b/tests/unit/backend/controllers/authentication.server.controller.spec.js
index 42c977bd9f..add4854d9d 100644
--- a/tests/unit/backend/controllers/authentication.server.controller.spec.js
+++ b/tests/unit/backend/controllers/authentication.server.controller.spec.js
@@ -66,7 +66,7 @@ describe('Authentication Controller', () => {
       let next = jasmine.createSpy()
       // Populate admin with partial user object
       let testFormObj = testForm.toObject()
-      testFormObj.admin = { id: req.session.user._id }
+      testFormObj.admin = { _id: req.session.user._id }
       req.form = testFormObj
       Controller.verifyPermission(PermissionLevel.Delete)(req, res, next)
       expect(next).toHaveBeenCalled()
@@ -75,7 +75,7 @@ describe('Authentication Controller', () => {
       let next = jasmine.createSpy()
       // Populate admin with partial user object
       let testFormObj = testForm.toObject()
-      testFormObj.admin = { id: mongoose.Types.ObjectId('000000000002') }
+      testFormObj.admin = { _id: mongoose.Types.ObjectId('000000000002') }
       testFormObj.permissionList.push(
         roles.collaborator(req.session.user.email),
       )
@@ -90,7 +90,7 @@ describe('Authentication Controller', () => {
       })
       // Populate admin with partial user object
       let testFormObj = testForm.toObject()
-      testFormObj.admin = { id: mongoose.Types.ObjectId('000000000002') }
+      testFormObj.admin = { _id: mongoose.Types.ObjectId('000000000002') }
       req.form = testFormObj
       Controller.verifyPermission(PermissionLevel.Write)(req, res, () => {})
     })

From f551ce73a8b73699c641f3b336dbde1879957baf Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 23 Nov 2020 13:25:03 +0800
Subject: [PATCH 4/7] feat(adminFormUtils): tighten permission assertion fns
 scope

---
 .../form/admin-form/admin-form.controller.ts  | 54 ++++++----
 .../form/admin-form/admin-form.types.ts       |  7 +-
 .../form/admin-form/admin-form.utils.ts       | 99 +++++++++----------
 3 files changed, 84 insertions(+), 76 deletions(-)

diff --git a/src/app/modules/form/admin-form/admin-form.controller.ts b/src/app/modules/form/admin-form/admin-form.controller.ts
index 8a3eda5855..9a4eb2b0aa 100644
--- a/src/app/modules/form/admin-form/admin-form.controller.ts
+++ b/src/app/modules/form/admin-form/admin-form.controller.ts
@@ -12,7 +12,11 @@ import {
   createPresignedPostForLogos,
   getDashboardForms,
 } from './admin-form.service'
-import { assertHasReadPermissions, mapRouteError } from './admin-form.utils'
+import {
+  assertFormAvailable,
+  assertHasReadPermissions,
+  mapRouteError,
+} from './admin-form.utils'
 
 const logger = createLoggerWithLabel(module)
 
@@ -138,16 +142,20 @@ export const handleCountFormSubmissions: RequestHandler<
   const { startDate, endDate } = req.query
   const sessionUserId = (req.session as Express.AuthedSession).user._id
 
+  const logMeta = {
+    action: 'handleCountFormSubmissions',
+    ...createReqMeta(req),
+    userId: sessionUserId,
+    formId,
+  }
+
   // Step 1: Retrieve currently logged in user.
   const adminResult = await UserService.getPopulatedUserById(sessionUserId)
   // Step 1a: Error retrieving logged in user.
   if (adminResult.isErr()) {
     logger.error({
       message: 'Error occurred whilst retrieving user',
-      meta: {
-        action: 'handleCountFormSubmissions',
-        userId: sessionUserId,
-      },
+      meta: logMeta,
       error: adminResult.error,
     })
 
@@ -163,11 +171,7 @@ export const handleCountFormSubmissions: RequestHandler<
   if (formResult.isErr()) {
     logger.error({
       message: 'Failed to retrieve form',
-      meta: {
-        action: 'handleCountFormSubmissions',
-        ...createReqMeta(req),
-        formId,
-      },
+      meta: logMeta,
       error: formResult.error,
     })
     const { errorMessage, statusCode } = mapRouteError(formResult.error)
@@ -176,30 +180,38 @@ export const handleCountFormSubmissions: RequestHandler<
   // Step 2b: Successfully retrieved form.
   const form = formResult.value
 
-  // Step 3: Check form permissions.
+  // Step 3: Check whether form is already archived.
+  const availableResult = assertFormAvailable(form)
+  // Step 3a: Form is already archived.
+  if (availableResult.isErr()) {
+    logger.warn({
+      message: 'User attempted to retrieve archived form',
+      meta: logMeta,
+      error: availableResult.error,
+    })
+    const { errorMessage, statusCode } = mapRouteError(availableResult.error)
+    return res.status(statusCode).json({ message: errorMessage })
+  }
+
+  // Step 4: Form is still available for retrieval, check form permissions.
   const permissionResult = assertHasReadPermissions(admin, form)
-  // Step 3a: Read permission error.
+  // Step 4a: Read permission error.
   if (permissionResult.isErr()) {
-    logger.error({
+    logger.warn({
       message: 'User does not have read permissions',
-      meta: {
-        action: 'handleCountFormSubmissions',
-        ...createReqMeta(req),
-        userId: sessionUserId,
-        formId,
-      },
+      meta: logMeta,
       error: permissionResult.error,
     })
     const { errorMessage, statusCode } = mapRouteError(permissionResult.error)
     return res.status(statusCode).json({ message: errorMessage })
   }
 
-  // Step 4: has permissions, continue to retrieve submission counts.
+  // Step 5: Has permissions, continue to retrieve submission counts.
   const countResult = await SubmissionService.getFormSubmissionsCount(
     String(form._id),
     { startDate, endDate },
   )
-  // Step 4a: Error retrieving form submissions counts.
+  // Step 5a: Error retrieving form submissions counts.
   if (countResult.isErr()) {
     logger.error({
       message: 'Error retrieving form submission count',
diff --git a/src/app/modules/form/admin-form/admin-form.types.ts b/src/app/modules/form/admin-form/admin-form.types.ts
index 2af602d89e..aa5f924b91 100644
--- a/src/app/modules/form/admin-form/admin-form.types.ts
+++ b/src/app/modules/form/admin-form/admin-form.types.ts
@@ -1,6 +1,6 @@
 import { Result } from 'neverthrow'
 
-import { ForbiddenFormError, FormDeletedError } from '../form.errors'
+import { ForbiddenFormError } from '../form.errors'
 
 export enum PermissionLevel {
   Read = 'read',
@@ -8,7 +8,4 @@ export enum PermissionLevel {
   Delete = 'delete',
 }
 
-export type FormPermissionResult = Result<
-  true,
-  ForbiddenFormError | FormDeletedError
->
+export type FormPermissionResult = Result<true, ForbiddenFormError>
diff --git a/src/app/modules/form/admin-form/admin-form.utils.ts b/src/app/modules/form/admin-form/admin-form.utils.ts
index b2cfb2026b..19f3196d69 100644
--- a/src/app/modules/form/admin-form/admin-form.utils.ts
+++ b/src/app/modules/form/admin-form/admin-form.utils.ts
@@ -87,7 +87,15 @@ export const mapRouteError = (
   }
 }
 
-const isFormActive = (form: IPopulatedForm): Result<true, FormDeletedError> => {
+/**
+ * Asserts whether a form is available for retrieval by users.
+ * @param form the form to check
+ * @returns ok(true) if form status is not archived.
+ * @returns err(FormDeletedError) if the form has already been archived
+ */
+export const assertFormAvailable = (
+  form: IPopulatedForm,
+): Result<true, FormDeletedError> => {
   return form.status === Status.Archived
     ? err(new FormDeletedError('Form has been archived'))
     : ok(true)
@@ -96,85 +104,76 @@ const isFormActive = (form: IPopulatedForm): Result<true, FormDeletedError> => {
 /**
  * Asserts that the given user has read access for the form.
  * @returns ok(true) if given user has read permissions
- * @returns err(FormDeletedError) if form has already been archived
  * @returns err(ForbiddenFormError) if user does not have read permissions
  */
 export const assertHasReadPermissions = (
   user: IUserSchema,
   form: IPopulatedForm,
 ): FormPermissionResult => {
-  return isFormActive(form).andThen(() => {
-    // Is form admin. Automatically has permissions.
-    if (String(user._id) === String(form.admin._id)) {
-      return ok(true)
-    }
+  // Is form admin. Automatically has permissions.
+  if (String(user._id) === String(form.admin._id)) {
+    return ok(true)
+  }
 
-    // Check if user email is currently in form's allowed list.
-    const hasReadPermissions = !!form.permissionList?.find(
-      (allowedUser) => allowedUser.email === user.email,
-    )
+  // Check if user email is currently in form's allowed list.
+  const hasReadPermissions = !!form.permissionList?.find(
+    (allowedUser) => allowedUser.email === user.email,
+  )
 
-    return hasReadPermissions
-      ? ok(true)
-      : err(
-          new ForbiddenFormError(
-            `User ${user.email} not authorized to perform read operation on Form ${form._id} with title: ${form.title}.`,
-          ),
-        )
-  })
+  return hasReadPermissions
+    ? ok(true)
+    : err(
+        new ForbiddenFormError(
+          `User ${user.email} not authorized to perform read operation on Form ${form._id} with title: ${form.title}.`,
+        ),
+      )
 }
 
 /**
  * Asserts that the given user has delete permissions for the form.
  * @returns ok(true) if given user has delete permissions
- * @returns err(FormDeletedError) if form has already been archived
  * @returns err(ForbiddenFormError) if user does not have delete permissions
  */
 export const assertHasDeletePermissions = (
   user: IUserSchema,
   form: IPopulatedForm,
 ): FormPermissionResult => {
-  return isFormActive(form).andThen(() => {
-    const isFormAdmin = String(user._id) === String(form.admin._id)
-    // If form admin
-    return isFormAdmin
-      ? ok(true)
-      : err(
-          new ForbiddenFormError(
-            `User ${user.email} not authorized to perform delete operation on Form ${form._id} with title: ${form.title}.`,
-          ),
-        )
-  })
+  const isFormAdmin = String(user._id) === String(form.admin._id)
+  // If form admin
+  return isFormAdmin
+    ? ok(true)
+    : err(
+        new ForbiddenFormError(
+          `User ${user.email} not authorized to perform delete operation on Form ${form._id} with title: ${form.title}.`,
+        ),
+      )
 }
 
 /**
  * Asserts that the given user has write permissions for the form.
  * @returns ok(true) if given user has write permissions
- * @returns err(FormDeletedError) if form has already been archived
  * @returns err(ForbiddenFormError) if user does not have write permissions
  */
 export const assertHasWritePermissions = (
   user: IUserSchema,
   form: IPopulatedForm,
 ): FormPermissionResult => {
-  return isFormActive(form).andThen(() => {
-    // Is form admin. Automatically has permissions.
-    if (String(user._id) === String(form.admin._id)) {
-      return ok(true)
-    }
+  // Is form admin. Automatically has permissions.
+  if (String(user._id) === String(form.admin._id)) {
+    return ok(true)
+  }
 
-    // Check if user email is currently in form's allowed list, and has write
-    // permissions.
-    const hasWritePermissions = !!form.permissionList?.find(
-      (allowedUser) => allowedUser.email === user.email && allowedUser.write,
-    )
+  // Check if user email is currently in form's allowed list, and has write
+  // permissions.
+  const hasWritePermissions = !!form.permissionList?.find(
+    (allowedUser) => allowedUser.email === user.email && allowedUser.write,
+  )
 
-    return hasWritePermissions
-      ? ok(true)
-      : err(
-          new ForbiddenFormError(
-            `User ${user.email} not authorized to perform write operation on Form ${form._id} with title: ${form.title}.`,
-          ),
-        )
-  })
+  return hasWritePermissions
+    ? ok(true)
+    : err(
+        new ForbiddenFormError(
+          `User ${user.email} not authorized to perform write operation on Form ${form._id} with title: ${form.title}.`,
+        ),
+      )
 }

From 14b838d731deaed2889bb907f263f093ccb32445 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 23 Nov 2020 13:27:42 +0800
Subject: [PATCH 5/7] test(adminFormUtils): remove tests checking for
 FormDeletedError

---
 .../__tests__/admin-form.utils.spec.ts        | 62 +------------------
 1 file changed, 1 insertion(+), 61 deletions(-)

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
index dc435c8c5d..4cd4a4afb9 100644
--- a/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
+++ b/src/app/modules/form/admin-form/__tests__/admin-form.utils.spec.ts
@@ -2,7 +2,7 @@ import { ObjectId } from 'bson-ext'
 
 import { IPopulatedForm, IPopulatedUser, Permission, Status } from 'src/types'
 
-import { ForbiddenFormError, FormDeletedError } from '../../form.errors'
+import { ForbiddenFormError } from '../../form.errors'
 import {
   assertHasDeletePermissions,
   assertHasReadPermissions,
@@ -59,26 +59,6 @@ describe('admin-form.utils', () => {
       expect(actualResult._unsafeUnwrap()).toEqual(true)
     })
 
-    it('should return FormDeletedError when given form is archived', async () => {
-      // Arrange
-      // Form is archived.
-      const mockForm = {
-        title: 'mockForm',
-        status: Status.Archived,
-        _id: new ObjectId(),
-        admin: MOCK_USER,
-      } as IPopulatedForm
-
-      // Act
-      const actualResult = assertHasReadPermissions(MOCK_USER, mockForm)
-
-      // Assert
-      expect(actualResult.isErr()).toEqual(true)
-      expect(actualResult._unsafeUnwrapErr()).toEqual(
-        new FormDeletedError('Form has been archived'),
-      )
-    })
-
     it('should return ForbiddenFormError when user does not have read permissions', async () => {
       // Arrange
       // Form is owned by another admin, and MOCK_USER does not have
@@ -149,26 +129,6 @@ describe('admin-form.utils', () => {
       expect(actualResult._unsafeUnwrap()).toEqual(true)
     })
 
-    it('should return FormDeletedError when given form is archived', async () => {
-      // Arrange
-      // Form is archived.
-      const mockForm = {
-        title: 'mockForm',
-        status: Status.Archived,
-        _id: new ObjectId(),
-        admin: MOCK_USER,
-      } as IPopulatedForm
-
-      // Act
-      const actualResult = assertHasWritePermissions(MOCK_USER, mockForm)
-
-      // Assert
-      expect(actualResult.isErr()).toEqual(true)
-      expect(actualResult._unsafeUnwrapErr()).toEqual(
-        new FormDeletedError('Form has been archived'),
-      )
-    })
-
     it('should return ForbiddenFormError when user has read but not write permissions', async () => {
       // Arrange
       // Form is owned by another admin, and MOCK_USER does not have
@@ -244,26 +204,6 @@ describe('admin-form.utils', () => {
       expect(actualResult._unsafeUnwrap()).toEqual(true)
     })
 
-    it('should return FormDeletedError when given form is archived', async () => {
-      // Arrange
-      // Form is archived.
-      const mockForm = {
-        title: 'mockForm',
-        status: Status.Archived,
-        _id: new ObjectId(),
-        admin: MOCK_USER,
-      } as IPopulatedForm
-
-      // Act
-      const actualResult = assertHasDeletePermissions(MOCK_USER, mockForm)
-
-      // Assert
-      expect(actualResult.isErr()).toEqual(true)
-      expect(actualResult._unsafeUnwrapErr()).toEqual(
-        new FormDeletedError('Form has been archived'),
-      )
-    })
-
     it('should return ForbiddenFormError when user is not admin even with read permissions', async () => {
       // Arrange
       // Form is owned by another admin, but MOCK_USER has permissions.

From 75c9fad73720c40d6ede1b6860c203d597b17d11 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 23 Nov 2020 13:39:19 +0800
Subject: [PATCH 6/7] test(AdminFormCtl): rename AuthUtils import to correct
 AdminFormUtils

---
 .../admin-form/__tests__/admin-form.controller.spec.ts | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
index 2497f01324..027d8dfe15 100644
--- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
+++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
@@ -24,7 +24,7 @@ import {
   InvalidFileTypeError,
 } from '../admin-form.errors'
 import * as AdminFormService from '../admin-form.service'
-import * as AuthUtils from '../admin-form.utils'
+import * as AdminFormUtils from '../admin-form.utils'
 
 jest.mock('src/app/modules/submission/submission.service')
 const MockSubmissionService = mocked(SubmissionService)
@@ -301,7 +301,7 @@ describe('admin-form.controller', () => {
         okAsync(MOCK_FORM as IPopulatedForm),
       )
       const readPermsSpy = jest
-        .spyOn(AuthUtils, 'assertHasReadPermissions')
+        .spyOn(AdminFormUtils, 'assertHasReadPermissions')
         .mockReturnValueOnce(ok(true))
       MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce(
         okAsync(expectedSubmissionCount),
@@ -352,7 +352,7 @@ describe('admin-form.controller', () => {
         okAsync(MOCK_FORM as IPopulatedForm),
       )
       const readPermsSpy = jest
-        .spyOn(AuthUtils, 'assertHasReadPermissions')
+        .spyOn(AdminFormUtils, 'assertHasReadPermissions')
         .mockReturnValueOnce(ok(true))
       MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce(
         okAsync(expectedSubmissionCount),
@@ -394,7 +394,7 @@ describe('admin-form.controller', () => {
       // Mock error here.
       const expectedErrorString = 'no read access'
       const readPermsSpy = jest
-        .spyOn(AuthUtils, 'assertHasReadPermissions')
+        .spyOn(AdminFormUtils, 'assertHasReadPermissions')
         .mockReturnValueOnce(err(new ForbiddenFormError(expectedErrorString)))
 
       // Act
@@ -606,7 +606,7 @@ describe('admin-form.controller', () => {
         okAsync(MOCK_FORM as IPopulatedForm),
       )
       const readPermsSpy = jest
-        .spyOn(AuthUtils, 'assertHasReadPermissions')
+        .spyOn(AdminFormUtils, 'assertHasReadPermissions')
         .mockReturnValueOnce(ok(true))
       const expectedErrorString = 'database goes boom'
       MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce(

From e32a7cb50d7409d71fb4cf46be57be9b07c27955 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 23 Nov 2020 13:40:28 +0800
Subject: [PATCH 7/7] test(AdminFormCtl): add form availability error test case

---
 .../__tests__/admin-form.controller.spec.ts   | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
index 027d8dfe15..9d124eb0e1 100644
--- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
+++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
@@ -496,6 +496,47 @@ describe('admin-form.controller', () => {
       })
     })
 
+    it('should return 410 when FormDeletedError is returned when checking form availability', async () => {
+      // Arrange
+      const mockRes = expressHandler.mockResponse()
+      // Mock various services to return expected results.
+      MockUserService.getPopulatedUserById.mockReturnValueOnce(
+        okAsync(MOCK_USER as IPopulatedUser),
+      )
+      MockFormService.retrieveFullFormById.mockReturnValueOnce(
+        okAsync(MOCK_FORM as IPopulatedForm),
+      )
+      // Mock error when checking form availability.
+      const expectedErrorString = 'form is archived'
+      const utilSpy = jest
+        .spyOn(AdminFormUtils, 'assertFormAvailable')
+        .mockReturnValueOnce(err(new FormDeletedError(expectedErrorString)))
+
+      // Act
+      await AdminFormController.handleCountFormSubmissions(
+        MOCK_REQ,
+        mockRes,
+        jest.fn(),
+      )
+
+      // Assert
+      // Check all arguments of called services.
+      expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith(
+        MOCK_USER_ID,
+      )
+      expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith(
+        MOCK_FORM_ID.toHexString(),
+      )
+      expect(utilSpy).toHaveBeenCalledWith(MOCK_FORM)
+      expect(
+        MockSubmissionService.getFormSubmissionsCount,
+      ).not.toHaveBeenCalled()
+      expect(mockRes.status).toHaveBeenCalledWith(410)
+      expect(mockRes.json).toHaveBeenCalledWith({
+        message: expectedErrorString,
+      })
+    })
+
     it('should return 422 when MissingUserError is returned when retrieving logged in user', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()