From cf3d64106f35943c34f2a9c5d1b4de63994aed2a Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Fri, 30 Dec 2022 12:12:16 -0600 Subject: [PATCH] fix(auth): revert to using decode instead verify for jwt (#227) The move to `verify` was a bit shortsighted. It requires a public key or secret as an argument to verify the signed token. Typical usage of IBM's IAM service doesn't lend itself to this flow and the tokens returned don't have valid JWT signatures. This led to a runtime error everytime the core made an IAM request - a bit of a showstopping bug. Additionally, we never had a goal of performing client-side validation of these tokens, we only decode them to determine the expiration time for usage in our refresh logic. The `decode` method is perfectly sufficient for that and indeed is called within the `verify` method anyways. Perhaps this flow will change in the future but this is all we need for now. This reverts the logic to what we were using before but still using the safe version of the dependency. The new version makes the `decode` function read-only so I had to adjust our approach to mocking in the unit tests. Signed-off-by: Dustin Popp --- auth/token-managers/jwt-token-manager.ts | 15 ++------ test/unit/iam-token-manager.test.js | 7 ++-- test/unit/jwt-token-manager.test.js | 46 ++++++++++-------------- 3 files changed, 25 insertions(+), 43 deletions(-) diff --git a/auth/token-managers/jwt-token-manager.ts b/auth/token-managers/jwt-token-manager.ts index 7f2aeba2b..4c66caccd 100644 --- a/auth/token-managers/jwt-token-manager.ts +++ b/auth/token-managers/jwt-token-manager.ts @@ -16,7 +16,7 @@ * limitations under the License. */ -import { verify } from 'jsonwebtoken'; +import { decode } from 'jsonwebtoken'; import logger from '../../lib/logger'; import { TokenManager, TokenManagerOptions } from './token-manager'; @@ -80,18 +80,7 @@ export class JwtTokenManager extends TokenManager { throw new Error(err); } - let decodedResponse; - try { - decodedResponse = verify(this.accessToken); - } catch (e) { - // the token is either an invalid JWT or it could not be verified - logger.error('Failed to verify the JWT. See error message:'); - logger.error(e); - throw new Error(e); - } - - // the 'catch' method above should handle any verificiation/decoding issues but - // this check is here as a failsafe + const decodedResponse = decode(this.accessToken); if (!decodedResponse) { const err = 'Access token recieved is not a valid JWT'; logger.error(err); diff --git a/test/unit/iam-token-manager.test.js b/test/unit/iam-token-manager.test.js index a2dd270d5..a2993ab34 100644 --- a/test/unit/iam-token-manager.test.js +++ b/test/unit/iam-token-manager.test.js @@ -16,13 +16,14 @@ * limitations under the License. */ -const jwt = require('jsonwebtoken'); +jest.mock('jsonwebtoken/decode'); +const decode = require('jsonwebtoken/decode'); + +decode.mockImplementation(() => ({ exp: 100, iat: 100 })); jest.mock('../../dist/lib/request-wrapper'); const { RequestWrapper } = require('../../dist/lib/request-wrapper'); -jwt.verify = jest.fn(() => ({ exp: 100, iat: 100 })); - const { IamTokenManager } = require('../../dist/auth'); const mockSendRequest = jest.fn(); diff --git a/test/unit/jwt-token-manager.test.js b/test/unit/jwt-token-manager.test.js index 377e050a4..8c25b0b4e 100644 --- a/test/unit/jwt-token-manager.test.js +++ b/test/unit/jwt-token-manager.test.js @@ -16,7 +16,11 @@ * limitations under the License. */ -const jwt = require('jsonwebtoken'); +jest.mock('jsonwebtoken/decode'); +const decode = require('jsonwebtoken/decode'); + +decode.mockImplementation(() => ({ exp: 100, iat: 100 })); + const { JwtTokenManager } = require('../../dist/auth'); function getCurrentTime() { @@ -47,9 +51,7 @@ describe('JWT Token Manager', () => { const instance = new JwtTokenManager(); const saveTokenInfoSpy = jest.spyOn(instance, 'saveTokenInfo'); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ iat: 10, exp: 100 })); + decode.mockImplementation((token) => ({ iat: 10, exp: 100 })); const requestTokenSpy = jest .spyOn(instance, 'requestToken') @@ -58,20 +60,18 @@ describe('JWT Token Manager', () => { const token = await instance.getToken(); expect(requestTokenSpy).toHaveBeenCalled(); expect(saveTokenInfoSpy).toHaveBeenCalled(); - expect(verifySpy).toHaveBeenCalled(); + expect(decode).toHaveBeenCalled(); expect(token).toBe(ACCESS_TOKEN); saveTokenInfoSpy.mockRestore(); - verifySpy.mockRestore(); + decode.mockRestore(); requestTokenSpy.mockRestore(); }); it('should pace token requests', async () => { const instance = new JwtTokenManager(); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ iat: 10, exp: 100 })); + decode.mockImplementation((token) => ({ iat: 10, exp: 100 })); const requestTokenSpy = jest.spyOn(instance, 'requestToken').mockImplementation( () => @@ -91,7 +91,7 @@ describe('JWT Token Manager', () => { expect(requestTokenSpy).toHaveBeenCalled(); expect(requestTokenSpy.mock.calls).toHaveLength(1); - verifySpy.mockRestore(); + decode.mockRestore(); requestTokenSpy.mockRestore(); }); @@ -131,9 +131,7 @@ describe('JWT Token Manager', () => { instance.tokenInfo.access_token = CURRENT_ACCESS_TOKEN; const saveTokenInfoSpy = jest.spyOn(instance, 'saveTokenInfo'); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ iat: 10, exp: 100 })); + decode.mockImplementation((token) => ({ iat: 10, exp: 100 })); const requestTokenSpy = jest .spyOn(instance, 'requestToken') @@ -142,11 +140,11 @@ describe('JWT Token Manager', () => { const token = await instance.getToken(); expect(requestTokenSpy).toHaveBeenCalled(); expect(saveTokenInfoSpy).toHaveBeenCalled(); - expect(verifySpy).toHaveBeenCalled(); + expect(decode).toHaveBeenCalled(); expect(token).toBe(CURRENT_ACCESS_TOKEN); saveTokenInfoSpy.mockRestore(); - verifySpy.mockRestore(); + decode.mockRestore(); requestTokenSpy.mockRestore(); }); @@ -271,16 +269,14 @@ describe('JWT Token Manager', () => { describe('saveTokenInfo', () => { it('should save information to object state', () => { const instance = new JwtTokenManager(); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ iat: 10, exp: 100 })); + decode.mockImplementation((token) => ({ iat: 10, exp: 100 })); const tokenResponse = { result: { access_token: ACCESS_TOKEN } }; instance.saveTokenInfo(tokenResponse); expect(instance.expireTime).toBe(100); expect(instance.tokenInfo).toEqual(tokenResponse.result); - verifySpy.mockRestore(); + decode.mockRestore(); }); it('should throw an error when access token is undefined', () => { @@ -294,15 +290,13 @@ describe('JWT Token Manager', () => { describe('calculateTimeForNewToken', () => { it('should calculate time for new token based on valid jwt', () => { const instance = new JwtTokenManager(); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ iat: 100, exp: 200 })); + decode.mockImplementation((token) => ({ iat: 100, exp: 200 })); const tokenResponse = { result: { access_token: ACCESS_TOKEN } }; instance.saveTokenInfo(tokenResponse); expect(instance.refreshTime).toBe(180); - verifySpy.mockRestore(); + decode.mockRestore(); }); it('should throw an error if token is not a valid jwt', () => { @@ -312,16 +306,14 @@ describe('JWT Token Manager', () => { it('should gracefully handle a jwt without exp or iat claims', () => { const instance = new JwtTokenManager(); - const verifySpy = jest - .spyOn(jwt, 'verify') - .mockImplementation((token) => ({ foo: 0, bar: 100 })); + decode.mockImplementation((token) => ({ foo: 0, bar: 100 })); const tokenResponse = { result: { access_token: ACCESS_TOKEN } }; instance.saveTokenInfo(tokenResponse); expect(instance.expireTime).toBe(0); expect(instance.tokenInfo).toEqual(tokenResponse.result); - verifySpy.mockRestore(); + decode.mockRestore(); }); }); });