From f04b12f7d4a7bb9235ec8f8c1dcfa446fb55a38f Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 16:39:43 +0800 Subject: [PATCH 01/22] feat: MyInfo over sgID backend --- shared/types/converter.ts | 37 ++++++ shared/types/index.ts | 1 + .../public-form/public-form.controller.ts | 7 +- src/app/modules/myinfo/myinfo.adapter.ts | 9 +- src/app/modules/myinfo/myinfo.service.ts | 3 +- .../sgid/__tests__/sgid.service.spec.ts | 49 +++++-- .../sgid/__tests__/sgid.test.constants.ts | 5 +- src/app/modules/sgid/sgid.adapter.ts | 124 ++++++++++++++++++ src/app/modules/sgid/sgid.constants.ts | 13 ++ src/app/modules/sgid/sgid.service.ts | 32 +++-- src/app/modules/sgid/sgid.types.ts | 2 + src/app/modules/sgid/sgid.util.ts | 8 +- 12 files changed, 260 insertions(+), 30 deletions(-) create mode 100644 shared/types/converter.ts create mode 100644 src/app/modules/sgid/sgid.adapter.ts diff --git a/shared/types/converter.ts b/shared/types/converter.ts new file mode 100644 index 0000000000..f4bd8f6aaf --- /dev/null +++ b/shared/types/converter.ts @@ -0,0 +1,37 @@ +export interface SGIDDataTransformer { + + /** + * NRIC/FIN. + */ + getUinFin(): string + + /** + * Formats the sgID information to a string. + */ + _formatFieldValue(attr: T): string | undefined + + /** + * Determine if frontend should lock the field to prevent it from being + * editable. The field is locked if it is government-verified and if it + * does not contain marriage-related information (decision by SNDGO & MSF due to + * overseas unregistered marriages). + * + * @param attr The field/attribute name directly obtained from the sgID + * information source. + * @param fieldValue FormSG field value. + */ + _isDataReadOnly( + attr: T, + fieldValue: string | undefined, + ): boolean + + /** + * Retrieves the fieldValue for the givern internal + * sgID-compatible attribute. + * @param attr Internal FormSG sgID attribute. + */ + getFieldValueForAttr(attr: U): { + fieldValue: string | undefined + isReadOnly: boolean + } +} \ No newline at end of file diff --git a/shared/types/index.ts b/shared/types/index.ts index 7441146342..d2d0f4fc08 100644 --- a/shared/types/index.ts +++ b/shared/types/index.ts @@ -11,3 +11,4 @@ export * from './submission' export * from './user' export * from './utils' export * from './payment' +export * from './converter' diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 3c69c27eef..3ec59e57b7 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -33,6 +33,7 @@ import { extractAuthCode, validateMyInfoForm, } from '../../myinfo/myinfo.util' +import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' @@ -300,11 +301,12 @@ export const handleGetPublicForm: ControllerHandler< } case FormAuthType.SGID: return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map((spcpSession) => { + .map((payload) => { + const data = new SGIDMyInfoData(payload) return res.json({ form: publicForm, isIntranetUser, - spcpSession, + spcpSession: { userName: data.getUinFin() }, }) }) .mapErr((error) => { @@ -403,6 +405,7 @@ export const _handleFormAuthRedirect: ControllerHandler< return SgidService.createRedirectUrl( formId, Boolean(isPersistentLogin), + [], encodedQuery, ) }) diff --git a/src/app/modules/myinfo/myinfo.adapter.ts b/src/app/modules/myinfo/myinfo.adapter.ts index 218f45e185..c028212d48 100644 --- a/src/app/modules/myinfo/myinfo.adapter.ts +++ b/src/app/modules/myinfo/myinfo.adapter.ts @@ -6,7 +6,10 @@ import { MyInfoSource, } from '@opengovsg/myinfo-gov-client' -import { MyInfoAttribute as InternalAttr } from '../../../../shared/types' +import { + MyInfoAttribute as InternalAttr, + SGIDDataTransformer, +} from '../../../../shared/types' import { formatAddress, @@ -155,7 +158,9 @@ export const internalAttrListToScopes = ( * extract the correct data by translating internal FormSG attributes * to the correct keys in the data. */ -export class MyInfoData { +export class MyInfoData + implements SGIDDataTransformer +{ #personData: IPerson #uinFin: string diff --git a/src/app/modules/myinfo/myinfo.service.ts b/src/app/modules/myinfo/myinfo.service.ts index 936b6a9c79..2bd343b000 100644 --- a/src/app/modules/myinfo/myinfo.service.ts +++ b/src/app/modules/myinfo/myinfo.service.ts @@ -27,6 +27,7 @@ import { AuthTypeMismatchError, FormAuthNoEsrvcIdError, } from '../form/form.errors' +import { SGIDMyInfoData } from '../sgid/sgid.adapter' import { ProcessedFieldResponse } from '../submission/submission.types' import { internalAttrListToScopes, MyInfoData } from './myinfo.adapter' @@ -241,7 +242,7 @@ export class MyInfoServiceClass { */ prefillAndSaveMyInfoFields( formId: string, - myInfoData: MyInfoData, + myInfoData: MyInfoData | SGIDMyInfoData, currFormFields: LeanDocument, ): ResultAsync { const prefilledFields = currFormFields.map((field) => { diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index 421b02950e..b2f97a2f27 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -2,7 +2,9 @@ import { SgidClient } from '@opengovsg/sgid-client' import fs from 'fs' import Jwt from 'jsonwebtoken' +import { MyInfoAttribute } from 'shared/types' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' import { SgidCreateRedirectUrlError, SgidFetchAccessTokenError, @@ -12,7 +14,7 @@ import { SgidMissingJwtError, SgidVerifyJwtError, } from '../sgid.errors' -import { SGID_SCOPES, SgidServiceClass } from '../sgid.service' +import { SgidServiceClass } from '../sgid.service' import { MOCK_ACCESS_TOKEN, @@ -29,6 +31,9 @@ import { MOCK_USER_INFO, } from './sgid.test.constants' +const SGID_DEFAULT_SCOPES = 'openid myinfo.nric_number' +const SGID_DEFAULT_ATTR_LIST: MyInfoAttribute[] = [] + jest.mock('@opengovsg/sgid-client') const MockSgidClient = jest.mocked(SgidClient) jest.mock('jsonwebtoken') @@ -68,14 +73,36 @@ describe('sgid.service', () => { const url = SgidService.createRedirectUrl( MOCK_DESTINATION, MOCK_REMEMBER_ME, + SGID_DEFAULT_ATTR_LIST, ) expect(url._unsafeUnwrap()).toEqual(MOCK_REDIRECT_URL) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_SCOPES, + SGID_DEFAULT_SCOPES, null, ) }) + + it('should return extract additional OAuth scopes from MyInfo', () => { + const SgidService = new SgidServiceClass(MOCK_OPTIONS) + const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) + sgidClient.authorizationUrl.mockReturnValue({ + url: MOCK_REDIRECT_URL, + nonce: MOCK_NONCE, + }) + const url = SgidService.createRedirectUrl( + MOCK_DESTINATION, + MOCK_REMEMBER_ME, + [MyInfoAttribute.RegisteredAddress, MyInfoAttribute.PassportExpiryDate], + ) + expect(url._unsafeUnwrap()).toEqual(MOCK_REDIRECT_URL) + expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( + MOCK_STATE, + 'openid myinfo.nric_number myinfo.registered_address myinfo.passport_expiry_date', + null, + ) + }) + it('should return error if not ok', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) @@ -87,11 +114,12 @@ describe('sgid.service', () => { const url = SgidService.createRedirectUrl( MOCK_DESTINATION, MOCK_REMEMBER_ME, + SGID_DEFAULT_ATTR_LIST, ) expect(url._unsafeUnwrapErr()).toBeInstanceOf(SgidCreateRedirectUrlError) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_SCOPES, + SGID_DEFAULT_SCOPES, null, ) }) @@ -135,17 +163,18 @@ describe('sgid.service', () => { it('should return the userinfo given the code', async () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) - sgidClient.userinfo.mockResolvedValue({ + const mockUserInfoWithAdditional = { sub: MOCK_USER_INFO.sub, data: { ...MOCK_USER_INFO.data, - 'myinfo.name': 'not supposed to be here', + 'myinfo.name': 'supposed to be here', }, - }) + } + sgidClient.userinfo.mockResolvedValue(mockUserInfoWithAdditional) const result = await SgidService.retrieveUserInfo({ accessToken: MOCK_ACCESS_TOKEN, }) - expect(result._unsafeUnwrap()).toStrictEqual(MOCK_USER_INFO) + expect(result._unsafeUnwrap()).toStrictEqual(mockUserInfoWithAdditional) expect(sgidClient.userinfo).toHaveBeenCalledWith(MOCK_ACCESS_TOKEN) }) it('should return error on error', async () => { @@ -171,6 +200,7 @@ describe('sgid.service', () => { }) expect(MockJwt.sign).toHaveBeenCalledWith( { + data: { 'myinfo.nric_number': 'S9322889A' }, userName: MOCK_USER_INFO.data['myinfo.nric_number'], rememberMe: false, }, @@ -193,7 +223,8 @@ describe('sgid.service', () => { }) expect(MockJwt.sign).toHaveBeenCalledWith( { - userName: MOCK_USER_INFO.data['myinfo.nric_number'], + data: { 'myinfo.nric_number': 'S9322889A' }, + userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], rememberMe: true, }, MOCK_OPTIONS.privateKeyPath, @@ -210,7 +241,7 @@ describe('sgid.service', () => { // @ts-ignore MockJwt.verify.mockReturnValue(MOCK_JWT_PAYLOAD) const result = SgidService.extractSgidJwtPayload(MOCK_JWT) - expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD) + expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD.data) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, MOCK_OPTIONS.publicKeyPath, diff --git a/src/app/modules/sgid/__tests__/sgid.test.constants.ts b/src/app/modules/sgid/__tests__/sgid.test.constants.ts index 814ca93757..1deec0a0d0 100644 --- a/src/app/modules/sgid/__tests__/sgid.test.constants.ts +++ b/src/app/modules/sgid/__tests__/sgid.test.constants.ts @@ -4,6 +4,7 @@ import _ from 'lodash' import { IPopulatedForm } from 'src/types' import { MOCK_COOKIE_AGE } from '../../myinfo/__tests__/myinfo.test.constants' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' export const MOCK_TARGET = new ObjectId().toHexString() export const MOCK_DESTINATION = `/${MOCK_TARGET}` @@ -32,7 +33,8 @@ export const MOCK_USER_INFO = { } export const MOCK_JWT_PAYLOAD = { - userName: MOCK_USER_INFO.data['myinfo.nric_number'], + data: { 'myinfo.nric_number': 'S9322889A' }, + userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], } export const MOCK_SGID_FORM = { @@ -70,6 +72,7 @@ export const MOCK_OPTIONS = { clientSecret: 'client-secret', privateKeyPath: 'private-key', publicKeyPath: 'public-key', + hostname: undefined, redirectUri: MOCK_REDIRECT_URL, cookieDomain: MOCK_COOKIE_SETTINGS.domain, cookieMaxAge: MOCK_COOKIE_AGE, diff --git a/src/app/modules/sgid/sgid.adapter.ts b/src/app/modules/sgid/sgid.adapter.ts new file mode 100644 index 0000000000..bd55255b77 --- /dev/null +++ b/src/app/modules/sgid/sgid.adapter.ts @@ -0,0 +1,124 @@ +import { + MyInfoAttribute as InternalAttr, + SGIDDataTransformer, +} from '../../../../shared/types' + +import { + SGID_MYINFO_NRIC_NUMBER_SCOPE, + SGIDScope as ExternalAttr, +} from './sgid.constants' +import { SGIDScopeToValue } from './sgid.types' + +export const internalAttrToScope = (attr: InternalAttr): ExternalAttr => { + switch (attr) { + case InternalAttr.Name: + return ExternalAttr.Name + case InternalAttr.DateOfBirth: + return ExternalAttr.DateOfBirth + case InternalAttr.PassportNumber: + return ExternalAttr.PassportNumber + case InternalAttr.PassportExpiryDate: + return ExternalAttr.PassportExpiryDate + case InternalAttr.MobileNo: + return ExternalAttr.MobileNumber + case InternalAttr.RegisteredAddress: + return ExternalAttr.RegisteredAddress + default: + // This should be removed once sgID reaches parity with MyInfo. + return new UnreachableCaseError(attr) + } +} + +export const internalAttrListToScopes = (attrs: InternalAttr[]): string => + // Always ask for NRIC + `openid ${ExternalAttr.NricFin} ${attrs + .map(internalAttrToScope) + .join(' ')}`.trim() + +const internalAttrToSGIDExternal = (attr: InternalAttr): ExternalAttr => { + switch (attr) { + case InternalAttr.Name: + return ExternalAttr.Name + case InternalAttr.DateOfBirth: + return ExternalAttr.DateOfBirth + case InternalAttr.PassportNumber: + return ExternalAttr.PassportNumber + case InternalAttr.PassportExpiryDate: + return ExternalAttr.PassportExpiryDate + case InternalAttr.MobileNo: + return ExternalAttr.MobileNumber + case InternalAttr.RegisteredAddress: + return ExternalAttr.RegisteredAddress + default: + return new UnreachableCaseError(attr) + } +} + +/** + * Wrapper class for SGIDData. + * + * Currently only supports MyInfo. + */ +export class SGIDMyInfoData + implements SGIDDataTransformer +{ + #payload: SGIDScopeToValue + constructor(payload: SGIDScopeToValue) { + this.#payload = payload + } + + getUinFin(): string { + return this.#payload[SGID_MYINFO_NRIC_NUMBER_SCOPE] + } + + /** + * Placeholder. For now, there are not enough public fields in + * sgID's MyInfo catalog to require significant formatting. + * @param attr sgID's MyInfo OAuth scope. + * @returns the formatted field. + */ + _formatFieldValue(attr: ExternalAttr): string | undefined { + return this.#payload[attr] + } + + /** + * Refer to the myInfo data catalogue to see which fields should be read-only + * and which fields should be editable by the user. + * @param attr sgID MyInfo OAuth scope. + * @param fieldValue FormSG field value. + * @returns Whether the data/field should be readonly. + */ + _isDataReadOnly(attr: ExternalAttr, fieldValue: string | undefined): boolean { + const data = this.#payload[attr] + if (!data || !fieldValue) return false + + switch (attr) { + case ExternalAttr.MobileNumber: + case ExternalAttr.RegisteredAddress: + case ExternalAttr.Name: + case ExternalAttr.PassportNumber: + case ExternalAttr.DateOfBirth: + case ExternalAttr.PassportExpiryDate: + return !!data + // Fall back to leaving field editable as data shape is unknown. + default: + return false + } + } + + /** + * Retrieves the fieldValue for the given internal MyInfo attribute. + * @param attr Internal FormSG MyInfo attribute + */ + getFieldValueForAttr(attr: InternalAttr): { + fieldValue: string | undefined + isReadOnly: boolean + } { + const externalAttr = internalAttrToSGIDExternal(attr) + const fieldValue = this._formatFieldValue(externalAttr) + return { + fieldValue, + isReadOnly: true, + } + } +} diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index 69545f5d9a..cce0e2f6a7 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -3,3 +3,16 @@ * token if login was successful. */ export const SGID_COOKIE_NAME = 'jwtSgid' + +export const SGID_MYINFO_NRIC_NUMBER_SCOPE = 'myinfo.nric_number' + +export enum SGIDScope { + Name = 'myinfo.name', + NricFin = 'myinfo.nric_number', + DateOfBirth = 'myinfo.date_of_birth', + PassportNumber = 'myinfo.passport_number', + PassportExpiryDate = 'myinfo.passport_expiry_date', + MobileNumber = 'myinfo.mobile_number', + Email = 'myinfo.email', + RegisteredAddress = 'myinfo.registered_address', +} diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 406c4397ec..242adb1ebf 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -3,11 +3,14 @@ import fs from 'fs' import Jwt from 'jsonwebtoken' import { err, ok, Result, ResultAsync } from 'neverthrow' +import { MyInfoAttribute as InternalAttr } from '../../../../shared/types' import { ISgidVarsSchema } from '../../../types' import { sgid } from '../../config/features/sgid.config' import { createLoggerWithLabel } from '../../config/logger' import { ApplicationError } from '../core/core.errors' +import { internalAttrListToScopes } from './sgid.adapter' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from './sgid.constants' import { SgidCreateRedirectUrlError, SgidFetchAccessTokenError, @@ -17,12 +20,12 @@ import { SgidMissingJwtError, SgidVerifyJwtError, } from './sgid.errors' +import { SGIDScopeToValue } from './sgid.types' import { isSgidJwtPayload } from './sgid.util' const logger = createLoggerWithLabel(module) const JWT_ALGORITHM = 'RS256' -export const SGID_SCOPES = 'openid myinfo.nric_number' export class SgidServiceClass { private client: SgidClient @@ -60,12 +63,14 @@ export class SgidServiceClass { * Create a URL to sgID which is used to redirect the user for authentication * @param formId - the form id to redirect to after authentication * @param rememberMe - whether we create a JWT that remembers the user + * @param requestedAttributes - sgID attributes requested by this form * @param encodedQuery base64 encoded queryId for frontend to retrieve stored query params (usually contains prefilled form information) * for an extended period of time */ createRedirectUrl( formId: string, rememberMe: boolean, + requestedAttributes: InternalAttr[], encodedQuery?: string, ): Result { const state = encodedQuery @@ -75,7 +80,8 @@ export class SgidServiceClass { action: 'createRedirectUrl', state, } - const result = this.client.authorizationUrl(state, SGID_SCOPES, null) + const scopes = internalAttrListToScopes(requestedAttributes) + const result = this.client.authorizationUrl(state, scopes, null) if (typeof result.url === 'string') { return ok(result.url) } else { @@ -167,14 +173,16 @@ export class SgidServiceClass { }: { accessToken: string }): ResultAsync< - { sub: string; data: { 'myinfo.nric_number': string } }, + { sub: string; data: SGIDScopeToValue }, SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => ({ - sub, - data: { 'myinfo.nric_number': data['myinfo.nric_number'] }, - })), + this.client.userinfo(accessToken).then(({ sub, data }) => { + return { + sub, + data, + } + }), (error) => { logger.error({ message: 'Failed to retrieve user info from sgID', @@ -195,11 +203,11 @@ export class SgidServiceClass { * @param rememberMe - determines how long the JWT is valid for */ createJwt( - data: { 'myinfo.nric_number': string }, + data: SGIDScopeToValue, rememberMe: boolean, ): Result<{ jwt: string; maxAge: number }, ApplicationError> { - const userName = data['myinfo.nric_number'] - const payload = { userName, rememberMe } + const userName = data[SGID_MYINFO_NRIC_NUMBER_SCOPE] + const payload = { userName, data, rememberMe } const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { algorithm: JWT_ALGORITHM, @@ -218,7 +226,7 @@ export class SgidServiceClass { extractSgidJwtPayload( jwtSgid: string, ): Result< - { userName: string }, + SGIDScopeToValue, SgidVerifyJwtError | SgidInvalidJwtError | SgidMissingJwtError > { const logMeta = { @@ -234,7 +242,7 @@ export class SgidServiceClass { }) if (isSgidJwtPayload(payload)) { - return ok(payload) + return ok(payload['data']) } const payloadIsDefined = !!payload diff --git a/src/app/modules/sgid/sgid.types.ts b/src/app/modules/sgid/sgid.types.ts index eb760b6506..e1ebe12b58 100644 --- a/src/app/modules/sgid/sgid.types.ts +++ b/src/app/modules/sgid/sgid.types.ts @@ -4,3 +4,5 @@ import { IFormSchema } from '../../../types' export type SgidForm = T & { authType: FormAuthType.SGID } + +export type SGIDScopeToValue = Record diff --git a/src/app/modules/sgid/sgid.util.ts b/src/app/modules/sgid/sgid.util.ts index 40dc76dd49..93287c5619 100644 --- a/src/app/modules/sgid/sgid.util.ts +++ b/src/app/modules/sgid/sgid.util.ts @@ -10,7 +10,7 @@ import { IFormSchema, SgidFieldTitle } from '../../../types' import { AuthTypeMismatchError } from '../form/form.errors' import { ProcessedSingleAnswerResponse } from '../submission/submission.types' -import { SgidForm } from './sgid.types' +import { SgidForm, SGIDScopeToValue } from './sgid.types' /** * Validates that a form is an sgID form @@ -53,11 +53,13 @@ export const createSgidParsedResponses = ( */ export const isSgidJwtPayload = ( payload: unknown, -): payload is { userName: string } => { +): payload is Record => { return ( typeof payload === 'object' && !!payload && hasProp(payload, 'userName') && - typeof payload.userName === 'string' + typeof payload.userName === 'string' && + hasProp(payload, 'data') && + typeof payload.data === 'object' ) } From efb3c540badbe4d2ea8d6b76f370b450204347a2 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 16:56:05 +0800 Subject: [PATCH 02/22] refactor: reduce diff --- src/app/modules/sgid/sgid.service.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 242adb1ebf..616dc0bad3 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -177,12 +177,10 @@ export class SgidServiceClass { SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => { - return { - sub, - data, - } - }), + this.client.userinfo(accessToken).then(({ sub, data }) => ({ + sub, + data, + })), (error) => { logger.error({ message: 'Failed to retrieve user info from sgID', From bf34bebd38d39e708aba10b6a39bd33ae306c631 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 16:39:43 +0800 Subject: [PATCH 03/22] feat: MyInfo over sgID backend --- shared/types/converter.ts | 37 ++++++ shared/types/index.ts | 1 + .../public-form/public-form.controller.ts | 7 +- src/app/modules/myinfo/myinfo.adapter.ts | 9 +- src/app/modules/myinfo/myinfo.service.ts | 3 +- .../sgid/__tests__/sgid.service.spec.ts | 49 +++++-- .../sgid/__tests__/sgid.test.constants.ts | 5 +- src/app/modules/sgid/sgid.adapter.ts | 124 ++++++++++++++++++ src/app/modules/sgid/sgid.constants.ts | 13 ++ src/app/modules/sgid/sgid.service.ts | 32 +++-- src/app/modules/sgid/sgid.types.ts | 2 + src/app/modules/sgid/sgid.util.ts | 8 +- 12 files changed, 260 insertions(+), 30 deletions(-) create mode 100644 shared/types/converter.ts create mode 100644 src/app/modules/sgid/sgid.adapter.ts diff --git a/shared/types/converter.ts b/shared/types/converter.ts new file mode 100644 index 0000000000..f4bd8f6aaf --- /dev/null +++ b/shared/types/converter.ts @@ -0,0 +1,37 @@ +export interface SGIDDataTransformer { + + /** + * NRIC/FIN. + */ + getUinFin(): string + + /** + * Formats the sgID information to a string. + */ + _formatFieldValue(attr: T): string | undefined + + /** + * Determine if frontend should lock the field to prevent it from being + * editable. The field is locked if it is government-verified and if it + * does not contain marriage-related information (decision by SNDGO & MSF due to + * overseas unregistered marriages). + * + * @param attr The field/attribute name directly obtained from the sgID + * information source. + * @param fieldValue FormSG field value. + */ + _isDataReadOnly( + attr: T, + fieldValue: string | undefined, + ): boolean + + /** + * Retrieves the fieldValue for the givern internal + * sgID-compatible attribute. + * @param attr Internal FormSG sgID attribute. + */ + getFieldValueForAttr(attr: U): { + fieldValue: string | undefined + isReadOnly: boolean + } +} \ No newline at end of file diff --git a/shared/types/index.ts b/shared/types/index.ts index 7441146342..d2d0f4fc08 100644 --- a/shared/types/index.ts +++ b/shared/types/index.ts @@ -11,3 +11,4 @@ export * from './submission' export * from './user' export * from './utils' export * from './payment' +export * from './converter' diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 3c69c27eef..3ec59e57b7 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -33,6 +33,7 @@ import { extractAuthCode, validateMyInfoForm, } from '../../myinfo/myinfo.util' +import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' @@ -300,11 +301,12 @@ export const handleGetPublicForm: ControllerHandler< } case FormAuthType.SGID: return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map((spcpSession) => { + .map((payload) => { + const data = new SGIDMyInfoData(payload) return res.json({ form: publicForm, isIntranetUser, - spcpSession, + spcpSession: { userName: data.getUinFin() }, }) }) .mapErr((error) => { @@ -403,6 +405,7 @@ export const _handleFormAuthRedirect: ControllerHandler< return SgidService.createRedirectUrl( formId, Boolean(isPersistentLogin), + [], encodedQuery, ) }) diff --git a/src/app/modules/myinfo/myinfo.adapter.ts b/src/app/modules/myinfo/myinfo.adapter.ts index 218f45e185..c028212d48 100644 --- a/src/app/modules/myinfo/myinfo.adapter.ts +++ b/src/app/modules/myinfo/myinfo.adapter.ts @@ -6,7 +6,10 @@ import { MyInfoSource, } from '@opengovsg/myinfo-gov-client' -import { MyInfoAttribute as InternalAttr } from '../../../../shared/types' +import { + MyInfoAttribute as InternalAttr, + SGIDDataTransformer, +} from '../../../../shared/types' import { formatAddress, @@ -155,7 +158,9 @@ export const internalAttrListToScopes = ( * extract the correct data by translating internal FormSG attributes * to the correct keys in the data. */ -export class MyInfoData { +export class MyInfoData + implements SGIDDataTransformer +{ #personData: IPerson #uinFin: string diff --git a/src/app/modules/myinfo/myinfo.service.ts b/src/app/modules/myinfo/myinfo.service.ts index 936b6a9c79..2bd343b000 100644 --- a/src/app/modules/myinfo/myinfo.service.ts +++ b/src/app/modules/myinfo/myinfo.service.ts @@ -27,6 +27,7 @@ import { AuthTypeMismatchError, FormAuthNoEsrvcIdError, } from '../form/form.errors' +import { SGIDMyInfoData } from '../sgid/sgid.adapter' import { ProcessedFieldResponse } from '../submission/submission.types' import { internalAttrListToScopes, MyInfoData } from './myinfo.adapter' @@ -241,7 +242,7 @@ export class MyInfoServiceClass { */ prefillAndSaveMyInfoFields( formId: string, - myInfoData: MyInfoData, + myInfoData: MyInfoData | SGIDMyInfoData, currFormFields: LeanDocument, ): ResultAsync { const prefilledFields = currFormFields.map((field) => { diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index 421b02950e..b2f97a2f27 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -2,7 +2,9 @@ import { SgidClient } from '@opengovsg/sgid-client' import fs from 'fs' import Jwt from 'jsonwebtoken' +import { MyInfoAttribute } from 'shared/types' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' import { SgidCreateRedirectUrlError, SgidFetchAccessTokenError, @@ -12,7 +14,7 @@ import { SgidMissingJwtError, SgidVerifyJwtError, } from '../sgid.errors' -import { SGID_SCOPES, SgidServiceClass } from '../sgid.service' +import { SgidServiceClass } from '../sgid.service' import { MOCK_ACCESS_TOKEN, @@ -29,6 +31,9 @@ import { MOCK_USER_INFO, } from './sgid.test.constants' +const SGID_DEFAULT_SCOPES = 'openid myinfo.nric_number' +const SGID_DEFAULT_ATTR_LIST: MyInfoAttribute[] = [] + jest.mock('@opengovsg/sgid-client') const MockSgidClient = jest.mocked(SgidClient) jest.mock('jsonwebtoken') @@ -68,14 +73,36 @@ describe('sgid.service', () => { const url = SgidService.createRedirectUrl( MOCK_DESTINATION, MOCK_REMEMBER_ME, + SGID_DEFAULT_ATTR_LIST, ) expect(url._unsafeUnwrap()).toEqual(MOCK_REDIRECT_URL) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_SCOPES, + SGID_DEFAULT_SCOPES, null, ) }) + + it('should return extract additional OAuth scopes from MyInfo', () => { + const SgidService = new SgidServiceClass(MOCK_OPTIONS) + const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) + sgidClient.authorizationUrl.mockReturnValue({ + url: MOCK_REDIRECT_URL, + nonce: MOCK_NONCE, + }) + const url = SgidService.createRedirectUrl( + MOCK_DESTINATION, + MOCK_REMEMBER_ME, + [MyInfoAttribute.RegisteredAddress, MyInfoAttribute.PassportExpiryDate], + ) + expect(url._unsafeUnwrap()).toEqual(MOCK_REDIRECT_URL) + expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( + MOCK_STATE, + 'openid myinfo.nric_number myinfo.registered_address myinfo.passport_expiry_date', + null, + ) + }) + it('should return error if not ok', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) @@ -87,11 +114,12 @@ describe('sgid.service', () => { const url = SgidService.createRedirectUrl( MOCK_DESTINATION, MOCK_REMEMBER_ME, + SGID_DEFAULT_ATTR_LIST, ) expect(url._unsafeUnwrapErr()).toBeInstanceOf(SgidCreateRedirectUrlError) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_SCOPES, + SGID_DEFAULT_SCOPES, null, ) }) @@ -135,17 +163,18 @@ describe('sgid.service', () => { it('should return the userinfo given the code', async () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) - sgidClient.userinfo.mockResolvedValue({ + const mockUserInfoWithAdditional = { sub: MOCK_USER_INFO.sub, data: { ...MOCK_USER_INFO.data, - 'myinfo.name': 'not supposed to be here', + 'myinfo.name': 'supposed to be here', }, - }) + } + sgidClient.userinfo.mockResolvedValue(mockUserInfoWithAdditional) const result = await SgidService.retrieveUserInfo({ accessToken: MOCK_ACCESS_TOKEN, }) - expect(result._unsafeUnwrap()).toStrictEqual(MOCK_USER_INFO) + expect(result._unsafeUnwrap()).toStrictEqual(mockUserInfoWithAdditional) expect(sgidClient.userinfo).toHaveBeenCalledWith(MOCK_ACCESS_TOKEN) }) it('should return error on error', async () => { @@ -171,6 +200,7 @@ describe('sgid.service', () => { }) expect(MockJwt.sign).toHaveBeenCalledWith( { + data: { 'myinfo.nric_number': 'S9322889A' }, userName: MOCK_USER_INFO.data['myinfo.nric_number'], rememberMe: false, }, @@ -193,7 +223,8 @@ describe('sgid.service', () => { }) expect(MockJwt.sign).toHaveBeenCalledWith( { - userName: MOCK_USER_INFO.data['myinfo.nric_number'], + data: { 'myinfo.nric_number': 'S9322889A' }, + userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], rememberMe: true, }, MOCK_OPTIONS.privateKeyPath, @@ -210,7 +241,7 @@ describe('sgid.service', () => { // @ts-ignore MockJwt.verify.mockReturnValue(MOCK_JWT_PAYLOAD) const result = SgidService.extractSgidJwtPayload(MOCK_JWT) - expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD) + expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD.data) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, MOCK_OPTIONS.publicKeyPath, diff --git a/src/app/modules/sgid/__tests__/sgid.test.constants.ts b/src/app/modules/sgid/__tests__/sgid.test.constants.ts index 814ca93757..1deec0a0d0 100644 --- a/src/app/modules/sgid/__tests__/sgid.test.constants.ts +++ b/src/app/modules/sgid/__tests__/sgid.test.constants.ts @@ -4,6 +4,7 @@ import _ from 'lodash' import { IPopulatedForm } from 'src/types' import { MOCK_COOKIE_AGE } from '../../myinfo/__tests__/myinfo.test.constants' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' export const MOCK_TARGET = new ObjectId().toHexString() export const MOCK_DESTINATION = `/${MOCK_TARGET}` @@ -32,7 +33,8 @@ export const MOCK_USER_INFO = { } export const MOCK_JWT_PAYLOAD = { - userName: MOCK_USER_INFO.data['myinfo.nric_number'], + data: { 'myinfo.nric_number': 'S9322889A' }, + userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], } export const MOCK_SGID_FORM = { @@ -70,6 +72,7 @@ export const MOCK_OPTIONS = { clientSecret: 'client-secret', privateKeyPath: 'private-key', publicKeyPath: 'public-key', + hostname: undefined, redirectUri: MOCK_REDIRECT_URL, cookieDomain: MOCK_COOKIE_SETTINGS.domain, cookieMaxAge: MOCK_COOKIE_AGE, diff --git a/src/app/modules/sgid/sgid.adapter.ts b/src/app/modules/sgid/sgid.adapter.ts new file mode 100644 index 0000000000..bd55255b77 --- /dev/null +++ b/src/app/modules/sgid/sgid.adapter.ts @@ -0,0 +1,124 @@ +import { + MyInfoAttribute as InternalAttr, + SGIDDataTransformer, +} from '../../../../shared/types' + +import { + SGID_MYINFO_NRIC_NUMBER_SCOPE, + SGIDScope as ExternalAttr, +} from './sgid.constants' +import { SGIDScopeToValue } from './sgid.types' + +export const internalAttrToScope = (attr: InternalAttr): ExternalAttr => { + switch (attr) { + case InternalAttr.Name: + return ExternalAttr.Name + case InternalAttr.DateOfBirth: + return ExternalAttr.DateOfBirth + case InternalAttr.PassportNumber: + return ExternalAttr.PassportNumber + case InternalAttr.PassportExpiryDate: + return ExternalAttr.PassportExpiryDate + case InternalAttr.MobileNo: + return ExternalAttr.MobileNumber + case InternalAttr.RegisteredAddress: + return ExternalAttr.RegisteredAddress + default: + // This should be removed once sgID reaches parity with MyInfo. + return new UnreachableCaseError(attr) + } +} + +export const internalAttrListToScopes = (attrs: InternalAttr[]): string => + // Always ask for NRIC + `openid ${ExternalAttr.NricFin} ${attrs + .map(internalAttrToScope) + .join(' ')}`.trim() + +const internalAttrToSGIDExternal = (attr: InternalAttr): ExternalAttr => { + switch (attr) { + case InternalAttr.Name: + return ExternalAttr.Name + case InternalAttr.DateOfBirth: + return ExternalAttr.DateOfBirth + case InternalAttr.PassportNumber: + return ExternalAttr.PassportNumber + case InternalAttr.PassportExpiryDate: + return ExternalAttr.PassportExpiryDate + case InternalAttr.MobileNo: + return ExternalAttr.MobileNumber + case InternalAttr.RegisteredAddress: + return ExternalAttr.RegisteredAddress + default: + return new UnreachableCaseError(attr) + } +} + +/** + * Wrapper class for SGIDData. + * + * Currently only supports MyInfo. + */ +export class SGIDMyInfoData + implements SGIDDataTransformer +{ + #payload: SGIDScopeToValue + constructor(payload: SGIDScopeToValue) { + this.#payload = payload + } + + getUinFin(): string { + return this.#payload[SGID_MYINFO_NRIC_NUMBER_SCOPE] + } + + /** + * Placeholder. For now, there are not enough public fields in + * sgID's MyInfo catalog to require significant formatting. + * @param attr sgID's MyInfo OAuth scope. + * @returns the formatted field. + */ + _formatFieldValue(attr: ExternalAttr): string | undefined { + return this.#payload[attr] + } + + /** + * Refer to the myInfo data catalogue to see which fields should be read-only + * and which fields should be editable by the user. + * @param attr sgID MyInfo OAuth scope. + * @param fieldValue FormSG field value. + * @returns Whether the data/field should be readonly. + */ + _isDataReadOnly(attr: ExternalAttr, fieldValue: string | undefined): boolean { + const data = this.#payload[attr] + if (!data || !fieldValue) return false + + switch (attr) { + case ExternalAttr.MobileNumber: + case ExternalAttr.RegisteredAddress: + case ExternalAttr.Name: + case ExternalAttr.PassportNumber: + case ExternalAttr.DateOfBirth: + case ExternalAttr.PassportExpiryDate: + return !!data + // Fall back to leaving field editable as data shape is unknown. + default: + return false + } + } + + /** + * Retrieves the fieldValue for the given internal MyInfo attribute. + * @param attr Internal FormSG MyInfo attribute + */ + getFieldValueForAttr(attr: InternalAttr): { + fieldValue: string | undefined + isReadOnly: boolean + } { + const externalAttr = internalAttrToSGIDExternal(attr) + const fieldValue = this._formatFieldValue(externalAttr) + return { + fieldValue, + isReadOnly: true, + } + } +} diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index 69545f5d9a..cce0e2f6a7 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -3,3 +3,16 @@ * token if login was successful. */ export const SGID_COOKIE_NAME = 'jwtSgid' + +export const SGID_MYINFO_NRIC_NUMBER_SCOPE = 'myinfo.nric_number' + +export enum SGIDScope { + Name = 'myinfo.name', + NricFin = 'myinfo.nric_number', + DateOfBirth = 'myinfo.date_of_birth', + PassportNumber = 'myinfo.passport_number', + PassportExpiryDate = 'myinfo.passport_expiry_date', + MobileNumber = 'myinfo.mobile_number', + Email = 'myinfo.email', + RegisteredAddress = 'myinfo.registered_address', +} diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 406c4397ec..242adb1ebf 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -3,11 +3,14 @@ import fs from 'fs' import Jwt from 'jsonwebtoken' import { err, ok, Result, ResultAsync } from 'neverthrow' +import { MyInfoAttribute as InternalAttr } from '../../../../shared/types' import { ISgidVarsSchema } from '../../../types' import { sgid } from '../../config/features/sgid.config' import { createLoggerWithLabel } from '../../config/logger' import { ApplicationError } from '../core/core.errors' +import { internalAttrListToScopes } from './sgid.adapter' +import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from './sgid.constants' import { SgidCreateRedirectUrlError, SgidFetchAccessTokenError, @@ -17,12 +20,12 @@ import { SgidMissingJwtError, SgidVerifyJwtError, } from './sgid.errors' +import { SGIDScopeToValue } from './sgid.types' import { isSgidJwtPayload } from './sgid.util' const logger = createLoggerWithLabel(module) const JWT_ALGORITHM = 'RS256' -export const SGID_SCOPES = 'openid myinfo.nric_number' export class SgidServiceClass { private client: SgidClient @@ -60,12 +63,14 @@ export class SgidServiceClass { * Create a URL to sgID which is used to redirect the user for authentication * @param formId - the form id to redirect to after authentication * @param rememberMe - whether we create a JWT that remembers the user + * @param requestedAttributes - sgID attributes requested by this form * @param encodedQuery base64 encoded queryId for frontend to retrieve stored query params (usually contains prefilled form information) * for an extended period of time */ createRedirectUrl( formId: string, rememberMe: boolean, + requestedAttributes: InternalAttr[], encodedQuery?: string, ): Result { const state = encodedQuery @@ -75,7 +80,8 @@ export class SgidServiceClass { action: 'createRedirectUrl', state, } - const result = this.client.authorizationUrl(state, SGID_SCOPES, null) + const scopes = internalAttrListToScopes(requestedAttributes) + const result = this.client.authorizationUrl(state, scopes, null) if (typeof result.url === 'string') { return ok(result.url) } else { @@ -167,14 +173,16 @@ export class SgidServiceClass { }: { accessToken: string }): ResultAsync< - { sub: string; data: { 'myinfo.nric_number': string } }, + { sub: string; data: SGIDScopeToValue }, SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => ({ - sub, - data: { 'myinfo.nric_number': data['myinfo.nric_number'] }, - })), + this.client.userinfo(accessToken).then(({ sub, data }) => { + return { + sub, + data, + } + }), (error) => { logger.error({ message: 'Failed to retrieve user info from sgID', @@ -195,11 +203,11 @@ export class SgidServiceClass { * @param rememberMe - determines how long the JWT is valid for */ createJwt( - data: { 'myinfo.nric_number': string }, + data: SGIDScopeToValue, rememberMe: boolean, ): Result<{ jwt: string; maxAge: number }, ApplicationError> { - const userName = data['myinfo.nric_number'] - const payload = { userName, rememberMe } + const userName = data[SGID_MYINFO_NRIC_NUMBER_SCOPE] + const payload = { userName, data, rememberMe } const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { algorithm: JWT_ALGORITHM, @@ -218,7 +226,7 @@ export class SgidServiceClass { extractSgidJwtPayload( jwtSgid: string, ): Result< - { userName: string }, + SGIDScopeToValue, SgidVerifyJwtError | SgidInvalidJwtError | SgidMissingJwtError > { const logMeta = { @@ -234,7 +242,7 @@ export class SgidServiceClass { }) if (isSgidJwtPayload(payload)) { - return ok(payload) + return ok(payload['data']) } const payloadIsDefined = !!payload diff --git a/src/app/modules/sgid/sgid.types.ts b/src/app/modules/sgid/sgid.types.ts index eb760b6506..e1ebe12b58 100644 --- a/src/app/modules/sgid/sgid.types.ts +++ b/src/app/modules/sgid/sgid.types.ts @@ -4,3 +4,5 @@ import { IFormSchema } from '../../../types' export type SgidForm = T & { authType: FormAuthType.SGID } + +export type SGIDScopeToValue = Record diff --git a/src/app/modules/sgid/sgid.util.ts b/src/app/modules/sgid/sgid.util.ts index 40dc76dd49..93287c5619 100644 --- a/src/app/modules/sgid/sgid.util.ts +++ b/src/app/modules/sgid/sgid.util.ts @@ -10,7 +10,7 @@ import { IFormSchema, SgidFieldTitle } from '../../../types' import { AuthTypeMismatchError } from '../form/form.errors' import { ProcessedSingleAnswerResponse } from '../submission/submission.types' -import { SgidForm } from './sgid.types' +import { SgidForm, SGIDScopeToValue } from './sgid.types' /** * Validates that a form is an sgID form @@ -53,11 +53,13 @@ export const createSgidParsedResponses = ( */ export const isSgidJwtPayload = ( payload: unknown, -): payload is { userName: string } => { +): payload is Record => { return ( typeof payload === 'object' && !!payload && hasProp(payload, 'userName') && - typeof payload.userName === 'string' + typeof payload.userName === 'string' && + hasProp(payload, 'data') && + typeof payload.data === 'object' ) } From 4d6c5eb6cb5da6b765a2bb3943fa220b6e0bed41 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 16:56:05 +0800 Subject: [PATCH 04/22] refactor: reduce diff --- src/app/modules/sgid/sgid.service.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 242adb1ebf..616dc0bad3 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -177,12 +177,10 @@ export class SgidServiceClass { SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => { - return { - sub, - data, - } - }), + this.client.userinfo(accessToken).then(({ sub, data }) => ({ + sub, + data, + })), (error) => { logger.error({ message: 'Failed to retrieve user info from sgID', From d0e1d3c1debcca6ddf49799016a1ba0af05f975f Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 17:27:56 +0800 Subject: [PATCH 05/22] fix: return undefined in default cases --- src/app/modules/sgid/sgid.adapter.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/app/modules/sgid/sgid.adapter.ts b/src/app/modules/sgid/sgid.adapter.ts index bd55255b77..6664f4daac 100644 --- a/src/app/modules/sgid/sgid.adapter.ts +++ b/src/app/modules/sgid/sgid.adapter.ts @@ -25,7 +25,9 @@ export const internalAttrToScope = (attr: InternalAttr): ExternalAttr => { return ExternalAttr.RegisteredAddress default: // This should be removed once sgID reaches parity with MyInfo. - return new UnreachableCaseError(attr) + // For now, the returned value will be automatically filtered + // out by other functions. + return ExternalAttr.NricFin } } @@ -35,7 +37,9 @@ export const internalAttrListToScopes = (attrs: InternalAttr[]): string => .map(internalAttrToScope) .join(' ')}`.trim() -const internalAttrToSGIDExternal = (attr: InternalAttr): ExternalAttr => { +const internalAttrToSGIDExternal = ( + attr: InternalAttr, +): ExternalAttr | undefined => { switch (attr) { case InternalAttr.Name: return ExternalAttr.Name @@ -50,7 +54,7 @@ const internalAttrToSGIDExternal = (attr: InternalAttr): ExternalAttr => { case InternalAttr.RegisteredAddress: return ExternalAttr.RegisteredAddress default: - return new UnreachableCaseError(attr) + return undefined } } @@ -115,6 +119,12 @@ export class SGIDMyInfoData isReadOnly: boolean } { const externalAttr = internalAttrToSGIDExternal(attr) + if (externalAttr === undefined) { + return { + fieldValue: undefined, + isReadOnly: false, + } + } const fieldValue = this._formatFieldValue(externalAttr) return { fieldValue, From c8b73c16e4858272f1bf8fe633fa6411ea64eb66 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 18:19:30 +0800 Subject: [PATCH 06/22] fix: sgID validation of forms --- .../form/public-form/public-form.controller.ts | 4 +--- .../modules/sgid/__tests__/sgid.service.spec.ts | 5 ++++- src/app/modules/sgid/sgid.service.ts | 6 +++--- .../email-submission.controller.ts | 14 ++++++-------- .../encrypt-submission.controller.ts | 2 +- 5 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 3ec59e57b7..71e8287891 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -33,7 +33,6 @@ import { extractAuthCode, validateMyInfoForm, } from '../../myinfo/myinfo.util' -import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' @@ -301,8 +300,7 @@ export const handleGetPublicForm: ControllerHandler< } case FormAuthType.SGID: return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map((payload) => { - const data = new SGIDMyInfoData(payload) + .map((data) => { return res.json({ form: publicForm, isIntranetUser, diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index b2f97a2f27..f38a533a9c 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -4,6 +4,7 @@ import fs from 'fs' import Jwt from 'jsonwebtoken' import { MyInfoAttribute } from 'shared/types' +import { SGIDMyInfoData } from '../sgid.adapter' import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' import { SgidCreateRedirectUrlError, @@ -241,7 +242,9 @@ describe('sgid.service', () => { // @ts-ignore MockJwt.verify.mockReturnValue(MOCK_JWT_PAYLOAD) const result = SgidService.extractSgidJwtPayload(MOCK_JWT) - expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD.data) + expect(result._unsafeUnwrap()).toStrictEqual( + new SGIDMyInfoData(MOCK_JWT_PAYLOAD.data), + ) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, MOCK_OPTIONS.publicKeyPath, diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 616dc0bad3..431f953d83 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -9,7 +9,7 @@ import { sgid } from '../../config/features/sgid.config' import { createLoggerWithLabel } from '../../config/logger' import { ApplicationError } from '../core/core.errors' -import { internalAttrListToScopes } from './sgid.adapter' +import { internalAttrListToScopes, SGIDMyInfoData } from './sgid.adapter' import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from './sgid.constants' import { SgidCreateRedirectUrlError, @@ -224,7 +224,7 @@ export class SgidServiceClass { extractSgidJwtPayload( jwtSgid: string, ): Result< - SGIDScopeToValue, + SGIDMyInfoData, SgidVerifyJwtError | SgidInvalidJwtError | SgidMissingJwtError > { const logMeta = { @@ -240,7 +240,7 @@ export class SgidServiceClass { }) if (isSgidJwtPayload(payload)) { - return ok(payload['data']) + return ok(new SGIDMyInfoData(payload['data'])) } const payloadIsDefined = !!payload diff --git a/src/app/modules/submission/email-submission/email-submission.controller.ts b/src/app/modules/submission/email-submission/email-submission.controller.ts index 3df9619d70..3c0f7c958f 100644 --- a/src/app/modules/submission/email-submission/email-submission.controller.ts +++ b/src/app/modules/submission/email-submission/email-submission.controller.ts @@ -234,15 +234,13 @@ const submitEmailModeForm: ControllerHandler< }) case FormAuthType.SGID: return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map( - ({ userName: uinFin }) => ({ - form, - parsedResponses: parsedResponses.addNdiResponses({ - authType, - uinFin, - }), + .map((data) => ({ + form, + parsedResponses: parsedResponses.addNdiResponses({ + authType, + uinFin: data.getUinFin(), }), - ) + })) .mapErr((error) => { spcpSubmissionFailure = true logger.error({ diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts index 136b191405..bb62088efe 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts @@ -276,7 +276,7 @@ const submitEncryptModeForm: ControllerHandler< spcpSubmissionFailure: true, }) } - uinFin = jwtPayloadResult.value.userName + uinFin = jwtPayloadResult.value.getUinFin() break } } From e263733ebeb0ca88cc9166d50949c0abecdc0ae1 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 21:29:31 +0800 Subject: [PATCH 07/22] fix: sgID submission tests --- .../__tests__/encrypt-submission.routes.spec.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts index 9ec7cd1a0b..5558bbea05 100644 --- a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts +++ b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts @@ -5,6 +5,7 @@ import { setupApp } from 'tests/integration/helpers/express-setup' import dbHandler from 'tests/unit/backend/helpers/jest-db' import { FormAuthType, FormStatus } from '../../../../../../shared/types' +import { SGIDMyInfoData } from '../../../sgid/sgid.adapter' import { SGID_COOKIE_NAME } from '../../../sgid/sgid.constants' import { SgidInvalidJwtError, @@ -332,9 +333,11 @@ describe('encrypt-submission.routes', () => { }) it('should return 200 when submission is valid', async () => { MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( - ok({ - userName: 'S1234567A', - }), + ok( + new SGIDMyInfoData({ + 'myinfo.nric_number': 'S1234567A', + }), + ), ) const { form } = await dbHandler.insertEncryptForm({ formOptions: { From 893c5c40647a54695825292ea1fc257425984688 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Tue, 16 May 2023 22:03:50 +0800 Subject: [PATCH 08/22] fix: email submission route tests --- .../__tests__/email-submission.routes.spec.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts b/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts index c5abb25afb..72f015e0a1 100644 --- a/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts +++ b/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts @@ -4,6 +4,7 @@ import mongoose from 'mongoose' import { err, ok } from 'neverthrow' import session, { Session } from 'supertest-session' +import { SGIDMyInfoData } from 'src/app/modules/sgid/sgid.adapter' import { FormFieldSchema } from 'src/types' import { setupApp } from 'tests/integration/helpers/express-setup' @@ -862,9 +863,11 @@ describe('email-submission.routes', () => { describe('SGID', () => { it('should return 200 when submission is valid', async () => { MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( - ok({ - userName: 'S1234567A', - }), + ok( + new SGIDMyInfoData({ + 'myinfo.nric_number': 'S1234567A', + }), + ), ) const { form } = await dbHandler.insertEmailForm({ formOptions: { From 046159f4a1846a1995929d282935bbb135d58f87 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 14:59:54 +0800 Subject: [PATCH 09/22] feat: split sgID auth and sgID MyInfo --- shared/types/form/form.ts | 1 + .../public-form/public-form.controller.ts | 20 +++- .../sgid/__tests__/sgid.service.spec.ts | 31 +++--- .../sgid/__tests__/sgid.test.constants.ts | 1 - src/app/modules/sgid/sgid.controller.ts | 7 +- src/app/modules/sgid/sgid.service.ts | 104 +++++++++++++++--- src/app/modules/sgid/sgid.types.ts | 11 ++ src/app/modules/sgid/sgid.util.ts | 31 +++++- .../ParsedResponsesObject.class.ts | 7 +- .../__tests__/email-submission.routes.spec.ts | 31 +++--- .../email-submission.controller.ts | 21 ++-- .../encrypt-submission.routes.spec.ts | 31 +++--- .../encrypt-submission.controller.ts | 4 +- 13 files changed, 212 insertions(+), 88 deletions(-) diff --git a/shared/types/form/form.ts b/shared/types/form/form.ts index 10a19d34c7..76ccdbe1a5 100644 --- a/shared/types/form/form.ts +++ b/shared/types/form/form.ts @@ -51,6 +51,7 @@ export enum FormAuthType { CP = 'CP', MyInfo = 'MyInfo', SGID = 'SGID', + SGID_MyInfo = 'SGID_MyInfo', } export enum FormStatus { diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 71e8287891..111d0cf6b4 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -33,6 +33,7 @@ import { extractAuthCode, validateMyInfoForm, } from '../../myinfo/myinfo.util' +import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' @@ -299,12 +300,14 @@ export const handleGetPublicForm: ControllerHandler< }) } case FormAuthType.SGID: - return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map((data) => { + return SgidService.extractSgidSingpassJwtPayload( + req.cookies[SGID_COOKIE_NAME], + ) + .map((spcpSession) => { return res.json({ form: publicForm, isIntranetUser, - spcpSession: { userName: data.getUinFin() }, + spcpSession, }) }) .mapErr((error) => { @@ -321,6 +324,8 @@ export const handleGetPublicForm: ControllerHandler< } return res.json({ form: publicForm, isIntranetUser }) }) + case FormAuthType.SGID_MyInfo: + return res.json({ form: publicForm, isIntranetUser: false }) default: return new UnreachableCaseError(authType) } @@ -407,6 +412,15 @@ export const _handleFormAuthRedirect: ControllerHandler< encodedQuery, ) }) + case FormAuthType.SGID_MyInfo: + return validateSgidForm(form).andThen(() => { + return SgidService.createRedirectUrl( + formId, + Boolean(isPersistentLogin), + form.getUniqueMyInfoAttrs(), + encodedQuery, + ) + }) default: return err( new AuthTypeMismatchError(form.authType), diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index f38a533a9c..aac0663276 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -4,7 +4,6 @@ import fs from 'fs' import Jwt from 'jsonwebtoken' import { MyInfoAttribute } from 'shared/types' -import { SGIDMyInfoData } from '../sgid.adapter' import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from '../sgid.constants' import { SgidCreateRedirectUrlError, @@ -32,7 +31,7 @@ import { MOCK_USER_INFO, } from './sgid.test.constants' -const SGID_DEFAULT_SCOPES = 'openid myinfo.nric_number' +const SGID_SINGPASS_SCOPE = 'openid myinfo.nric_number' const SGID_DEFAULT_ATTR_LIST: MyInfoAttribute[] = [] jest.mock('@opengovsg/sgid-client') @@ -79,7 +78,7 @@ describe('sgid.service', () => { expect(url._unsafeUnwrap()).toEqual(MOCK_REDIRECT_URL) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_DEFAULT_SCOPES, + SGID_SINGPASS_SCOPE, null, ) }) @@ -120,7 +119,7 @@ describe('sgid.service', () => { expect(url._unsafeUnwrapErr()).toBeInstanceOf(SgidCreateRedirectUrlError) expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( MOCK_STATE, - SGID_DEFAULT_SCOPES, + SGID_SINGPASS_SCOPE, null, ) }) @@ -194,14 +193,16 @@ describe('sgid.service', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) // @ts-ignore MockJwt.sign.mockReturnValue(MOCK_JWT) - const result = SgidService.createJwt(MOCK_USER_INFO.data, false) + const result = SgidService.createSgidSingpassJwt( + MOCK_USER_INFO.data, + false, + ) expect(result._unsafeUnwrap()).toStrictEqual({ jwt: MOCK_JWT, maxAge: MOCK_OPTIONS.cookieMaxAge, }) expect(MockJwt.sign).toHaveBeenCalledWith( { - data: { 'myinfo.nric_number': 'S9322889A' }, userName: MOCK_USER_INFO.data['myinfo.nric_number'], rememberMe: false, }, @@ -217,14 +218,16 @@ describe('sgid.service', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) // @ts-ignore MockJwt.sign.mockReturnValue(MOCK_JWT) - const result = SgidService.createJwt(MOCK_USER_INFO.data, true) + const result = SgidService.createSgidSingpassJwt( + MOCK_USER_INFO.data, + true, + ) expect(result._unsafeUnwrap()).toStrictEqual({ jwt: MOCK_JWT, maxAge: MOCK_OPTIONS.cookieMaxAgePreserved, }) expect(MockJwt.sign).toHaveBeenCalledWith( { - data: { 'myinfo.nric_number': 'S9322889A' }, userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], rememberMe: true, }, @@ -241,10 +244,8 @@ describe('sgid.service', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) // @ts-ignore MockJwt.verify.mockReturnValue(MOCK_JWT_PAYLOAD) - const result = SgidService.extractSgidJwtPayload(MOCK_JWT) - expect(result._unsafeUnwrap()).toStrictEqual( - new SGIDMyInfoData(MOCK_JWT_PAYLOAD.data), - ) + const result = SgidService.extractSgidSingpassJwtPayload(MOCK_JWT) + expect(result._unsafeUnwrap()).toStrictEqual(MOCK_JWT_PAYLOAD) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, MOCK_OPTIONS.publicKeyPath, @@ -255,7 +256,7 @@ describe('sgid.service', () => { it('should return SgidMissingJwtError on malformed payload', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) // @ts-ignore - const result = SgidService.extractSgidJwtPayload(undefined) + const result = SgidService.extractSgidSingpassJwtPayload(undefined) expect(result._unsafeUnwrapErr()).toBeInstanceOf(SgidMissingJwtError) expect(MockJwt.verify).not.toHaveBeenCalled() }) @@ -264,7 +265,7 @@ describe('sgid.service', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) // @ts-ignore MockJwt.verify.mockReturnValue({}) - const result = SgidService.extractSgidJwtPayload(MOCK_JWT) + const result = SgidService.extractSgidSingpassJwtPayload(MOCK_JWT) expect(result._unsafeUnwrapErr()).toBeInstanceOf(SgidInvalidJwtError) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, @@ -277,7 +278,7 @@ describe('sgid.service', () => { MockJwt.verify.mockImplementation(() => { throw new Error() }) - const result = SgidService.extractSgidJwtPayload(MOCK_JWT) + const result = SgidService.extractSgidSingpassJwtPayload(MOCK_JWT) expect(result._unsafeUnwrapErr()).toBeInstanceOf(SgidVerifyJwtError) expect(MockJwt.verify).toHaveBeenCalledWith( MOCK_JWT, diff --git a/src/app/modules/sgid/__tests__/sgid.test.constants.ts b/src/app/modules/sgid/__tests__/sgid.test.constants.ts index 1deec0a0d0..69254c7f38 100644 --- a/src/app/modules/sgid/__tests__/sgid.test.constants.ts +++ b/src/app/modules/sgid/__tests__/sgid.test.constants.ts @@ -33,7 +33,6 @@ export const MOCK_USER_INFO = { } export const MOCK_JWT_PAYLOAD = { - data: { 'myinfo.nric_number': 'S9322889A' }, userName: MOCK_USER_INFO.data[SGID_MYINFO_NRIC_NUMBER_SCOPE], } diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 9a9a2ec557..0bb3c20a28 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -61,7 +61,12 @@ export const handleLogin: ControllerHandler< const jwtResult = await SgidService.retrieveAccessToken(code) .andThen((data) => SgidService.retrieveUserInfo(data)) - .andThen(({ data }) => SgidService.createJwt(data, rememberMe)) + .andThen(({ data }) => + SgidService.createSgidSingpassJwt( + { 'myinfo.nric_number': data['myinfo.nric_number'] }, + rememberMe, + ), + ) if (jwtResult.isErr()) { logger.error({ diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index 431f953d83..a2a18b3c43 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -9,8 +9,7 @@ import { sgid } from '../../config/features/sgid.config' import { createLoggerWithLabel } from '../../config/logger' import { ApplicationError } from '../core/core.errors' -import { internalAttrListToScopes, SGIDMyInfoData } from './sgid.adapter' -import { SGID_MYINFO_NRIC_NUMBER_SCOPE } from './sgid.constants' +import { internalAttrListToScopes } from './sgid.adapter' import { SgidCreateRedirectUrlError, SgidFetchAccessTokenError, @@ -20,8 +19,13 @@ import { SgidMissingJwtError, SgidVerifyJwtError, } from './sgid.errors' -import { SGIDScopeToValue } from './sgid.types' -import { isSgidJwtPayload } from './sgid.util' +import { + SGIDJwtAccessPayload, + SGIDJwtSingpassPayload, + SGIDJwtVerifierFunction, + SGIDScopeToValue, +} from './sgid.types' +import { isSgidJwtAccessPayload, isSgidJwtSingpassPayload } from './sgid.util' const logger = createLoggerWithLabel(module) @@ -177,10 +181,12 @@ export class SgidServiceClass { SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => ({ - sub, - data, - })), + this.client.userinfo(accessToken).then(({ sub, data }) => { + return { + sub, + data, + } + }), (error) => { logger.error({ message: 'Failed to retrieve user info from sgID', @@ -200,12 +206,33 @@ export class SgidServiceClass { * @param data - the userinfo as obtained from sgID * @param rememberMe - determines how long the JWT is valid for */ - createJwt( - data: SGIDScopeToValue, + createSgidSingpassJwt( + data: { 'myinfo.nric_number': string }, + rememberMe: boolean, + ): Result<{ jwt: string; maxAge: number }, ApplicationError> { + const userName = data['myinfo.nric_number'] + const payload = { userName, rememberMe } + const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge + const jwt = Jwt.sign(payload, this.privateKey, { + algorithm: JWT_ALGORITHM, + expiresIn: maxAge / 1000, + }) + return ok({ + jwt, + maxAge, + }) + } + + /** + * Create a JWT based with access token from sgID + * @param accessToken - sgID access token + * @param rememberMe - determines how long the JWT is valid for + */ + createSgidMyInfoJwt( + accessToken: string, rememberMe: boolean, ): Result<{ jwt: string; maxAge: number }, ApplicationError> { - const userName = data[SGID_MYINFO_NRIC_NUMBER_SCOPE] - const payload = { userName, data, rememberMe } + const payload: SGIDJwtAccessPayload = { accessToken, rememberMe } const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { algorithm: JWT_ALGORITHM, @@ -218,17 +245,58 @@ export class SgidServiceClass { } /** - * Verifies a sgID JWT and extracts its payload. + * Verifies a sgID JWT and extracts its payload (Singpass userName). * @param jwtSgid The contents of the JWT cookie */ - extractSgidJwtPayload( + extractSgidSingpassJwtPayload( jwtSgid: string, ): Result< - SGIDMyInfoData, + SGIDJwtSingpassPayload, SgidVerifyJwtError | SgidInvalidJwtError | SgidMissingJwtError > { + return this._extractSgidJwtGenericPayload( + jwtSgid, + 'extractSgidJwtSingpassPayload', + isSgidJwtSingpassPayload, + ) + } + + /** + * Verifies a sgID JWT and extracts its payload (access token). + * @param jwtSgid The contents of the JWT cookie + */ + extractSgidJwtMyInfoPayload( + jwtSgid: string, + ): Result< + SGIDJwtAccessPayload, + SgidVerifyJwtError | SgidInvalidJwtError | SgidMissingJwtError + > { + return this._extractSgidJwtGenericPayload( + jwtSgid, + 'extractSgidJwtAccessPayload', + isSgidJwtAccessPayload, + ) + } + + /** + * Verifies a sgID JWT and extract its payload. + * sgID JWT has two types/modes + * 1. Simple auth mode (Singpass auth replacement). + * 2. MyInfo access token mode (used to fill up MyInfo fields over sgID). + * @param jwtSgid The contents of the JWT cookie. + * @param action Name of the calling function. + * @param verifier Function to verify the contents of the JWT + * @returns + */ + _extractSgidJwtGenericPayload< + R extends SGIDJwtSingpassPayload | SGIDJwtAccessPayload, + >( + jwtSgid: string, + action: string, + verifier: SGIDJwtVerifierFunction, + ): Result { const logMeta = { - action: 'extractSgidJwtPayload', + action, } try { if (!jwtSgid) { @@ -239,8 +307,8 @@ export class SgidServiceClass { algorithms: [JWT_ALGORITHM], }) - if (isSgidJwtPayload(payload)) { - return ok(new SGIDMyInfoData(payload['data'])) + if (verifier(payload)) { + return ok(payload) } const payloadIsDefined = !!payload diff --git a/src/app/modules/sgid/sgid.types.ts b/src/app/modules/sgid/sgid.types.ts index e1ebe12b58..5f056f9bae 100644 --- a/src/app/modules/sgid/sgid.types.ts +++ b/src/app/modules/sgid/sgid.types.ts @@ -6,3 +6,14 @@ export type SgidForm = T & { } export type SGIDScopeToValue = Record + +export type SGIDJwtSingpassPayload = { userName: string } +export type SGIDJwtAccessPayload = { + userName: string | undefined + accessToken: string + rememberMe: boolean +} + +export type SGIDJwtVerifierFunction< + T extends SGIDJwtSingpassPayload | SGIDJwtAccessPayload, +> = (payload: unknown) => payload is T diff --git a/src/app/modules/sgid/sgid.util.ts b/src/app/modules/sgid/sgid.util.ts index 93287c5619..6f72f83d27 100644 --- a/src/app/modules/sgid/sgid.util.ts +++ b/src/app/modules/sgid/sgid.util.ts @@ -10,7 +10,11 @@ import { IFormSchema, SgidFieldTitle } from '../../../types' import { AuthTypeMismatchError } from '../form/form.errors' import { ProcessedSingleAnswerResponse } from '../submission/submission.types' -import { SgidForm, SGIDScopeToValue } from './sgid.types' +import { + SgidForm, + SGIDJwtAccessPayload, + SGIDJwtSingpassPayload, +} from './sgid.types' /** * Validates that a form is an sgID form @@ -51,15 +55,30 @@ export const createSgidParsedResponses = ( * Typeguard for SingPass JWT payload. * @param payload Payload decrypted from JWT */ -export const isSgidJwtPayload = ( +export const isSgidJwtSingpassPayload = ( payload: unknown, -): payload is Record => { +): payload is SGIDJwtSingpassPayload => { return ( typeof payload === 'object' && !!payload && hasProp(payload, 'userName') && - typeof payload.userName === 'string' && - hasProp(payload, 'data') && - typeof payload.data === 'object' + typeof payload.userName === 'string' + ) +} + +/** + * Typeguard for SGID JWT access token payload. + * @param payload Payload decrypted from JWT + */ +export const isSgidJwtAccessPayload = ( + payload: unknown, +): payload is SGIDJwtAccessPayload => { + return ( + typeof payload === 'object' && + !!payload && + hasProp(payload, 'accessToken') && + typeof payload.accessToken === 'string' && + hasProp(payload, 'rememberMe') && + typeof payload.rememberMe === 'boolean' ) } diff --git a/src/app/modules/submission/email-submission/ParsedResponsesObject.class.ts b/src/app/modules/submission/email-submission/ParsedResponsesObject.class.ts index c220dc77ad..8df76207be 100644 --- a/src/app/modules/submission/email-submission/ParsedResponsesObject.class.ts +++ b/src/app/modules/submission/email-submission/ParsedResponsesObject.class.ts @@ -26,7 +26,11 @@ import { getFilteredResponses } from '../submission.utils' type NdiUserInfo = | { - authType: FormAuthType.SP | FormAuthType.MyInfo | FormAuthType.SGID + authType: + | FormAuthType.SP + | FormAuthType.MyInfo + | FormAuthType.SGID + | FormAuthType.SGID_MyInfo uinFin: string } | { authType: FormAuthType.CP; uinFin: string; userInfo: string } @@ -53,6 +57,7 @@ export default class ParsedResponsesObject { ) break case FormAuthType.SGID: + case FormAuthType.SGID_MyInfo: this.ndiResponses = createSgidParsedResponses(info.uinFin) break } diff --git a/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts b/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts index 72f015e0a1..09fb200010 100644 --- a/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts +++ b/src/app/modules/submission/email-submission/__tests__/email-submission.routes.spec.ts @@ -4,7 +4,6 @@ import mongoose from 'mongoose' import { err, ok } from 'neverthrow' import session, { Session } from 'supertest-session' -import { SGIDMyInfoData } from 'src/app/modules/sgid/sgid.adapter' import { FormFieldSchema } from 'src/types' import { setupApp } from 'tests/integration/helpers/express-setup' @@ -862,12 +861,10 @@ describe('email-submission.routes', () => { describe('SGID', () => { it('should return 200 when submission is valid', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( - ok( - new SGIDMyInfoData({ - 'myinfo.nric_number': 'S1234567A', - }), - ), + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( + ok({ + userName: 'S1234567A', + }), ) const { form } = await dbHandler.insertEmailForm({ formOptions: { @@ -893,7 +890,7 @@ describe('email-submission.routes', () => { }) it('should return 401 when submission does not have JWT', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const { form } = await dbHandler.insertEmailForm({ @@ -918,13 +915,13 @@ describe('email-submission.routes', () => { spcpSubmissionFailure: true, }) // Should be undefined, since there was no SGID cookie - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenLastCalledWith( - undefined, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenLastCalledWith(undefined) }) it('should return 401 when submission has the wrong JWT type', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const { form } = await dbHandler.insertEmailForm({ @@ -950,13 +947,13 @@ describe('email-submission.routes', () => { spcpSubmissionFailure: true, }) // Should be undefined, since there was no SGID cookie - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenLastCalledWith( - undefined, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenLastCalledWith(undefined) }) it('should return 401 when submission has invalid JWT', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const { form } = await dbHandler.insertEmailForm({ @@ -983,7 +980,7 @@ describe('email-submission.routes', () => { }) it('should return 401 when submission has JWT with the wrong shape', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const { form } = await dbHandler.insertEmailForm({ diff --git a/src/app/modules/submission/email-submission/email-submission.controller.ts b/src/app/modules/submission/email-submission/email-submission.controller.ts index 3c0f7c958f..bf14cff7c3 100644 --- a/src/app/modules/submission/email-submission/email-submission.controller.ts +++ b/src/app/modules/submission/email-submission/email-submission.controller.ts @@ -21,6 +21,7 @@ import { } from '../../myinfo/myinfo.constants' import { MyInfoService } from '../../myinfo/myinfo.service' import { extractMyInfoLoginJwt } from '../../myinfo/myinfo.util' +import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' import { SgidService } from '../../sgid/sgid.service' import { getOidcService } from '../../spcp/spcp.oidc.service' import * as EmailSubmissionMiddleware from '../email-submission/email-submission.middleware' @@ -232,15 +233,21 @@ const submitEmailModeForm: ControllerHandler< }) return error }) + // SGID_MyInfo is temporarily placed here for now. + case FormAuthType.SGID_MyInfo: case FormAuthType.SGID: - return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) - .map((data) => ({ - form, - parsedResponses: parsedResponses.addNdiResponses({ - authType, - uinFin: data.getUinFin(), + return SgidService.extractSgidSingpassJwtPayload( + req.cookies[SGID_COOKIE_NAME], + ) + .map( + ({ userName: uinFin }) => ({ + form, + parsedResponses: parsedResponses.addNdiResponses({ + authType, + uinFin, + }), }), - })) + ) .mapErr((error) => { spcpSubmissionFailure = true logger.error({ diff --git a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts index 5558bbea05..e0699d5d2f 100644 --- a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts +++ b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts @@ -5,7 +5,6 @@ import { setupApp } from 'tests/integration/helpers/express-setup' import dbHandler from 'tests/unit/backend/helpers/jest-db' import { FormAuthType, FormStatus } from '../../../../../../shared/types' -import { SGIDMyInfoData } from '../../../sgid/sgid.adapter' import { SGID_COOKIE_NAME } from '../../../sgid/sgid.constants' import { SgidInvalidJwtError, @@ -332,12 +331,10 @@ describe('encrypt-submission.routes', () => { jest.resetAllMocks() }) it('should return 200 when submission is valid', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( - ok( - new SGIDMyInfoData({ - 'myinfo.nric_number': 'S1234567A', - }), - ), + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( + ok({ + userName: 'S1234567A', + }), ) const { form } = await dbHandler.insertEncryptForm({ formOptions: { @@ -363,7 +360,7 @@ describe('encrypt-submission.routes', () => { }) it('should return 401 when submission does not have JWT', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const { form } = await dbHandler.insertEncryptForm({ @@ -388,13 +385,13 @@ describe('encrypt-submission.routes', () => { spcpSubmissionFailure: true, }) // Should be undefined, since there was no SGID cookie - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenLastCalledWith( - undefined, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenLastCalledWith(undefined) }) it('should return 401 when submission has the wrong JWT type', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const { form } = await dbHandler.insertEncryptForm({ @@ -420,13 +417,13 @@ describe('encrypt-submission.routes', () => { spcpSubmissionFailure: true, }) // Should be undefined, since there was no SGID cookie - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenLastCalledWith( - undefined, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenLastCalledWith(undefined) }) it('should return 401 when submission has invalid JWT', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const { form } = await dbHandler.insertEncryptForm({ @@ -453,7 +450,7 @@ describe('encrypt-submission.routes', () => { }) it('should return 401 when submission has JWT with the wrong shape', async () => { - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const { form } = await dbHandler.insertEncryptForm({ diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts index bb62088efe..cac9763323 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts @@ -259,7 +259,7 @@ const submitEncryptModeForm: ControllerHandler< break } case FormAuthType.SGID: { - const jwtPayloadResult = SgidService.extractSgidJwtPayload( + const jwtPayloadResult = SgidService.extractSgidSingpassJwtPayload( req.cookies.jwtSgid, ) if (jwtPayloadResult.isErr()) { @@ -276,7 +276,7 @@ const submitEncryptModeForm: ControllerHandler< spcpSubmissionFailure: true, }) } - uinFin = jwtPayloadResult.value.getUinFin() + uinFin = jwtPayloadResult.value.userName break } } From 35357d715c51c435e90fcd2b9f6ea3be3889caf0 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 15:07:37 +0800 Subject: [PATCH 10/22] fix: rename functions --- src/app/modules/sgid/sgid.types.ts | 1 - src/app/modules/verification/verification.controller.ts | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/modules/sgid/sgid.types.ts b/src/app/modules/sgid/sgid.types.ts index 5f056f9bae..43a46d72e7 100644 --- a/src/app/modules/sgid/sgid.types.ts +++ b/src/app/modules/sgid/sgid.types.ts @@ -9,7 +9,6 @@ export type SGIDScopeToValue = Record export type SGIDJwtSingpassPayload = { userName: string } export type SGIDJwtAccessPayload = { - userName: string | undefined accessToken: string rememberMe: boolean } diff --git a/src/app/modules/verification/verification.controller.ts b/src/app/modules/verification/verification.controller.ts index 94268120dd..e8a814d900 100644 --- a/src/app/modules/verification/verification.controller.ts +++ b/src/app/modules/verification/verification.controller.ts @@ -263,8 +263,12 @@ export const _handleGenerateOtp: ControllerHandler< return error }) } + // SGID_MyInfo temporarily here + case FormAuthType.SGID_MyInfo: case FormAuthType.SGID: - return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid) + return SgidService.extractSgidSingpassJwtPayload( + req.cookies.jwtSgid, + ) .map(() => form) .mapErr((error) => { logger.error({ From 9a7e45b2c421c80aed3a40869d25310e06798916 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 15:43:42 +0800 Subject: [PATCH 11/22] fix: tests, add SGID_MyInfo form filling --- .../public-form/public-form.controller.ts | 40 ++++++++++++++++++- .../sgid/__tests__/sgid.controller.spec.ts | 20 +++++----- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 111d0cf6b4..a2f1d7758c 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -33,6 +33,7 @@ import { extractAuthCode, validateMyInfoForm, } from '../../myinfo/myinfo.util' +import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' @@ -324,8 +325,43 @@ export const handleGetPublicForm: ControllerHandler< } return res.json({ form: publicForm, isIntranetUser }) }) - case FormAuthType.SGID_MyInfo: - return res.json({ form: publicForm, isIntranetUser: false }) + case FormAuthType.SGID_MyInfo: { + const accessTokenCookie = req.cookies[SGID_COOKIE_NAME] + res.clearCookie(SGID_COOKIE_NAME) + return SgidService.extractSgidJwtMyInfoPayload(accessTokenCookie) + .asyncAndThen((auth) => + SgidService.retrieveUserInfo({ accessToken: auth.accessToken }), + ) + .andThen((userInfo) => { + const data = new SGIDMyInfoData(userInfo.data) + return MyInfoService.prefillAndSaveMyInfoFields( + form._id, + data, + form.toJSON().form_fields, + ).map((prefilledFields) => { + return res.json({ + form: { + ...publicForm, + form_fields: prefilledFields as FormFieldDto[], + }, + spcpSession: { userName: data.getUinFin() }, + isIntranetUser, + }) + }) + }) + .mapErr((error) => { + logger.error({ + message: 'sgID: MyInfo login error', + meta: logMeta, + error, + }) + return res.json({ + form: publicForm, + myInfoError: true, + isIntranetUser, + }) + }) + } default: return new UnreachableCaseError(authType) } diff --git a/src/app/modules/sgid/__tests__/sgid.controller.spec.ts b/src/app/modules/sgid/__tests__/sgid.controller.spec.ts index 3eff697b62..2c1a1a40a0 100644 --- a/src/app/modules/sgid/__tests__/sgid.controller.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.controller.spec.ts @@ -61,7 +61,7 @@ describe('sgid.controller', () => { okAsync(MOCK_TOKEN_RESULT), ) SgidService.retrieveUserInfo.mockReturnValue(okAsync(MOCK_USER_INFO)) - SgidService.createJwt.mockReturnValue( + SgidService.createSgidSingpassJwt.mockReturnValue( ok({ jwt: MOCK_JWT, maxAge: MOCK_COOKIE_AGE }), ) SgidService.getCookieSettings.mockReturnValue(MOCK_COOKIE_SETTINGS) @@ -77,7 +77,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).not.toHaveBeenCalled() expect(SgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(SgidService.createJwt).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -93,7 +93,7 @@ describe('sgid.controller', () => { expect(MOCK_RESPONSE.redirect).not.toHaveBeenCalled() expect(SgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(SgidService.createJwt).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -109,7 +109,7 @@ describe('sgid.controller', () => { expect(MOCK_RESPONSE.redirect).toHaveBeenCalledWith(MOCK_DESTINATION) expect(SgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(SgidService.createJwt).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -126,7 +126,7 @@ describe('sgid.controller', () => { MOCK_AUTH_CODE, ) expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(SgidService.createJwt).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -145,12 +145,14 @@ describe('sgid.controller', () => { ) expect(MOCK_RESPONSE.cookie).toHaveBeenCalledWith('isLoginError', true) expect(MOCK_RESPONSE.redirect).toHaveBeenCalledWith(MOCK_DESTINATION) - expect(SgidService.createJwt).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) it('should set isLoginError cookie and redirect when createJWT errors', async () => { - SgidService.createJwt.mockReturnValue(err(new ApplicationError())) + SgidService.createSgidSingpassJwt.mockReturnValue( + err(new ApplicationError()), + ) await SgidController.handleLogin(MOCK_LOGIN_REQ, MOCK_RESPONSE, jest.fn()) expect(SgidService.parseState).toHaveBeenCalledWith(MOCK_STATE) expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) @@ -160,7 +162,7 @@ describe('sgid.controller', () => { expect(SgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, ) - expect(SgidService.createJwt).toHaveBeenCalledWith( + expect(SgidService.createSgidSingpassJwt).toHaveBeenCalledWith( MOCK_USER_INFO.data, MOCK_REMEMBER_ME, ) @@ -178,7 +180,7 @@ describe('sgid.controller', () => { expect(SgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, ) - expect(SgidService.createJwt).toHaveBeenCalledWith( + expect(SgidService.createSgidSingpassJwt).toHaveBeenCalledWith( MOCK_USER_INFO.data, MOCK_REMEMBER_ME, ) From f7ca1ce6b80428da96168de154ef48f9e25453d7 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 17:36:28 +0800 Subject: [PATCH 12/22] feat: backend submissions and logout done --- src/app/models/form.server.model.ts | 8 +++-- .../public-form/public-form.controller.ts | 31 ++++++++++++++----- .../form/public-form/public-form.service.ts | 4 ++- src/app/modules/sgid/sgid.controller.ts | 18 +++++++++++ src/app/modules/sgid/sgid.util.ts | 5 ++- .../email-submission.controller.ts | 7 +++-- 6 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index 31e8960b1e..e62a0ca38e 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -264,7 +264,8 @@ const compileFormModel = (db: Mongoose): IFormModel => { ) return ( myInfoFieldCount === 0 || - (this.authType === FormAuthType.MyInfo && + ((this.authType === FormAuthType.MyInfo || + this.authType == FormAuthType.SGID_MyInfo) && this.responseMode === FormResponseMode.Email && myInfoFieldCount <= 30) ) @@ -559,7 +560,10 @@ const compileFormModel = (db: Mongoose): IFormModel => { // Method to return myInfo attributes FormSchema.methods.getUniqueMyInfoAttrs = function () { - if (this.authType !== FormAuthType.MyInfo) { + if ( + this.authType !== FormAuthType.MyInfo && + this.authType !== FormAuthType.SGID_MyInfo + ) { return [] } diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index a2f1d7758c..87ceb00e82 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -327,7 +327,14 @@ export const handleGetPublicForm: ControllerHandler< }) case FormAuthType.SGID_MyInfo: { const accessTokenCookie = req.cookies[SGID_COOKIE_NAME] + if (!accessTokenCookie) { + return res.json({ + form: publicForm, + isIntranetUser, + }) + } res.clearCookie(SGID_COOKIE_NAME) + res.clearCookie(MYINFO_LOGIN_COOKIE_NAME) return SgidService.extractSgidJwtMyInfoPayload(accessTokenCookie) .asyncAndThen((auth) => SgidService.retrieveUserInfo({ accessToken: auth.accessToken }), @@ -339,14 +346,20 @@ export const handleGetPublicForm: ControllerHandler< data, form.toJSON().form_fields, ).map((prefilledFields) => { - return res.json({ - form: { - ...publicForm, - form_fields: prefilledFields as FormFieldDto[], - }, - spcpSession: { userName: data.getUinFin() }, - isIntranetUser, - }) + return res + .cookie( + MYINFO_LOGIN_COOKIE_NAME, + createMyInfoLoginCookie(data.getUinFin()), + MYINFO_LOGIN_COOKIE_OPTIONS, + ) + .json({ + form: { + ...publicForm, + form_fields: prefilledFields as FormFieldDto[], + }, + spcpSession: { userName: data.getUinFin() }, + isIntranetUser, + }) }) }) .mapErr((error) => { @@ -519,6 +532,7 @@ export const _handlePublicAuthLogout: ControllerHandler< | FormAuthType.CP | FormAuthType.MyInfo | FormAuthType.SGID + | FormAuthType.SGID_MyInfo }, PublicFormAuthLogoutDto > = (req, res) => { @@ -545,6 +559,7 @@ export const handlePublicAuthLogout = [ FormAuthType.CP, FormAuthType.MyInfo, FormAuthType.SGID, + FormAuthType.SGID_MyInfo, ) .required(), }), diff --git a/src/app/modules/form/public-form/public-form.service.ts b/src/app/modules/form/public-form/public-form.service.ts index 6ef9d56188..5c47bc02fb 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -73,9 +73,11 @@ export const getCookieNameByAuthType = ( | FormAuthType.SP | FormAuthType.CP | FormAuthType.MyInfo - | FormAuthType.SGID, + | FormAuthType.SGID + | FormAuthType.SGID_MyInfo, ): string => { switch (authType) { + case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: return MYINFO_LOGIN_COOKIE_NAME case FormAuthType.SGID: diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 0bb3c20a28..b89e59ca83 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -46,6 +46,24 @@ export const handleLogin: ControllerHandler< } const form = formResult.value + // @TODO move this to separate route. + if (form.authType === FormAuthType.SGID_MyInfo) { + const jwtResult = await SgidService.retrieveAccessToken(code).andThen( + (data) => SgidService.createSgidMyInfoJwt(data.accessToken, rememberMe), + ) + + if (!jwtResult.isErr()) { + const { maxAge, jwt } = jwtResult.value + res.cookie(SGID_COOKIE_NAME, jwt, { + maxAge, + httpOnly: true, + sameSite: 'lax', // Setting to 'strict' prevents Singpass login on Safari, Firefox + secure: !config.isDev, + ...SgidService.getCookieSettings(), + }) + return res.redirect(target) + } + } if (form.authType !== FormAuthType.SGID) { logger.error({ message: "Log in attempt to wrong endpoint for form's authType", diff --git a/src/app/modules/sgid/sgid.util.ts b/src/app/modules/sgid/sgid.util.ts index 6f72f83d27..a617fa18de 100644 --- a/src/app/modules/sgid/sgid.util.ts +++ b/src/app/modules/sgid/sgid.util.ts @@ -30,7 +30,10 @@ export const validateSgidForm = ( // Typeguard to ensure that form has the correct authType const isSgidForm = (form: F): form is SgidForm => { - return form.authType === FormAuthType.SGID + return ( + form.authType === FormAuthType.SGID || + form.authType === FormAuthType.SGID_MyInfo + ) } /** diff --git a/src/app/modules/submission/email-submission/email-submission.controller.ts b/src/app/modules/submission/email-submission/email-submission.controller.ts index bf14cff7c3..80a5f78c83 100644 --- a/src/app/modules/submission/email-submission/email-submission.controller.ts +++ b/src/app/modules/submission/email-submission/email-submission.controller.ts @@ -202,6 +202,7 @@ const submitEmailModeForm: ControllerHandler< return error }) } + case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: return extractMyInfoLoginJwt(req.cookies) .andThen(MyInfoService.verifyLoginJwt) @@ -227,14 +228,14 @@ const submitEmailModeForm: ControllerHandler< .mapErr((error) => { spcpSubmissionFailure = true logger.error({ - message: 'Error verifying MyInfo hashes', + message: `Error verifying MyInfo${ + authType === FormAuthType.SGID_MyInfo ? '(over SGID)' : '' + } hashes`, meta: logMeta, error, }) return error }) - // SGID_MyInfo is temporarily placed here for now. - case FormAuthType.SGID_MyInfo: case FormAuthType.SGID: return SgidService.extractSgidSingpassJwtPayload( req.cookies[SGID_COOKIE_NAME], From 477078ce8865cb0fd2b7d21f48fa385da5fed9d4 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 17:37:08 +0800 Subject: [PATCH 13/22] fix: sgID routes tests --- .../field-panels/sgIDPanel.tsx | 173 ++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx diff --git a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx new file mode 100644 index 0000000000..3182f5b1d2 --- /dev/null +++ b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx @@ -0,0 +1,173 @@ +import { useMemo } from 'react' +import { Droppable } from 'react-beautiful-dnd' +import { Box, Text } from '@chakra-ui/react' + +import { AdminFormDto, FormAuthType, FormResponseMode } from '~shared/types' + +import { GUIDE_EMAIL_MODE } from '~constants/links' +import InlineMessage from '~components/InlineMessage' +import Link from '~components/Link' + +import { + CREATE_MYINFO_CONTACT_DROP_ID, + CREATE_MYINFO_CONTACT_FIELDS_ORDERED, + CREATE_MYINFO_MARRIAGE_DROP_ID, + CREATE_MYINFO_MARRIAGE_FIELDS_ORDERED, + CREATE_MYINFO_PARTICULARS_DROP_ID, + CREATE_MYINFO_PARTICULARS_FIELDS_ORDERED, + CREATE_MYINFO_PERSONAL_DROP_ID, + CREATE_MYINFO_PERSONAL_FIELDS_ORDERED, +} from '~features/admin-form/create/builder-and-design/constants' +import { isMyInfo } from '~features/myinfo/utils' + +import { useCreateTabForm } from '../../../../builder-and-design/useCreateTabForm' +import { DraggableMyInfoFieldListOption } from '../FieldListOption' + +import { FieldSection } from './FieldSection' + +export const SGIDFieldPanel = () => { + const { data: form, isLoading } = useCreateTabForm() + // myInfo should be disabled if + // 1. form response mode is not email mode + // 2. form auth type is not myInfo + // 3. # of myInfo fields >= 30 + const isSGIDDisabled = useMemo( + () => + form?.responseMode !== FormResponseMode.Email || + form?.authType !== FormAuthType.SGID_MyInfo || + (form ? form.form_fields.filter(isMyInfo).length >= 30 : true), + [form], + ) + + // @TODO remove this always false. + const isDisabled = isSGIDDisabled || isLoading + + return ( + <> + + + {(provided) => ( + + + {CREATE_MYINFO_PERSONAL_FIELDS_ORDERED.map((fieldType, index) => ( + + ))} + + {provided.placeholder} + + )} + + + {(provided) => ( + + + {CREATE_MYINFO_CONTACT_FIELDS_ORDERED.map((fieldType, index) => ( + + ))} + + {provided.placeholder} + + )} + + + {(provided) => ( + + + {CREATE_MYINFO_PARTICULARS_FIELDS_ORDERED.map( + (fieldType, index) => ( + + ), + )} + + {provided.placeholder} + + )} + + + {(provided) => ( + + + {CREATE_MYINFO_MARRIAGE_FIELDS_ORDERED.map((fieldType, index) => ( + + ))} + + {provided.placeholder} + + )} + + + ) +} + +type MyInfoTextProps = Pick< + AdminFormDto, + 'authType' | 'responseMode' | 'form_fields' +> + +const MyInfoText = ({ + authType, + responseMode, + form_fields, +}: MyInfoTextProps): JSX.Element => { + // @TODO disable me + const isSGIDDisabled = authType !== FormAuthType.SGID_MyInfo + + const numMyInfoFields = useMemo( + () => form_fields.filter((ff) => isMyInfo(ff)).length, + [form_fields], + ) + + if (responseMode !== FormResponseMode.Email) { + return sgID fields are not available in Storage mode forms. + } + + if (isSGIDDisabled) { + return Enable sgID in the Settings tab to access these fields. + } + + return ( + + {`Only 30 sgID fields are allowed in Email mode (${numMyInfoFields}/30).`}{' '} + + Learn more + + + ) +} + +const MyInfoMessage = (): JSX.Element | null => { + const { data: form } = useCreateTabForm() + const numMyInfoFields = form?.form_fields.filter((ff) => isMyInfo(ff)).length + const hasExceededLimit = useMemo( + () => numMyInfoFields && numMyInfoFields >= 30, + [numMyInfoFields], + ) + + return form ? ( + + + + + + ) : null +} From 4e547c01687e89b26943bf40dc8df572bde8672d Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 17:39:11 +0800 Subject: [PATCH 14/22] fix: actually fix sgID routes tests --- .../field-panels/sgIDPanel.tsx | 173 ------------------ .../sgid/__tests__/sgid.routes.spec.ts | 24 +-- 2 files changed, 13 insertions(+), 184 deletions(-) delete mode 100644 frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx diff --git a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx deleted file mode 100644 index 3182f5b1d2..0000000000 --- a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/sgIDPanel.tsx +++ /dev/null @@ -1,173 +0,0 @@ -import { useMemo } from 'react' -import { Droppable } from 'react-beautiful-dnd' -import { Box, Text } from '@chakra-ui/react' - -import { AdminFormDto, FormAuthType, FormResponseMode } from '~shared/types' - -import { GUIDE_EMAIL_MODE } from '~constants/links' -import InlineMessage from '~components/InlineMessage' -import Link from '~components/Link' - -import { - CREATE_MYINFO_CONTACT_DROP_ID, - CREATE_MYINFO_CONTACT_FIELDS_ORDERED, - CREATE_MYINFO_MARRIAGE_DROP_ID, - CREATE_MYINFO_MARRIAGE_FIELDS_ORDERED, - CREATE_MYINFO_PARTICULARS_DROP_ID, - CREATE_MYINFO_PARTICULARS_FIELDS_ORDERED, - CREATE_MYINFO_PERSONAL_DROP_ID, - CREATE_MYINFO_PERSONAL_FIELDS_ORDERED, -} from '~features/admin-form/create/builder-and-design/constants' -import { isMyInfo } from '~features/myinfo/utils' - -import { useCreateTabForm } from '../../../../builder-and-design/useCreateTabForm' -import { DraggableMyInfoFieldListOption } from '../FieldListOption' - -import { FieldSection } from './FieldSection' - -export const SGIDFieldPanel = () => { - const { data: form, isLoading } = useCreateTabForm() - // myInfo should be disabled if - // 1. form response mode is not email mode - // 2. form auth type is not myInfo - // 3. # of myInfo fields >= 30 - const isSGIDDisabled = useMemo( - () => - form?.responseMode !== FormResponseMode.Email || - form?.authType !== FormAuthType.SGID_MyInfo || - (form ? form.form_fields.filter(isMyInfo).length >= 30 : true), - [form], - ) - - // @TODO remove this always false. - const isDisabled = isSGIDDisabled || isLoading - - return ( - <> - - - {(provided) => ( - - - {CREATE_MYINFO_PERSONAL_FIELDS_ORDERED.map((fieldType, index) => ( - - ))} - - {provided.placeholder} - - )} - - - {(provided) => ( - - - {CREATE_MYINFO_CONTACT_FIELDS_ORDERED.map((fieldType, index) => ( - - ))} - - {provided.placeholder} - - )} - - - {(provided) => ( - - - {CREATE_MYINFO_PARTICULARS_FIELDS_ORDERED.map( - (fieldType, index) => ( - - ), - )} - - {provided.placeholder} - - )} - - - {(provided) => ( - - - {CREATE_MYINFO_MARRIAGE_FIELDS_ORDERED.map((fieldType, index) => ( - - ))} - - {provided.placeholder} - - )} - - - ) -} - -type MyInfoTextProps = Pick< - AdminFormDto, - 'authType' | 'responseMode' | 'form_fields' -> - -const MyInfoText = ({ - authType, - responseMode, - form_fields, -}: MyInfoTextProps): JSX.Element => { - // @TODO disable me - const isSGIDDisabled = authType !== FormAuthType.SGID_MyInfo - - const numMyInfoFields = useMemo( - () => form_fields.filter((ff) => isMyInfo(ff)).length, - [form_fields], - ) - - if (responseMode !== FormResponseMode.Email) { - return sgID fields are not available in Storage mode forms. - } - - if (isSGIDDisabled) { - return Enable sgID in the Settings tab to access these fields. - } - - return ( - - {`Only 30 sgID fields are allowed in Email mode (${numMyInfoFields}/30).`}{' '} - - Learn more - - - ) -} - -const MyInfoMessage = (): JSX.Element | null => { - const { data: form } = useCreateTabForm() - const numMyInfoFields = form?.form_fields.filter((ff) => isMyInfo(ff)).length - const hasExceededLimit = useMemo( - () => numMyInfoFields && numMyInfoFields >= 30, - [numMyInfoFields], - ) - - return form ? ( - - - - - - ) : null -} diff --git a/src/app/modules/sgid/__tests__/sgid.routes.spec.ts b/src/app/modules/sgid/__tests__/sgid.routes.spec.ts index f36add71cb..704fbeee95 100644 --- a/src/app/modules/sgid/__tests__/sgid.routes.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.routes.spec.ts @@ -62,7 +62,7 @@ describe('sgid.controller', () => { okAsync(MOCK_TOKEN_RESULT), ) sgidService.retrieveUserInfo.mockReturnValue(okAsync(MOCK_USER_INFO)) - sgidService.createJwt.mockReturnValue( + sgidService.createSgidSingpassJwt.mockReturnValue( ok({ jwt: MOCK_JWT, maxAge: MOCK_COOKIE_AGE }), ) sgidService.getCookieSettings.mockReturnValue(MOCK_COOKIE_SETTINGS) @@ -83,7 +83,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).not.toHaveBeenCalled() expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -102,7 +102,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).not.toHaveBeenCalled() expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -118,7 +118,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).not.toHaveBeenCalled() expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -136,7 +136,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -159,7 +159,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -183,7 +183,7 @@ describe('sgid.controller', () => { MOCK_AUTH_CODE, ) expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) @@ -209,12 +209,14 @@ describe('sgid.controller', () => { expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, ) - expect(sgidService.createJwt).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) it('should set isLoginError cookie and redirect when createJWT errors', async () => { - sgidService.createJwt.mockReturnValue(err(new ApplicationError())) + sgidService.createSgidSingpassJwt.mockReturnValue( + err(new ApplicationError()), + ) const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) @@ -233,7 +235,7 @@ describe('sgid.controller', () => { expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, ) - expect(sgidService.createJwt).toHaveBeenCalledWith( + expect(sgidService.createSgidSingpassJwt).toHaveBeenCalledWith( MOCK_USER_INFO.data, MOCK_REMEMBER_ME, ) @@ -258,7 +260,7 @@ describe('sgid.controller', () => { expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, ) - expect(sgidService.createJwt).toHaveBeenCalledWith( + expect(sgidService.createSgidSingpassJwt).toHaveBeenCalledWith( MOCK_USER_INFO.data, MOCK_REMEMBER_ME, ) From 646f503d6f4e489e78901a87d3126ef1ea864938 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Wed, 17 May 2023 18:24:51 +0800 Subject: [PATCH 15/22] fix: verification controller tests --- .../__tests__/verification.controller.spec.ts | 56 ++++++++++--------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts index e9758fb4c8..92d5afd558 100644 --- a/src/app/modules/verification/__tests__/verification.controller.spec.ts +++ b/src/app/modules/verification/__tests__/verification.controller.spec.ts @@ -723,7 +723,9 @@ describe('Verification controller', () => { expect(mockSpOidcServiceClass.extractJwtPayload).not.toHaveBeenCalled() expect(mockCpOidcServiceClass.extractJwtPayload).not.toHaveBeenCalled() - expect(MockSgidService.extractSgidJwtPayload).not.toHaveBeenCalled() + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).not.toHaveBeenCalled() expect(MockMyInfoUtil.extractMyInfoLoginJwt).not.toHaveBeenCalled() expect(MockMyInfoService.verifyLoginJwt).not.toHaveBeenCalled() expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() @@ -865,7 +867,7 @@ describe('Verification controller', () => { MockVerificationService.disableVerifiedFieldsIfRequired.mockReturnValueOnce( okAsync(true), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( ok(MOCK_VALID_SGID_PAYLOAD), ) @@ -880,9 +882,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwtSgid, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).toHaveBeenCalledWith( EXPECTED_PARAMS_FOR_SENDING_FORM_OTP, @@ -1291,7 +1293,7 @@ describe('Verification controller', () => { MockFormService.retrieveFullFormById.mockReturnValueOnce( okAsync(MOCK_SGID_FORM), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const expectedResponse = { @@ -1309,9 +1311,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwtSgid, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) @@ -1333,7 +1335,7 @@ describe('Verification controller', () => { MockFormService.retrieveFullFormById.mockReturnValueOnce( okAsync(MOCK_SGID_FORM), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const expectedResponse = { @@ -1351,9 +1353,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwt, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwt) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) @@ -1620,7 +1622,9 @@ describe('Verification controller', () => { expect(mockSpOidcServiceClass.extractJwtPayload).not.toHaveBeenCalled() expect(mockCpOidcServiceClass.extractJwtPayload).not.toHaveBeenCalled() - expect(MockSgidService.extractSgidJwtPayload).not.toHaveBeenCalled() + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).not.toHaveBeenCalled() expect(MockMyInfoUtil.extractMyInfoLoginJwt).not.toHaveBeenCalled() expect(MockMyInfoService.verifyLoginJwt).not.toHaveBeenCalled() expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() @@ -1762,7 +1766,7 @@ describe('Verification controller', () => { MockVerificationService.disableVerifiedFieldsIfRequired.mockReturnValueOnce( okAsync(true), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( ok(MOCK_VALID_SGID_PAYLOAD), ) @@ -1777,9 +1781,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwtSgid, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).toHaveBeenCalledWith( EXPECTED_PARAMS_FOR_SENDING_PAYMENT_OTP, @@ -2188,7 +2192,7 @@ describe('Verification controller', () => { MockFormService.retrieveFullFormById.mockReturnValueOnce( okAsync(MOCK_SGID_FORM), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidMissingJwtError()), ) const expectedResponse = { @@ -2206,9 +2210,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwtSgid, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) @@ -2230,7 +2234,7 @@ describe('Verification controller', () => { MockFormService.retrieveFullFormById.mockReturnValueOnce( okAsync(MOCK_SGID_FORM), ) - MockSgidService.extractSgidJwtPayload.mockReturnValueOnce( + MockSgidService.extractSgidSingpassJwtPayload.mockReturnValueOnce( err(new SgidInvalidJwtError()), ) const expectedResponse = { @@ -2248,9 +2252,9 @@ describe('Verification controller', () => { expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( MOCK_FORM_ID, ) - expect(MockSgidService.extractSgidJwtPayload).toHaveBeenCalledWith( - MOCK_SGID_REQ.cookies.jwt, - ) + expect( + MockSgidService.extractSgidSingpassJwtPayload, + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwt) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) From 0f1cbe3b0f9fe9dfab812e31c835238e16abf796 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Thu, 18 May 2023 14:49:54 +0800 Subject: [PATCH 16/22] Address Shu Li's review --- .../public-form/public-form.controller.ts | 9 ++++--- src/app/modules/sgid/sgid.constants.ts | 6 +++++ src/app/modules/sgid/sgid.controller.ts | 26 ++++++++++++------- .../encrypt-submission.controller.ts | 3 ++- .../__tests__/verification.controller.spec.ts | 9 ++++--- .../verification/verification.controller.ts | 10 ++++--- 6 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 87ceb00e82..d80af47b05 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -34,7 +34,10 @@ import { validateMyInfoForm, } from '../../myinfo/myinfo.util' import { SGIDMyInfoData } from '../../sgid/sgid.adapter' -import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' +import { + SGID_COOKIE_NAME, + SGID_MYINFO_COOKIE_NAME, +} from '../../sgid/sgid.constants' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' @@ -326,14 +329,14 @@ export const handleGetPublicForm: ControllerHandler< return res.json({ form: publicForm, isIntranetUser }) }) case FormAuthType.SGID_MyInfo: { - const accessTokenCookie = req.cookies[SGID_COOKIE_NAME] + const accessTokenCookie = req.cookies[SGID_MYINFO_COOKIE_NAME] if (!accessTokenCookie) { return res.json({ form: publicForm, isIntranetUser, }) } - res.clearCookie(SGID_COOKIE_NAME) + res.clearCookie(SGID_MYINFO_COOKIE_NAME) res.clearCookie(MYINFO_LOGIN_COOKIE_NAME) return SgidService.extractSgidJwtMyInfoPayload(accessTokenCookie) .asyncAndThen((auth) => diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index cce0e2f6a7..e4bfa247c7 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -4,6 +4,12 @@ */ export const SGID_COOKIE_NAME = 'jwtSgid' +/** + * Name of cookie containing sgID access token to + * MyInfo scopes. + */ +export const SGID_MYINFO_COOKIE_NAME = 'jwtSgidMyInfo' + export const SGID_MYINFO_NRIC_NUMBER_SCOPE = 'myinfo.nric_number' export enum SGIDScope { diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index b89e59ca83..6d53714bae 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -7,7 +7,7 @@ import { createLoggerWithLabel } from '../../config/logger' import { ControllerHandler } from '../core/core.types' import * as FormService from '../form/form.service' -import { SGID_COOKIE_NAME } from './sgid.constants' +import { SGID_COOKIE_NAME, SGID_MYINFO_COOKIE_NAME } from './sgid.constants' import { SgidService } from './sgid.service' const logger = createLoggerWithLabel(module) @@ -52,17 +52,25 @@ export const handleLogin: ControllerHandler< (data) => SgidService.createSgidMyInfoJwt(data.accessToken, rememberMe), ) - if (!jwtResult.isErr()) { - const { maxAge, jwt } = jwtResult.value - res.cookie(SGID_COOKIE_NAME, jwt, { - maxAge, - httpOnly: true, - sameSite: 'lax', // Setting to 'strict' prevents Singpass login on Safari, Firefox - secure: !config.isDev, - ...SgidService.getCookieSettings(), + if (jwtResult.isErr()) { + logger.error({ + message: 'Error while handling login via sgID (MyInfo)', + meta, + error: jwtResult.error, }) + res.cookie('isLoginError', true) return res.redirect(target) } + + const { maxAge, jwt } = jwtResult.value + res.cookie(SGID_MYINFO_COOKIE_NAME, jwt, { + maxAge, + httpOnly: true, + sameSite: 'lax', // Setting to 'strict' prevents Singpass login on Safari, Firefox + secure: !config.isDev, + ...SgidService.getCookieSettings(), + }) + return res.redirect(target) } if (form.authType !== FormAuthType.SGID) { logger.error({ diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts index cac9763323..65282c835f 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts @@ -37,6 +37,7 @@ import { ControllerHandler } from '../../core/core.types' import { setFormTags } from '../../datadog/datadog.utils' import { PermissionLevel } from '../../form/admin-form/admin-form.types' import * as FormService from '../../form/form.service' +import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' import { SgidService } from '../../sgid/sgid.service' import { getOidcService } from '../../spcp/spcp.oidc.service' import { getPopulatedUserById } from '../../user/user.service' @@ -260,7 +261,7 @@ const submitEncryptModeForm: ControllerHandler< } case FormAuthType.SGID: { const jwtPayloadResult = SgidService.extractSgidSingpassJwtPayload( - req.cookies.jwtSgid, + req.cookies[SGID_COOKIE_NAME], ) if (jwtPayloadResult.isErr()) { const { statusCode, errorMessage } = mapRouteError( diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts index 92d5afd558..55beb283e1 100644 --- a/src/app/modules/verification/__tests__/verification.controller.spec.ts +++ b/src/app/modules/verification/__tests__/verification.controller.spec.ts @@ -37,6 +37,7 @@ import { MyInfoInvalidLoginCookieError, MyInfoMissingLoginCookieError, } from '../../myinfo/myinfo.errors' +import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' import { SgidInvalidJwtError, SgidMissingJwtError, @@ -884,7 +885,7 @@ describe('Verification controller', () => { ) expect( MockSgidService.extractSgidSingpassJwtPayload, - ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies[SGID_COOKIE_NAME]) expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).toHaveBeenCalledWith( EXPECTED_PARAMS_FOR_SENDING_FORM_OTP, @@ -1313,7 +1314,7 @@ describe('Verification controller', () => { ) expect( MockSgidService.extractSgidSingpassJwtPayload, - ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies[SGID_COOKIE_NAME]) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) @@ -1783,7 +1784,7 @@ describe('Verification controller', () => { ) expect( MockSgidService.extractSgidSingpassJwtPayload, - ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies[SGID_COOKIE_NAME]) expect(MockOtpUtils.generateOtpWithHash).toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).toHaveBeenCalledWith( EXPECTED_PARAMS_FOR_SENDING_PAYMENT_OTP, @@ -2212,7 +2213,7 @@ describe('Verification controller', () => { ) expect( MockSgidService.extractSgidSingpassJwtPayload, - ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies.jwtSgid) + ).toHaveBeenCalledWith(MOCK_SGID_REQ.cookies[SGID_COOKIE_NAME]) expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() expect(MockVerificationService.sendNewOtp).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) diff --git a/src/app/modules/verification/verification.controller.ts b/src/app/modules/verification/verification.controller.ts index e8a814d900..bc384e07a0 100644 --- a/src/app/modules/verification/verification.controller.ts +++ b/src/app/modules/verification/verification.controller.ts @@ -16,6 +16,7 @@ import { setFormTags } from '../datadog/datadog.utils' import * as FormService from '../form/form.service' import { MyInfoService } from '../myinfo/myinfo.service' import * as MyInfoUtil from '../myinfo/myinfo.util' +import { SGID_COOKIE_NAME } from '../sgid/sgid.constants' import { SgidService } from '../sgid/sgid.service' import { getOidcService } from '../spcp/spcp.oidc.service' @@ -263,11 +264,9 @@ export const _handleGenerateOtp: ControllerHandler< return error }) } - // SGID_MyInfo temporarily here - case FormAuthType.SGID_MyInfo: case FormAuthType.SGID: return SgidService.extractSgidSingpassJwtPayload( - req.cookies.jwtSgid, + req.cookies[SGID_COOKIE_NAME], ) .map(() => form) .mapErr((error) => { @@ -278,13 +277,16 @@ export const _handleGenerateOtp: ControllerHandler< }) return error }) + case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: return MyInfoUtil.extractMyInfoLoginJwt(req.cookies) .andThen(MyInfoService.verifyLoginJwt) .map(() => form) .mapErr((error) => { logger.error({ - message: 'Failed to verify MyInfo hashes', + message: `Failed to verify MyInfo${ + authType === FormAuthType.SGID_MyInfo ? '(over sgID)' : '' + } hashes`, meta: logMeta, error, }) From d999e4ffaa75304b54fd6704e53c5ddb7ce6b633 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Fri, 19 May 2023 11:07:28 +0800 Subject: [PATCH 17/22] Address second round of reviews --- src/app/modules/sgid/sgid.controller.ts | 39 +++++++++++-------- src/app/modules/sgid/sgid.service.ts | 20 +++++++--- src/app/modules/sgid/sgid.types.ts | 3 +- .../encrypt-submission.controller.ts | 6 ++- 4 files changed, 42 insertions(+), 26 deletions(-) diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 6d53714bae..0eda565c48 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -7,7 +7,11 @@ import { createLoggerWithLabel } from '../../config/logger' import { ControllerHandler } from '../core/core.types' import * as FormService from '../form/form.service' -import { SGID_COOKIE_NAME, SGID_MYINFO_COOKIE_NAME } from './sgid.constants' +import { + SGID_COOKIE_NAME, + SGID_MYINFO_COOKIE_NAME, + SGID_MYINFO_NRIC_NUMBER_SCOPE, +} from './sgid.constants' import { SgidService } from './sgid.service' const logger = createLoggerWithLabel(module) @@ -46,10 +50,25 @@ export const handleLogin: ControllerHandler< } const form = formResult.value - // @TODO move this to separate route. + + if ( + form.authType !== FormAuthType.SGID && + form.authType !== FormAuthType.SGID_MyInfo + ) { + logger.error({ + message: "Log in attempt to wrong endpoint for form's authType", + meta: { + ...meta, + formAuthType: form.authType, + endpointAuthType: FormAuthType.SGID, + }, + }) + res.cookie('isLoginError', true) + return res.redirect(target) + } if (form.authType === FormAuthType.SGID_MyInfo) { const jwtResult = await SgidService.retrieveAccessToken(code).andThen( - (data) => SgidService.createSgidMyInfoJwt(data.accessToken, rememberMe), + (data) => SgidService.createSgidMyInfoJwt(data.accessToken), ) if (jwtResult.isErr()) { @@ -72,24 +91,12 @@ export const handleLogin: ControllerHandler< }) return res.redirect(target) } - if (form.authType !== FormAuthType.SGID) { - logger.error({ - message: "Log in attempt to wrong endpoint for form's authType", - meta: { - ...meta, - formAuthType: form.authType, - endpointAuthType: FormAuthType.SGID, - }, - }) - res.cookie('isLoginError', true) - return res.redirect(target) - } const jwtResult = await SgidService.retrieveAccessToken(code) .andThen((data) => SgidService.retrieveUserInfo(data)) .andThen(({ data }) => SgidService.createSgidSingpassJwt( - { 'myinfo.nric_number': data['myinfo.nric_number'] }, + { 'myinfo.nric_number': data[SGID_MYINFO_NRIC_NUMBER_SCOPE] }, rememberMe, ), ) diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index a2a18b3c43..f0c45b058d 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -169,7 +169,8 @@ export class SgidServiceClass { /** * Given the OIDC access token from sgID, obtain the - * user's NRIC number and proxy id + * user's information (depending on OAuth scopes + * associated with the accessToken) and proxy id * @param accessToken - the access token */ retrieveUserInfo({ @@ -211,7 +212,7 @@ export class SgidServiceClass { rememberMe: boolean, ): Result<{ jwt: string; maxAge: number }, ApplicationError> { const userName = data['myinfo.nric_number'] - const payload = { userName, rememberMe } + const payload: SGIDJwtSingpassPayload = { userName, rememberMe } const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { algorithm: JWT_ALGORITHM, @@ -224,16 +225,23 @@ export class SgidServiceClass { } /** - * Create a JWT based with access token from sgID + * Create a JWT with access token from sgID + * + * This access token is then used to exchange for MyInfo data in the + * public form controller. + * + * Unlike createSgidSingpassJwt, where the access token is exchanged for + * userinfo upfront and userinfo (NRIC) is stored in the JWT. + * + * Note: sgID access token is tied to the sgID OAuth scopes requested. * @param accessToken - sgID access token * @param rememberMe - determines how long the JWT is valid for */ createSgidMyInfoJwt( accessToken: string, - rememberMe: boolean, ): Result<{ jwt: string; maxAge: number }, ApplicationError> { - const payload: SGIDJwtAccessPayload = { accessToken, rememberMe } - const maxAge = rememberMe ? this.cookieMaxAgePreserved : this.cookieMaxAge + const payload: SGIDJwtAccessPayload = { accessToken } + const maxAge = this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { algorithm: JWT_ALGORITHM, expiresIn: maxAge / 1000, diff --git a/src/app/modules/sgid/sgid.types.ts b/src/app/modules/sgid/sgid.types.ts index 43a46d72e7..ad35366664 100644 --- a/src/app/modules/sgid/sgid.types.ts +++ b/src/app/modules/sgid/sgid.types.ts @@ -7,10 +7,9 @@ export type SgidForm = T & { export type SGIDScopeToValue = Record -export type SGIDJwtSingpassPayload = { userName: string } +export type SGIDJwtSingpassPayload = { userName: string; rememberMe: boolean } export type SGIDJwtAccessPayload = { accessToken: string - rememberMe: boolean } export type SGIDJwtVerifierFunction< diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts index 65282c835f..34fb6b2d83 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts @@ -201,10 +201,12 @@ const submitEncryptModeForm: ControllerHandler< let userInfo const { authType } = form switch (authType) { + case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: { logger.error({ - message: - 'Storage mode form is not allowed to have MyInfo authorisation', + message: `Storage mode form is not allowed to have MyInfo${ + authType == FormAuthType.SGID_MyInfo ? '(over sgID)' : '' + } authorisation`, meta: logMeta, }) const { errorMessage, statusCode } = mapRouteError( From 56cfb0a2e858ee27b1b41caf43a9705bf720ee3c Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Fri, 19 May 2023 11:40:31 +0800 Subject: [PATCH 18/22] Split up login cookies for MyInfo and sgID-MyInfo --- .../form/public-form/public-form.controller.ts | 5 +++-- src/app/modules/myinfo/myinfo.util.ts | 9 ++++++++- src/app/modules/sgid/sgid.constants.ts | 14 ++++++++++++-- src/app/modules/sgid/sgid.util.ts | 4 +--- .../email-submission.controller.ts | 12 ++++++++++-- .../__tests__/verification.controller.spec.ts | 6 ++++++ .../verification/verification.controller.ts | 2 +- 7 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index d80af47b05..c23f70407c 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -37,6 +37,7 @@ import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { SGID_COOKIE_NAME, SGID_MYINFO_COOKIE_NAME, + SGID_MYINFO_LOGIN_COOKIE_NAME, } from '../../sgid/sgid.constants' import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' @@ -337,7 +338,7 @@ export const handleGetPublicForm: ControllerHandler< }) } res.clearCookie(SGID_MYINFO_COOKIE_NAME) - res.clearCookie(MYINFO_LOGIN_COOKIE_NAME) + res.clearCookie(SGID_MYINFO_LOGIN_COOKIE_NAME) return SgidService.extractSgidJwtMyInfoPayload(accessTokenCookie) .asyncAndThen((auth) => SgidService.retrieveUserInfo({ accessToken: auth.accessToken }), @@ -351,7 +352,7 @@ export const handleGetPublicForm: ControllerHandler< ).map((prefilledFields) => { return res .cookie( - MYINFO_LOGIN_COOKIE_NAME, + SGID_MYINFO_LOGIN_COOKIE_NAME, createMyInfoLoginCookie(data.getUinFin()), MYINFO_LOGIN_COOKIE_OPTIONS, ) diff --git a/src/app/modules/myinfo/myinfo.util.ts b/src/app/modules/myinfo/myinfo.util.ts index 9ec50a820f..6d43c31a23 100644 --- a/src/app/modules/myinfo/myinfo.util.ts +++ b/src/app/modules/myinfo/myinfo.util.ts @@ -24,6 +24,7 @@ import { FormAuthNoEsrvcIdError, FormNotFoundError, } from '../form/form.errors' +import { SGID_MYINFO_LOGIN_COOKIE_NAME } from '../sgid/sgid.constants' import { ProcessedFieldResponse } from '../submission/submission.types' import { MYINFO_LOGIN_COOKIE_NAME } from './myinfo.constants' @@ -299,8 +300,14 @@ export const isMyInfoAuthCodeCookie = ( */ export const extractMyInfoLoginJwt = ( cookies: Record, + authType: FormAuthType.MyInfo | FormAuthType.SGID_MyInfo, ): Result => { - const jwt = cookies[MYINFO_LOGIN_COOKIE_NAME] + const jwt = + cookies[ + authType === FormAuthType.MyInfo + ? MYINFO_LOGIN_COOKIE_NAME + : SGID_MYINFO_LOGIN_COOKIE_NAME + ] if (typeof jwt === 'string' && !!jwt) { return ok(jwt) } diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index e4bfa247c7..765a0cabde 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -1,6 +1,6 @@ /** - * Name of cookie which contains state of sgid login, and access - * token if login was successful. + * Name of cookie which contains state of sgID login, and NRIC + * if login was successful. */ export const SGID_COOKIE_NAME = 'jwtSgid' @@ -10,6 +10,16 @@ export const SGID_COOKIE_NAME = 'jwtSgid' */ export const SGID_MYINFO_COOKIE_NAME = 'jwtSgidMyInfo' +/** + * Name of cookie which contains state of sgID login, and NRIC + * if login was successful. + * + * Note: same purpose as SGID_COOKIE_NAME. Just separate cookie + * to prevent interaction between SGID Auth only mode and + * SGID MyInfo mode. + */ +export const SGID_MYINFO_LOGIN_COOKIE_NAME = 'jwtSgidMyInfoLogin' + export const SGID_MYINFO_NRIC_NUMBER_SCOPE = 'myinfo.nric_number' export enum SGIDScope { diff --git a/src/app/modules/sgid/sgid.util.ts b/src/app/modules/sgid/sgid.util.ts index a617fa18de..72b793c35e 100644 --- a/src/app/modules/sgid/sgid.util.ts +++ b/src/app/modules/sgid/sgid.util.ts @@ -80,8 +80,6 @@ export const isSgidJwtAccessPayload = ( typeof payload === 'object' && !!payload && hasProp(payload, 'accessToken') && - typeof payload.accessToken === 'string' && - hasProp(payload, 'rememberMe') && - typeof payload.rememberMe === 'boolean' + typeof payload.accessToken === 'string' ) } diff --git a/src/app/modules/submission/email-submission/email-submission.controller.ts b/src/app/modules/submission/email-submission/email-submission.controller.ts index 80a5f78c83..67e633a046 100644 --- a/src/app/modules/submission/email-submission/email-submission.controller.ts +++ b/src/app/modules/submission/email-submission/email-submission.controller.ts @@ -21,7 +21,10 @@ import { } from '../../myinfo/myinfo.constants' import { MyInfoService } from '../../myinfo/myinfo.service' import { extractMyInfoLoginJwt } from '../../myinfo/myinfo.util' -import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' +import { + SGID_COOKIE_NAME, + SGID_MYINFO_LOGIN_COOKIE_NAME, +} from '../../sgid/sgid.constants' import { SgidService } from '../../sgid/sgid.service' import { getOidcService } from '../../spcp/spcp.oidc.service' import * as EmailSubmissionMiddleware from '../email-submission/email-submission.middleware' @@ -204,7 +207,7 @@ const submitEmailModeForm: ControllerHandler< } case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: - return extractMyInfoLoginJwt(req.cookies) + return extractMyInfoLoginJwt(req.cookies, authType) .andThen(MyInfoService.verifyLoginJwt) .asyncAndThen(({ uinFin }) => MyInfoService.fetchMyInfoHashes(uinFin, formId) @@ -365,8 +368,13 @@ const submitEmailModeForm: ControllerHandler< }) }) // MyInfo access token is single-use, so clear it + // Similarly for sgID-MyInfo return res .clearCookie(MYINFO_LOGIN_COOKIE_NAME, MYINFO_LOGIN_COOKIE_OPTIONS) + .clearCookie( + SGID_MYINFO_LOGIN_COOKIE_NAME, + MYINFO_LOGIN_COOKIE_OPTIONS, + ) .json({ // Return the reply early to the submitter message: 'Form submission successful.', diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts index 55beb283e1..65871d9a79 100644 --- a/src/app/modules/verification/__tests__/verification.controller.spec.ts +++ b/src/app/modules/verification/__tests__/verification.controller.spec.ts @@ -928,6 +928,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_FORM_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).toHaveBeenCalledWith( MOCK_MYINFO_JWT, @@ -1388,6 +1389,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_FORM_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).not.toHaveBeenCalled() expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() @@ -1424,6 +1426,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_FORM_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).toHaveBeenCalledWith( MOCK_MYINFO_JWT, @@ -1827,6 +1830,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_PAYMENT_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).toHaveBeenCalledWith( MOCK_MYINFO_JWT, @@ -2287,6 +2291,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_PAYMENT_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).not.toHaveBeenCalled() expect(MockOtpUtils.generateOtpWithHash).not.toHaveBeenCalled() @@ -2323,6 +2328,7 @@ describe('Verification controller', () => { ) expect(MockMyInfoUtil.extractMyInfoLoginJwt).toHaveBeenCalledWith( MOCK_PAYMENT_REQ.cookies, + FormAuthType.MyInfo, ) expect(MockMyInfoService.verifyLoginJwt).toHaveBeenCalledWith( MOCK_MYINFO_JWT, diff --git a/src/app/modules/verification/verification.controller.ts b/src/app/modules/verification/verification.controller.ts index bc384e07a0..7122927718 100644 --- a/src/app/modules/verification/verification.controller.ts +++ b/src/app/modules/verification/verification.controller.ts @@ -279,7 +279,7 @@ export const _handleGenerateOtp: ControllerHandler< }) case FormAuthType.SGID_MyInfo: case FormAuthType.MyInfo: - return MyInfoUtil.extractMyInfoLoginJwt(req.cookies) + return MyInfoUtil.extractMyInfoLoginJwt(req.cookies, authType) .andThen(MyInfoService.verifyLoginJwt) .map(() => form) .mapErr((error) => { From f7e90f7866595b9eac46b6ad956cb8983f42210b Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Mon, 22 May 2023 10:29:51 +0800 Subject: [PATCH 19/22] Remove unsued param docs --- src/app/modules/sgid/sgid.service.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index f0c45b058d..5727bd4ae4 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -235,7 +235,6 @@ export class SgidServiceClass { * * Note: sgID access token is tied to the sgID OAuth scopes requested. * @param accessToken - sgID access token - * @param rememberMe - determines how long the JWT is valid for */ createSgidMyInfoJwt( accessToken: string, From 4d6a58db449970b3df1c4e42250a4be1183f0e82 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Mon, 22 May 2023 16:32:10 +0800 Subject: [PATCH 20/22] fix: turn off rememberMe for sgID over MyInfo --- src/app/modules/form/public-form/public-form.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index c23f70407c..cdcca1ad78 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -469,7 +469,7 @@ export const _handleFormAuthRedirect: ControllerHandler< return validateSgidForm(form).andThen(() => { return SgidService.createRedirectUrl( formId, - Boolean(isPersistentLogin), + false, form.getUniqueMyInfoAttrs(), encodedQuery, ) From d88b4c0c66ea89c722fc3fd5b7efccda365cccb1 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Thu, 25 May 2023 13:25:16 +0800 Subject: [PATCH 21/22] fix: address reviews --- src/app/modules/form/public-form/public-form.service.ts | 6 +++++- src/app/modules/sgid/sgid.controller.ts | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/app/modules/form/public-form/public-form.service.ts b/src/app/modules/form/public-form/public-form.service.ts index 5c47bc02fb..094fc016e0 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -8,7 +8,10 @@ import getFormModel from '../../../models/form.server.model' import getFormFeedbackModel from '../../../models/form_feedback.server.model' import { DatabaseError } from '../../core/core.errors' import { MYINFO_LOGIN_COOKIE_NAME } from '../../myinfo/myinfo.constants' -import { SGID_COOKIE_NAME } from '../../sgid/sgid.constants' +import { + SGID_COOKIE_NAME, + SGID_MYINFO_LOGIN_COOKIE_NAME, +} from '../../sgid/sgid.constants' import { JwtName } from '../../spcp/spcp.types' import { FormNotFoundError } from '../form.errors' @@ -78,6 +81,7 @@ export const getCookieNameByAuthType = ( ): string => { switch (authType) { case FormAuthType.SGID_MyInfo: + return SGID_MYINFO_LOGIN_COOKIE_NAME case FormAuthType.MyInfo: return MYINFO_LOGIN_COOKIE_NAME case FormAuthType.SGID: diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 0eda565c48..73b5835068 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -96,7 +96,9 @@ export const handleLogin: ControllerHandler< .andThen((data) => SgidService.retrieveUserInfo(data)) .andThen(({ data }) => SgidService.createSgidSingpassJwt( - { 'myinfo.nric_number': data[SGID_MYINFO_NRIC_NUMBER_SCOPE] }, + { + [SGID_MYINFO_NRIC_NUMBER_SCOPE]: data[SGID_MYINFO_NRIC_NUMBER_SCOPE], + }, rememberMe, ), ) From 39f311bafdbec256872a47acbb90c3c803038291 Mon Sep 17 00:00:00 2001 From: Ken Jin <119096102+kenjin-work@users.noreply.github.com> Date: Fri, 9 Jun 2023 11:20:37 +0800 Subject: [PATCH 22/22] refactor: address review --- shared/types/converter.ts | 2 +- src/app/modules/myinfo/myinfo.adapter.ts | 4 ++-- src/app/modules/sgid/__tests__/sgid.service.spec.ts | 2 +- src/app/modules/sgid/sgid.adapter.ts | 13 +++++++------ 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/shared/types/converter.ts b/shared/types/converter.ts index f4bd8f6aaf..6b7c7df0e7 100644 --- a/shared/types/converter.ts +++ b/shared/types/converter.ts @@ -1,4 +1,4 @@ -export interface SGIDDataTransformer { +export interface MyInfoDataTransformer { /** * NRIC/FIN. diff --git a/src/app/modules/myinfo/myinfo.adapter.ts b/src/app/modules/myinfo/myinfo.adapter.ts index c028212d48..81493a8170 100644 --- a/src/app/modules/myinfo/myinfo.adapter.ts +++ b/src/app/modules/myinfo/myinfo.adapter.ts @@ -8,7 +8,7 @@ import { import { MyInfoAttribute as InternalAttr, - SGIDDataTransformer, + MyInfoDataTransformer, } from '../../../../shared/types' import { @@ -159,7 +159,7 @@ export const internalAttrListToScopes = ( * to the correct keys in the data. */ export class MyInfoData - implements SGIDDataTransformer + implements MyInfoDataTransformer { #personData: IPerson #uinFin: string diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index aac0663276..a7aa4a5efc 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -83,7 +83,7 @@ describe('sgid.service', () => { ) }) - it('should return extract additional OAuth scopes from MyInfo', () => { + it('should extract additional OAuth scopes from MyInfo', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) sgidClient.authorizationUrl.mockReturnValue({ diff --git a/src/app/modules/sgid/sgid.adapter.ts b/src/app/modules/sgid/sgid.adapter.ts index 6664f4daac..1885c239f7 100644 --- a/src/app/modules/sgid/sgid.adapter.ts +++ b/src/app/modules/sgid/sgid.adapter.ts @@ -1,6 +1,6 @@ import { MyInfoAttribute as InternalAttr, - SGIDDataTransformer, + MyInfoDataTransformer, } from '../../../../shared/types' import { @@ -32,10 +32,11 @@ export const internalAttrToScope = (attr: InternalAttr): ExternalAttr => { } export const internalAttrListToScopes = (attrs: InternalAttr[]): string => - // Always ask for NRIC - `openid ${ExternalAttr.NricFin} ${attrs - .map(internalAttrToScope) - .join(' ')}`.trim() + [ + 'openid', + // Deduplicate and always ask for NRIC + ...new Set([ExternalAttr.NricFin, ...attrs.map(internalAttrToScope)]), + ].join(' ') const internalAttrToSGIDExternal = ( attr: InternalAttr, @@ -64,7 +65,7 @@ const internalAttrToSGIDExternal = ( * Currently only supports MyInfo. */ export class SGIDMyInfoData - implements SGIDDataTransformer + implements MyInfoDataTransformer { #payload: SGIDScopeToValue constructor(payload: SGIDScopeToValue) {