fix: check duplicate MyInfo fields (#702)

* fix: include fieldId in map key

* ref: convert Map to fieldId-boolean

* ref: improve variable names and docs

* test: fix jasmine tests

* test: update jest tests

src/app/controllers/admin-forms.server.controller.js

@@ -600,7 +600,7 @@ function makeModule(connection) {
             (field) => field.myInfo && field.myInfo.attr,
           )
           for (let field of actualMyInfoFields) {
-            res.locals.hashedFields.add(field.myInfo.attr)
+            res.locals.hashedFields.add(field._id)
           }
           break
         }

src/app/controllers/email-submissions.server.controller.js

@@ -336,7 +336,7 @@ exports.prepareEmailSubmission = (req, res, next) => {
  * @returns {Boolean} true if response is verified
  */
 const isMyInfoVerifiedResponse = (response, hashedFields) => {
-  return !!(hashedFields && hashedFields.has(_.get(response, 'myInfo.attr')))
+  return !!(hashedFields && hashedFields.has(response._id))
 }
 
 /**
@@ -382,7 +382,7 @@ const getAnswerForCheckbox = (response) => {
  * @param {String} response.answer
  * @param {String} response.fieldType
  * @param {Boolean} response.isVisible
- * @param {Set} hashedFields Fields hashed to verify answers provided by MyInfo
+ * @param {Set} hashedFields Field IDs hashed to verify answers provided by MyInfo
  * @returns {Object} an object containing three sets of formatted responses
  */
 const getFormattedResponse = (response, hashedFields) => {

src/app/services/myinfo/__tests__/myinfo.service.spec.ts

@@ -26,9 +26,9 @@ import {
   MOCK_FETCH_PARAMS,
   MOCK_FORM_FIELDS,
   MOCK_FORM_ID,
+  MOCK_HASHED_FIELD_IDS,
   MOCK_HASHES,
   MOCK_KEY_PATH,
-  MOCK_MATCHED_ATTRS,
   MOCK_MYINFO_DATA,
   MOCK_POPULATED_FORM_FIELDS,
   MOCK_REALM,
@@ -255,7 +255,7 @@ describe('MyInfoService', () => {
         MOCK_HASHES as IHashes,
       )
 
-      expect(result._unsafeUnwrap()).toEqual(MOCK_MATCHED_ATTRS)
+      expect(result._unsafeUnwrap()).toEqual(MOCK_HASHED_FIELD_IDS)
     })
 
     it('should return HashingError when hashing fails', async () => {

src/app/services/myinfo/__tests__/myinfo.test.constants.ts

@@ -99,13 +99,33 @@ export const MOCK_MYINFO_FORMAT_DATA = {
 
 export const MOCK_FORM_FIELDS = [
   // Some MyInfo fields
-  { fieldType: 'textfield', isVisible: true, myInfo: { attr: 'name' } },
-  { fieldType: 'textfield', isVisible: false, myInfo: { attr: 'mobileno' } },
-  { fieldType: 'textfield', isVisible: true, myInfo: { attr: 'homeno' } },
-  { fieldType: 'textfield', isVisible: true, myInfo: { attr: 'mailadd' } },
+  {
+    fieldType: 'textfield',
+    isVisible: true,
+    myInfo: { attr: 'name' },
+    _id: new ObjectId().toHexString(),
+  },
+  {
+    fieldType: 'textfield',
+    isVisible: false,
+    myInfo: { attr: 'mobileno' },
+    _id: new ObjectId().toHexString(),
+  },
+  {
+    fieldType: 'textfield',
+    isVisible: true,
+    myInfo: { attr: 'homeno' },
+    _id: new ObjectId().toHexString(),
+  },
+  {
+    fieldType: 'textfield',
+    isVisible: true,
+    myInfo: { attr: 'mailadd' },
+    _id: new ObjectId().toHexString(),
+  },
   // Some non-MyInfo fields
-  { fieldType: 'dropdown' },
-  { fieldType: 'textfield' },
+  { fieldType: 'dropdown', _id: new ObjectId().toHexString() },
+  { fieldType: 'textfield', _id: new ObjectId().toHexString() },
 ]
 
 export const MOCK_HASHES = {
@@ -117,7 +137,11 @@ export const MOCK_HASHES = {
 // Based on MOCK_FORM_FIELDS and MOCK_HASHES, only expect these attributes to
 // be matched based on hashes. mobileno does not match because its isVisible is false,
 // and homeno does not match because it does not have a hash.
-export const MOCK_MATCHED_ATTRS = new Set(['name', 'mailadd'])
+export const MOCK_HASHED_FIELD_IDS = new Set(
+  MOCK_FORM_FIELDS.filter(
+    (field) => field.myInfo && ['name', 'mailadd'].includes(field.myInfo?.attr),
+  ).map((field) => field._id),
+)
 
 const populatedValues = [
   { fieldValue: 'TAN XIAO HUI', disabled: true },

src/app/services/myinfo/myinfo.factory.ts

@@ -7,12 +7,7 @@ import FeatureManager, {
   FeatureNames,
   RegisteredFeature,
 } from '../../../config/feature-manager'
-import {
-  IFieldSchema,
-  IHashes,
-  IMyInfoHashSchema,
-  MyInfoAttribute,
-} from '../../../types'
+import { IFieldSchema, IHashes, IMyInfoHashSchema } from '../../../types'
 import {
   DatabaseError,
   MissingFeatureError,
@@ -59,7 +54,7 @@ interface IMyInfoFactory {
     responses: ProcessedFieldResponse[],
     hashes: IHashes,
   ) => ResultAsync<
-    Set<MyInfoAttribute>,
+    Set<string>,
     HashingError | HashDidNotMatchError | MissingFeatureError
   >
 }

src/app/services/myinfo/myinfo.service.ts

@@ -18,7 +18,6 @@ import {
   IFieldSchema,
   IHashes,
   IMyInfoHashSchema,
-  MyInfoAttribute,
 } from '../../../types'
 import { DatabaseError } from '../../modules/core/core.errors'
 import { ProcessedFieldResponse } from '../../modules/submission/submission.types'
@@ -278,14 +277,14 @@ export class MyInfoService {
    * Checks that the given responses match the given hashes.
    * @param responses Fields processed with the isVisible attribute
    * @param hashes MyInfo value hashes retrieved from the database
-   * @returns the set of MyInfo attributes which were verified using their hashes
+   * @returns the set of field IDs which were verified using their hashes
    * @throws if an error occurred while comparing the responses and their hashes, or if any
    * hash did not match the submitted value
    */
   checkMyInfoHashes(
     responses: ProcessedFieldResponse[],
     hashes: IHashes,
-  ): ResultAsync<Set<MyInfoAttribute>, HashingError | HashDidNotMatchError> {
+  ): ResultAsync<Set<string>, HashingError | HashDidNotMatchError> {
     const comparisonPromises = compareHashedValues(responses, hashes)
     return ResultAsync.fromPromise(
       Bluebird.props(comparisonPromises),
@@ -300,22 +299,22 @@ export class MyInfoService {
         return new HashingError()
       },
     ).andThen((comparisonResults) => {
-      const comparedAttrs = Array.from(comparisonResults.keys())
+      const comparedFieldIds = Array.from(comparisonResults.keys())
       // All outcomes should be true
-      const failedAttrs = comparedAttrs.filter(
+      const failedFieldIds = comparedFieldIds.filter(
         (attr) => !comparisonResults.get(attr),
       )
-      if (failedAttrs.length > 0) {
+      if (failedFieldIds.length > 0) {
         logger.error({
           message: 'MyInfo Hash did not match',
           meta: {
             action: 'checkMyInfoHashes',
-            failedAttrs,
+            failedFields: failedFieldIds,
           },
         })
         return errAsync(new HashDidNotMatchError())
       }
-      return okAsync(new Set(comparedAttrs))
+      return okAsync(new Set(comparedFieldIds))
     })
   }
 }

src/app/services/myinfo/myinfo.types.ts

@@ -1,6 +1,4 @@
-import { MyInfoAttribute } from '@opengovsg/myinfo-gov-client'
-
-import { IFieldSchema, IMyInfo } from '../../../types'
+import { IFieldSchema, IMyInfo, MyInfoAttribute } from '../../../types'
 import { ProcessedFieldResponse } from '../../modules/submission/submission.types'
 
 export interface IPossiblyPrefilledField extends IFieldSchema {
@@ -16,3 +14,5 @@ export type VisibleMyInfoResponse = ProcessedFieldResponse & {
   isVisible: true
   answer: string
 }
+
+export type MyInfoComparePromises = Map<string, Promise<boolean>>

src/app/services/myinfo/myinfo.util.ts

@@ -30,6 +30,7 @@ import {
 import { formatAddress, formatPhoneNumber } from './myinfo.format'
 import {
   IPossiblyPrefilledField,
+  MyInfoComparePromises,
   MyInfoHashPromises,
   VisibleMyInfoResponse,
 } from './myinfo.types'
@@ -202,15 +203,15 @@ const compareSingleHash = (
 export const compareHashedValues = (
   responses: ProcessedFieldResponse[],
   hashes: IHashes,
-): Map<MyInfoAttribute, Promise<boolean>> => {
+): MyInfoComparePromises => {
   // Filter responses to only those fields with a corresponding hash
   const fieldsWithHashes = filterFieldsWithHashes(responses, hashes)
   // Map MyInfoAttribute to response
-  const myInfoResponsesMap = new Map<MyInfoAttribute, Promise<boolean>>()
+  const myInfoResponsesMap: MyInfoComparePromises = new Map()
   fieldsWithHashes.forEach((field) => {
     const attr = field.myInfo.attr
     // Already checked that hashes contains this attr
-    myInfoResponsesMap.set(attr, compareSingleHash(hashes[attr]!, field))
+    myInfoResponsesMap.set(field._id, compareSingleHash(hashes[attr]!, field))
   })
   return myInfoResponsesMap
 }

src/types/express.locals.ts

@@ -1,6 +1,5 @@
 // TODO (#42): remove these types when migrating away from middleware pattern
 
-import { MyInfoAttribute } from './field/fieldTypes'
 import { IPopulatedForm } from './form'
 import { SpcpSession } from './spcp'
 
@@ -17,5 +16,5 @@ export type ResWithUinFin<T> = T & {
 }
 
 export type ResWithHashedFields<T> = T & {
-  locals: { hashedFields?: Set<MyInfoAttribute> }
+  locals: { hashedFields?: Set<string> }
 }

tests/unit/backend/controllers/email-submissions.server.controller.spec.js

@@ -981,7 +981,7 @@ describe('Email Submissions Controller', () => {
       const fieldId = new ObjectID()
 
       const attr = 'passportnumber'
-      resLocalFixtures.hashedFields = new Set([attr])
+      resLocalFixtures.hashedFields = new Set([fieldId.toHexString()])
       const responseField = {
         _id: String(fieldId),
         question: 'myinfo',