From 5c79ac816a829db96f4a7c2f9bcc83a157091342 Mon Sep 17 00:00:00 2001 From: Chiew Kok Seng <47116462+kschiew@users.noreply.github.com> Date: Fri, 18 Aug 2023 15:58:32 +0800 Subject: [PATCH] feat: upgrade sgid sdk (#6584) * feat: upgrade to sgID v2 SDK * chore: update docker setup * chore: update package.json * fix: update bson version for tests * chore: update package-lock.json * chore: commit package.json files with no-verify * chore: remove console logs * chore: update package-lock * chore: update package-lock * chore: change node version * chore: change npm version * chore: pin bson version * chore: actually pin bson versoin * fix: fix helmet type import * chore: update deps * chore: update mockpass dev dependency version * chore: revert out-of-scope changes * chore: refactors * chore: remove unneeded packages * test: update sgid service tests with generatePkcePair mocks * test: fix tests again --------- Co-authored-by: Justyn Oh --- __tests__/setup/.test-env | 2 +- .../unit/backend/helpers/jest-express.ts | 4 + docker-compose.yml | 4 +- package-lock.json | 90 ++++------ package.json | 4 +- .../modules/auth/sgid/auth-sgid.controller.ts | 34 +++- .../modules/auth/sgid/auth-sgid.service.ts | 29 ++-- .../public-form/public-form.controller.ts | 164 ++++++++++++------ .../sgid/__tests__/sgid.controller.spec.ts | 35 +++- .../sgid/__tests__/sgid.routes.spec.ts | 69 +++++++- .../sgid/__tests__/sgid.service.spec.ts | 109 +++++++++--- .../sgid/__tests__/sgid.test.constants.ts | 4 + src/app/modules/sgid/sgid.constants.ts | 5 + src/app/modules/sgid/sgid.controller.ts | 26 ++- src/app/modules/sgid/sgid.errors.ts | 6 + src/app/modules/sgid/sgid.service.ts | 36 +++- 16 files changed, 440 insertions(+), 181 deletions(-) diff --git a/__tests__/setup/.test-env b/__tests__/setup/.test-env index 9c0e6ebdbd..2f7409d26c 100644 --- a/__tests__/setup/.test-env +++ b/__tests__/setup/.test-env @@ -21,7 +21,7 @@ MYINFO_CLIENT_SECRET=mockClientSecret MYINFO_JWT_SECRET=mockJwtSecret SINGPASS_ESRVC_ID=Test-eServiceId-Sp # Needed for MyInfo -SGID_HOSTNAME=http://localhost:5156/sgid +SGID_HOSTNAME=http://localhost:5156 SGID_CLIENT_ID=sgidclientid SGID_CLIENT_SECRET=sgidclientsecret SGID_FORM_LOGIN_REDIRECT_URI=http://localhost:5000/sgid/login diff --git a/__tests__/unit/backend/helpers/jest-express.ts b/__tests__/unit/backend/helpers/jest-express.ts index fb08c6de4d..b78c7c4127 100644 --- a/__tests__/unit/backend/helpers/jest-express.ts +++ b/__tests__/unit/backend/helpers/jest-express.ts @@ -6,6 +6,7 @@ const mockRequest =

, B, Q = any>({ session, query, secure, + cookies, others = {}, }: { params?: P @@ -13,6 +14,7 @@ const mockRequest =

, B, Q = any>({ session?: Record query?: Q secure?: boolean + cookies?: Record others?: Partial, 'query'>> } = {}): Request => { return { @@ -21,6 +23,7 @@ const mockRequest =

, B, Q = any>({ session: session ?? {}, query: query ?? {}, secure: secure ?? true, + cookies: cookies ?? {}, get(name: string) { if (name === 'cf-connecting-ip') return 'MOCK_IP' return undefined @@ -47,6 +50,7 @@ const mockResponse = ( redirect: jest.fn(), cookie: jest.fn(), set: jest.fn(), + clearCookie: jest.fn(), ...extraArgs, } return mockRes as Response diff --git a/docker-compose.yml b/docker-compose.yml index a4c068e345..c6e9ebb2b3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,7 +81,7 @@ services: - MYINFO_CLIENT_ID=mockClientId - MYINFO_CLIENT_SECRET=mockClientSecret - MYINFO_JWT_SECRET=mockJwtSecret - - SGID_HOSTNAME=http://localhost:5156/sgid + - SGID_HOSTNAME=http://localhost:5156 - SGID_CLIENT_ID=sgidclientid - SGID_CLIENT_SECRET=sgidclientsecret - SGID_ADMIN_LOGIN_REDIRECT_URI=http://localhost:5001/api/v3/auth/sgid/login @@ -121,7 +121,7 @@ services: - API_KEY_VERSION=v1 mockpass: - build: https://github.com/opengovsg/mockpass.git#v3.1.3 + build: https://github.com/opengovsg/mockpass.git#v4.0.4 depends_on: - backend environment: diff --git a/package-lock.json b/package-lock.json index 3225c9a8b7..da99d079b5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,7 +15,7 @@ "@joi/date": "^2.1.0", "@opengovsg/formsg-sdk": "^0.10.0", "@opengovsg/myinfo-gov-client": "^4.1.1", - "@opengovsg/sgid-client": "^1.0.4", + "@opengovsg/sgid-client": "^2.0.0", "@sentry/browser": "^7.51.2", "@sentry/integrations": "^6.19.7", "@stablelib/base64": "^1.0.1", @@ -117,7 +117,7 @@ "@babel/core": "^7.20.7", "@babel/plugin-transform-runtime": "^7.21.4", "@babel/preset-env": "^7.22.5", - "@opengovsg/mockpass": "^3.1.0", + "@opengovsg/mockpass": "^4.0.4", "@playwright/test": "^1.34.3", "@types/bcrypt": "^5.0.0", "@types/bluebird": "^3.5.38", @@ -4989,9 +4989,9 @@ } }, "node_modules/@opengovsg/mockpass": { - "version": "3.1.0", - "resolved": "https://registry.npmjs.org/@opengovsg/mockpass/-/mockpass-3.1.0.tgz", - "integrity": "sha512-n3Ik1xKX4Hlqn2lpbMkUaFwRBPNXdKTXJuCdedqmMpxPF/U/kK0N6BxirZ2yqnXM319RqE4H1/H321usvrTLGQ==", + "version": "4.0.8", + "resolved": "https://registry.npmjs.org/@opengovsg/mockpass/-/mockpass-4.0.8.tgz", + "integrity": "sha512-+/IZ14C+cpNSBjLqfW8sms+OW9am+4FEHHii/gf/O6TJkf4UID9jvIrHtW909tFZ5n/R/SJfowwofRaVeMrKKQ==", "dev": true, "dependencies": { "base-64": "^1.0.0", @@ -4999,6 +4999,7 @@ "dotenv": "^16.0.0", "expiry-map": "^2.0.0", "express": "^4.16.3", + "jose": "^4.14.4", "jsonwebtoken": "^9.0.0", "lodash": "^4.17.11", "morgan": "^1.9.1", @@ -5088,34 +5089,19 @@ } }, "node_modules/@opengovsg/sgid-client": { - "version": "1.0.4", - "resolved": "https://registry.npmjs.org/@opengovsg/sgid-client/-/sgid-client-1.0.4.tgz", - "integrity": "sha512-4FJ7i87S5tP48T+NlTZ4QXNPpAG3eDAg5koHL+twXZsY20e3oW36QpWePbh+ctS893+z0cYvoGWd+/JfnTx2jA==", + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@opengovsg/sgid-client/-/sgid-client-2.0.0.tgz", + "integrity": "sha512-zqcVQz03zB7dAwWh2MJVRAmHYjK1EryqOPnbBgrkr8Jx8BjtcjFa4cCrHstwWP1kVkGomhi0C7e3TRvf1qYSFQ==", "dependencies": { "jose": "4.9.2", "node-rsa": "1.1.1", - "openid-client": "5.1.5" + "openid-client": "5.4.0" } }, "node_modules/@opengovsg/sgid-client/node_modules/jose": { "version": "4.9.2", - "license": "MIT", - "funding": { - "url": "https://github.com/sponsors/panva" - } - }, - "node_modules/@opengovsg/sgid-client/node_modules/openid-client": { - "version": "5.1.5", - "license": "MIT", - "dependencies": { - "jose": "^4.1.4", - "lru-cache": "^6.0.0", - "object-hash": "^2.0.1", - "oidc-token-hash": "^5.0.1" - }, - "engines": { - "node": "^12.19.0 || ^14.15.0 || ^16.13.0" - }, + "resolved": "https://registry.npmjs.org/jose/-/jose-4.9.2.tgz", + "integrity": "sha512-EqKvu2PqJCD3Jrg3PvcYZVS7D21qMVLSYMDAFcOdGUEOpJSLNtJO7NjLANvu3SYHVl6pdP2ff7ve6EZW2nX7Nw==", "funding": { "url": "https://github.com/sponsors/panva" } @@ -18511,9 +18497,9 @@ } }, "node_modules/jose": { - "version": "4.13.1", - "resolved": "https://registry.npmjs.org/jose/-/jose-4.13.1.tgz", - "integrity": "sha512-MSJQC5vXco5Br38mzaQKiq9mwt7lwj2eXpgpRyQYNHYt2lq1PjkWa7DLXX0WVcQLE9HhMh3jPiufS7fhJf+CLQ==", + "version": "4.14.4", + "resolved": "https://registry.npmjs.org/jose/-/jose-4.14.4.tgz", + "integrity": "sha512-j8GhLiKmUAh+dsFXlX1aJCbt5KMibuKb+d7j1JaOJG6s2UjX1PQlW+OKB/sD4a/5ZYF4RcmYmLSndOoU3Lt/3g==", "funding": { "url": "https://github.com/sponsors/panva" } @@ -21922,9 +21908,9 @@ } }, "node_modules/openid-client": { - "version": "5.3.1", - "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-5.3.1.tgz", - "integrity": "sha512-RLfehQiHch9N6tRWNx68cicf3b1WR0x74bJWHRc25uYIbSRwjxYcTFaRnzbbpls5jroLAaB/bFIodTgA5LJMvw==", + "version": "5.4.0", + "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-5.4.0.tgz", + "integrity": "sha512-hgJa2aQKcM2hn3eyVtN12tEA45ECjTJPXCgUh5YzTzy9qwapCvmDTVPWOcWVL0d34zeQoQ/hbG9lJhl3AYxJlQ==", "dependencies": { "jose": "^4.10.0", "lru-cache": "^6.0.0", @@ -32984,9 +32970,9 @@ } }, "@opengovsg/mockpass": { - "version": "3.1.0", - "resolved": "https://registry.npmjs.org/@opengovsg/mockpass/-/mockpass-3.1.0.tgz", - "integrity": "sha512-n3Ik1xKX4Hlqn2lpbMkUaFwRBPNXdKTXJuCdedqmMpxPF/U/kK0N6BxirZ2yqnXM319RqE4H1/H321usvrTLGQ==", + "version": "4.0.8", + "resolved": "https://registry.npmjs.org/@opengovsg/mockpass/-/mockpass-4.0.8.tgz", + "integrity": "sha512-+/IZ14C+cpNSBjLqfW8sms+OW9am+4FEHHii/gf/O6TJkf4UID9jvIrHtW909tFZ5n/R/SJfowwofRaVeMrKKQ==", "dev": true, "requires": { "base-64": "^1.0.0", @@ -32994,6 +32980,7 @@ "dotenv": "^16.0.0", "expiry-map": "^2.0.0", "express": "^4.16.3", + "jose": "^4.14.4", "jsonwebtoken": "^9.0.0", "lodash": "^4.17.11", "morgan": "^1.9.1", @@ -33054,26 +33041,19 @@ } }, "@opengovsg/sgid-client": { - "version": "1.0.4", - "resolved": "https://registry.npmjs.org/@opengovsg/sgid-client/-/sgid-client-1.0.4.tgz", - "integrity": "sha512-4FJ7i87S5tP48T+NlTZ4QXNPpAG3eDAg5koHL+twXZsY20e3oW36QpWePbh+ctS893+z0cYvoGWd+/JfnTx2jA==", + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@opengovsg/sgid-client/-/sgid-client-2.0.0.tgz", + "integrity": "sha512-zqcVQz03zB7dAwWh2MJVRAmHYjK1EryqOPnbBgrkr8Jx8BjtcjFa4cCrHstwWP1kVkGomhi0C7e3TRvf1qYSFQ==", "requires": { "jose": "4.9.2", "node-rsa": "1.1.1", - "openid-client": "5.1.5" + "openid-client": "5.4.0" }, "dependencies": { "jose": { - "version": "4.9.2" - }, - "openid-client": { - "version": "5.1.5", - "requires": { - "jose": "^4.1.4", - "lru-cache": "^6.0.0", - "object-hash": "^2.0.1", - "oidc-token-hash": "^5.0.1" - } + "version": "4.9.2", + "resolved": "https://registry.npmjs.org/jose/-/jose-4.9.2.tgz", + "integrity": "sha512-EqKvu2PqJCD3Jrg3PvcYZVS7D21qMVLSYMDAFcOdGUEOpJSLNtJO7NjLANvu3SYHVl6pdP2ff7ve6EZW2nX7Nw==" } } }, @@ -42379,9 +42359,9 @@ } }, "jose": { - "version": "4.13.1", - "resolved": "https://registry.npmjs.org/jose/-/jose-4.13.1.tgz", - "integrity": "sha512-MSJQC5vXco5Br38mzaQKiq9mwt7lwj2eXpgpRyQYNHYt2lq1PjkWa7DLXX0WVcQLE9HhMh3jPiufS7fhJf+CLQ==" + "version": "4.14.4", + "resolved": "https://registry.npmjs.org/jose/-/jose-4.14.4.tgz", + "integrity": "sha512-j8GhLiKmUAh+dsFXlX1aJCbt5KMibuKb+d7j1JaOJG6s2UjX1PQlW+OKB/sD4a/5ZYF4RcmYmLSndOoU3Lt/3g==" }, "js-sdsl": { "version": "4.1.5", @@ -44698,9 +44678,9 @@ } }, "openid-client": { - "version": "5.3.1", - "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-5.3.1.tgz", - "integrity": "sha512-RLfehQiHch9N6tRWNx68cicf3b1WR0x74bJWHRc25uYIbSRwjxYcTFaRnzbbpls5jroLAaB/bFIodTgA5LJMvw==", + "version": "5.4.0", + "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-5.4.0.tgz", + "integrity": "sha512-hgJa2aQKcM2hn3eyVtN12tEA45ECjTJPXCgUh5YzTzy9qwapCvmDTVPWOcWVL0d34zeQoQ/hbG9lJhl3AYxJlQ==", "requires": { "jose": "^4.10.0", "lru-cache": "^6.0.0", diff --git a/package.json b/package.json index 8d7a06583b..c215f8ff23 100644 --- a/package.json +++ b/package.json @@ -59,7 +59,7 @@ "@joi/date": "^2.1.0", "@opengovsg/formsg-sdk": "^0.10.0", "@opengovsg/myinfo-gov-client": "^4.1.1", - "@opengovsg/sgid-client": "^1.0.4", + "@opengovsg/sgid-client": "^2.0.0", "@sentry/browser": "^7.51.2", "@sentry/integrations": "^6.19.7", "@stablelib/base64": "^1.0.1", @@ -161,7 +161,7 @@ "@babel/core": "^7.20.7", "@babel/plugin-transform-runtime": "^7.21.4", "@babel/preset-env": "^7.22.5", - "@opengovsg/mockpass": "^3.1.0", + "@opengovsg/mockpass": "^4.0.4", "@playwright/test": "^1.34.3", "@types/bcrypt": "^5.0.0", "@types/bluebird": "^3.5.38", diff --git a/src/app/modules/auth/sgid/auth-sgid.controller.ts b/src/app/modules/auth/sgid/auth-sgid.controller.ts index 238b2bbdf4..99033b4703 100644 --- a/src/app/modules/auth/sgid/auth-sgid.controller.ts +++ b/src/app/modules/auth/sgid/auth-sgid.controller.ts @@ -4,6 +4,7 @@ import { ErrorDto, GetSgidAuthUrlResponseDto } from 'shared/types' import { createLoggerWithLabel } from '../../../config/logger' import { createReqMeta } from '../../../utils/request' import { ControllerHandler } from '../../core/core.types' +import { SGID_CODE_VERIFIER_COOKIE_NAME } from '../../sgid/sgid.constants' import * as UserService from '../../user/user.service' import * as AuthService from '../auth.service' import { SessionUser } from '../auth.types' @@ -23,17 +24,25 @@ export const generateAuthUrl: ControllerHandler< } return AuthSgidService.createRedirectUrl() - .map((redirectUrl) => res.status(StatusCodes.OK).send({ redirectUrl })) + .map(({ redirectUrl, codeVerifier }) => + res + .status(StatusCodes.OK) + .cookie(SGID_CODE_VERIFIER_COOKIE_NAME, codeVerifier) + .send({ redirectUrl }), + ) .mapErr((error) => { logger.error({ message: 'Failed to generate SGID auth url', meta: logMeta, error, }) - return res.sendStatus(StatusCodes.INTERNAL_SERVER_ERROR).json({ - message: - 'Generating SGID authentication url failed. Please try again later.', - }) + return res + .sendStatus(StatusCodes.INTERNAL_SERVER_ERROR) + .json({ + message: + 'Generating SGID authentication url failed. Please try again later.', + }) + .clearCookie(SGID_CODE_VERIFIER_COOKIE_NAME) }) } @@ -44,6 +53,8 @@ export const handleLogin: ControllerHandler< { code: string; state: string } > = async (req, res) => { const { code, state } = req.query + const codeVerifier = req.cookies[SGID_CODE_VERIFIER_COOKIE_NAME] + res.clearCookie(SGID_CODE_VERIFIER_COOKIE_NAME) const logMeta = { action: 'handleLogin', @@ -63,11 +74,18 @@ export const handleLogin: ControllerHandler< meta: logMeta, }) + status = StatusCodes.BAD_REQUEST + } else if (!codeVerifier) { + logger.error({ + message: 'Error logging in via sgID: code verifier cookie is empty', + meta: logMeta, + }) + status = StatusCodes.BAD_REQUEST } else { - await AuthSgidService.retrieveAccessToken(code) - .andThen(({ accessToken }) => - AuthSgidService.retrieveUserInfo(accessToken), + await AuthSgidService.retrieveAccessToken(code, codeVerifier) + .andThen(({ accessToken, sub }) => + AuthSgidService.retrieveUserInfo(accessToken, sub), ) .andThen((email) => AuthService.validateEmailDomain(email).andThen((agency) => diff --git a/src/app/modules/auth/sgid/auth-sgid.service.ts b/src/app/modules/auth/sgid/auth-sgid.service.ts index fb753b7875..3322b95e75 100644 --- a/src/app/modules/auth/sgid/auth-sgid.service.ts +++ b/src/app/modules/auth/sgid/auth-sgid.service.ts @@ -1,4 +1,4 @@ -import { SgidClient } from '@opengovsg/sgid-client' +import { generatePkcePair, SgidClient } from '@opengovsg/sgid-client' import fs from 'fs' import { err, ok, Result, ResultAsync } from 'neverthrow' @@ -41,19 +41,26 @@ export class AuthSgidServiceClass { /** * Create a URL to sgID which is used to redirect the user for authentication + * @returns The redirectUrl and the associated code verifier */ - createRedirectUrl(): Result { + createRedirectUrl(): Result< + { redirectUrl: string; codeVerifier: string }, + SgidCreateRedirectUrlError + > { const logMeta = { action: 'createRedirectUrl', } + const { codeChallenge, codeVerifier } = generatePkcePair() + try { - const result = this.client.authorizationUrl( - SGID_LOGIN_OAUTH_STATE, - ['openid', SGID_OGP_WORK_EMAIL_SCOPE].join(' '), - null, - ) - return ok(result.url) + const result = this.client.authorizationUrl({ + state: SGID_LOGIN_OAUTH_STATE, + scope: ['openid', SGID_OGP_WORK_EMAIL_SCOPE].join(' '), + nonce: null, + codeChallenge, + }) + return ok({ redirectUrl: result.url, codeVerifier }) } catch (error) { logger.error({ message: 'Error while creating redirect URL', @@ -71,12 +78,13 @@ export class AuthSgidServiceClass { */ retrieveAccessToken( code: string, + codeVerifier: string, ): ResultAsync< { sub: string; accessToken: string }, SgidFetchAccessTokenError > { return ResultAsync.fromPromise( - this.client.callback(code, null), + this.client.callback({ code, nonce: null, codeVerifier }), (error) => { logger.error({ message: 'Failed to retrieve access token from sgID', @@ -99,10 +107,11 @@ export class AuthSgidServiceClass { */ retrieveUserInfo( accessToken: string, + sub: string, ): ResultAsync { return ResultAsync.fromPromise( this.client - .userinfo(accessToken) + .userinfo({ accessToken, sub }) .then(({ data }) => data[SGID_OGP_WORK_EMAIL_SCOPE]), (error) => { logger.error({ 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 2f79ce0e6a..29d27ecc67 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -1,6 +1,6 @@ import { celebrate, Joi, Segments } from 'celebrate' import { StatusCodes } from 'http-status-codes' -import { err } from 'neverthrow' +import { err, ok, Result } from 'neverthrow' import { UnreachableCaseError } from 'ts-essentials' import { @@ -33,11 +33,16 @@ import { } from '../../myinfo/myinfo.util' import { SGIDMyInfoData } from '../../sgid/sgid.adapter' import { + SGID_CODE_VERIFIER_COOKIE_NAME, SGID_COOKIE_NAME, SGID_MYINFO_COOKIE_NAME, SGID_MYINFO_LOGIN_COOKIE_NAME, } from '../../sgid/sgid.constants' -import { SgidInvalidJwtError, SgidVerifyJwtError } from '../../sgid/sgid.errors' +import { + SgidInvalidJwtError, + SgidMalformedMyInfoCookieError, + SgidVerifyJwtError, +} from '../../sgid/sgid.errors' import { SgidService } from '../../sgid/sgid.service' import { validateSgidForm } from '../../sgid/sgid.util' import { InvalidJwtError, VerifyJwtError } from '../../spcp/spcp.errors' @@ -274,53 +279,79 @@ export const handleGetPublicForm: ControllerHandler< return res.json({ form: publicForm, isIntranetUser }) }) case FormAuthType.SGID_MyInfo: { - const accessTokenCookie = req.cookies[SGID_MYINFO_COOKIE_NAME] - if (!accessTokenCookie) { - return res.json({ - form: publicForm, - isIntranetUser, - }) - } - res.clearCookie(SGID_MYINFO_COOKIE_NAME) - res.clearCookie(SGID_MYINFO_LOGIN_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 - .cookie( - SGID_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) => { + const parseSgidMyInfoCookie = Result.fromThrowable( + () => + JSON.parse(req.cookies[SGID_MYINFO_COOKIE_NAME] ?? '{}') as { + jwt?: string + sub?: string + }, + (error) => { logger.error({ - message: 'sgID: MyInfo login error', + message: 'Error while calling JSON.parse on SGID MyInfo cookie', meta: logMeta, error, }) - return res.json({ + return new SgidMalformedMyInfoCookieError() + }, + ) + return parseSgidMyInfoCookie() + .mapErr(() => + res.json({ form: publicForm, - myInfoError: true, isIntranetUser, - }) + }), + ) + .map(({ jwt: accessToken = '', sub = '' }) => { + if (!accessToken) { + return res.json({ + form: publicForm, + isIntranetUser, + }) + } + res.clearCookie(SGID_MYINFO_COOKIE_NAME) + res.clearCookie(SGID_MYINFO_LOGIN_COOKIE_NAME) + return SgidService.extractSgidJwtMyInfoPayload(accessToken) + .asyncAndThen((auth) => + SgidService.retrieveUserInfo({ + accessToken: auth.accessToken, + sub, + }), + ) + .andThen((userInfo) => { + const data = new SGIDMyInfoData(userInfo.data) + return MyInfoService.prefillAndSaveMyInfoFields( + form._id, + data, + form.toJSON().form_fields, + ).map((prefilledFields) => { + return res + .cookie( + SGID_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) => { + logger.error({ + message: 'sgID: MyInfo login error', + meta: logMeta, + error, + }) + return res.json({ + form: publicForm, + myInfoError: true, + isIntranetUser, + }) + }) }) } default: @@ -455,23 +486,41 @@ export const _handleFormAuthRedirect: ControllerHandler< }) } case FormAuthType.SGID: - return validateSgidForm(form).andThen(() => { - return SgidService.createRedirectUrl( - formId, - Boolean(isPersistentLogin), - [], - encodedQuery, + return validateSgidForm(form) + .andThen(() => + SgidService.createRedirectUrl( + formId, + Boolean(isPersistentLogin), + [], + encodedQuery, + ), ) - }) + .andThen(({ redirectUrl, codeVerifier }) => { + res.cookie( + SGID_CODE_VERIFIER_COOKIE_NAME, + codeVerifier, + SgidService.getCookieSettings(), + ) + return ok(redirectUrl) + }) case FormAuthType.SGID_MyInfo: - return validateSgidForm(form).andThen(() => { - return SgidService.createRedirectUrl( - formId, - false, - form.getUniqueMyInfoAttrs(), - encodedQuery, + return validateSgidForm(form) + .andThen(() => + SgidService.createRedirectUrl( + formId, + false, + form.getUniqueMyInfoAttrs(), + encodedQuery, + ), ) - }) + .andThen(({ redirectUrl, codeVerifier }) => { + res.cookie( + SGID_CODE_VERIFIER_COOKIE_NAME, + codeVerifier, + SgidService.getCookieSettings(), + ) + return ok(redirectUrl) + }) default: return err( new AuthTypeMismatchError(form.authType), @@ -499,6 +548,7 @@ export const _handleFormAuthRedirect: ControllerHandler< error, }) const { statusCode, errorMessage } = mapFormAuthError(error) + res.clearCookie(SGID_CODE_VERIFIER_COOKIE_NAME) return res.status(statusCode).json({ message: errorMessage }) }) } diff --git a/src/app/modules/sgid/__tests__/sgid.controller.spec.ts b/src/app/modules/sgid/__tests__/sgid.controller.spec.ts index 048ba1dce7..9473f424fc 100644 --- a/src/app/modules/sgid/__tests__/sgid.controller.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.controller.spec.ts @@ -7,7 +7,10 @@ import { MOCK_COOKIE_AGE } from 'src/app/modules/myinfo/__tests__/myinfo.test.co import { ApplicationError } from '../../core/core.errors' import { FormNotFoundError } from '../../form/form.errors' -import { SGID_COOKIE_NAME } from '../sgid.constants' +import { + SGID_CODE_VERIFIER_COOKIE_NAME, + SGID_COOKIE_NAME, +} from '../sgid.constants' import * as SgidController from '../sgid.controller' import { SgidFetchAccessTokenError, @@ -18,6 +21,7 @@ import { SgidService as RealSgidService } from '../sgid.service' import { MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, MOCK_COOKIE_SETTINGS, MOCK_DESTINATION, MOCK_JWT, @@ -41,6 +45,10 @@ MockConfig.isDev = false const MOCK_RESPONSE = expressHandler.mockResponse() const MOCK_LOGIN_REQ = expressHandler.mockRequest({ query: { code: MOCK_AUTH_CODE, state: MOCK_STATE }, + cookies: { [SGID_CODE_VERIFIER_COOKIE_NAME]: MOCK_CODE_VERIFIER }, +}) +const MOCK_LOGIN_REQ_WITHOUT_CODE_VERIFIER_COOKIE = expressHandler.mockRequest({ + query: { code: MOCK_AUTH_CODE, state: MOCK_STATE }, }) describe('sgid.controller', () => { @@ -53,6 +61,7 @@ describe('sgid.controller', () => { ok({ formId: MOCK_TARGET, rememberMe: MOCK_REMEMBER_ME, + decodedQuery: '', }), ) FormService.retrieveFullFormById.mockReturnValue(okAsync(MOCK_SGID_FORM)) @@ -123,6 +132,7 @@ describe('sgid.controller', () => { expect(MOCK_RESPONSE.redirect).toHaveBeenCalledWith(MOCK_DESTINATION) expect(SgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() @@ -138,6 +148,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(SgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(SgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, @@ -157,6 +168,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(SgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(SgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, @@ -169,12 +181,30 @@ describe('sgid.controller', () => { expect(MOCK_RESPONSE.redirect).toHaveBeenCalledWith(MOCK_DESTINATION) expect(SgidService.getCookieSettings).not.toHaveBeenCalled() }) + + it('should set isLoginError cookie and redirect when code verifier cookie is empty', async () => { + await SgidController.handleLogin( + MOCK_LOGIN_REQ_WITHOUT_CODE_VERIFIER_COOKIE, + MOCK_RESPONSE, + jest.fn(), + ) + expect(SgidService.parseState).toHaveBeenCalledWith(MOCK_STATE) + expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) + expect(SgidService.retrieveAccessToken).not.toHaveBeenCalled() + expect(SgidService.retrieveUserInfo).not.toHaveBeenCalled() + expect(SgidService.createSgidSingpassJwt).not.toHaveBeenCalled() + expect(MOCK_RESPONSE.cookie).toHaveBeenCalledWith('isLoginError', true) + expect(MOCK_RESPONSE.redirect).toHaveBeenCalledWith(MOCK_DESTINATION) + expect(SgidService.getCookieSettings).not.toHaveBeenCalled() + }) + it('should set the cookie with the correct params and redirect to the destination', async () => { await SgidController.handleLogin(MOCK_LOGIN_REQ, MOCK_RESPONSE, jest.fn()) expect(SgidService.parseState).toHaveBeenCalledWith(MOCK_STATE) expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(SgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(SgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, @@ -183,6 +213,9 @@ describe('sgid.controller', () => { MOCK_USER_INFO.data, MOCK_REMEMBER_ME, ) + expect(MOCK_RESPONSE.clearCookie).toHaveBeenCalledWith( + SGID_CODE_VERIFIER_COOKIE_NAME, + ) expect(MOCK_RESPONSE.cookie).toHaveBeenCalledWith( SGID_COOKIE_NAME, MOCK_JWT, diff --git a/src/app/modules/sgid/__tests__/sgid.routes.spec.ts b/src/app/modules/sgid/__tests__/sgid.routes.spec.ts index fbef3e64b0..94d8ce04eb 100644 --- a/src/app/modules/sgid/__tests__/sgid.routes.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.routes.spec.ts @@ -9,7 +9,10 @@ import { MOCK_COOKIE_AGE } from 'src/app/modules/myinfo/__tests__/myinfo.test.co import { ApplicationError } from '../../core/core.errors' import { FormNotFoundError } from '../../form/form.errors' -import { SGID_COOKIE_NAME } from '../sgid.constants' +import { + SGID_CODE_VERIFIER_COOKIE_NAME, + SGID_COOKIE_NAME, +} from '../sgid.constants' import { SgidFetchAccessTokenError, SgidFetchUserInfoError, @@ -20,6 +23,7 @@ import { SgidService as RealSgidService } from '../sgid.service' import { MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, MOCK_COOKIE_SETTINGS, MOCK_DESTINATION, MOCK_JWT, @@ -44,7 +48,7 @@ const app = setupApp('/sgid', SgidRouter) const MOCK_LOGIN_QUERY = { code: MOCK_AUTH_CODE, state: MOCK_STATE } -describe('sgid.controller', () => { +describe('sgid.routes', () => { beforeEach(() => jest.clearAllMocks()) afterAll(() => jest.clearAllMocks()) const LOGIN_ROUTE = '/sgid/login' @@ -54,6 +58,7 @@ describe('sgid.controller', () => { ok({ formId: MOCK_TARGET, rememberMe: MOCK_REMEMBER_ME, + decodedQuery: '', }), ) FormService.retrieveFullFormById.mockReturnValue(okAsync(MOCK_SGID_FORM)) @@ -72,6 +77,10 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query({ state: MOCK_LOGIN_QUERY.state }) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.status).toBe(400) expect(response.body).toEqual( @@ -91,6 +100,10 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query({ code: MOCK_LOGIN_QUERY.code }) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.status).toBe(400) expect(response.body).toEqual( @@ -110,6 +123,10 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.status).toBe(400) @@ -139,6 +156,25 @@ describe('sgid.controller', () => { expect(sgidService.getCookieSettings).not.toHaveBeenCalled() }) + it('should set isLoginError cookie and redirect when code verifier cookie is missing', async () => { + const response = await session(app) + .get(LOGIN_ROUTE) + .query(MOCK_LOGIN_QUERY) + + expect(response.headers['set-cookie']).toEqual([ + expect.stringContaining('isLoginError=true'), + ]) + expect(response.status).toBe(302) + expect(response.headers['location']).toEqual(MOCK_DESTINATION) + + expect(sgidService.parseState).toHaveBeenCalledWith(MOCK_STATE) + expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) + expect(sgidService.retrieveAccessToken).not.toHaveBeenCalled() + expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() + expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() + expect(sgidService.getCookieSettings).not.toHaveBeenCalled() + }) + it('should set isLoginError cookie and redirect when form has wrong auth type', async () => { FormService.retrieveFullFormById.mockReturnValue( // Note that this is a SingPass form @@ -147,6 +183,10 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.headers['set-cookie']).toEqual([ expect.stringContaining('isLoginError=true'), @@ -169,8 +209,13 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.headers['set-cookie']).toEqual([ + expect.stringContaining('sgidCodeVerifier=;'), expect.stringContaining('isLoginError=true'), ]) expect(response.status).toBe(302) @@ -180,6 +225,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(sgidService.retrieveUserInfo).not.toHaveBeenCalled() expect(sgidService.createSgidSingpassJwt).not.toHaveBeenCalled() @@ -193,8 +239,13 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.headers['set-cookie']).toEqual([ + expect.stringContaining('sgidCodeVerifier=;'), expect.stringContaining('isLoginError=true'), ]) expect(response.status).toBe(302) @@ -204,6 +255,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, @@ -219,8 +271,13 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.headers['set-cookie']).toEqual([ + expect.stringContaining('sgidCodeVerifier=;'), expect.stringContaining('isLoginError=true'), ]) expect(response.status).toBe(302) @@ -230,6 +287,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, @@ -244,8 +302,14 @@ describe('sgid.controller', () => { const response = await session(app) .get(LOGIN_ROUTE) .query(MOCK_LOGIN_QUERY) + .set( + 'cookie', + `${SGID_CODE_VERIFIER_COOKIE_NAME}=${MOCK_CODE_VERIFIER}`, + ) expect(response.headers['set-cookie']).toEqual([ + // code_verifier cookie is cleared after being retrieved + expect.stringContaining(`${SGID_CODE_VERIFIER_COOKIE_NAME}=;`), expect.stringContaining(`${SGID_COOKIE_NAME}=${MOCK_JWT}`), ]) expect(response.status).toBe(302) @@ -255,6 +319,7 @@ describe('sgid.controller', () => { expect(FormService.retrieveFullFormById).toHaveBeenCalledWith(MOCK_TARGET) expect(sgidService.retrieveAccessToken).toHaveBeenCalledWith( MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, ) expect(sgidService.retrieveUserInfo).toHaveBeenCalledWith( MOCK_TOKEN_RESULT, diff --git a/src/app/modules/sgid/__tests__/sgid.service.spec.ts b/src/app/modules/sgid/__tests__/sgid.service.spec.ts index 529acd7164..144dae7505 100644 --- a/src/app/modules/sgid/__tests__/sgid.service.spec.ts +++ b/src/app/modules/sgid/__tests__/sgid.service.spec.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/ban-ts-comment */ -import { SgidClient } from '@opengovsg/sgid-client' +import { generatePkcePair, SgidClient } from '@opengovsg/sgid-client' import fs from 'fs' import Jwt from 'jsonwebtoken' import { MyInfoAttribute } from 'shared/types' @@ -19,6 +19,8 @@ import { SgidServiceClass } from '../sgid.service' import { MOCK_ACCESS_TOKEN, MOCK_AUTH_CODE, + MOCK_CODE_CHALLENGE, + MOCK_CODE_VERIFIER, MOCK_DESTINATION, MOCK_JWT, MOCK_JWT_PAYLOAD, @@ -27,6 +29,7 @@ import { MOCK_REDIRECT_URL, MOCK_REMEMBER_ME, MOCK_STATE, + MOCK_SUBJECT, MOCK_TOKEN_RESULT, MOCK_USER_INFO, } from './sgid.test.constants' @@ -68,65 +71,93 @@ describe('sgid.service', () => { }) }) describe('createRedirectUrl', () => { - it('should return a string if ok', () => { + it('should return the redirect url if ok', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) + const mockGeneratePkcePair = jest.mocked(generatePkcePair) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) + mockGeneratePkcePair.mockReturnValue({ + codeChallenge: MOCK_CODE_CHALLENGE, + codeVerifier: MOCK_CODE_VERIFIER, + }) sgidClient.authorizationUrl.mockReturnValue({ url: MOCK_REDIRECT_URL, nonce: MOCK_NONCE, }) - const url = SgidService.createRedirectUrl( + const result = 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_SINGPASS_SCOPE, - null, - ) + const unwrappedResult = result._unsafeUnwrap() + expect(unwrappedResult).toHaveProperty('redirectUrl', MOCK_REDIRECT_URL) + expect(unwrappedResult).toHaveProperty('codeVerifier', MOCK_CODE_VERIFIER) + expect(mockGeneratePkcePair).toHaveBeenCalledOnce() + expect(sgidClient.authorizationUrl).toHaveBeenCalledWith({ + state: MOCK_STATE, + scope: SGID_SINGPASS_SCOPE, + nonce: null, + codeChallenge: MOCK_CODE_CHALLENGE, + }) }) it('should extract additional OAuth scopes from MyInfo', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) + const mockGeneratePkcePair = jest.mocked(generatePkcePair) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) + mockGeneratePkcePair.mockReturnValue({ + codeChallenge: MOCK_CODE_CHALLENGE, + codeVerifier: MOCK_CODE_VERIFIER, + }) sgidClient.authorizationUrl.mockReturnValue({ url: MOCK_REDIRECT_URL, nonce: MOCK_NONCE, }) - const url = SgidService.createRedirectUrl( + const result = 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, - ) + const unwrappedResult = result._unsafeUnwrap() + expect(unwrappedResult).toHaveProperty('redirectUrl', MOCK_REDIRECT_URL) + expect(unwrappedResult).toHaveProperty('codeVerifier', MOCK_CODE_VERIFIER) + expect(mockGeneratePkcePair).toHaveBeenCalledOnce() + expect(sgidClient.authorizationUrl).toHaveBeenCalledWith({ + state: MOCK_STATE, + scope: + 'openid myinfo.nric_number myinfo.registered_address myinfo.passport_expiry_date', + nonce: null, + codeChallenge: MOCK_CODE_CHALLENGE, + }) }) it('should return error if not ok', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) + const mockGeneratePkcePair = jest.mocked(generatePkcePair) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) + mockGeneratePkcePair.mockReturnValue({ + codeChallenge: MOCK_CODE_CHALLENGE, + codeVerifier: MOCK_CODE_VERIFIER, + }) sgidClient.authorizationUrl.mockReturnValue({ // @ts-ignore url: undefined, nonce: MOCK_NONCE, }) - const url = SgidService.createRedirectUrl( + const result = SgidService.createRedirectUrl( MOCK_DESTINATION, MOCK_REMEMBER_ME, SGID_DEFAULT_ATTR_LIST, ) - expect(url._unsafeUnwrapErr()).toBeInstanceOf(SgidCreateRedirectUrlError) - expect(sgidClient.authorizationUrl).toHaveBeenCalledWith( - MOCK_STATE, - SGID_SINGPASS_SCOPE, - null, + expect(result._unsafeUnwrapErr()).toBeInstanceOf( + SgidCreateRedirectUrlError, ) + expect(mockGeneratePkcePair).toHaveBeenCalledOnce() + expect(sgidClient.authorizationUrl).toHaveBeenCalledWith({ + state: MOCK_STATE, + scope: 'openid myinfo.nric_number', + nonce: null, + codeChallenge: MOCK_CODE_CHALLENGE, + }) }) }) describe('parseState', () => { @@ -149,19 +180,33 @@ describe('sgid.service', () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) sgidClient.callback.mockResolvedValue(MOCK_TOKEN_RESULT) - const result = await SgidService.retrieveAccessToken(MOCK_AUTH_CODE) + const result = await SgidService.retrieveAccessToken( + MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, + ) expect(result._unsafeUnwrap()).toStrictEqual(MOCK_TOKEN_RESULT) - expect(sgidClient.callback).toHaveBeenCalledWith(MOCK_AUTH_CODE, null) + expect(sgidClient.callback).toHaveBeenCalledWith({ + code: MOCK_AUTH_CODE, + nonce: null, + codeVerifier: MOCK_CODE_VERIFIER, + }) }) it('should return error on error', async () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) const sgidClient = jest.mocked(MockSgidClient.mock.instances[0]) sgidClient.callback.mockRejectedValue(new Error()) - const result = await SgidService.retrieveAccessToken(MOCK_AUTH_CODE) + const result = await SgidService.retrieveAccessToken( + MOCK_AUTH_CODE, + MOCK_CODE_VERIFIER, + ) expect(result._unsafeUnwrapErr()).toBeInstanceOf( SgidFetchAccessTokenError, ) - expect(sgidClient.callback).toHaveBeenCalledWith(MOCK_AUTH_CODE, null) + expect(sgidClient.callback).toHaveBeenCalledWith({ + code: MOCK_AUTH_CODE, + nonce: null, + codeVerifier: MOCK_CODE_VERIFIER, + }) }) }) describe('userInfo', () => { @@ -178,9 +223,13 @@ describe('sgid.service', () => { sgidClient.userinfo.mockResolvedValue(mockUserInfoWithAdditional) const result = await SgidService.retrieveUserInfo({ accessToken: MOCK_ACCESS_TOKEN, + sub: MOCK_SUBJECT, }) expect(result._unsafeUnwrap()).toStrictEqual(mockUserInfoWithAdditional) - expect(sgidClient.userinfo).toHaveBeenCalledWith(MOCK_ACCESS_TOKEN) + expect(sgidClient.userinfo).toHaveBeenCalledWith({ + accessToken: MOCK_ACCESS_TOKEN, + sub: MOCK_SUBJECT, + }) }) it('should return error on error', async () => { const SgidService = new SgidServiceClass(MOCK_OPTIONS) @@ -188,9 +237,13 @@ describe('sgid.service', () => { sgidClient.userinfo.mockRejectedValue(new Error()) const result = await SgidService.retrieveUserInfo({ accessToken: MOCK_ACCESS_TOKEN, + sub: MOCK_SUBJECT, }) expect(result._unsafeUnwrapErr()).toBeInstanceOf(SgidFetchUserInfoError) - expect(sgidClient.userinfo).toHaveBeenCalledWith(MOCK_ACCESS_TOKEN) + expect(sgidClient.userinfo).toHaveBeenCalledWith({ + accessToken: MOCK_ACCESS_TOKEN, + sub: MOCK_SUBJECT, + }) }) }) describe('createJwt', () => { diff --git a/src/app/modules/sgid/__tests__/sgid.test.constants.ts b/src/app/modules/sgid/__tests__/sgid.test.constants.ts index 1fc5d8799b..6754c0d251 100644 --- a/src/app/modules/sgid/__tests__/sgid.test.constants.ts +++ b/src/app/modules/sgid/__tests__/sgid.test.constants.ts @@ -22,6 +22,10 @@ export const MOCK_ACCESS_TOKEN = 'mock-access-token' export const MOCK_SUBJECT = 'mock-subject-proxy-id' +export const MOCK_CODE_VERIFIER = 'mock_code_verifier' + +export const MOCK_CODE_CHALLENGE = 'mock_code_challenge' + export const MOCK_TOKEN_RESULT = { sub: MOCK_SUBJECT, accessToken: MOCK_ACCESS_TOKEN, diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index 765a0cabde..57ccc8855a 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -10,6 +10,11 @@ export const SGID_COOKIE_NAME = 'jwtSgid' */ export const SGID_MYINFO_COOKIE_NAME = 'jwtSgidMyInfo' +/** + * Name of cookie containing the PKCE code verifier for sgID flow(s). + */ +export const SGID_CODE_VERIFIER_COOKIE_NAME = 'sgidCodeVerifier' + /** * Name of cookie which contains state of sgID login, and NRIC * if login was successful. diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 73b5835068..8223649dae 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -8,6 +8,7 @@ import { ControllerHandler } from '../core/core.types' import * as FormService from '../form/form.service' import { + SGID_CODE_VERIFIER_COOKIE_NAME, SGID_COOKIE_NAME, SGID_MYINFO_COOKIE_NAME, SGID_MYINFO_NRIC_NUMBER_SCOPE, @@ -66,10 +67,23 @@ export const handleLogin: ControllerHandler< res.cookie('isLoginError', true) return res.redirect(target) } + + const codeVerifier = req.cookies[SGID_CODE_VERIFIER_COOKIE_NAME] + if (!codeVerifier) { + logger.error({ + message: 'Error logging in via sgID: code verifier cookie is empty', + meta, + }) + res.cookie('isLoginError', true) + return res.redirect(target) + } + res.clearCookie(SGID_CODE_VERIFIER_COOKIE_NAME) + if (form.authType === FormAuthType.SGID_MyInfo) { - const jwtResult = await SgidService.retrieveAccessToken(code).andThen( - (data) => SgidService.createSgidMyInfoJwt(data.accessToken), - ) + const jwtResult = await SgidService.retrieveAccessToken( + code, + codeVerifier, + ).andThen((data) => SgidService.createSgidMyInfoJwt(data)) if (jwtResult.isErr()) { logger.error({ @@ -81,8 +95,8 @@ export const handleLogin: ControllerHandler< return res.redirect(target) } - const { maxAge, jwt } = jwtResult.value - res.cookie(SGID_MYINFO_COOKIE_NAME, jwt, { + const { maxAge, jwt, sub } = jwtResult.value + res.cookie(SGID_MYINFO_COOKIE_NAME, JSON.stringify({ jwt, sub }), { maxAge, httpOnly: true, sameSite: 'lax', // Setting to 'strict' prevents Singpass login on Safari, Firefox @@ -92,7 +106,7 @@ export const handleLogin: ControllerHandler< return res.redirect(target) } - const jwtResult = await SgidService.retrieveAccessToken(code) + const jwtResult = await SgidService.retrieveAccessToken(code, codeVerifier) .andThen((data) => SgidService.retrieveUserInfo(data)) .andThen(({ data }) => SgidService.createSgidSingpassJwt( diff --git a/src/app/modules/sgid/sgid.errors.ts b/src/app/modules/sgid/sgid.errors.ts index 0d0467ddbe..7860f1c9ab 100644 --- a/src/app/modules/sgid/sgid.errors.ts +++ b/src/app/modules/sgid/sgid.errors.ts @@ -24,6 +24,12 @@ export class SgidFetchUserInfoError extends ApplicationError { } } +export class SgidMalformedMyInfoCookieError extends ApplicationError { + constructor(message = 'SGID MyInfo cookie is malformed') { + super(message) + } +} + /** * JWT could not be decoded. */ diff --git a/src/app/modules/sgid/sgid.service.ts b/src/app/modules/sgid/sgid.service.ts index cbf52b38d0..5533791300 100644 --- a/src/app/modules/sgid/sgid.service.ts +++ b/src/app/modules/sgid/sgid.service.ts @@ -1,4 +1,4 @@ -import { SgidClient } from '@opengovsg/sgid-client' +import { generatePkcePair, SgidClient } from '@opengovsg/sgid-client' import fs from 'fs' import Jwt from 'jsonwebtoken' import { err, ok, Result, ResultAsync } from 'neverthrow' @@ -74,13 +74,17 @@ export class SgidServiceClass { * @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 + * @returns The redirectUrl and the associated code verifier */ createRedirectUrl( formId: string, rememberMe: boolean, requestedAttributes: InternalAttr[], encodedQuery?: string, - ): Result { + ): Result< + { redirectUrl: string; codeVerifier: string }, + SgidCreateRedirectUrlError + > { const state = encodedQuery ? `${formId},${rememberMe},${encodedQuery}` : `${formId},${rememberMe}` @@ -90,9 +94,15 @@ export class SgidServiceClass { state, } const scopes = internalAttrListToScopes(requestedAttributes) - const result = this.client.authorizationUrl(state, scopes, null) + const { codeChallenge, codeVerifier } = generatePkcePair() + const result = this.client.authorizationUrl({ + state, + scope: scopes, + nonce: null, + codeChallenge, + }) if (typeof result.url === 'string') { - return ok(result.url) + return ok({ redirectUrl: result.url, codeVerifier }) } else { logger.error({ message: 'Error while creating redirect URL', @@ -152,12 +162,13 @@ export class SgidServiceClass { */ retrieveAccessToken( code: string, + codeVerifier: string, ): ResultAsync< { sub: string; accessToken: string }, SgidFetchAccessTokenError > { return ResultAsync.fromPromise( - this.client.callback(code, null), + this.client.callback({ code, nonce: null, codeVerifier }), (error) => { logger.error({ message: 'Failed to retrieve access token from sgID', @@ -180,14 +191,16 @@ export class SgidServiceClass { */ retrieveUserInfo({ accessToken, + sub, }: { accessToken: string + sub: string }): ResultAsync< { sub: string; data: SGIDScopeToValue }, SgidFetchUserInfoError > { return ResultAsync.fromPromise( - this.client.userinfo(accessToken).then(({ sub, data }) => { + this.client.userinfo({ accessToken, sub }).then(({ sub, data }) => { return { sub, data, @@ -241,9 +254,13 @@ export class SgidServiceClass { * Note: sgID access token is tied to the sgID OAuth scopes requested. * @param accessToken - sgID access token */ - createSgidMyInfoJwt( - accessToken: string, - ): Result<{ jwt: string; maxAge: number }, ApplicationError> { + createSgidMyInfoJwt({ + sub, + accessToken, + }: { + sub: string + accessToken: string + }): Result<{ jwt: string; maxAge: number; sub: string }, ApplicationError> { const payload: SGIDJwtAccessPayload = { accessToken } const maxAge = this.cookieMaxAge const jwt = Jwt.sign(payload, this.privateKey, { @@ -251,6 +268,7 @@ export class SgidServiceClass { expiresIn: maxAge / 1000, }) return ok({ + sub, jwt, maxAge, })