diff --git a/.changeset/external-object-org-scope-exemption.md b/.changeset/external-object-org-scope-exemption.md new file mode 100644 index 0000000000..84169a812a --- /dev/null +++ b/.changeset/external-object-org-scope-exemption.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): stop injecting the org-scope predicate onto federated (external) objects (#7738) + +`ObjectQL.buildDriverOptions` folded the caller's `ExecutionContext.tenantId` +into `DriverOptions.tenantId` for **every** object, including ADR-0015 +federated ones. The SQL driver turns that into the platform's implicit tenant +wall — `(organization_id = :tenant OR organization_id IS NULL)` — so an +authenticated read of a correctly-bound external object issued + +```sql +select * from `customers` where (`organization_id` = ? or `organization_id` is null) +-- bindings=["org_msoroxgurm6423gz"] +``` + +against a remote `customers` table whose columns are `id, created_at, +updated_at, name, email, region, lifetime_value`. On Postgres/MySQL that is a +remote SQL error. On SQLite it is worse: the quoted-identifier fallback +reinterprets the unresolvable identifier as the string literal +`'organization_id'`, both disjuncts go constant-false, and the object answers +**0 rows with HTTP 200** — a declared, correctly-bound federated object +silently reads empty, with nothing in the response to say so. + +`tenantId` (and the `group`-posture `tenantIds` union) is now withheld for an +object with `external != null`, alongside the existing `tenancy.enabled: false` +exemption (ADR-0066 / #3249). Withholding it at the engine covers every driver +at the source rather than one driver's opt-out. + +**Why the exemption is unconditional**, rather than conditioned on whether the +object carries an `organization_id` column: that column is the platform's own. +`applySystemFields` (`resolveInjectedSystemColumns`) injects `organization_id` +into every object it registers and has no `external` branch, and +`SqlDriver.registerExternalObject` is DDL-free by design and runs no +introspection — it computes the tenant column from the platform's field set, +never from the remote's. On a federated object the column's presence is +therefore always the injection and never evidence about the remote schema, so +there is no shape in which scoping by it is known-correct. + +**What does not change.** An ordinary object still carries the wall for a +normal non-system caller, on every read door (`find`, `findOne`, `count`, +`aggregate`) and on the write-side stamp; the `group`-posture `tenantIds` union +is still threaded; and a `tenantId` a caller passes **by name** in the option +bag still wins under both exemptions — the exemption governs what the engine +folds in from the execution context, not what a caller asked for explicitly. +Tenant isolation for federated data remains the remote's and the layers above +(RBAC/RLS, the datasource binding). + +Note this is the org-scope half only. The boot-ordering defect tracked +separately by #7737 is untouched, and this fix does not depend on it. diff --git a/packages/objectql/src/engine-external-tenant-scope.test.ts b/packages/objectql/src/engine-external-tenant-scope.test.ts new file mode 100644 index 0000000000..a79819a0eb --- /dev/null +++ b/packages/objectql/src/engine-external-tenant-scope.test.ts @@ -0,0 +1,287 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ── The org-scope wall is withheld from FEDERATED objects, and ONLY from them (#7738) ── +// +// `buildDriverOptions` folds the caller's `ExecutionContext.tenantId` into +// `DriverOptions.tenantId` on every read. The SQL driver's `applyTenantScope` +// turns that into `(organization_id = :tenant OR organization_id IS NULL)` — +// the platform's implicit tenant wall. For an ADR-0015 federated object that +// predicate is issued against a table the platform does not own: +// +// select * from `customers` where (`organization_id` = ? or `organization_id` is null) +// -- bindings=["org_msoroxgurm6423gz"] +// +// against a remote `customers` whose columns are `id, created_at, updated_at, +// name, email, region, lifetime_value`. On Postgres/MySQL that is a remote SQL +// error; on SQLite the quoted-identifier fallback reinterprets the unresolvable +// identifier as the string literal `'organization_id'`, both disjuncts go +// constant-false, and a correctly-bound external object answers **0 rows, HTTP +// 200** (#7738, measured on the #7737 lane). +// +// ## Why the platform may not scope a federated object at all +// +// Not "because the showcase table happens to lack the column" — because the +// column it detects is **its own**. `applySystemFields` +// (`resolveInjectedSystemColumns`) injects `organization_id` into EVERY object +// it registers, external ones included: there is no `external` branch in that +// derivation. `SqlDriver.registerExternalObject` is DDL-free by design (ADR-0015 +// forbids DDL on a remote schema) and runs no `columnInfo` introspection — it +// computes the tenant column from the PLATFORM's field set. So on a federated +// object `organization_id`'s presence is always the platform's injection and +// never evidence about the remote schema, and scoping by it is a guess about a +// table the platform does not own. +// +// ## This file asserts BOTH directions, and the negative one is load-bearing +// +// Withholding the tenant wall is punching a hole in tenant isolation for one +// class of object. A test that proved only the permissive direction — "the +// external read is no longer scoped" — would stay green if the fix withheld +// `tenantId` from EVERY object, which is how a tenant leak ships green. So +// every case below runs `it.each(READ_DOORS)` over an external object AND an +// ordinary one, and the ordinary object must still carry the wall for a normal +// non-system caller. +// +// ## The seam under test is `DriverOptions`, not SQL +// +// `@objectstack/objectql` cannot import `@objectstack/driver-sql` (the +// dependency runs the other way), so this file pins the exact input the +// driver's wall keys off: `applyTenantScope` early-returns an unmodified +// builder when `options.tenantId` is `undefined | null | ''`, and +// `injectTenantOnInsert` does the same. No `tenantId` in DriverOptions is +// precisely "no `organization_id` predicate in the emitted SQL" — and it is +// withheld at the ENGINE rather than in one driver so every driver is covered +// at the source (the same reason `tenancy.enabled: false` is withheld here, +// #3249). + +import { describe, it, expect } from 'vitest'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { ObjectQL } from './engine.js'; + +/** A normal, non-system caller with an active org — the #7738 repro identity. */ +const MEMBER: ExecutionContext = { userId: 'u_member', tenantId: 'org_msoroxgurm6423gz' }; + +interface ObservedCall { + object: string; + method: string; + options: Record | undefined; +} + +function makeDriver(name: string, observed: ObservedCall[]) { + const record = (object: string, method: string, options: any) => { + observed.push({ object, method, options }); + }; + const driver: any = { + name, + version: '0.0.0', + // No `aggregate` capability: the engine falls back to `find` + in-memory + // aggregation, which is itself a read door built from buildDriverOptions. + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, _ast: any, options: any) { record(object, 'find', options); return []; }, + async findOne(object: string, _ast: any, options: any) { record(object, 'findOne', options); return null; }, + async count(object: string, _ast: any, options: any) { record(object, 'count', options); return 0; }, + async create(object: string, data: any, options: any) { record(object, 'create', options); return { id: 'r_1', ...data }; }, + async update(object: string, id: string, data: any, options: any) { record(object, 'update', options); return { id, ...data }; }, + async delete() { return true; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + // DDL-free federated registration — the ADR-0015 seam `syncObjectSchema` + // routes an `external != null` object to. Present so this engine takes the + // real external path rather than the managed one. + registerExternalObject() {}, + async syncSchema() {}, + }; + return driver; +} + +/** + * The federated object, declared as `examples/app-showcase` declares it: an + * `external.remoteName` binding, and NO `organization_id` field of its own — + * the one the registry will nevertheless inject. + */ +const EXTERNAL_OBJECT = { + name: 'showcase_ext_customer', + datasource: 'showcase_external', + external: { remoteName: 'customers' }, + fields: { + name: { type: 'text' }, + email: { type: 'text' }, + region: { type: 'text' }, + }, +} as any; + +/** The owning package every object below is registered under (`registerObject` arg 2). */ +const PACKAGE_ID = 'com.example.showcase'; + +/** An ordinary managed object. Its wall must not move. */ +const MANAGED_OBJECT = { + name: 'showcase_account', + fields: { + name: { type: 'text' }, + region: { type: 'text' }, + }, +} as any; + +async function makeEngine(opts: { posture?: string } = {}) { + const observed: ObservedCall[] = []; + const engine = new ObjectQL(); + // Two drivers, as the showcase has: the platform's default, and the remote + // the federated object is bound to by `datasource: 'showcase_external'`. Both + // record into one log, so a read landing on the wrong one is still observed. + engine.registerDriver(makeDriver('memory', observed), true); + engine.registerDriver(makeDriver('showcase_external', observed)); + await engine.init(); + engine.registry.registerObject(EXTERNAL_OBJECT, PACKAGE_ID); + engine.registry.registerObject(MANAGED_OBJECT, PACKAGE_ID); + if (opts.posture) engine.setTenancyPostureProvider(() => opts.posture); + return { engine, observed }; +} + +/** + * The premise every assertion below rests on: the registry injects + * `organization_id` into the FEDERATED object too. If this ever stops being + * true the defect changes shape (the driver's implicit detection would no + * longer fire) and the rest of this file would be pinning a fix for a + * mechanism that no longer exists — so it is asserted, not assumed. + */ +describe('#7738 premise — the platform injects its tenant column into a federated object', () => { + it('registers `organization_id` on an external object that declares no such field', async () => { + const { engine } = await makeEngine(); + const stored = engine.registry.getObject('showcase_ext_customer') as any; + expect(EXTERNAL_OBJECT.fields.organization_id).toBeUndefined(); + expect(stored.fields.organization_id).toBeDefined(); + expect(stored.external).toEqual({ remoteName: 'customers' }); + }); +}); + +/** + * Every read door that reaches the driver through `buildDriverOptions`, and the + * driver method each one lands on. `aggregate` is included and lands on `find`: + * this driver advertises no native aggregation, so the engine takes its + * in-memory fallback — which still builds DriverOptions and still sends a read + * to the remote. + */ +const READ_DOORS = [ + { name: 'find', driverMethod: 'find', run: (e: ObjectQL, o: string, ctx: ExecutionContext) => e.find(o, {}, { context: ctx }) }, + // `findOne` refuses a predicate-free query (#4419), so it carries one. The + // caller's own `where` is orthogonal to the wall this file measures. + { name: 'findOne', driverMethod: 'findOne', run: (e: ObjectQL, o: string, ctx: ExecutionContext) => e.findOne(o, { where: { region: 'EU' } }, { context: ctx }) }, + { name: 'count', driverMethod: 'count', run: (e: ObjectQL, o: string, ctx: ExecutionContext) => e.count(o, {}, { context: ctx }) }, + { name: 'aggregate', driverMethod: 'find', run: (e: ObjectQL, o: string, ctx: ExecutionContext) => e.aggregate(o, { aggregations: [{ function: 'count', field: 'id', alias: 'n' }] }, { context: ctx }) }, +] as const; + +describe('#7738 — a federated read carries NO org-scope predicate', () => { + it.each(READ_DOORS)( + '$name: DriverOptions for an external object omit tenantId', + async ({ driverMethod, run }) => { + const { engine, observed } = await makeEngine(); + await run(engine, 'showcase_ext_customer', MEMBER); + + const call = observed.find((c) => c.object === 'showcase_ext_customer' && c.method === driverMethod); + expect(call, `driver.${driverMethod} was never reached`).toBeDefined(); + // `applyTenantScope` early-returns on undefined/null/'' — any of the + // three means no `organization_id` predicate reaches the remote. + expect(call!.options?.tenantId ?? undefined).toBeUndefined(); + // The `group`-posture union (`IN (...) OR IS NULL`) is the SAME predicate + // on the same absent column, so it must not survive either. + expect(call!.options?.tenantIds ?? undefined).toBeUndefined(); + }, + ); + + it('withholds the tenantIds union too under the `group` posture', async () => { + const { engine, observed } = await makeEngine({ posture: 'group' }); + await engine.find( + 'showcase_ext_customer', + {}, + { context: { ...MEMBER, accessible_org_ids: ['org_a', 'org_b'] } }, + ); + const call = observed.find((c) => c.object === 'showcase_ext_customer' && c.method === 'find'); + expect(call!.options?.tenantId ?? undefined).toBeUndefined(); + expect(call!.options?.tenantIds ?? undefined).toBeUndefined(); + }); +}); + +// ── The load-bearing half ──────────────────────────────────────────────────── +// +// If the fix over-reaches, THIS is what goes red — not the block above. A +// change to an implicit tenant wall that only tests the permissive direction is +// how a leak ships green, so these cases are the reason this file exists. +describe('#7738 non-regression — an ORDINARY object is still org-scoped', () => { + it.each(READ_DOORS)( + '$name: DriverOptions for a managed object still carry tenantId for a non-system caller', + async ({ driverMethod, run }) => { + const { engine, observed } = await makeEngine(); + await run(engine, 'showcase_account', MEMBER); + + const call = observed.find((c) => c.object === 'showcase_account' && c.method === driverMethod); + expect(call, `driver.${driverMethod} was never reached`).toBeDefined(); + expect(call!.options?.tenantId).toBe('org_msoroxgurm6423gz'); + }, + ); + + it('still threads the `group`-posture tenantIds union for a managed object', async () => { + const { engine, observed } = await makeEngine({ posture: 'group' }); + await engine.find( + 'showcase_account', + {}, + { context: { ...MEMBER, accessible_org_ids: ['org_a', 'org_b'] } }, + ); + const call = observed.find((c) => c.object === 'showcase_account' && c.method === 'find'); + expect(call!.options?.tenantId).toBe('org_msoroxgurm6423gz'); + expect(call!.options?.tenantIds).toEqual(['org_a', 'org_b']); + }); + + it('still stamps the tenant column on an ordinary WRITE', async () => { + // The write half of the same wall (`injectTenantOnInsert`) reads the same + // `DriverOptions.tenantId`. The read-path exemption must not reach it. + const { engine, observed } = await makeEngine(); + await engine.insert('showcase_account', { name: 'A-1' }, { context: MEMBER }); + const call = observed.find((c) => c.object === 'showcase_account' && c.method === 'create'); + expect(call!.options?.tenantId).toBe('org_msoroxgurm6423gz'); + }); + + it('leaves the pre-existing `tenancy.enabled: false` exemption exactly as it was', async () => { + // ADR-0066 / #3249. A second, older reason to withhold the wall — asserted + // here so the federation exemption is proved to be an ADDITION to it and + // not a rewrite of it. + const { engine, observed } = await makeEngine(); + engine.registry.registerObject({ + name: 'sys_license_probe', + tenancy: { enabled: false }, + fields: { name: { type: 'text' } }, + } as any, PACKAGE_ID); + await engine.find('sys_license_probe', {}, { context: MEMBER }); + const call = observed.find((c) => c.object === 'sys_license_probe' && c.method === 'find'); + expect(call!.options?.tenantId ?? undefined).toBeUndefined(); + }); +}); + +describe('#7738 — an explicitly-passed tenantId is still deliberate caller intent', () => { + it('does not strip a tenantId the caller passed by name', async () => { + // On `find`/`findOne`/`update`/`delete` the option bag IS the base of the + // driver options (`ENGINE_DRIVER_PASSTHROUGH_KEYS`, #4371), and + // `buildDriverOptions` documents that an explicit `base.tenantId` wins. + // + // The federation exemption governs what the engine FOLDS IN from the + // execution context; it is not a scrubber for what a caller asked for by + // name — a caller who names the column has asserted something about the + // remote that the engine has no standing to contradict. This is also + // exactly how the older `tenancy.enabled: false` exemption behaves, so the + // two stay one shape rather than two. + // + // `tenantId` is a RUNTIME passthrough key, not a declared one: + // `EngineQueryOptionsSchema` does not carry it, so the input is + // deliberately off-contract at the type level and says so with + // `as unknown as` rather than erasing the bag to `any` — that names the + // contract being bypassed and leaves the rest of the call checked (#4918). + const { engine, observed } = await makeEngine(); + await engine.find( + 'showcase_ext_customer', + { tenantId: 'org_explicit' } as unknown as EngineQueryOptions, + { context: MEMBER }, + ); + const call = observed.find((c) => c.object === 'showcase_ext_customer' && c.method === 'find'); + expect(call!.options?.tenantId ?? undefined).toBe('org_explicit'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 9f5481f9b2..d70eeb41d9 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2491,17 +2491,41 @@ export class ObjectQL implements IObjectQLEngine { * * Carries `tenantId` from the active ExecutionContext so the driver can * enforce per-tenant isolation (SQL driver auto-scopes reads and - * auto-injects the tenant column on writes) — EXCEPT for objects that - * declare `tenancy.enabled: false` (ADR-0066 platform-global posture, - * e.g. `sys_license`): stamping the caller's active-org tenantId there - * would org-scope a global catalog at the driver, and its NULL-org rows - * would vanish for authenticated org-context reads while anonymous - * reads still see them (#3249). The SQL driver has its own opt-out - * (sticky tenant-field cache), but withholding tenantId here protects - * every driver at the source. Existing user-supplied shapes - * (transactions, AST extras) are preserved by spreading them first — an - * explicitly-passed `base.tenantId` is deliberate caller intent and - * still wins. + * auto-injects the tenant column on writes) — EXCEPT for the two object + * postures below. The SQL driver has its own opt-out (sticky tenant-field + * cache), but withholding tenantId here protects every driver at the + * source. Existing user-supplied shapes (transactions, AST extras) are + * preserved by spreading them first — an explicitly-passed `base.tenantId` + * is deliberate caller intent and still wins, under both exemptions. + * + * 1. **`tenancy.enabled: false`** (ADR-0066 platform-global posture, e.g. + * `sys_license`): stamping the caller's active-org tenantId there would + * org-scope a global catalog at the driver, and its NULL-org rows would + * vanish for authenticated org-context reads while anonymous reads still + * see them (#3249). + * + * 2. **`external != null`** — a federated object (ADR-0015), whose schema + * is owned by the REMOTE database (#7738). The driver turns `tenantId` + * into `(organization_id = :tenant OR organization_id IS NULL)`, and + * against a remote table that carries no such column that is a SQL error + * on Postgres/MySQL — or worse on SQLite, whose quoted-identifier + * fallback reinterprets the unresolvable identifier as the string + * literal `'organization_id'`, makes both disjuncts constant-false, and + * answers **0 rows with HTTP 200**: a correctly-bound external object + * silently reads empty. + * + * Note the reason is NOT "the remote happens to lack the column". The + * column the driver detects is the platform's OWN: `applySystemFields` + * (`resolveInjectedSystemColumns`) injects `organization_id` into every + * object it registers, with no `external` branch, and + * `SqlDriver.registerExternalObject` is DDL-free by design and runs no + * introspection — so it computes the tenant column from the platform's + * field set, never from the remote's. On a federated object + * `organization_id`'s presence is therefore always the injection and + * never evidence about the remote, which leaves the engine no ground on + * which to scope by it. Tenant isolation for federated data belongs to + * the remote and to the layers above (RBAC/RLS, the datasource binding), + * not to a predicate the platform guesses onto someone else's table. * * System / isSystem callers may still cross tenants by clearing * `tenantId` themselves on the resulting object; this helper does not @@ -2525,9 +2549,15 @@ export class ObjectQL implements IObjectQLEngine { // reaches the driver that owns it. It covers READS too, which have no gate // of their own and were riding the same wrong connection. const hasTx = tx !== undefined && this.transactionCoversDriverFor(object, tx); + const objectSchema = this._registry.getObject(object) as any; + // `external != null` is the same predicate `syncObjectSchema` routes a + // federated object by — one spelling of "this schema is the remote's", + // not a second reading of it. + const isFederated = objectSchema?.external != null; const hasTenant = execCtx?.tenantId !== undefined && - !isTenancyDisabled(this._registry.getObject(object)); + !isTenancyDisabled(objectSchema) && + !isFederated; const hasTz = execCtx?.timezone !== undefined; const isSystem = execCtx?.isSystem === true; const preserveAudit = (execCtx as any)?.preserveAudit === true;