diff --git a/.changeset/federation-family-capability-gate.md b/.changeset/federation-family-capability-gate.md new file mode 100644 index 0000000000..37fc9f011d --- /dev/null +++ b/.changeset/federation-family-capability-gate.md @@ -0,0 +1,21 @@ +--- +"@objectstack/rest": patch +--- + +**Behaviour change (tightening) — a capability is now required on the external-datasource federation family** (`/api/v1/datasources/:name/external/*`, #9901). These routes previously admitted **any authenticated caller**; four of the five now also require a platform capability. Maintainer ruling, 2026-08-20 (verbatim: 「其他接受你的建议。」). + +**This is published SDK surface.** `datasources.external.*` on `ObjectStackClient` reaches exactly these routes, and the CLI's `datasource` commands go through them. An existing integration that presents a valid credential — a better-auth session or a `sys_api_key` — and holds neither capability was served before and is **refused now**. Nothing about the credential itself changed; what changed is what the credential must carry. + +| route | SDK call | now requires | +| --- | --- | --- | +| `GET /:name/external/tables` | `datasources.external.listTables` | `manage_platform_settings` | +| `POST /:name/external/tables/:remote/draft` | `datasources.external.draft` | `manage_platform_settings` | +| `POST /:name/external/tables/:remote/import` | `datasources.external.import` | `manage_metadata` | +| `POST /:name/external/refresh-catalog` | `datasources.external.refreshCatalog` | `manage_metadata` | +| `POST /:name/external/validate` | `datasources.external.validate` | *(unchanged — authentication only)* | + +A refusal is **`403` with the standard catalog code `PERMISSION_DENIED`** (ADR-0112; deliberately not the grandfathered `FORBIDDEN` synonym), and the message names the missing capability so the caller knows which grant to request. The anonymous floor is unchanged: no identity is still `401 UNAUTHENTICATED`. + +**Why these two capabilities.** The first two routes are the declared twins of `GET /:name/remote-tables` and `POST /:name/object-draft` on the datasource-admin spelling, which has required `manage_platform_settings` since #9593 — the same operation was reachable through two mounted routes with two different admission policies, so an agent or integration refused at one spelling was served at the other. The two write routes have no twin and create live metadata (the import mounts a runtime-origin federated object; the refresh rewrites the cached catalog snapshot), so they take `manage_metadata`, this package's existing gate for metadata creation. + +**Migration.** Grant the caller's permission set the capability its routes need — `manage_platform_settings` for remote-schema introspection, `manage_metadata` for import/refresh. The platform's `admin_full_access` set already carries both, so admin-credentialed integrations are unaffected; a purpose-built operator set is the case to check. diff --git a/packages/rest/src/discovery-advertised-direct-mounts.parity.test.ts b/packages/rest/src/discovery-advertised-direct-mounts.parity.test.ts index 7e2d2d45c2..cdf0dc2773 100644 --- a/packages/rest/src/discovery-advertised-direct-mounts.parity.test.ts +++ b/packages/rest/src/discovery-advertised-direct-mounts.parity.test.ts @@ -143,8 +143,16 @@ function boot(opts: { // authorized caller reaches it in production, keeping this test's subject // (does the advertised URL resolve and answer in the mounted table) intact. // The gate itself is pinned in `package-envelope.conformance.test.ts`. + // [#9901] `manage_platform_settings` joins the set for the same reason: + // the federation family's read routes now carry a capability gate too, and + // the advertised `…/external/tables` URL is driven below. Without it that + // probe would read a 403 and this pin's subject (does the advertised URL + // resolve and answer in the mounted table) would quietly become an authz + // assertion. That gate is pinned in + // `external-datasource-routes-auth-guard.test.ts`. resolveExecutionContext: async () => ({ - userId: 'u_pkg', systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], + userId: 'u_pkg', + systemPermissions: ['manage_metadata', 'studio.access', 'setup.access', 'manage_platform_settings'], }), enableProjectScoping: opts.enableProjectScoping, projectResolution: opts.projectResolution, diff --git a/packages/rest/src/external-datasource-envelope.conformance.test.ts b/packages/rest/src/external-datasource-envelope.conformance.test.ts index 8f3054f1a4..3db0c09422 100644 --- a/packages/rest/src/external-datasource-envelope.conformance.test.ts +++ b/packages/rest/src/external-datasource-envelope.conformance.test.ts @@ -51,10 +51,22 @@ interface Captured { * would read the 401 body instead of the arm it names, and this file would * silently stop measuring what it exists to measure. * + * [#9901] …and an ENTITLED one: four of the five routes now also require a + * capability (`manage_platform_settings` on the reads, `manage_metadata` on the + * writes), so this stub holds both. Same reasoning one step further — a + * resolver carrying an identity but no grants would turn every case below into + * a reading of the 403 body. Holding both rather than one per case is + * deliberate: which capability each route requires is not this file's subject, + * and pinning it twice would make the split harder to change in the one place + * that does own it. + * * The guard itself is pinned in `external-datasource-routes-auth-guard.test.ts`; * the 401's own envelope is the last case below, which is this file's business. */ -const CREDENTIALED = async () => ({ userId: 'u_env_conformance' }); +const CREDENTIALED = async () => ({ + userId: 'u_env_conformance', + systemPermissions: ['manage_platform_settings', 'manage_metadata'], +}); /** * A resolver that RESOLVES, and resolves to no identity — the anonymous case as diff --git a/packages/rest/src/external-datasource-routes-auth-guard.test.ts b/packages/rest/src/external-datasource-routes-auth-guard.test.ts index f397b13fd1..15c5a8eec1 100644 --- a/packages/rest/src/external-datasource-routes-auth-guard.test.ts +++ b/packages/rest/src/external-datasource-routes-auth-guard.test.ts @@ -2,7 +2,8 @@ /** * [#9686] The `/api/v1/datasources/:name/external/*` federation family requires - * an authenticated caller — on every route, read and write alike. + * an authenticated caller — on every route, read and write alike — and + * [#9901] a CAPABILITY above that on four of the five. * * ## What this pins, and why it is driven through the real plugin * @@ -37,6 +38,29 @@ * refused credentialed callers would be this fix breaking the feature, and * a one-sided pin could not tell the two apart. * + * ## [#9901] "Entitled" is now two facts, and the middle of the axis is pinned + * + * Authentication was the whole gate until the 2026-08-20 ruling (verbatim: + * 「其他接受你的建议。」) put `manage_platform_settings` on the two read twins + * and `manage_metadata` on the two writes. So a THIRD posture now exists + * between "anonymous" and "entitled" — authenticated, holding nothing — and it + * gets its own cases below rather than being left to the two ends. Each asserts + * `403` AND the machine-readable `PERMISSION_DENIED`, never "not 200": the 401 + * the anonymous cases already cover would satisfy that, which would mean the + * credential was never read at all. + * + * The split itself is asserted, not just the refusals: a caller holding ONLY + * `manage_platform_settings` clears the reads and is refused the writes, and a + * caller holding ONLY `manage_metadata` the reverse. A single gate keyed on + * either capability alone would pass an "unentitled is refused" case and fail + * here, which is what makes the read/write split falsifiable rather than + * merely written down. + * + * `POST /external/validate` is the one route the ruling does not name: it has + * no admin twin and creates no metadata, so it keeps the #9686 authentication + * floor. That is pinned too — an un-ruled route silently acquiring a + * neighbour's gate is a change nobody decided. + * * Both credential kinds the platform admits are exercised, because the cheap * mistake here is to read only a better-auth session: that would refuse a * caller presenting a valid `sys_api_key`, a credential admitted everywhere @@ -64,21 +88,33 @@ const API_KEY = 'osk_federation_caller_secret'; type Handler = (req: any, res: any) => any; /** - * The five routes of the family, each with the status it answers a credentialed - * caller and the service method it dispatches to. + * The five routes of the family, each with the status it answers an entitled + * caller, the service method it dispatches to, and [#9901] the capability it + * requires above authentication. * * `writes` marks the two that change state — the import creates a live * runtime-origin federated object, the refresh rewrites the cached catalog * snapshot. Both are asserted to be unreachable without an identity. + * + * `capability: null` is `POST /external/validate`, the one route the ruling + * does not name. Spelled as an explicit `null` rather than omitted so that a + * later edit which gates it has to change this table — an absent field would + * let that happen silently. */ +const READ_CAPABILITY = 'manage_platform_settings'; +const WRITE_CAPABILITY = 'manage_metadata'; + const FAMILY = [ - { method: 'GET', url: `${BASE}/datasources/${DS}/external/tables`, ok: 200, call: 'listRemoteTables', writes: false }, - { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/draft`, ok: 200, call: 'generateObjectDraft', writes: false }, - { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/import`, ok: 201, call: 'importObject', writes: true }, - { method: 'POST', url: `${BASE}/datasources/${DS}/external/refresh-catalog`, ok: 200, call: 'refreshCatalog', writes: true }, - { method: 'POST', url: `${BASE}/datasources/${DS}/external/validate`, ok: 200, call: 'validateAll', writes: false }, + { method: 'GET', url: `${BASE}/datasources/${DS}/external/tables`, ok: 200, call: 'listRemoteTables', writes: false, capability: READ_CAPABILITY }, + { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/draft`, ok: 200, call: 'generateObjectDraft', writes: false, capability: READ_CAPABILITY }, + { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/import`, ok: 201, call: 'importObject', writes: true, capability: WRITE_CAPABILITY }, + { method: 'POST', url: `${BASE}/datasources/${DS}/external/refresh-catalog`, ok: 200, call: 'refreshCatalog', writes: true, capability: WRITE_CAPABILITY }, + { method: 'POST', url: `${BASE}/datasources/${DS}/external/validate`, ok: 200, call: 'validateAll', writes: false, capability: null }, ] as const; +/** Every capability an entitled caller needs to clear all five routes. */ +const FULL_GRANT = [READ_CAPABILITY, WRITE_CAPABILITY] as const; + /** A host server whose registrations land in a real handler table. */ function createRecordingServer() { const table = new Map(); @@ -141,7 +177,9 @@ function federationServiceSpies() { * consults to resolve a caller; a boot may wire either, both or neither, which * is how the cases below separate the credential kinds and the anonymous floor. */ -async function bootFederation(opts: { withAuth?: boolean; withEngine?: boolean } = {}) { +async function bootFederation( + opts: { withAuth?: boolean; withEngine?: boolean; grants?: readonly string[] } = {}, +) { const server = createRecordingServer(); const service = federationServiceSpies(); const lookups: string[] = []; @@ -155,15 +193,52 @@ async function bootFederation(opts: { withAuth?: boolean; withEngine?: boolean } }, }; - // A minimal engine: the api-key admission path reads `sys_api_key` by the - // at-rest hash of the presented secret, and the grant aggregation that - // follows reads membership/position objects that simply have no rows here. + /** + * A minimal engine: the api-key admission path reads `sys_api_key` by the + * at-rest hash of the presented secret, and the grant aggregation that + * follows reads membership/position objects that simply have no rows here. + * + * [#9901] …except the two it now MUST have rows in. The capability gate reads + * `systemPermissions`, which `resolveAuthzContext` aggregates off + * `sys_user_permission_set` → `sys_permission_set`, so a boot with no engine + * resolves an identity holding NOTHING — which is a real posture (pinned + * below) but not the entitled one. `opts.grants` is what the caller + * `u_federation` holds, so one fixture expresses every posture on the axis. + * + * The set is deliberately not `admin_full_access`: that platform set carries + * `manage_platform_settings` among six other capabilities, so a gate keyed on + * platform-admin posture rather than on the named capability would pass here + * unnoticed — the same reason the twin-equivalence fixture builds its own. + */ + const grants = opts.grants ?? FULL_GRANT; + const GRANT_SET_ID = 'ps_federation_caller'; const engine = { find: async (object: string, query: any) => { - if (object !== 'sys_api_key') return []; - return query?.where?.key === hashApiKey(API_KEY) && query?.where?.revoked === false - ? [{ id: 'key_1', key: hashApiKey(API_KEY), user_id: 'u_federation', revoked: false }] - : []; + if (object === 'sys_api_key') { + return query?.where?.key === hashApiKey(API_KEY) && query?.where?.revoked === false + ? [{ id: 'key_1', key: hashApiKey(API_KEY), user_id: 'u_federation', revoked: false }] + : []; + } + if (object === 'sys_user_permission_set') { + return query?.where?.user_id === 'u_federation' && grants.length > 0 + ? [{ id: 'ups_1', user_id: 'u_federation', permission_set_id: GRANT_SET_ID, organization_id: null }] + : []; + } + if (object === 'sys_permission_set') { + const ids: string[] = query?.where?.id?.$in ?? []; + return ids.includes(GRANT_SET_ID) + ? [{ + id: GRANT_SET_ID, + name: 'federation_caller', + // JSON string — the spelling SQLite hands back, which the + // resolver parses. Pinning the stored shape keeps the fixture on + // the real read path. + system_permissions: JSON.stringify([...grants]), + object_permissions: '{}', + }] + : []; + } + return []; }, }; @@ -283,7 +358,12 @@ describe('[#9686] the external-datasource federation family refuses an anonymous describe('[#9686] the same boot still serves an entitled caller', () => { it('answers every route with its real success status for a session-authenticated caller', async () => { - const { table, service } = await bootFederation({ withAuth: true }); + // [#9901] The engine is now part of what makes this caller ENTITLED, not + // fixture noise: `systemPermissions` is aggregated off it, so a boot + // without one resolves an identity holding nothing and every gated route + // would answer 403. Wiring it here keeps this case measuring what it names + // — the success arm — rather than quietly becoming a refusal case. + const { table, service } = await bootFederation({ withAuth: true, withEngine: true }); for (const route of FAMILY) { const { statusCode, body } = await call(table, route, { authorization: `Bearer ${SESSION}` }); @@ -312,3 +392,105 @@ describe('[#9686] the same boot still serves an entitled caller', () => { expect(service.importObject).toHaveBeenCalledWith(DS, 'customers', {}); }); }); + +describe('[#9901] the family requires a capability above authentication', () => { + it('refuses an authenticated caller holding NOTHING on all four ruled routes — 403 PERMISSION_DENIED, before the service', async () => { + const { table, service, lookups } = await bootFederation({ + withAuth: true, withEngine: true, grants: [], + }); + + for (const route of FAMILY.filter((r) => r.capability !== null)) { + const { statusCode, body } = await call(table, route, { authorization: `Bearer ${SESSION}` }); + + // Status AND code. "not 200" would be satisfied by the 401 the anonymous + // cases already cover, which would mean the credential was never read. + expect(statusCode, `${route.method} ${route.url}`).toBe(403); + expect(body?.success, `${route.method} ${route.url}`).toBe(false); + expect(body?.error?.code, `${route.method} ${route.url}`).toBe('PERMISSION_DENIED'); + // The named capability is in the message, because that is the one thing a + // refused caller must be able to act on. + expect(body?.error?.message, `${route.method} ${route.url}`).toContain(route.capability); + } + + // The refusal precedes dispatch — so on the two routes that WRITE, nothing + // was created before the caller was turned away. + for (const route of FAMILY.filter((r) => r.capability !== null)) { + expect( + (service as any)[route.call], + `${route.call} must not run for an unentitled caller`, + ).not.toHaveBeenCalled(); + } + expect(lookups).not.toContain('external-datasource'); + }); + + it('the read/write split is real: `manage_platform_settings` alone clears the reads and is refused the writes', async () => { + const { table } = await bootFederation({ + withAuth: true, withEngine: true, grants: [READ_CAPABILITY], + }); + + for (const route of FAMILY.filter((r) => r.capability === READ_CAPABILITY)) { + const { statusCode } = await call(table, route, { authorization: `Bearer ${SESSION}` }); + expect(statusCode, `${route.method} ${route.url}`).toBe(route.ok); + } + for (const route of FAMILY.filter((r) => r.capability === WRITE_CAPABILITY)) { + const { statusCode, body } = await call(table, route, { authorization: `Bearer ${SESSION}` }); + expect(statusCode, `${route.method} ${route.url}`).toBe(403); + expect(body?.error?.code, `${route.method} ${route.url}`).toBe('PERMISSION_DENIED'); + } + }); + + it('…and the other way round: `manage_metadata` alone clears the writes and is refused the reads', async () => { + // Both directions, because a gate that required EITHER capability on every + // route would satisfy the unentitled case above and the two halves of the + // previous case would still pass one at a time. Only the crossed pair can + // tell "two capabilities" from "one capability spelled twice". + const { table } = await bootFederation({ + withAuth: true, withEngine: true, grants: [WRITE_CAPABILITY], + }); + + for (const route of FAMILY.filter((r) => r.capability === WRITE_CAPABILITY)) { + const { statusCode } = await call(table, route, { authorization: `Bearer ${SESSION}` }); + expect(statusCode, `${route.method} ${route.url}`).toBe(route.ok); + } + for (const route of FAMILY.filter((r) => r.capability === READ_CAPABILITY)) { + const { statusCode, body } = await call(table, route, { authorization: `Bearer ${SESSION}` }); + expect(statusCode, `${route.method} ${route.url}`).toBe(403); + expect(body?.error?.code, `${route.method} ${route.url}`).toBe('PERMISSION_DENIED'); + } + }); + + it('POST /external/validate keeps the #9686 authentication floor — the ruling does not name it', async () => { + // The route the 2026-08-20 ruling enumerates NO capability for: no admin + // twin, no metadata created. An authenticated caller holding nothing is + // served here while being refused the other four on the same boot, which is + // the difference stated rather than implied. A later card may change this; + // it will have to change this case to do it. + const { table, service } = await bootFederation({ + withAuth: true, withEngine: true, grants: [], + }); + + const validate = FAMILY.find((r) => r.capability === null)!; + const { statusCode, body } = await call(table, validate, { authorization: `Bearer ${SESSION}` }); + + expect(statusCode).toBe(validate.ok); + expect(body?.success).toBe(true); + expect(service.validateAll).toHaveBeenCalled(); + }); + + it('a capability the caller does not hold is not granted by an api key either', async () => { + // The api-key admission path seeds `permissions` from the key's scopes and + // then aggregates grants off the SAME `sys_*` tables — so an unentitled key + // holder is refused exactly like an unentitled session. A gate that read + // the key's scopes instead of `systemPermissions` would pass the session + // cases above and open the family to every key. + const { table, service } = await bootFederation({ + withAuth: true, withEngine: true, grants: [], + }); + + const { statusCode, body } = await call(table, FAMILY[2], { 'x-api-key': API_KEY }); + + expect(statusCode).toBe(403); + expect(body?.error?.code).toBe('PERMISSION_DENIED'); + expect(service.importObject).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index fc6ed46b1e..53b5bd245e 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -90,13 +90,56 @@ export interface ExternalDatasourceRoutesOptions { * * Absent ⇒ the gate FAILS CLOSED (401). `isSystem` is never resolved from * inbound HTTP. + * + * [#9901] `systemPermissions` is the resolver's aggregate of every permission + * set the caller holds — the SAME field the sibling `package-routes.ts` gate, + * the `/meta` REST gate and the declared admin twin + * (`service-datasource/src/admin-routes.ts`) each read. It was already + * supplied at run time by the composition, which types this option as + * `PackageRoutesOptions['resolveExecutionContext']`; only this local + * declaration narrowed it away, so the capability gate below reads the + * platform's one capability resolution rather than a second reading of + * `sys_*`. */ resolveExecutionContext?: (req: any) => Promise<{ userId?: string | null; isSystem?: boolean; + systemPermissions?: string[]; } | undefined>; } +/** + * [#9901] The capability the federation family's READ routes require: + * `GET /external/tables` and `POST /external/tables/:remote/draft`. + * + * It is `manage_platform_settings` because these two routes are the DECLARED + * TWINS of `GET /:name/remote-tables` and `POST /:name/object-draft` on the + * admin spelling, which measured exactly this capability in #9593 + * (`DATASOURCE_ADMIN_CAPABILITY`). One operation reached through two mounted + * routes cannot admit two different sets of callers — that asymmetry is what + * this card closes, and `remote-tables-twin.equivalence.test.ts` is where the + * two spellings are compared rather than merely each pinned. + * + * Maintainer ruling, 2026-08-20 (verbatim: 「其他接受你的建议。」): the + * federation family is NOT deliberately the lower-privilege door. + */ +export const FEDERATION_READ_CAPABILITY = 'manage_platform_settings'; + +/** + * [#9901] The capability the federation family's WRITE routes require: + * `POST /external/tables/:remote/import` and `POST /external/refresh-catalog`. + * + * Not the read capability, and deliberately so: these two have no twin on the + * admin spelling to converge with, and what they do is CREATE METADATA — the + * import mounts a live runtime-origin federated object, the refresh rewrites + * the cached catalog snapshot. `manage_metadata` is this package's established + * gate for exactly that (`package-routes.ts`, whose write arm requires it for + * `POST /packages/publish` and `DELETE /packages/:id`), so the split follows + * the platform's existing division rather than inventing a second policy for + * this family. + */ +export const FEDERATION_WRITE_CAPABILITY = 'manage_metadata'; + export function registerExternalDatasourceRoutes( server: IHttpServer, ctx: PluginContext, @@ -106,9 +149,10 @@ export function registerExternalDatasourceRoutes( const ext = `${basePath}/datasources/:name/external`; /** - * [#9686] The authentication floor for this whole family. Answers - * `401 UNAUTHENTICATED` and returns `true` when the caller must be refused, - * so every handler opens with `if (await refuseAnonymous(req, res)) return;`. + * [#9686 authentication, #9901 capability] The admission floor for this whole + * family. Answers `401 UNAUTHENTICATED` or `403 PERMISSION_DENIED` and + * returns `true` when the caller must be refused, so every handler opens with + * `if (await refuseFederationRequest(req, res, '')) return;`. * * ## Why this registrar needs its own line * @@ -150,22 +194,98 @@ export function registerExternalDatasourceRoutes( * `503` which services a deployment has wired, and — for the two routes that * write — so the refusal provably precedes the write rather than following it. * - * Authentication and nothing more: whether these routes should FURTHER - * require a capability is the separately-ruled question #9593 asks of the - * admin family, and is deliberately absent here. + * ## [#9901] …and a CAPABILITY above it, on four of the five routes + * + * #9686 left this family gated on authentication alone and pointed the + * capability question at #9593, which answered it for the admin half only. + * The residue was a governance asymmetry rather than an oversight: an + * authenticated caller holding no platform capability was refused live + * remote-schema introspection at `GET /:name/remote-tables` and SERVED the + * same operation at `GET /:name/external/tables`. Maintainer ruling, + * 2026-08-20 (verbatim: 「其他接受你的建议。」): tighten this family — reads + * on {@link FEDERATION_READ_CAPABILITY}, writes on + * {@link FEDERATION_WRITE_CAPABILITY}. + * + * ## One resolution, two decisions — deliberately not two guards + * + * The identity and the held capabilities come out of the SAME + * `resolveExecutionContext` call, the shape the declared admin twin took for + * the same reason: splitting this into an authentication guard followed by a + * capability guard would resolve one request twice, and two resolutions of + * one request can disagree — the second read is a fresh set of `sys_*` + * queries against a store another request may have written in between. + * + * The order is fixed, anonymous first. A caller with no identity must be told + * it has no identity, not that its (empty) capability set is insufficient — + * the latter is both wrong and a hint that the credential was read and + * rejected on other grounds. + * + * ## The refusal code, and why it is not this package's other spelling + * + * `403 PERMISSION_DENIED` — the STANDARD-catalog code for its status, which + * `HttpStatusErrorCodeMap` names for 403. Deliberately NOT `FORBIDDEN`, which + * the sibling `package-routes.ts` emits: that spelling is a grandfathered + * pre-gate synonym (ADR-0112 D3's `STANDARD_SYNONYM_WAIVERS`), and the waiver + * schema's own words are that it "keeps a WIRE VALUE registered; it does not + * endorse the spelling for new code". It is also the code the declared admin + * twin answers, which the twin-equivalence suite compares directly: the read + * routes must agree with it on status AND machine-readable code, not merely + * both be "not 200". + * + * ## `isSystem` is not a capability bypass here + * + * The anonymous decision reads it because `shouldDenyAnonymous` does; the + * capability decision reads the HELD SET only, exactly as the admin twin + * does. An `isSystem` arm would be a SECOND policy beside the capability, and + * — since `isSystem` is never resolved from inbound HTTP — one no wire caller + * could ever take, so it would be unfalsifiable divergence from the twin. + * + * ## `'authenticated'`: the one route the ruling does not name + * + * `POST /external/validate` has no twin on the admin spelling, creates no + * metadata, and is NOT one of the four routes the 2026-08-20 ruling + * enumerates. It keeps the #9686 authentication floor and says so with its + * own kind rather than silently inheriting a neighbour's gate — an un-ruled + * route that shared a constant would read as ruled. Filed separately rather + * than decided here. */ - const refuseAnonymous = async (req: any, res: any): Promise => { - let authz: { userId?: string | null; isSystem?: boolean } | undefined; + const refuseFederationRequest = async ( + req: any, + res: any, + kind: 'read' | 'write' | 'authenticated', + ): Promise => { + let authz: + | { userId?: string | null; isSystem?: boolean; systemPermissions?: string[] } + | undefined; try { authz = await options.resolveExecutionContext?.(req); } catch { - // An identity that could not be resolved is not an identity. + // An identity that could not be resolved is not an identity, and grants + // that could not be read are not grants. authz = undefined; } if (shouldDenyAnonymous({ userId: authz?.userId, isSystem: authz?.isSystem, method: req?.method })) { sendError(res, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY_CODE, ANONYMOUS_DENY_MESSAGE); return true; } + if (kind === 'authenticated') return false; + const required = kind === 'write' ? FEDERATION_WRITE_CAPABILITY : FEDERATION_READ_CAPABILITY; + const held = Array.isArray(authz?.systemPermissions) ? authz.systemPermissions : []; + if (!held.includes(required)) { + // The capability's name and nothing else. A refused caller needs to know + // which grant to ask an administrator for; it must not learn whether the + // named datasource exists or which services this deployment wired — + // which is also why this runs BEFORE the service lookup below. + sendError( + res, + 403, + 'PERMISSION_DENIED', + kind === 'write' + ? `Creating metadata from an external datasource requires the \`${FEDERATION_WRITE_CAPABILITY}\` capability.` + : `Introspecting an external datasource requires the \`${FEDERATION_READ_CAPABILITY}\` capability.`, + ); + return true; + } return false; }; @@ -218,7 +338,7 @@ export function registerExternalDatasourceRoutes( path: `${ext}/tables`, metadata: { summary: 'List remote tables on an external datasource', tags: ['datasources'] }, handler: async (req: any, res: any) => { - if (await refuseAnonymous(req, res)) return; + if (await refuseFederationRequest(req, res, 'read')) return; const svc = externalService(); if (!svc?.listRemoteTables) return unavailable(res); try { @@ -237,7 +357,7 @@ export function registerExternalDatasourceRoutes( path: `${ext}/tables/:remote/draft`, metadata: { summary: 'Generate an Object draft from a remote table', tags: ['datasources'] }, handler: async (req: any, res: any) => { - if (await refuseAnonymous(req, res)) return; + if (await refuseFederationRequest(req, res, 'read')) return; const svc = externalService(); if (!svc?.generateObjectDraft) return unavailable(res); try { @@ -262,7 +382,7 @@ export function registerExternalDatasourceRoutes( path: `${ext}/tables/:remote/import`, metadata: { summary: 'Import a remote table as a federated object', tags: ['datasources'] }, handler: async (req: any, res: any) => { - if (await refuseAnonymous(req, res)) return; + if (await refuseFederationRequest(req, res, 'write')) return; const svc = externalService(); if (!svc?.importObject) return unavailable(res); try { @@ -289,7 +409,7 @@ export function registerExternalDatasourceRoutes( path: `${ext}/refresh-catalog`, metadata: { summary: 'Refresh the external datasource catalog snapshot', tags: ['datasources'] }, handler: async (req: any, res: any) => { - if (await refuseAnonymous(req, res)) return; + if (await refuseFederationRequest(req, res, 'write')) return; const svc = externalService(); if (!svc?.refreshCatalog) return unavailable(res); try { @@ -307,7 +427,7 @@ export function registerExternalDatasourceRoutes( path: `${ext}/validate`, metadata: { summary: 'Validate the federated objects on a datasource', tags: ['datasources'] }, handler: async (req: any, res: any) => { - if (await refuseAnonymous(req, res)) return; + if (await refuseFederationRequest(req, res, 'authenticated')) return; const svc = externalService(); if (!svc?.validateAll) return unavailable(res); try { diff --git a/packages/rest/src/remote-tables-twin.equivalence.test.ts b/packages/rest/src/remote-tables-twin.equivalence.test.ts index 80c97e65e6..37483261a7 100644 --- a/packages/rest/src/remote-tables-twin.equivalence.test.ts +++ b/packages/rest/src/remote-tables-twin.equivalence.test.ts @@ -109,16 +109,29 @@ const REMOTE: IntrospectedSchema = { const SESSION = 'Bearer twin-session'; /** - * [#9593] A second credential: authenticated, holding nothing. This is the - * posture on which the two spellings now genuinely DIVERGE, and the divergence - * gets its own pinned case at the bottom of this file rather than being - * papered over here. + * [#9593] A second credential: authenticated, holding nothing. This was the + * posture the two spellings diverged on until #9901; it now has its own + * AGREEMENT case at the bottom of this file — both spellings refuse it, and the + * two refusals are compared to each other. */ const UNENTITLED_SESSION = 'Bearer twin-session-unentitled'; +/** + * [#9901] A third credential: authenticated and holding a real permission set + * that simply is not the one either spelling requires. + * + * Without it, "unentitled" in this file would only ever mean "holds no grant at + * all", and a gate that asked whether the caller holds ANYTHING would satisfy + * every case here. The two spellings must agree on the capability, not on the + * existence of a grant — which is what makes the equivalence a statement about + * `manage_platform_settings` rather than about permission sets in general. + */ +const OTHER_GRANT_SESSION = 'Bearer twin-session-other-grant'; + const USERS: Record = { [SESSION]: 'u_twin', [UNENTITLED_SESSION]: 'u_twin_plain', + [OTHER_GRANT_SESSION]: 'u_twin_other', }; const authService = { @@ -133,6 +146,12 @@ const authService = { /** The permission set carrying the grant `u_twin` holds and `u_twin_plain` does not. */ const GRANT_SET_ID = 'ps_twin_datasource_operator'; +/** + * [#9901] A permission set that is real, non-empty, and carries neither + * capability this family gates on — held by `u_twin_other`. + */ +const OTHER_SET_ID = 'ps_twin_org_user_admin'; + /** * The RBAC tables `resolveAuthzContext` reads, as a fake data engine — the * same idiom this package's other authz fixtures use @@ -151,27 +170,44 @@ const GRANT_SET_ID = 'ps_twin_datasource_operator'; * `manage_platform_settings` among six other capabilities, so a gate keyed on * platform-admin posture rather than on the capability would pass unnoticed. */ +const SETS: Record = { + [GRANT_SET_ID]: { name: 'twin_datasource_operator', systemPermissions: ['manage_platform_settings'] }, + // [#9901] Deliberately a DECLARED capability (`packages/spec`'s capability + // catalog) from a different family: a set whose grants are real but + // irrelevant here. An invented name would make this case pass for the wrong + // reason — because nothing recognises it — rather than because the gate reads + // the capability it names. + [OTHER_SET_ID]: { name: 'twin_org_user_admin', systemPermissions: ['manage_org_users'] }, +}; + +const HOLDS: Record = { + u_twin: GRANT_SET_ID, + u_twin_other: OTHER_SET_ID, + // `u_twin_plain` holds nothing, deliberately — it is not listed. +}; + const makeQl = () => ({ find: async (object: string, opts: any) => { const where = opts?.where ?? {}; if (object === 'sys_user_permission_set') { - return where.user_id === 'u_twin' - ? [{ id: 'ups_twin', user_id: 'u_twin', permission_set_id: GRANT_SET_ID, organization_id: null }] + const setId = HOLDS[where.user_id as string]; + return setId + ? [{ id: `ups_${where.user_id}`, user_id: where.user_id, permission_set_id: setId, organization_id: null }] : []; } if (object === 'sys_permission_set') { const ids: string[] = where.id?.$in ?? []; - return ids.includes(GRANT_SET_ID) - ? [{ - id: GRANT_SET_ID, - name: 'twin_datasource_operator', - // JSON string — the spelling SQLite hands back, which the resolver - // parses. Pinning the stored shape keeps the fixture on the real - // read path. - system_permissions: JSON.stringify(['manage_platform_settings']), - object_permissions: '{}', - }] - : []; + return ids + .filter((id) => id in SETS) + .map((id) => ({ + id, + name: SETS[id].name, + // JSON string — the spelling SQLite hands back, which the resolver + // parses. Pinning the stored shape keeps the fixture on the real + // read path. + system_permissions: JSON.stringify(SETS[id].systemPermissions), + object_permissions: '{}', + })); } return []; }, @@ -245,10 +281,11 @@ function mountBoth() { headers, getSession: async (h: any) => authService.api.getSession({ headers: h }), }); - // `systemPermissions` is carried through even though no route in this - // package reads it today: the federation spelling gates on authentication - // only (see the divergence case at the bottom of this file), and the day - // that changes, this resolver already supplies what such a gate would read. + // [#9901] `systemPermissions` is carried through because the federation + // spelling now READS it: its capability gate resolves the caller through + // this resolver exactly as production does. It was already supplied here + // while the federation spelling gated on authentication alone, which is why + // closing that gap needed no change on this line. return authz.userId ? { userId: authz.userId, systemPermissions: authz.systemPermissions } : undefined; @@ -385,29 +422,42 @@ describe('listRemoteTables twins agree on the request shape (#7955)', () => { * to one spelling and not the other now fails here, whichever side it is added * to — which is the property the equivalence is for. * - * ## [#9593] The axis is no longer a single line, and this block says so + * ## [#9593 → #9901] The axis diverged in the middle for one card's width * * #9593 raised the ADMIN spelling from "any authenticated caller" to - * `manage_platform_settings`; the federation spelling still gates on - * authentication alone, by its own registrar's stated decision (#9686 ruled - * the capability question out of its scope and pointed it here). So the - * spellings now agree at the two ends of the axis and diverge in the middle: + * `manage_platform_settings` while the federation spelling still gated on + * authentication alone (#9686 ruled the capability question out of its scope + * and pointed it at #9901). For that interval the two spellings agreed at the + * ends of the axis and disagreed in the middle, and the disagreement was pinned + * here as a record of a known gap — with the standing instruction that closing + * it should fold the row back into the agreement rather than delete the case. * - * - anonymous — both refuse `401 UNAUTHENTICATED` (unchanged); - * - unrecognised credential — both refuse `401 UNAUTHENTICATED` (unchanged); - * - authenticated AND entitled — both serve, identically (unchanged); - * - authenticated but UNENTITLED — the admin spelling refuses `403`, the - * federation spelling serves. + * #9901 closed it (maintainer ruling, 2026-08-20, verbatim: + * 「其他接受你的建议。」 — the federation family is NOT deliberately the + * lower-privilege door), and this is that fold-back. The axis is one line + * again: * - * The last row is a real governance asymmetry — one operation, two doors, one - * gate — and it is FILED, not accepted: #9901. It is pinned here rather than left unasserted for the reason this - * whole block exists: an axis nothing drives is an axis that goes silently - * false, which is exactly how the pre-#9686 gap survived. ⚠️ When the - * federation spelling grows its own capability gate, that case is EXPECTED to - * fail — it is a record of a known gap, not a defence of it, and the correct - * response is to fold the row back into the agreement above. + * - anonymous — both refuse `401 UNAUTHENTICATED`; + * - unrecognised credential — both refuse `401 UNAUTHENTICATED`; + * - authenticated but UNENTITLED — both refuse `403 PERMISSION_DENIED`; + * - authenticated AND entitled — both serve, identically. + * + * The middle row is asserted as an EQUIVALENCE now, exactly like the other + * three: both spellings' status and machine-readable code are compared to each + * other, not merely each checked against a literal. A capability gate added to + * one spelling and not the other — or added to both with different codes — + * fails here, whichever side it is added to, which is the property this file + * exists for. The row keeps its own case rather than being merged into the + * anonymous one: `401` and `403` are different facts about a caller, and a + * single "both refuse" case could be satisfied by either. + * + * ⚠️ Note which capability the twins are compared on: the READ capability. + * The federation family also carries three routes this file's operation has no + * twin for, two of which #9901 gates on `manage_metadata` instead — that split + * is pinned in `external-datasource-routes-auth-guard.test.ts`, since a + * spelling with no twin has no equivalence to assert. */ -describe('listRemoteTables twins agree on WHO may ask (#9686, #9593)', () => { +describe('listRemoteTables twins agree on WHO may ask (#9686, #9593, #9901)', () => { it('an anonymous caller is refused identically on both spellings', async () => { const { federation, admin } = await readBoth('', {}); @@ -443,32 +493,41 @@ describe('listRemoteTables twins agree on WHO may ask (#9686, #9593)', () => { expect(qualified(admin)).toEqual(qualified(federation)); }); - it('[#9593] an authenticated but UNENTITLED caller diverges: admin refuses 403, federation serves', async () => { - // ⚠️ A pinned RECORD OF A KNOWN GAP, not a contract worth keeping — see - // this block's header and the finding card #9901. The two - // halves are asserted separately and in full so that closing the gap - // fails this case loudly instead of drifting past it. + it('[#9901] an authenticated but UNENTITLED caller is refused identically on both spellings', async () => { + // The row #9593 could only record as a divergence, folded back into the + // agreement now that #9901 has closed it. It stays a case of its own + // because the middle of the axis is a distinct fact from its ends: this + // caller WAS read (unlike the anonymous one) and still holds nothing. const { federation, admin } = await readBoth('', { authorization: UNENTITLED_SESSION }); - // The admin spelling: the #9593 refusal, asserted by status AND - // machine-readable code (ADR-0112 envelope) — "not 200" would be satisfied - // by the 401 the anonymous case already covers, which would mean the - // credential was never read. - expect(admin.status).toBe(403); - expect(admin.code).toBe('PERMISSION_DENIED'); - // …and it refused before serving anything. + // Status AND machine-readable code (ADR-0112 envelope) — "not 200" would be + // satisfied by the 401 the anonymous case already covers, which would mean + // the credential was never read. + expect(federation.status).toBe(403); + expect(federation.code).toBe('PERMISSION_DENIED'); + // The equivalence itself: compared to the other spelling, not to a literal, + // so this fails whichever side drifts. + expect(admin.status).toBe(federation.status); + expect(admin.code).toBe(federation.code); + // …and neither leaked the listing it refused to serve. + expect(federation.tables).toEqual([]); expect(admin.tables).toEqual([]); + }); - // The federation spelling: authentication was the whole gate here, so the - // same caller is served. This is the asymmetry, stated rather than implied. - expect(federation.status).toBe(200); - expect(qualified(federation)).toEqual([ - 'analytics.events', - 'public.customers', - 'public.orders', - ]); - - // And it is genuinely a divergence — the point the equivalence axis makes. - expect(admin.status).not.toBe(federation.status); + it('[#9901] …and the refusal is keyed on the CAPABILITY, not on merely holding some grant', async () => { + // `u_twin_plain` holds no permission set at all, so the case above would + // also pass against a gate that asked "does this caller hold anything?". + // This one presents a caller who holds a real, non-empty grant that simply + // is not `manage_platform_settings` — the posture a deployment produces the + // moment it defines any operator set. Both spellings must still refuse, and + // still identically. + const { federation, admin } = await readBoth('', { authorization: OTHER_GRANT_SESSION }); + + expect(federation.status).toBe(403); + expect(federation.code).toBe('PERMISSION_DENIED'); + expect(admin.status).toBe(federation.status); + expect(admin.code).toBe(federation.code); + expect(federation.tables).toEqual([]); + expect(admin.tables).toEqual([]); }); });