Dynamic plugin poc - #1110
Conversation
Add plugin loading support to the Modern WebUI using @freeipa/plugin-sdk. Plugins are discovered via a server manifest and loaded at runtime using dynamic import(), without requiring a rebuild of the host application. Changes: - Expose shared dependencies (React, PatternFly, Redux) for plugins - Load plugins asynchronously before initial render - Support dynamic route and navigation item registration from plugins - Support dynamic Redux reducer injection via store.replaceReducer() - Add Dashboard page with ExtensionSlot for plugin-contributed widgets Signed-off-by: Erik Belko <ebelko@redhat.com>
Signed-off-by: Erik Belko <ebelko@redhat.com>
Include the plugin SDK (freeipa-webui-plugin-sdk) and the example hello-world plugin (freeipa-webui-plugin-example) in-tree so colleagues can review all PoC code in a single PR. In production these will be separate repositories. Signed-off-by: Erik Belko <ebelko@redhat.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Consider wrapping the async
bootstrap()plugin loading (loadPlugins+pluginRegistry.runPhase) in a try/catch so that an unexpected error there doesn’t prevent the main app from rendering and can fall back to a no-plugins startup path. - In
PluginRegistry,injectedEndpointsis recorded but never read; if you don’t plan to consume this history, you can drop the array to reduce state and simplify the registry, or otherwise expose a getter so it has a clear use. - The
devPluginsServerVite middleware uses synchronousfscalls on every request; even though this is dev-only, switching to asyncfs.promisesor caching file existence would avoid blocking the dev server’s event loop on each plugin asset request.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the async `bootstrap()` plugin loading (`loadPlugins` + `pluginRegistry.runPhase`) in a try/catch so that an unexpected error there doesn’t prevent the main app from rendering and can fall back to a no-plugins startup path.
- In `PluginRegistry`, `injectedEndpoints` is recorded but never read; if you don’t plan to consume this history, you can drop the array to reduce state and simplify the registry, or otherwise expose a getter so it has a clear use.
- The `devPluginsServer` Vite middleware uses synchronous `fs` calls on every request; even though this is dev-only, switching to async `fs.promises` or caching file existence would avoid blocking the dev server’s event loop on each plugin asset request.
## Individual Comments
### Comment 1
<location path="freeipa-webui-plugin-sdk/src/PluginLoader.ts" line_range="45-54" />
<code_context>
+ `[PluginLoader] Loading ${enabledPlugins.length} plugin(s)...`
+ );
+
+ const results = await Promise.allSettled(
+ enabledPlugins.map(async (entry) => {
+ try {
+ const mod = await import(/* @vite-ignore */ entry.entrypoint);
+ const pluginModule: PluginModule = mod.default ?? mod;
+
+ if (!pluginModule.id || !pluginModule.register) {
+ console.error(
+ `[PluginLoader] Plugin at "${entry.entrypoint}" does not export a valid PluginModule.`
+ );
+ return;
+ }
+
+ await pluginRegistry.registerPlugin(pluginModule);
+ } catch (err) {
+ console.error(
+ `[PluginLoader] Failed to load plugin "${entry.id}" from "${entry.entrypoint}":`,
+ err
+ );
+ }
+ })
+ );
+
+ const failed = results.filter((r) => r.status === "rejected").length;
+ if (failed > 0) {
+ console.warn(`[PluginLoader] ${failed} plugin(s) failed to load.`);
</code_context>
<issue_to_address>
**issue (bug_risk):** The failure counting logic in loadPlugins never reports failures because errors are caught inside the mapped promises.
Because the `enabledPlugins.map` callback catches and swallows all errors, each async function always resolves, so `Promise.allSettled` never sees any rejections. Consequently, `results.filter((r) => r.status === "rejected")` is always empty and `failed` is always 0.
To fix this, either let the error escape the callback so the promise rejects, or keep the `try/catch` but update a separate failure counter when a plugin fails instead of relying on `status === 'rejected'`.
</issue_to_address>
### Comment 2
<location path="freeipa-webui-plugin-sdk/src/PluginRegistry.ts" line_range="184-34" />
<code_context>
+
+ // -- Lifecycle --------------------------------------------------------
+
+ async runPhase(phase: string): Promise<void> {
+ const handlers = this.phaseHandlers.get(phase) || [];
+ for (const handler of handlers) {
+ try {
+ handler();
+ } catch (err) {
+ console.error(
+ `[PluginRegistry] Error in "${phase}" phase handler:`,
+ err
+ );
+ }
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** runPhase is async but cannot await async phase handlers, which may lead to misleading usage.
`runPhase` returns `Promise<void>` and callers `await` it, but `PhaseHandler` is `() => void`, so any `async` handler becomes fire-and-forget and isn’t awaited. This can cause phases to appear complete while async work is still running. Either support async handlers (e.g. `type PhaseHandler = () => void | Promise<void>` and `await handler()`), or make `runPhase` synchronous so its completion semantics are clear.
Suggested implementation:
```typescript
// -- Lifecycle --------------------------------------------------------
+
+ async runPhase(phase: string): Promise<void> {
+ const handlers = this.phaseHandlers.get(phase) || [];
+ for (const handler of handlers) {
+ try {
+ await handler();
+ } catch (err) {
+ console.error(
+ `[PluginRegistry] Error in "${phase}" phase handler:`,
+ err
+ );
+ }
+ }
+ }
```
1. Locate the `PhaseHandler` type (or equivalent) in `PluginRegistry.ts`. Change it from something like:
`type PhaseHandler = () => void;`
to:
`type PhaseHandler = () => void | Promise<void>;`
2. Ensure any `phaseHandlers` collection is typed with the updated `PhaseHandler` (e.g. `Map<string, PhaseHandler[]>`).
3. No call sites of `runPhase` need changes; they already `await` the returned `Promise<void>`, which will now correctly represent completion of all async handler work.
</issue_to_address>
### Comment 3
<location path="vite.config.ts" line_range="24-33" />
<code_context>
+ return {
+ name: "dev-plugins-server",
+ configureServer(server: any) {
+ server.middlewares.use((req: any, res: any, next: any) => {
+ const urlPath = (req.url || "").split("?")[0];
+ if (!urlPath.startsWith(PLUGINS_PREFIX)) {
+ return next();
+ }
+
+ const relative = urlPath.slice(PLUGINS_PREFIX.length);
+ const filePath = path.join(PLUGINS_DIR, relative);
+
+ if (!fs.existsSync(filePath) || !fs.statSync(filePath).isFile()) {
+ return next();
+ }
+
+ const ext = path.extname(filePath);
+ const mimeTypes: Record<string, string> = {
+ ".js": "application/javascript",
+ ".json": "application/json",
+ ".map": "application/json",
+ };
+
+ res.setHeader("Content-Type", mimeTypes[ext] || "application/octet-stream");
+ res.setHeader("Access-Control-Allow-Origin", "*");
+ fs.createReadStream(filePath).pipe(res);
+ });
+ },
</code_context>
<issue_to_address>
**🚨 suggestion (security):** The devPluginsServer middleware should guard against path traversal from plugin URLs and handle stream errors more defensively.
`relative` comes straight from the URL and is joined with `PLUGINS_DIR`, so `..` segments could escape the intended directory. Normalize and resolve the path, then verify the resolved `filePath` is still under `PLUGINS_DIR` (e.g. via `path.resolve` and a prefix check) before serving. Also, `fs.createReadStream(filePath)` may emit errors after the `statSync` check; add an `on('error', ...)` handler that calls `next(err)` or returns a 404 to avoid uncaught errors.
Suggested implementation:
```typescript
configureServer(server: any) {
server.middlewares.use((req: any, res: any, next: any) => {
const urlPath = (req.url || "").split("?")[0];
if (!urlPath.startsWith(PLUGINS_PREFIX)) {
return next();
}
// Normalize relative path to avoid path traversal (e.g. ".." segments)
const rawRelative = urlPath.slice(PLUGINS_PREFIX.length);
const normalizedRelative = path
.normalize(rawRelative)
.replace(/^([/\\])+/, ""); // strip any leading slashes/backslashes
const pluginsRoot = path.resolve(PLUGINS_DIR);
const filePath = path.resolve(PLUGINS_DIR, normalizedRelative);
// Ensure resolved path is still within the plugins root directory
if (filePath !== pluginsRoot && !filePath.startsWith(pluginsRoot + path.sep)) {
return next();
}
let stats: fs.Stats;
try {
stats = fs.statSync(filePath);
} catch {
return next();
}
if (!stats.isFile()) {
return next();
}
const ext = path.extname(filePath);
const mimeTypes: Record<string, string> = {
".js": "application/javascript",
".json": "application/json",
".map": "application/json",
};
res.setHeader("Content-Type", mimeTypes[ext] || "application/octet-stream");
res.setHeader("Access-Control-Allow-Origin", "*");
const stream = fs.createReadStream(filePath);
stream.on("error", (err: NodeJS.ErrnoException) => {
if (err.code === "ENOENT") {
if (!res.headersSent) {
res.statusCode = 404;
res.end("Not found");
}
return;
}
if (!res.headersSent) {
res.statusCode = 500;
res.end("Error reading file");
}
// Let Vite/Connect error handling deal with unexpected errors
next(err);
});
stream.pipe(res);
});
},
```
This edit assumes `fs` and `path` are already imported in `vite.config.ts` as they are used elsewhere in the file. If not, you should add:
- `import fs from "fs";`
- `import path from "path";`
near the top of the file, matching the existing import style.
</issue_to_address>
### Comment 4
<location path="src/pages/Dashboard/Dashboard.tsx" line_range="38-40" />
<code_context>
+ </CardBody>
+ </Card>
+ </GalleryItem>
+ <ExtensionSlot
+ extensionPointId={EXTENSION_POINTS.DASHBOARD_CONTENT}
+ />
+ </Gallery>
+ </PageSection>
</code_context>
<issue_to_address>
**question (bug_risk):** ExtensionSlot is rendered directly inside Gallery, which may break layout if plugin components don’t return GalleryItem nodes.
If plugin components render arbitrary blocks (e.g. a bare `Card`), the PatternFly `Gallery` layout may break because it expects children wrapped in `GalleryItem`. Either document that extensions must render `GalleryItem` themselves, or have the slot map each extension into a `GalleryItem` so the layout contract is enforced by the host.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const results = await Promise.allSettled( | ||
| enabledPlugins.map(async (entry) => { | ||
| try { | ||
| const mod = await import(/* @vite-ignore */ entry.entrypoint); | ||
| const pluginModule: PluginModule = mod.default ?? mod; | ||
|
|
||
| if (!pluginModule.id || !pluginModule.register) { | ||
| console.error( | ||
| `[PluginLoader] Plugin at "${entry.entrypoint}" does not export a valid PluginModule.` | ||
| ); |
There was a problem hiding this comment.
issue (bug_risk): The failure counting logic in loadPlugins never reports failures because errors are caught inside the mapped promises.
Because the enabledPlugins.map callback catches and swallows all errors, each async function always resolves, so Promise.allSettled never sees any rejections. Consequently, results.filter((r) => r.status === "rejected") is always empty and failed is always 0.
To fix this, either let the error escape the callback so the promise rejects, or keep the try/catch but update a separate failure counter when a plugin fails instead of relying on status === 'rejected'.
| /** Host calls this so plugins can read IPA config. */ | ||
| setConfigGetter(getter: () => Record<string, unknown>): void { | ||
| this.configGetter = getter; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): runPhase is async but cannot await async phase handlers, which may lead to misleading usage.
runPhase returns Promise<void> and callers await it, but PhaseHandler is () => void, so any async handler becomes fire-and-forget and isn’t awaited. This can cause phases to appear complete while async work is still running. Either support async handlers (e.g. type PhaseHandler = () => void | Promise<void> and await handler()), or make runPhase synchronous so its completion semantics are clear.
Suggested implementation:
// -- Lifecycle --------------------------------------------------------
+
+ async runPhase(phase: string): Promise<void> {
+ const handlers = this.phaseHandlers.get(phase) || [];
+ for (const handler of handlers) {
+ try {
+ await handler();
+ } catch (err) {
+ console.error(
+ `[PluginRegistry] Error in "${phase}" phase handler:`,
+ err
+ );
+ }
+ }
+ }- Locate the
PhaseHandlertype (or equivalent) inPluginRegistry.ts. Change it from something like:
type PhaseHandler = () => void;
to:
type PhaseHandler = () => void | Promise<void>; - Ensure any
phaseHandlerscollection is typed with the updatedPhaseHandler(e.g.Map<string, PhaseHandler[]>). - No call sites of
runPhaseneed changes; they alreadyawaitthe returnedPromise<void>, which will now correctly represent completion of all async handler work.
| server.middlewares.use((req: any, res: any, next: any) => { | ||
| const urlPath = (req.url || "").split("?")[0]; | ||
| if (!urlPath.startsWith(PLUGINS_PREFIX)) { | ||
| return next(); | ||
| } | ||
|
|
||
| const relative = urlPath.slice(PLUGINS_PREFIX.length); | ||
| const filePath = path.join(PLUGINS_DIR, relative); | ||
|
|
||
| if (!fs.existsSync(filePath) || !fs.statSync(filePath).isFile()) { |
There was a problem hiding this comment.
🚨 suggestion (security): The devPluginsServer middleware should guard against path traversal from plugin URLs and handle stream errors more defensively.
relative comes straight from the URL and is joined with PLUGINS_DIR, so .. segments could escape the intended directory. Normalize and resolve the path, then verify the resolved filePath is still under PLUGINS_DIR (e.g. via path.resolve and a prefix check) before serving. Also, fs.createReadStream(filePath) may emit errors after the statSync check; add an on('error', ...) handler that calls next(err) or returns a 404 to avoid uncaught errors.
Suggested implementation:
configureServer(server: any) {
server.middlewares.use((req: any, res: any, next: any) => {
const urlPath = (req.url || "").split("?")[0];
if (!urlPath.startsWith(PLUGINS_PREFIX)) {
return next();
}
// Normalize relative path to avoid path traversal (e.g. ".." segments)
const rawRelative = urlPath.slice(PLUGINS_PREFIX.length);
const normalizedRelative = path
.normalize(rawRelative)
.replace(/^([/\\])+/, ""); // strip any leading slashes/backslashes
const pluginsRoot = path.resolve(PLUGINS_DIR);
const filePath = path.resolve(PLUGINS_DIR, normalizedRelative);
// Ensure resolved path is still within the plugins root directory
if (filePath !== pluginsRoot && !filePath.startsWith(pluginsRoot + path.sep)) {
return next();
}
let stats: fs.Stats;
try {
stats = fs.statSync(filePath);
} catch {
return next();
}
if (!stats.isFile()) {
return next();
}
const ext = path.extname(filePath);
const mimeTypes: Record<string, string> = {
".js": "application/javascript",
".json": "application/json",
".map": "application/json",
};
res.setHeader("Content-Type", mimeTypes[ext] || "application/octet-stream");
res.setHeader("Access-Control-Allow-Origin", "*");
const stream = fs.createReadStream(filePath);
stream.on("error", (err: NodeJS.ErrnoException) => {
if (err.code === "ENOENT") {
if (!res.headersSent) {
res.statusCode = 404;
res.end("Not found");
}
return;
}
if (!res.headersSent) {
res.statusCode = 500;
res.end("Error reading file");
}
// Let Vite/Connect error handling deal with unexpected errors
next(err);
});
stream.pipe(res);
});
},This edit assumes fs and path are already imported in vite.config.ts as they are used elsewhere in the file. If not, you should add:
import fs from "fs";import path from "path";
near the top of the file, matching the existing import style.
| <ExtensionSlot | ||
| extensionPointId={EXTENSION_POINTS.DASHBOARD_CONTENT} | ||
| /> |
There was a problem hiding this comment.
question (bug_risk): ExtensionSlot is rendered directly inside Gallery, which may break layout if plugin components don’t return GalleryItem nodes.
If plugin components render arbitrary blocks (e.g. a bare Card), the PatternFly Gallery layout may break because it expects children wrapped in GalleryItem. Either document that extensions must render GalleryItem themselves, or have the slot map each extension into a GalleryItem so the layout contract is enforced by the host.
duzda
left a comment
duzda
left a comment
There was a problem hiding this comment.
It would make sense, to create packages folder and move each of the project there, then we can treat freeipa-webui as a monorepo of multiple related projects. This makes sense for plugins and we should decouple our code anyways and break it into multiple smaller packages.
There's A LOT happening in the sdk, this is not important now, please ensure the production environment works, as that is where the complexity lies.
Please don't touch code you don't have to, don't remove comments for the sake of removing.
A README on how all of this is put together would be helpful. Consider adding a page into /docs.
I've tried to run and grasp all of this together, however I've stumbled across the same issues as Carla did. I could resolve freeipa/sdk by moving it one level up and running npm run build, but this leads nowhere, as I've stumbled across following error:
After following the steps in the doc, Hello world from a dynamically loaded plugin indeed works, but that is purposeless, as we're simply reading a file from a dist folder, it's the same as loading an image from the dist folder, just with a bit more fluff
There was a problem hiding this comment.
This is duplicated from manifest.json, you can import json into .js file, but this defeats the purpose, the example needs to include vite (or any other build tool), which is where the "difficult" part comes into play.
There was a problem hiding this comment.
I think we can remove this example, as it's not realistic, it's too simple to really tell us anything.
| "private": true, | ||
| "type": "module", | ||
| "description": "Example plugin for FreeIPA Modern WebUI - demonstrates dynamic plugin loading.", | ||
| "license": "GPL-3.0-or-later", |
There was a problem hiding this comment.
If we decide to use examples, it shouldn't really be GPL licensed, I would steer very far away from any plugins as a developer. It's an issue for the future, and will have to be discussed team-wide, but instead of just using GPL rather omit the license for now.
There was a problem hiding this comment.
Also, I'm not sure if I would add the license parameter here. I would make sense to infer that the license for a custom plugin would be the same as FreeIPA's, IMHO.
| fileName: () => "plugin.js", | ||
| }, | ||
| sourcemap: true, | ||
| minify: false, |
There was a problem hiding this comment.
minify should be true, that's the purpose.
| targets: string[]; | ||
| title: string; | ||
| description: string; | ||
| component: ComponentType<any>; |
There was a problem hiding this comment.
Why this and any other component are of this type instead of ReactNode?
| "src" | ||
| ], | ||
| "scripts": { | ||
| "build": "tsc -b", |
There was a problem hiding this comment.
Please add a build system instead of using only tsc, you're including vite, I see no vite config and no invoking the build tool.
There was a problem hiding this comment.
That is a lot of types for MVP
There was a problem hiding this comment.
Adding Dashboard is out of scope, the Dashboard here is not related to plugins at all.
| * Vite plugin that serves plugin files from dev-plugins/ during development. | ||
| * In production, plugins are served by the IPA server (Apache/httpd). | ||
| */ | ||
| function devPluginsServer() { |
There was a problem hiding this comment.
Dev server is nice, but we need to make sure this works in production.
| import { api } from "./services/rpc"; | ||
| import { injectPluginReducer } from "./store/store"; | ||
|
|
||
| exposeSharedDependencies({ |
There was a problem hiding this comment.
I'm really unsure whether this works.
|
This PR has not received any attention in 60 days. |
3 participants
Summary by Sourcery
Introduce a dynamic plugin architecture and dashboard entry point for the FreeIPA Modern WebUI, enabling runtime-loaded plugins to extend navigation, routing, and UI content.
New Features:
Enhancements:
Build: