Skip to content

Commit

Permalink
fix(auth): revert to using decode instead verify for jwt (#227)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@dpopp07
dpopp07 authored Dec 30, 2022
1 parent 8bcc6a0 commit cf3d641
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 43 deletions.
15 changes: 2 additions & 13 deletions auth/token-managers/jwt-token-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
Expand Down
7 changes: 4 additions & 3 deletions test/unit/iam-token-manager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
46 changes: 19 additions & 27 deletions test/unit/jwt-token-manager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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')
Expand All @@ -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(
() =>
Expand All @@ -91,7 +91,7 @@ describe('JWT Token Manager', () => {
expect(requestTokenSpy).toHaveBeenCalled();
expect(requestTokenSpy.mock.calls).toHaveLength(1);

verifySpy.mockRestore();
decode.mockRestore();
requestTokenSpy.mockRestore();
});

Expand Down Expand Up @@ -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')
Expand All @@ -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();
});

Expand Down Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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();
});
});
});

0 comments on commit cf3d641

Please sign in to comment.