From 8a001f8202f2249a645a7f980b46d653dd075035 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Tue, 23 Nov 2021 17:16:57 +0000 Subject: [PATCH] backend-common: TokenManager throws for invalid tokens rather than returning boolean Signed-off-by: Mike Lewis --- docs/tutorials/authenticate-api-requests.md | 5 +- packages/backend-common/api-report.md | 4 +- .../src/tokens/ServerTokenManager.test.ts | 57 +++++++++++-------- .../src/tokens/ServerTokenManager.ts | 9 +-- packages/backend-common/src/tokens/types.ts | 2 +- .../src/search/DefaultCatalogCollator.test.ts | 2 +- .../search/DefaultTechDocsCollator.test.ts | 4 +- 7 files changed, 47 insertions(+), 36 deletions(-) diff --git a/docs/tutorials/authenticate-api-requests.md b/docs/tutorials/authenticate-api-requests.md index 54952272da..ea7ae72a27 100644 --- a/docs/tutorials/authenticate-api-requests.md +++ b/docs/tutorials/authenticate-api-requests.md @@ -95,9 +95,8 @@ async function main() { req.cookies['token']; // Authenticate all requests originating from backends by default - const isValidServerToken = await authEnv.tokenManager.validateServerToken( - token, - ); + const isValidServerToken = + authEnv.tokenManager.validateServerToken(token); if (!isValidServerToken) { req.user = await identity.authenticate(token); } diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 01d16b587c..6cd1364285 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -539,7 +539,7 @@ export class ServerTokenManager implements TokenManager { // (undocumented) static noop(): ServerTokenManager; // (undocumented) - validateServerToken(token: string): Promise; + validateServerToken(token: string): void; } // @public (undocumented) @@ -608,7 +608,7 @@ export interface TokenManager { token: string; }>; // (undocumented) - validateServerToken: (token: string) => Promise; + validateServerToken: (token: string) => void; } // @public diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index 6d06c874ea..82936d8f82 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -39,31 +39,29 @@ describe('ServerTokenManager', () => { }); describe('validateServerToken', () => { - it('should return true if token is valid', async () => { + it('should not throw if token is valid', async () => { const tokenManager = ServerTokenManager.fromConfig(configWithSecret); const { token } = await tokenManager.getServerToken(); - const isValidServerToken = await tokenManager.validateServerToken(token); - expect(isValidServerToken).toBe(true); + expect(() => tokenManager.validateServerToken(token)).not.toThrow(); }); - it('should return false if token is invalid', async () => { + it('should throw if token is invalid', () => { const tokenManager = ServerTokenManager.fromConfig(configWithSecret); - const isValidServerToken = await tokenManager.validateServerToken( - 'random-string', - ); - expect(isValidServerToken).toBe(false); + expect(() => + tokenManager.validateServerToken('random-string'), + ).toThrowError(/invalid server token/i); }); - it('should return true for server tokens created by a different instance using the same secret', async () => { + it('should validate server tokens created by a different instance using the same secret', async () => { const tokenManager1 = ServerTokenManager.fromConfig(configWithSecret); const tokenManager2 = ServerTokenManager.fromConfig(configWithSecret); const { token } = await tokenManager1.getServerToken(); - expect(await tokenManager2.validateServerToken(token)).toBe(true); + expect(() => tokenManager2.validateServerToken(token)).not.toThrow(); }); - it('should return false for server tokens created by a different instance using a different secret', async () => { + it('should throw for server tokens created using a different secret', async () => { const tokenManager1 = ServerTokenManager.fromConfig( new ConfigReader({ backend: { auth: { secret: 'a1b2c3' } } }), ); @@ -73,20 +71,33 @@ describe('ServerTokenManager', () => { const { token } = await tokenManager1.getServerToken(); - expect(await tokenManager2.validateServerToken(token)).toBe(false); + expect(() => tokenManager2.validateServerToken(token)).toThrowError( + /invalid server token/i, + ); + }); + }); + + describe('ServerTokenManager.noop', () => { + let noopTokenManager: ServerTokenManager; + + beforeEach(() => { + noopTokenManager = ServerTokenManager.noop(); }); - it('should always return true if using noop TokenManager', async () => { - const tokenManager = ServerTokenManager.noop(); - const { token } = await tokenManager.getServerToken(); - const isValidServerToken0 = await tokenManager.validateServerToken(token); - const isValidServerToken1 = await tokenManager.validateServerToken( - 'random-string', - ); - const isValidServerToken2 = await tokenManager.validateServerToken(''); - expect(isValidServerToken0).toBe(true); - expect(isValidServerToken1).toBe(true); - expect(isValidServerToken2).toBe(true); + it('should accept tokens it generates', async () => { + const { token } = await noopTokenManager.getServerToken(); + + expect(() => noopTokenManager.validateServerToken(token)).not.toThrow(); + }); + + it('should accept arbitrary strings', async () => { + expect(() => + noopTokenManager.validateServerToken('random-string'), + ).not.toThrow(); + }); + + it('should accept empty strings', async () => { + expect(() => noopTokenManager.validateServerToken('')).not.toThrow(); }); }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index f7fccd30b2..6354162d38 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -16,6 +16,7 @@ import { JWK, JWT } from 'jose'; import { Config } from '@backstage/config'; +import { AuthenticationError } from '@backstage/errors'; import { TokenManager } from './types'; export class ServerTokenManager implements TokenManager { @@ -46,16 +47,16 @@ export class ServerTokenManager implements TokenManager { return { token: jwt }; } - async validateServerToken(token: string): Promise { + validateServerToken(token: string): void { if (this.key === JWK.None) { - return true; + return; } try { JWT.verify(token, this.key); - return true; + return; } catch (e) { - return false; + throw new AuthenticationError('Invalid server token'); } } } diff --git a/packages/backend-common/src/tokens/types.ts b/packages/backend-common/src/tokens/types.ts index 506e1191ec..65f3a749a4 100644 --- a/packages/backend-common/src/tokens/types.ts +++ b/packages/backend-common/src/tokens/types.ts @@ -16,5 +16,5 @@ export interface TokenManager { getServerToken: () => Promise<{ token: string }>; - validateServerToken: (token: string) => Promise; + validateServerToken: (token: string) => void; } diff --git a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts index 9f42c7de83..ba8cb64f02 100644 --- a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts +++ b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts @@ -68,7 +68,7 @@ describe('DefaultCatalogCollator', () => { }; mockTokenManager = { getServerToken: jest.fn().mockResolvedValue({ token: '' }), - validateServerToken: jest.fn().mockResolvedValue(true), + validateServerToken: jest.fn(), }; collator = new DefaultCatalogCollator({ discovery: mockDiscoveryApi, diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts index e689cd9f61..ad6f5c3520 100644 --- a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts @@ -100,7 +100,7 @@ describe('DefaultTechDocsCollator with legacyPathCasing configuration', () => { }; mockTokenManager = { getServerToken: jest.fn().mockResolvedValue({ token: '' }), - validateServerToken: jest.fn().mockResolvedValue(true), + validateServerToken: jest.fn(), }; const mockConfig = new ConfigReader({ techdocs: { @@ -166,7 +166,7 @@ describe('DefaultTechDocsCollator', () => { }; mockTokenManager = { getServerToken: jest.fn().mockResolvedValue({ token: '' }), - validateServerToken: jest.fn().mockResolvedValue(true), + validateServerToken: jest.fn(), }; collator = DefaultTechDocsCollator.fromConfig(new ConfigReader({}), { discovery: mockDiscoveryApi,