From b54672edf57c12c1b672b8133c3369c219ea2d08 Mon Sep 17 00:00:00 2001 From: Leo <145344770+LGin-BJSS@users.noreply.github.com> Date: Wed, 27 Mar 2024 10:15:41 +0000 Subject: [PATCH] CB2-10665: Remove JWT from Log (#98) * feat(cb2-10665): do not log a valid JWT token * feat(cb2-10665): revert use of test jwt json in logger unit tests * feat(cb2-10665): add unit tests to check email is correct in logs * feat(cb2-10665): refactor code to ensure logs are always populated * feat(cb2-10665): add roles to log event * feat(cb2-10665): bump axios version * feat(cb2-10665): remove duplicate information from the log error --------- Co-authored-by: Michael --- package-lock.json | 103 +++++++++---------------- package.json | 2 +- src/common/Logger.ts | 3 +- src/functions/authorizer.ts | 6 +- src/models/ILogError.ts | 2 +- src/services/tokens.ts | 6 +- tests/unit/common/Logger.unitTest.ts | 29 ++++--- tests/unit/services/tokens.unitTest.ts | 62 +++++++++++++++ 8 files changed, 128 insertions(+), 85 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2489385..ccf16a5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,7 +11,7 @@ "dependencies": { "aws-sdk": "2.1358.0", "aws-xray-sdk": "3.5.0", - "axios": "1.3.5", + "axios": "1.6.8", "js-yaml": "4.1.0", "jsonwebtoken": "9.0.0", "qs": "6.11.1" @@ -4615,28 +4615,15 @@ } }, "node_modules/axios": { - "version": "1.3.5", - "resolved": "https://registry.npmjs.org/axios/-/axios-1.3.5.tgz", - "integrity": "sha512-glL/PvG/E+xCWwV8S6nCHcrfg1exGx7vxyUIivIA1iL7BIh6bePylCfVHwp6k13ao7SATxB6imau2kqY+I67kw==", + "version": "1.6.8", + "resolved": "https://registry.npmjs.org/axios/-/axios-1.6.8.tgz", + "integrity": "sha512-v/ZHtJDU39mDpyBoFVkETcd/uNdxrWRrg3bKpOKzXFA6Bvqopts6ALSMU3y6ijYxbw2B+wPrIv46egTzJXCLGQ==", "dependencies": { - "follow-redirects": "^1.15.0", + "follow-redirects": "^1.15.6", "form-data": "^4.0.0", "proxy-from-env": "^1.1.0" } }, - "node_modules/axios/node_modules/form-data": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", - "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", - "dependencies": { - "asynckit": "^0.4.0", - "combined-stream": "^1.0.8", - "mime-types": "^2.1.12" - }, - "engines": { - "node": ">= 6" - } - }, "node_modules/babel-jest": { "version": "29.5.0", "resolved": "https://registry.npmjs.org/babel-jest/-/babel-jest-29.5.0.tgz", @@ -6896,9 +6883,9 @@ } }, "node_modules/follow-redirects": { - "version": "1.15.1", - "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.1.tgz", - "integrity": "sha512-yLAMQs+k0b2m7cVxpS1VKJVvoz7SS9Td1zss3XRwXj+ZDH00RJgnuLx7E44wx02kQLrdM3aOOy+FpzS7+8OizA==", + "version": "1.15.6", + "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.6.tgz", + "integrity": "sha512-wWN62YITEaOpSK584EZXJafH1AGpO8RVgElfkuXbTOrPX4fIfOyEpW/CsiNd8JdYrAoOvafRTOEnvsO++qCqFA==", "funding": [ { "type": "individual", @@ -6922,6 +6909,19 @@ "is-callable": "^1.1.3" } }, + "node_modules/form-data": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", + "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", + "dependencies": { + "asynckit": "^0.4.0", + "combined-stream": "^1.0.8", + "mime-types": "^2.1.12" + }, + "engines": { + "node": ">= 6" + } + }, "node_modules/formdata-polyfill": { "version": "4.0.10", "resolved": "https://registry.npmjs.org/formdata-polyfill/-/formdata-polyfill-4.0.10.tgz", @@ -12037,20 +12037,6 @@ "node": ">=6.4.0 <13 || >=14" } }, - "node_modules/superagent/node_modules/form-data": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", - "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", - "dev": true, - "dependencies": { - "asynckit": "^0.4.0", - "combined-stream": "^1.0.8", - "mime-types": "^2.1.12" - }, - "engines": { - "node": ">= 6" - } - }, "node_modules/superagent/node_modules/semver": { "version": "7.5.4", "resolved": "https://registry.npmjs.org/semver/-/semver-7.5.4.tgz", @@ -17232,25 +17218,13 @@ } }, "axios": { - "version": "1.3.5", - "resolved": "https://registry.npmjs.org/axios/-/axios-1.3.5.tgz", - "integrity": "sha512-glL/PvG/E+xCWwV8S6nCHcrfg1exGx7vxyUIivIA1iL7BIh6bePylCfVHwp6k13ao7SATxB6imau2kqY+I67kw==", + "version": "1.6.8", + "resolved": "https://registry.npmjs.org/axios/-/axios-1.6.8.tgz", + "integrity": "sha512-v/ZHtJDU39mDpyBoFVkETcd/uNdxrWRrg3bKpOKzXFA6Bvqopts6ALSMU3y6ijYxbw2B+wPrIv46egTzJXCLGQ==", "requires": { - "follow-redirects": "^1.15.0", + "follow-redirects": "^1.15.6", "form-data": "^4.0.0", "proxy-from-env": "^1.1.0" - }, - "dependencies": { - "form-data": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", - "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", - "requires": { - "asynckit": "^0.4.0", - "combined-stream": "^1.0.8", - "mime-types": "^2.1.12" - } - } } }, "babel-jest": { @@ -18957,9 +18931,9 @@ "dev": true }, "follow-redirects": { - "version": "1.15.1", - "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.1.tgz", - "integrity": "sha512-yLAMQs+k0b2m7cVxpS1VKJVvoz7SS9Td1zss3XRwXj+ZDH00RJgnuLx7E44wx02kQLrdM3aOOy+FpzS7+8OizA==" + "version": "1.15.6", + "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.6.tgz", + "integrity": "sha512-wWN62YITEaOpSK584EZXJafH1AGpO8RVgElfkuXbTOrPX4fIfOyEpW/CsiNd8JdYrAoOvafRTOEnvsO++qCqFA==" }, "for-each": { "version": "0.3.3", @@ -18969,6 +18943,16 @@ "is-callable": "^1.1.3" } }, + "form-data": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", + "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", + "requires": { + "asynckit": "^0.4.0", + "combined-stream": "^1.0.8", + "mime-types": "^2.1.12" + } + }, "formdata-polyfill": { "version": "4.0.10", "resolved": "https://registry.npmjs.org/formdata-polyfill/-/formdata-polyfill-4.0.10.tgz", @@ -22775,17 +22759,6 @@ "semver": "^7.3.7" }, "dependencies": { - "form-data": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", - "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", - "dev": true, - "requires": { - "asynckit": "^0.4.0", - "combined-stream": "^1.0.8", - "mime-types": "^2.1.12" - } - }, "semver": { "version": "7.5.4", "resolved": "https://registry.npmjs.org/semver/-/semver-7.5.4.tgz", diff --git a/package.json b/package.json index f2906b4..eb7bfae 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "dependencies": { "aws-sdk": "2.1358.0", "aws-xray-sdk": "3.5.0", - "axios": "1.3.5", + "axios": "1.6.8", "js-yaml": "4.1.0", "jsonwebtoken": "9.0.0", "qs": "6.11.1" diff --git a/src/common/Logger.ts b/src/common/Logger.ts index d343c0b..ca54b7d 100644 --- a/src/common/Logger.ts +++ b/src/common/Logger.ts @@ -2,7 +2,7 @@ import { ILogEvent } from "../models/ILogEvent"; import { JWT_MESSAGE } from "../models/enums"; import { ILogError } from "../models/ILogError"; -export const writeLogMessage = (log: ILogEvent, jwt: string, error?: any) => { +export const writeLogMessage = (log: ILogEvent, error?: any) => { if (!error) { log.statusCode = 200; console.log(log); @@ -32,7 +32,6 @@ export const writeLogMessage = (log: ILogEvent, jwt: string, error?: any) => { } } log.error = logError; - log.error.token = jwt; console.error(log); } return log; diff --git a/src/functions/authorizer.ts b/src/functions/authorizer.ts index f9e1fb2..db54252 100644 --- a/src/functions/authorizer.ts +++ b/src/functions/authorizer.ts @@ -21,7 +21,7 @@ export const authorizer = async (event: APIGatewayTokenAuthorizerEvent, context: const logEvent: ILogEvent = {}; if (!process.env.AZURE_TENANT_ID || !process.env.AZURE_CLIENT_ID) { - writeLogMessage(logEvent, event.authorizationToken, JWT_MESSAGE.INVALID_ID_SETUP); + writeLogMessage(logEvent, JWT_MESSAGE.INVALID_ID_SETUP); return unauthorisedPolicy(); } @@ -36,10 +36,10 @@ export const authorizer = async (event: APIGatewayTokenAuthorizerEvent, context: } reportNoValidRoles(jwt, event, context, logEvent); - writeLogMessage(logEvent, event.authorizationToken, JWT_MESSAGE.INVALID_ROLES); + writeLogMessage(logEvent, JWT_MESSAGE.INVALID_ROLES); return unauthorisedPolicy(); } catch (error: any) { - writeLogMessage(logEvent, event.authorizationToken, error); + writeLogMessage(logEvent, error); return unauthorisedPolicy(); } }; diff --git a/src/models/ILogError.ts b/src/models/ILogError.ts index caec30d..c18d485 100644 --- a/src/models/ILogError.ts +++ b/src/models/ILogError.ts @@ -1,5 +1,5 @@ +import Role from "../services/roles"; export interface ILogError { name?: string; message?: string; - token?: string; } diff --git a/src/services/tokens.ts b/src/services/tokens.ts index ebd0c61..02f7ac8 100644 --- a/src/services/tokens.ts +++ b/src/services/tokens.ts @@ -19,8 +19,6 @@ export const getValidJwt = async (authorizationToken: string, logEvent: ILogEven throw new Error(JWT_MESSAGE.DECODE_FAILED); } - await checkSignature(authorizationToken, decoded, tenantId, clientId); - let username; const payload = decoded.payload as CVSJWTPayload; @@ -35,7 +33,11 @@ export const getValidJwt = async (authorizationToken: string, logEvent: ILogEven } logEvent.email = username; + logEvent.roles = (decoded.payload as JwtPayload).roles; logEvent.tokenExpiry = new Date((payload.exp as number) * 1000).toISOString(); + + await checkSignature(authorizationToken, decoded, tenantId, clientId); + return decoded; }; diff --git a/tests/unit/common/Logger.unitTest.ts b/tests/unit/common/Logger.unitTest.ts index 77403c2..9601bf7 100644 --- a/tests/unit/common/Logger.unitTest.ts +++ b/tests/unit/common/Logger.unitTest.ts @@ -3,15 +3,16 @@ import { ILogEvent } from "../../../src/models/ILogEvent"; import errorLogEvent from "../../resources/errorLogEvent.json"; import { writeLogMessage } from "../../../src/common/Logger"; import successLogEvent from "../../resources/successLogEvent.json"; +import Role from "../../../src/services/roles"; describe("test writeLogMessage method", () => { const logError: ILogError = {}; const logErrorEvent: ILogEvent = errorLogEvent; - logErrorEvent.roles = [{ name: "name1", access: "read" }]; + logErrorEvent.roles = [{name: "test", access: "read"}] as Role[]; context("when only the log event is passed in", () => { it("should return no errors", () => { - const returnValue: ILogEvent = writeLogMessage(successLogEvent, "mock-jwt", null); + const returnValue: ILogEvent = writeLogMessage(successLogEvent, null); expect(returnValue.statusCode).toBe(200); }); @@ -23,11 +24,13 @@ describe("test writeLogMessage method", () => { console.log = jest.fn(); logError.name = "TokenExpiredError"; - const returnValue: ILogEvent = writeLogMessage(logErrorEvent,"mock-jwt", error); + const returnValue: ILogEvent = writeLogMessage(logErrorEvent, error); expect(returnValue.error?.name).toBe("TokenExpiredError"); expect(returnValue.error?.message).toBe("[JWT-ERROR-07] Error at undefined"); - expect(returnValue.error?.token).toBe("mock-jwt"); + expect(returnValue.email).toBe(logErrorEvent.email); + expect(returnValue.roles).toBe(logErrorEvent.roles); + }); }); it("should log NotBeforeError", () => { @@ -35,11 +38,13 @@ describe("test writeLogMessage method", () => { console.log = jest.fn(); logError.name = "NotBeforeError"; - const returnValue: ILogEvent = writeLogMessage(logErrorEvent,"mock-jwt", error); + const returnValue: ILogEvent = writeLogMessage(logErrorEvent, error); expect(returnValue.error?.name).toBe("NotBeforeError"); expect(returnValue.error?.message).toBe("[JWT-ERROR-08] undefined until undefined"); - expect(returnValue.error?.token).toBe("mock-jwt"); + expect(returnValue.email).toBe(logErrorEvent.email); + expect(returnValue.roles).toBe(logErrorEvent.roles); + }); it("should log JsonWebTokenError", () => { @@ -47,22 +52,24 @@ describe("test writeLogMessage method", () => { console.log = jest.fn(); logError.name = "JsonWebTokenError"; - const returnValue: ILogEvent = writeLogMessage(logErrorEvent,"mock-jwt", error); + const returnValue: ILogEvent = writeLogMessage(logErrorEvent, error); expect(returnValue.error?.name).toBe("JsonWebTokenError"); expect(returnValue.error?.message).toBe("[JWT-ERROR-09] test"); - expect(returnValue.error?.token).toBe("mock-jwt"); + expect(returnValue.email).toBe(logErrorEvent.email); + expect(returnValue.roles).toBe(logErrorEvent.roles); + }); it("should log the default error", () => { const error: ILogError = { name: "Error", message: "Error" }; console.log = jest.fn(); - const returnValue: ILogEvent = writeLogMessage(logErrorEvent,"mock-jwt", error); + const returnValue: ILogEvent = writeLogMessage(logErrorEvent, error); expect(returnValue.error?.name).toBe("Error"); expect(returnValue.error?.message).toBe("Error"); - expect(returnValue.error?.token).toBe("mock-jwt"); + expect(returnValue.email).toBe(logErrorEvent.email); + expect(returnValue.roles).toBe(logErrorEvent.roles); }); - }); }); diff --git a/tests/unit/services/tokens.unitTest.ts b/tests/unit/services/tokens.unitTest.ts index 34d5233..bfd6bff 100644 --- a/tests/unit/services/tokens.unitTest.ts +++ b/tests/unit/services/tokens.unitTest.ts @@ -1,5 +1,6 @@ import { getValidJwt } from "../../../src/services/tokens"; import { JWT_MESSAGE } from "../../../src/models/enums"; +import { ILogEvent } from "../../../src/models/ILogEvent"; jest.mock("../../../src/services/signature-check", () => { return { checkSignature: jest.fn().mockResolvedValue(true) }; @@ -76,4 +77,65 @@ describe("getValidJwt()", () => { }, }); }); + + context("when preferred_username is present in the token", () => { + it("should set the email in the log event to preferred_username", async () => { + const jwt = require('jsonwebtoken'); + const token = jwt.sign({ + "sub": "1234567890", + "name": "John Doe", + "iat": 1516239022, + "tid": "123456", + "exp": 631005334, + "preferred_username": "test_username" + }, "testSignature"); + + const logEvent: ILogEvent = {} + + await getValidJwt(`Bearer ${token}`, logEvent, DEFAULT_TENANT_ID, DEFAULT_CLIENT_ID); + + expect(logEvent.email).toBe("test_username"); + }); + }); + + context("when preferred_username and unique_name are present in the token", () => { + it("should set the email in the log event to preferred_username", async () => { + const jwt = require('jsonwebtoken'); + const token = jwt.sign({ + "sub": "1234567890", + "name": "John Doe", + "iat": 1516239022, + "tid": "123456", + "exp": 631005334, + "preferred_username": "test_username", + "unique_name": "test_unique_name" + }, "testSignature"); + + const logEvent: ILogEvent = {} + + await getValidJwt(`Bearer ${token}`, logEvent, DEFAULT_TENANT_ID, DEFAULT_CLIENT_ID); + + expect(logEvent.email).toBe("test_username"); + }); + }); + + context("when unique_name is present in the token without preferred_username", () => { + it("should set the email in the log event to unique_name", async () => { + const jwt = require('jsonwebtoken'); + const token = jwt.sign({ + "sub": "1234567890", + "name": "John Doe", + "iat": 1516239022, + "tid": "123456", + "exp": 631005334, + "unique_name": "test_unique_name" + }, "testSignature"); + + const logEvent: ILogEvent = {} + + await getValidJwt(`Bearer ${token}`, logEvent, DEFAULT_TENANT_ID, DEFAULT_CLIENT_ID); + + expect(logEvent.email).toBe("test_unique_name"); + }); + }); });