diff --git a/src/app/controllers/admin-forms.server.controller.js b/src/app/controllers/admin-forms.server.controller.js index 1349d77da5..9c8c8c8e4a 100644 --- a/src/app/controllers/admin-forms.server.controller.js +++ b/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 } diff --git a/src/app/controllers/email-submissions.server.controller.js b/src/app/controllers/email-submissions.server.controller.js index df59150e23..0d34a9a51b 100644 --- a/src/app/controllers/email-submissions.server.controller.js +++ b/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) => { diff --git a/src/app/services/myinfo/__tests__/myinfo.service.spec.ts b/src/app/services/myinfo/__tests__/myinfo.service.spec.ts index 36a900962b..87b5cc7a65 100644 --- a/src/app/services/myinfo/__tests__/myinfo.service.spec.ts +++ b/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 () => { diff --git a/src/app/services/myinfo/__tests__/myinfo.test.constants.ts b/src/app/services/myinfo/__tests__/myinfo.test.constants.ts index 0e56b323b0..a324dd6a7b 100644 --- a/src/app/services/myinfo/__tests__/myinfo.test.constants.ts +++ b/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 }, diff --git a/src/app/services/myinfo/myinfo.factory.ts b/src/app/services/myinfo/myinfo.factory.ts index 2a2bb4f0a3..d47e63574d 100644 --- a/src/app/services/myinfo/myinfo.factory.ts +++ b/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, + Set, HashingError | HashDidNotMatchError | MissingFeatureError > } diff --git a/src/app/services/myinfo/myinfo.service.ts b/src/app/services/myinfo/myinfo.service.ts index 553d9a37f4..19f03edb33 100644 --- a/src/app/services/myinfo/myinfo.service.ts +++ b/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, HashingError | HashDidNotMatchError> { + ): ResultAsync, 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)) }) } } diff --git a/src/app/services/myinfo/myinfo.types.ts b/src/app/services/myinfo/myinfo.types.ts index 24f5a2ed15..ec7c5026ef 100644 --- a/src/app/services/myinfo/myinfo.types.ts +++ b/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> diff --git a/src/app/services/myinfo/myinfo.util.ts b/src/app/services/myinfo/myinfo.util.ts index 53fc002052..37427a30a6 100644 --- a/src/app/services/myinfo/myinfo.util.ts +++ b/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> => { +): MyInfoComparePromises => { // Filter responses to only those fields with a corresponding hash const fieldsWithHashes = filterFieldsWithHashes(responses, hashes) // Map MyInfoAttribute to response - const myInfoResponsesMap = new Map>() + 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 } diff --git a/src/types/express.locals.ts b/src/types/express.locals.ts index 819dfb1fd9..157b9c6ee4 100644 --- a/src/types/express.locals.ts +++ b/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 & { } export type ResWithHashedFields = T & { - locals: { hashedFields?: Set } + locals: { hashedFields?: Set } } diff --git a/tests/unit/backend/controllers/email-submissions.server.controller.spec.js b/tests/unit/backend/controllers/email-submissions.server.controller.spec.js index 1b5316cd94..fa1fa9d438 100644 --- a/tests/unit/backend/controllers/email-submissions.server.controller.spec.js +++ b/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',