Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: MyInfo over sgID backend #6337

Merged
merged 23 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f04b12f
feat: MyInfo over sgID backend
kenjin-work May 16, 2023
efb3c54
refactor: reduce diff
kenjin-work May 16, 2023
bf34beb
feat: MyInfo over sgID backend
kenjin-work May 16, 2023
4d6c5eb
refactor: reduce diff
kenjin-work May 16, 2023
55a7636
Merge branch 'feat/sgid-myinfo-backend-beta' of https://github.com/op…
kenjin-work May 16, 2023
d0e1d3c
fix: return undefined in default cases
kenjin-work May 16, 2023
c8b73c1
fix: sgID validation of forms
kenjin-work May 16, 2023
e263733
fix: sgID submission tests
kenjin-work May 16, 2023
893c5c4
fix: email submission route tests
kenjin-work May 16, 2023
046159f
feat: split sgID auth and sgID MyInfo
kenjin-work May 17, 2023
35357d7
fix: rename functions
kenjin-work May 17, 2023
9a7e45b
fix: tests, add SGID_MyInfo form filling
kenjin-work May 17, 2023
f7ca1ce
feat: backend submissions and logout done
kenjin-work May 17, 2023
477078c
fix: sgID routes tests
kenjin-work May 17, 2023
4e547c0
fix: actually fix sgID routes tests
kenjin-work May 17, 2023
646f503
fix: verification controller tests
kenjin-work May 17, 2023
0f1cbe3
Address Shu Li's review
kenjin-work May 18, 2023
d999e4f
Address second round of reviews
kenjin-work May 19, 2023
56cfb0a
Split up login cookies for MyInfo and sgID-MyInfo
kenjin-work May 19, 2023
f7e90f7
Remove unsued param docs
kenjin-work May 22, 2023
4d6a58d
fix: turn off rememberMe for sgID over MyInfo
kenjin-work May 22, 2023
d88b4c0
fix: address reviews
kenjin-work May 25, 2023
39f311b
refactor: address review
kenjin-work Jun 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions shared/types/converter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
export interface SGIDDataTransformer<T, U> {

/**
* 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
}
}
1 change: 1 addition & 0 deletions shared/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ export * from './submission'
export * from './user'
export * from './utils'
export * from './payment'
export * from './converter'
5 changes: 3 additions & 2 deletions src/app/modules/form/public-form/public-form.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,11 @@ export const handleGetPublicForm: ControllerHandler<
}
case FormAuthType.SGID:
return SgidService.extractSgidJwtPayload(req.cookies.jwtSgid)
.map((spcpSession) => {
.map((data) => {
return res.json({
form: publicForm,
isIntranetUser,
spcpSession,
spcpSession: { userName: data.getUinFin() },
})
})
.mapErr((error) => {
Expand Down Expand Up @@ -403,6 +403,7 @@ export const _handleFormAuthRedirect: ControllerHandler<
return SgidService.createRedirectUrl(
formId,
Boolean(isPersistentLogin),
[],
kenjin-work marked this conversation as resolved.
Show resolved Hide resolved
encodedQuery,
)
})
Expand Down
9 changes: 7 additions & 2 deletions src/app/modules/myinfo/myinfo.adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<ExternalAttr, InternalAttr>
kenjin-work marked this conversation as resolved.
Show resolved Hide resolved
{
#personData: IPerson
#uinFin: string

Expand Down
3 changes: 2 additions & 1 deletion src/app/modules/myinfo/myinfo.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -241,7 +242,7 @@ export class MyInfoServiceClass {
*/
prefillAndSaveMyInfoFields(
formId: string,
myInfoData: MyInfoData,
myInfoData: MyInfoData | SGIDMyInfoData,
currFormFields: LeanDocument<IFieldSchema[]>,
): ResultAsync<PossiblyPrefilledField[], MyInfoHashingError | DatabaseError> {
const prefilledFields = currFormFields.map((field) => {
Expand Down
52 changes: 43 additions & 9 deletions src/app/modules/sgid/__tests__/sgid.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
import { SgidClient } from '@opengovsg/sgid-client'
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,
SgidFetchAccessTokenError,
Expand All @@ -12,7 +15,7 @@ import {
SgidMissingJwtError,
SgidVerifyJwtError,
} from '../sgid.errors'
import { SGID_SCOPES, SgidServiceClass } from '../sgid.service'
import { SgidServiceClass } from '../sgid.service'

import {
MOCK_ACCESS_TOKEN,
Expand All @@ -29,6 +32,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')
Expand Down Expand Up @@ -68,14 +74,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_DEFAULT_SCOPES,
null,
)
})

it('should return extract additional OAuth scopes from MyInfo', () => {
kenjin-work marked this conversation as resolved.
Show resolved Hide resolved
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,
SGID_SCOPES,
'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])
Expand All @@ -87,11 +115,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,
)
})
Expand Down Expand Up @@ -135,17 +164,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 () => {
Expand All @@ -171,6 +201,7 @@ describe('sgid.service', () => {
})
expect(MockJwt.sign).toHaveBeenCalledWith(
{
data: { 'myinfo.nric_number': 'S9322889A' },
userName: MOCK_USER_INFO.data['myinfo.nric_number'],
rememberMe: false,
},
Expand All @@ -193,7 +224,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,
Expand All @@ -210,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)
expect(result._unsafeUnwrap()).toStrictEqual(
new SGIDMyInfoData(MOCK_JWT_PAYLOAD.data),
)
expect(MockJwt.verify).toHaveBeenCalledWith(
MOCK_JWT,
MOCK_OPTIONS.publicKeyPath,
Expand Down
5 changes: 4 additions & 1 deletion src/app/modules/sgid/__tests__/sgid.test.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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,
Expand Down
Loading