Merge pull request #19354 from backstage/rugvip/ep-check

backend-app-api: validate extension point deps

.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.

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<string>({ 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'",
+    );
+  });
 });

packages/backend-app-api/src/wiring/BackendInitializer.ts

@@ -42,7 +42,10 @@ export interface BackendRegisterInit {
 export class BackendInitializer {
   #startPromise?: Promise<void>;
   #features = new Array<InternalBackendFeature>();
-  #extensionPoints = new Map<ExtensionPoint<unknown>, unknown>();
+  #extensionPoints = new Map<
+    ExtensionPoint<unknown>,
+    { impl: unknown; pluginId: string }
+  >();
   #serviceHolder: EnumerableServiceHolder;
 
   constructor(serviceHolder: EnumerableServiceHolder) {
@@ -57,11 +60,14 @@ export class BackendInitializer {
     const missingRefs = new Set<ServiceOrExtensionPoint>();
 
     for (const [name, ref] of Object.entries(deps)) {
-      const extensionPoint = this.#extensionPoints.get(
-        ref as ExtensionPoint<unknown>,
-      );
-      if (extensionPoint) {
-        result.set(name, extensionPoint);
+      const ep = this.#extensionPoints.get(ref as ExtensionPoint<unknown>);
+      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<unknown>,
@@ -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);
           }
         }

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<string>({ id: 'a' });
+    const extensionPointB = createExtensionPoint<string>({ 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<string>({ 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<string>({ 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.",
+    );
+  });
 });

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<unknown>,
+    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<string, string[]>();
+
+  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<Backend>();
 
 /** @public */
@@ -98,7 +188,7 @@ export async function startTestBackend<
 ): Promise<TestBackend> {
   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);