diff --git a/.changeset/16746-connect-agent-account-nav.md b/.changeset/16746-connect-agent-account-nav.md new file mode 100644 index 0000000000..f0808a8d62 --- /dev/null +++ b/.changeset/16746-connect-agent-account-nav.md @@ -0,0 +1,50 @@ +--- +'@objectstack/mcp': patch +--- + +Connect an Agent is reachable from the Account app, so a non-admin can mint their own key + +`POST /api/v1/keys` mints a `sys_api_key` bound to the **caller**, and the +Connect-an-Agent page says the key "acts as you". But the page's only navigation +entry sat in the Setup app, which declares `requiredPermissions: +['setup.access']` — so every non-admin following the shipped two-step guide, and +every reader of the runtime's own error text (`packages/mcp/src/plugin.ts`: +*"mint an API key (Setup → Connect an Agent, or POST /api/v1/keys)"*, and +`README.md`), stopped at step 1 while the endpoint behind the button had accepted +them all along. Measured before: a principal with no system permissions gets +`403 PERMISSION_DENIED` on `GET /api/v1/meta/apps/setup` and `nav_connect_agent` +is absent from the wire. + +`CONNECT_AGENT_UI_BUNDLE` now carries a **second** `navigationContributions` +entry, targeting the `account` app's `grp_account_developer` group beside the +`nav_account_api_keys` entry already shipping there. Measured after, over the +real composition (real `SETUP_APP` / `ACCOUNT_APP` / `SETUP_NAV_CONTRIBUTIONS`, +the real fold and the real RBAC-by-route filter): the same permissionless +principal gets `200` on `GET /api/v1/meta/apps/account` with +`grp_account_developer` carrying `['nav_account_api_keys', +'nav_account_oauth_apps', 'nav_connect_agent']`, while `apps/setup` still +answers `403 PERMISSION_DENIED` with `connect_agent` absent from that body. + +**Nothing else moves.** No backend change, no authorization change, no change to +which permissions exist, and the published "acts as you" promise is unchanged — +it simply becomes keepable for the users it was written for. The Setup entry +stays exactly as it was, so admins keep the page where the guide points, and no +gate is added or removed anywhere: a navigation contribution registers exactly +when the page registers, so an opted-out deployment +(`OS_MCP_SERVER_ENABLED=false`) still gets no page and neither entry. + +⛔ Ungating Setup was **not** the fix, and was measured rather than assumed: the +app-level `setup.access` gate fires before the group gate, so dropping the group +gate alone changes nothing, and dropping both serves 14+ unrelated Setup +surfaces (Users, Organization, Business Units, Branding, Feature Flags, …) to +every signed-in user. ⛔ Nor was a `requiresService: 'mcp'` gate on an +`account.app.ts` entry: the `mcp` service registers unconditionally in `init()` +while this bundle registers behind `isMcpServerEnabled()`, so such an entry +would outlive its page and 404 for every signed-in user on an opted-out +deployment. + +Both entries deliberately share the item id `nav_connect_agent` — one +destination, one identity. That is scoped, not a collision: `SchemaRegistry` +keys contributions by target app and `applyNavContributions(app)` consults only +that app's bucket, so a nav item id is unique within one app's navigation tree, +and the translation bundles are keyed `apps..navigation.`. diff --git a/packages/mcp/src/connect-agent-account-nav.test.ts b/packages/mcp/src/connect-agent-account-nav.test.ts new file mode 100644 index 0000000000..f71f47e376 --- /dev/null +++ b/packages/mcp/src/connect-agent-account-nav.test.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #16746 — the Connect-an-Agent page must be reachable by the principal the +// page's own promise is about. +// +// `POST /api/v1/keys` mints a `sys_api_key` bound to the CALLER and the page +// says the key "acts as you". Reached only through Setup it could not keep that +// promise: `SETUP_APP` declares `requiredPermissions: ['setup.access']`, so +// every non-admin following the shipped two-step guide — and the runtime's own +// error text (`plugin.ts`, `README.md`: "mint a key in Setup → Connect an +// Agent") — stopped at step 1 while the endpoint behind the button accepted +// them all along. Maintainer ruling 2026-09-08, option A: open the key card to +// every signed-in user; backend, authorization and the published promise do +// not move. +// +// --------------------------------------------------------------------------- +// What this file pins, and what it deliberately does NOT claim +// --------------------------------------------------------------------------- +// ⚠️ The acceptance criterion is a WIRE fact about two apps — a permissionless +// principal sees `nav_connect_agent` in the `account` app's nav while +// `GET /api/v1/meta/apps/setup` keeps answering 403 PERMISSION_DENIED — and +// that fact cannot be measured from this package: `@objectstack/mcp` declares +// no dependency on `@objectstack/rest` (the RBAC-by-route harness), on +// `@objectstack/objectql` (`SchemaRegistry.applyNavContributions`, the fold) or +// on `@objectstack/platform-objects` (`SETUP_APP` / `ACCOUNT_APP`). ⛔ So this +// file does not reimplement any of them — a second copy of the fold is exactly +// the divergence `cli/src/utils/nav-contribution-groups.ts` refuses to write. +// +// What it pins instead is the half this package OWNS, stated as the two +// properties the wire fact rests on: +// +// 1. the contribution is aimed at the ungated app and group, and carries +// nothing the server-side nav filter could strip for a permissionless +// caller (`requiredPermissions` / `requiresService`); +// 2. this bundle cannot widen Setup — the accident that would make a +// "the entry is visible" assertion pass for the wrong reason. +// +// The second is the load-bearing one. Ungating Setup was measured on the real +// composition and refused: the app-level `setup.access` gate fires BEFORE the +// group gate (so dropping `group_integrations`' gate alone changes nothing), +// and dropping both serves 14+ unrelated Setup surfaces to every signed-in +// user. Nothing in this bundle may reintroduce that, so the walk below asserts +// the bundle declares no permission key anywhere and no `apps` collection that +// could redeclare `SETUP_APP` without its gate. + +import { describe, it, expect } from 'vitest'; +import { NavigationContributionSchema } from '@objectstack/spec/ui'; + +import { CONNECT_AGENT_UI_BUNDLE } from './connect-ui.js'; + +type AnyRec = Record; + +const contributions = CONNECT_AGENT_UI_BUNDLE.navigationContributions as AnyRec[]; +const byApp = (app: string): AnyRec[] => contributions.filter((c) => c.app === app); +const itemsOf = (c: AnyRec): AnyRec[] => (c.items ?? []) as AnyRec[]; + +/** Every key present anywhere in a value, depth-first — objects and arrays. */ +function everyKey(value: unknown, out: string[] = []): string[] { + if (Array.isArray(value)) { + for (const entry of value) everyKey(entry, out); + } else if (value && typeof value === 'object') { + for (const [key, child] of Object.entries(value)) { + out.push(key); + everyKey(child, out); + } + } + return out; +} + +describe('#16746 — Connect an Agent reaches the per-user Account app', () => { + it('contributes into the `account` app, into the group that already ships API Keys', () => { + const account = byApp('account'); + expect(account).toHaveLength(1); + // `ACCOUNT_APP` declares no `requiredPermissions` (deliberately: every + // authenticated user must reach their own security surface, RLS scopes the + // rows), and `grp_account_developer` is the group already carrying + // `nav_account_api_keys`. Both are read by NAME here — this card edits + // nothing in `@objectstack/platform-objects`. + expect(account[0]).toMatchObject({ app: 'account', group: 'grp_account_developer' }); + }); + + it('aims one `page` item at `connect_agent`, the page this same bundle registers', () => { + const items = itemsOf(byApp('account')[0]); + expect(items).toHaveLength(1); + expect(items[0]).toMatchObject({ + id: 'nav_connect_agent', + type: 'page', + pageName: 'connect_agent', + }); + // The destination must be a page this bundle actually ships, or the entry + // is a 404-when-clicked shown to every signed-in user — the precise defect + // that ruled out gating an `account.app.ts` entry on `requiresService: + // 'mcp'` (the service registers unconditionally in `init()`; this bundle + // registers behind `isMcpServerEnabled()`). + const pageNames = (CONNECT_AGENT_UI_BUNDLE.pages ?? []).map((p) => p.name); + expect(pageNames).toContain(items[0].pageName); + }); + + it('carries nothing the per-user caller could be filtered on', () => { + // THE criterion, stated at the layer this package owns. The server-side + // nav filter strips an item for a caller missing its `requiredPermissions` + // and for an absent `requiresService` capability. An entry carrying either + // would be invisible to exactly the principal this card is about — a + // permissionless one — while every assertion above still passed. + const item = itemsOf(byApp('account')[0])[0]; + expect(item).not.toHaveProperty('requiredPermissions'); + expect(item).not.toHaveProperty('requiresService'); + expect(item).not.toHaveProperty('visible'); + }); + + it('leaves the Setup entry exactly as it was — admins keep the page where the guide points', () => { + const setup = byApp('setup'); + expect(setup).toHaveLength(1); + expect(setup[0]).toMatchObject({ app: 'setup', group: 'group_integrations', priority: 110 }); + expect(itemsOf(setup[0])).toEqual([ + { id: 'nav_connect_agent', type: 'page', pageName: 'connect_agent', label: 'Connect an Agent', icon: 'bot' }, + ]); + }); + + it('⛔ cannot widen Setup — no permission key and no app redeclaration anywhere in the bundle', () => { + // The risk the green has to prove, not merely pass. A test asserting only + // "the account entry exists" would go green just as happily on a diff that + // reached the card by ungating Setup instead — measured to serve 14+ + // unrelated Setup surfaces (Users, Organization, Branding, Feature Flags, + // …) to every signed-in user. This bundle is the one file that changed, so + // it is where that accident would have to be written. + const keys = new Set(everyKey(JSON.parse(JSON.stringify(CONNECT_AGENT_UI_BUNDLE)))); + expect([...keys].filter((k) => k === 'requiredPermissions')).toEqual([]); + // An `apps: [...]` collection here could redeclare `SETUP_APP` — last + // registration wins — and drop its `setup.access` gate without touching + // `platform-objects` at all. + expect(CONNECT_AGENT_UI_BUNDLE).not.toHaveProperty('apps'); + // Every contribution aims at a named group. A contribution with no `group` + // appends at the app's TOP level, which for Setup would put the entry + // outside `group_integrations`' gate. + for (const c of contributions) expect(typeof c.group).toBe('string'); + }); + + it('shares the item id across the two apps, and the fold says that is scoped per app', () => { + // Answered from the fold rather than from taste, because a wrong answer + // here is a silent one. `SchemaRegistry` keys contributions by TARGET APP + // (`appNavContributions: Map`) and `applyNavContributions(app)` + // consults only `get(app.name)`, so a nav item id is unique within ONE + // app's navigation tree; nothing indexes it across apps (no id-keyed + // registry, no de-duplication by id), and the translation bundles are + // keyed `apps..navigation.`, which makes one shared id two + // distinct keys. One destination therefore keeps one identity. + const targets = contributions.map((c) => c.app); + expect(new Set(targets).size).toBe(targets.length); + expect(contributions.flatMap((c) => itemsOf(c).map((i) => i.id))).toEqual([ + 'nav_connect_agent', + 'nav_connect_agent', + ]); + // ⛔ …and they are two literals, not one shared const. The fold + // `structuredClone`s the APP but pushes `...c.items` BY REFERENCE, so one + // shared object would sit in two apps' navigation trees at once and any + // in-place consumer edit would leak from one app into the other. + expect(itemsOf(byApp('setup')[0])[0]).not.toBe(itemsOf(byApp('account')[0])[0]); + }); + + it('both contributions parse against the real spec contract', () => { + // The shared id is not merely unenforced — it is accepted by the schema + // that governs the surface, checked against the spec rather than asserted + // about it. + for (const c of contributions) { + const parsed = NavigationContributionSchema.safeParse(c); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.success).toBe(true); + } + }); +}); diff --git a/packages/mcp/src/connect-ui.ts b/packages/mcp/src/connect-ui.ts index 42ada6a679..886bd3a7a7 100644 --- a/packages/mcp/src/connect-ui.ts +++ b/packages/mcp/src/connect-ui.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. /** - * "Connect an agent" Setup page — plugin-carried UI metadata (#2714 Phase 1, + * "Connect an agent" page — plugin-carried UI metadata (#2714 Phase 1, * objectui#2363). * * The page ships WITH the MCP capability (same principle as the marketplace @@ -16,6 +16,29 @@ * Registered by {@link MCPServerPlugin} on `kernel:ready`, gated on the same * default-on switch as the HTTP surface — an opted-out deployment * (`OS_MCP_SERVER_ENABLED=false`) gets no page and no nav entry. + * + * ## Two nav entry points, one page (#16746) + * + * `POST /api/v1/keys` mints a `sys_api_key` bound to the CALLER, and this page + * says the key "acts as you" — so the surface that mints it is a per-user + * credential surface, not an administrative one. Reached only through Setup it + * could not keep that promise: Setup declares `requiredPermissions: + * ['setup.access']`, so every non-admin following the two-step guide stopped at + * step 1 while the endpoint behind the button accepted them all along. + * + * The fix is a SECOND contribution, into the `account` app's Developer group + * beside the `nav_account_api_keys` entry already shipping there. The Setup + * entry stays for admins. ⛔ Ungating Setup is NOT the fix and was measured: + * the app-level `setup.access` gate fires BEFORE the group gate, so dropping + * the group gate alone changes nothing, and dropping both serves 14+ unrelated + * Setup surfaces (Users, Organization, Branding, Feature Flags, …) to every + * signed-in user — far past what this card asks for. ⛔ Nor is a + * `requiresService: 'mcp'` gate on an `account.app.ts` entry: the `mcp` service + * registers unconditionally in `init()` while this bundle registers behind + * `isMcpServerEnabled()`, so such an entry outlives its page and 404s on an + * opted-out deployment. A contribution registers exactly when the page + * registers, which is why both entries live in THIS bundle and neither carries + * a gate of its own. */ import type { Page } from '@objectstack/spec/ui'; @@ -59,8 +82,22 @@ export const CONNECT_AGENT_UI_BUNDLE = { type: 'plugin', scope: 'system', name: 'Connect an Agent UI', - description: 'Setup page + navigation for connecting MCP clients to this environment.', + description: 'Connect-an-Agent page + Setup and Account navigation for connecting MCP clients to this environment.', pages: [CONNECT_AGENT_PAGE], + // Both entries point at the one `connect_agent` page and share the item id. + // That is legal and deliberate, read off the fold rather than assumed: the + // registry keys contributions by TARGET APP + // (`appNavContributions: Map`) and `applyNavContributions(app)` + // consults only `get(app.name)`, so a nav item id is unique within one app's + // navigation tree and nothing indexes it across apps — no id-keyed registry, + // no de-duplication by id, and the translation bundles are keyed + // `apps..navigation.` (per-app namespaces, so one id yields two + // distinct keys). Sharing it keeps ONE identity for one destination. + // + // ⛔ The two items are separate object literals, not one shared const: the + // fold `structuredClone`s the APP but pushes `...c.items` by reference, so a + // shared literal would put the same object in two apps' navigation trees and + // any in-place consumer edit would leak across them. navigationContributions: [ { app: 'setup', @@ -76,5 +113,29 @@ export const CONNECT_AGENT_UI_BUNDLE = { }, ], }, + { + // The per-user half. `ACCOUNT_APP` declares no `requiredPermissions` — + // deliberately, so every authenticated user reaches their own security + // surface — and `grp_account_developer` already carries + // `nav_account_api_keys`. Targeted by NAME; `packages/platform-objects` + // is not edited. + app: 'account', + group: 'grp_account_developer', + // `priority` orders contributions among THEMSELVES within the group, and + // the group's own static children always precede them. Stated rather + // than defaulted, at the schema's declared default: no other package + // contributes into this group today, so there is nothing to interleave + // with and no reason to claim a position. + priority: 200, + items: [ + { + id: 'nav_connect_agent', + type: 'page', + pageName: 'connect_agent', + label: 'Connect an Agent', + icon: 'bot', + }, + ], + }, ], };