From 57a10c6c69cc0bfb6033626f8c8513eb58347ca5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 12 Aug 2023 13:58:08 +0200 Subject: [PATCH 1/2] backend-app-api: validate extension point deps Signed-off-by: Patrik Oldsberg --- .changeset/quick-cups-float.md | 5 +++ .../src/wiring/BackendInitializer.test.ts | 32 +++++++++++++++++++ .../src/wiring/BackendInitializer.ts | 23 +++++++++---- 3 files changed, 53 insertions(+), 7 deletions(-) create mode 100644 .changeset/quick-cups-float.md diff --git a/.changeset/quick-cups-float.md b/.changeset/quick-cups-float.md new file mode 100644 index 0000000000..21a2f856ec --- /dev/null +++ b/.changeset/quick-cups-float.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Add validation to make sure that extension points do not cross plugin boundaries. diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index ba637b7b5b..b90bcf6691 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -294,4 +294,36 @@ describe('BackendInitializer', () => { "Circular dependency detected for modules of plugin 'test', 'modA' -> 'modB' -> 'modA'", ); }); + + it('should reject modules that depend on extension points other plugins', async () => { + const init = new BackendInitializer(new ServiceRegistry(baseFactories)); + const extA = createExtensionPoint({ id: 'a' }); + init.add( + createBackendPlugin({ + pluginId: 'testA', + register(reg) { + reg.registerExtensionPoint(extA, 'a'); + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + })(), + ); + init.add( + createBackendModule({ + pluginId: 'testB', + moduleId: 'mod', + register(reg) { + reg.registerInit({ + deps: { ext: extA }, + async init() {}, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Extension point registered for plugin 'testA' may not be used by module for plugin 'testB'", + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 06201dfb2b..f71256d04e 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -42,7 +42,10 @@ export interface BackendRegisterInit { export class BackendInitializer { #startPromise?: Promise; #features = new Array(); - #extensionPoints = new Map, unknown>(); + #extensionPoints = new Map< + ExtensionPoint, + { impl: unknown; pluginId: string } + >(); #serviceHolder: EnumerableServiceHolder; constructor(serviceHolder: EnumerableServiceHolder) { @@ -57,11 +60,14 @@ export class BackendInitializer { const missingRefs = new Set(); for (const [name, ref] of Object.entries(deps)) { - const extensionPoint = this.#extensionPoints.get( - ref as ExtensionPoint, - ); - if (extensionPoint) { - result.set(name, extensionPoint); + const ep = this.#extensionPoints.get(ref as ExtensionPoint); + if (ep) { + if (ep.pluginId !== pluginId) { + throw new Error( + `Extension point registered for plugin '${ep.pluginId}' may not be used by module for plugin '${pluginId}'`, + ); + } + result.set(name, ep.impl); } else { const impl = await this.#serviceHolder.get( ref as ServiceRef, @@ -169,7 +175,10 @@ export class BackendInitializer { `ExtensionPoint with ID '${extRef.id}' is already registered`, ); } - this.#extensionPoints.set(extRef, extImpl); + this.#extensionPoints.set(extRef, { + impl: extImpl, + pluginId: r.pluginId, + }); provides.add(extRef); } } From 4b0894972f7b963c74f2d1e3b9ed8c10e7150563 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 14 Aug 2023 12:58:05 +0200 Subject: [PATCH 2/2] backend-test-utils: automatically determine the plugin ID of tested extension points Signed-off-by: Patrik Oldsberg --- .../src/next/wiring/TestBackend.test.ts | 85 ++++++++++++++ .../src/next/wiring/TestBackend.ts | 109 +++++++++++++++--- 2 files changed, 180 insertions(+), 14 deletions(-) diff --git a/packages/backend-test-utils/src/next/wiring/TestBackend.test.ts b/packages/backend-test-utils/src/next/wiring/TestBackend.test.ts index cc916d4309..13b57f54ba 100644 --- a/packages/backend-test-utils/src/next/wiring/TestBackend.test.ts +++ b/packages/backend-test-utils/src/next/wiring/TestBackend.test.ts @@ -220,4 +220,89 @@ describe('TestBackend', () => { expect(res.status).toEqual(200); expect(res.body).toEqual({ message: 'pong' }); }); + + it('should provide extension point implementations', async () => { + expect.assertions(3); + + const extensionPointA = createExtensionPoint({ id: 'a' }); + const extensionPointB = createExtensionPoint({ id: 'b' }); + await expect( + startTestBackend({ + extensionPoints: [ + [extensionPointA, 'a'], + [extensionPointB, 'b'], + ], + features: [ + createBackendModule({ + pluginId: 'testA', + moduleId: 'test', + register(reg) { + reg.registerInit({ + deps: { ext: extensionPointA }, + async init({ ext }) { + expect(ext).toBe('a'); + }, + }); + }, + })(), + createBackendModule({ + pluginId: 'testB', + moduleId: 'test', + register(reg) { + reg.registerInit({ + deps: { ext: extensionPointB }, + async init({ ext }) { + expect(ext).toBe('b'); + }, + }); + }, + })(), + ], + }), + ).resolves.not.toBeUndefined(); + }); + + it('should reject extension point used by multiple plugins', async () => { + const extensionPointA = createExtensionPoint({ id: 'a' }); + await expect( + startTestBackend({ + extensionPoints: [[extensionPointA, 'a']], + features: [ + createBackendModule({ + pluginId: 'testA', + moduleId: 'test', + register(reg) { + reg.registerInit({ + deps: { ext: extensionPointA }, + async init() {}, + }); + }, + })(), + createBackendModule({ + pluginId: 'testB', + moduleId: 'test', + register(reg) { + reg.registerInit({ + deps: { ext: extensionPointA }, + async init() {}, + }); + }, + })(), + ], + }), + ).rejects.toThrow( + "Extension point registered for plugin 'testA' may not be used by module for plugin 'testB'", + ); + }); + + it('should reject extension point not used by any plugin', async () => { + const extensionPointA = createExtensionPoint({ id: 'a' }); + await expect( + startTestBackend({ + extensionPoints: [[extensionPointA, 'a']], + }), + ).rejects.toThrow( + "Unable to determine the plugin ID of extension point(s) 'a'. Tested extension points must be depended on by one or more tested modules.", + ); + }); }); diff --git a/packages/backend-test-utils/src/next/wiring/TestBackend.ts b/packages/backend-test-utils/src/next/wiring/TestBackend.ts index 8a4e1d8471..06e27e5c07 100644 --- a/packages/backend-test-utils/src/next/wiring/TestBackend.ts +++ b/packages/backend-test-utils/src/next/wiring/TestBackend.ts @@ -30,11 +30,14 @@ import { BackendFeature, ExtensionPoint, coreServices, - createBackendPlugin, + createBackendModule, } from '@backstage/backend-plugin-api'; import { mockServices } from '../services'; import { ConfigReader } from '@backstage/config'; import express from 'express'; +// Direct internal import to avoid duplication +// eslint-disable-next-line @backstage/no-forbidden-package-imports +import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; /** @public */ export interface TestBackendOptions< @@ -87,6 +90,93 @@ const defaultServiceFactories = [ mockServices.urlReader.factory(), ]; +/** + * Given a set of extension points and plugins, find + * @returns + */ +function createExtensionPointTestModules( + features: BackendFeature[], + extensionPointTuples?: readonly [ + ref: ExtensionPoint, + impl: unknown, + ][], +): BackendFeature[] { + if (!extensionPointTuples) { + return []; + } + + const registrations = features.flatMap(feature => { + if (feature.$$type !== '@backstage/BackendFeature') { + throw new Error( + `Failed to add feature, invalid type '${feature.$$type}'`, + ); + } + const internalFeature = feature as InternalBackendFeature; + if (internalFeature.version !== 'v1') { + throw new Error( + `Failed to add feature, invalid version '${internalFeature.version}'`, + ); + } + return internalFeature.getRegistrations(); + }); + + const extensionPointMap = new Map( + extensionPointTuples.map(ep => [ep[0].id, ep]), + ); + const extensionPointsToSort = new Set(extensionPointMap.keys()); + const extensionPointsByPlugin = new Map(); + + for (const registration of registrations) { + if (registration.type === 'module') { + const testDep = Object.values(registration.init.deps).filter(dep => + extensionPointsToSort.has(dep.id), + ); + if (testDep.length > 0) { + let points = extensionPointsByPlugin.get(registration.pluginId); + if (!points) { + points = []; + extensionPointsByPlugin.set(registration.pluginId, points); + } + for (const { id } of testDep) { + points.push(id); + extensionPointsToSort.delete(id); + } + } + } + } + + if (extensionPointsToSort.size > 0) { + const list = Array.from(extensionPointsToSort) + .map(id => `'${id}'`) + .join(', '); + throw new Error( + `Unable to determine the plugin ID of extension point(s) ${list}. ` + + 'Tested extension points must be depended on by one or more tested modules.', + ); + } + + const modules = []; + + for (const [pluginId, pluginExtensionPointIds] of extensionPointsByPlugin) { + modules.push( + createBackendModule({ + pluginId, + moduleId: 'testExtensionPointRegistration', + register(reg) { + for (const id of pluginExtensionPointIds) { + const tuple = extensionPointMap.get(id)!; + reg.registerExtensionPoint(...tuple); + } + + reg.registerInit({ deps: {}, async init() {} }); + }, + })(), + ); + } + + return modules; +} + const backendInstancesToCleanUp = new Array(); /** @public */ @@ -98,7 +188,7 @@ export async function startTestBackend< ): Promise { const { services = [], - extensionPoints = [], + extensionPoints, features = [], ...otherOptions } = options; @@ -194,18 +284,9 @@ export async function startTestBackend< backendInstancesToCleanUp.push(backend); - backend.add( - createBackendPlugin({ - pluginId: `---test-extension-point-registrar`, - register(reg) { - for (const [ref, impl] of extensionPoints) { - reg.registerExtensionPoint(ref, impl); - } - - reg.registerInit({ deps: {}, async init() {} }); - }, - })(), - ); + for (const m of createExtensionPointTestModules(features, extensionPoints)) { + backend.add(m); + } for (const feature of features) { backend.add(feature);