diff --git a/.changeset/21908-by-id-producers-opt-in.md b/.changeset/21908-by-id-producers-opt-in.md new file mode 100644 index 00000000000..b1d6f8b1cae --- /dev/null +++ b/.changeset/21908-by-id-producers-opt-in.md @@ -0,0 +1,13 @@ +--- +"@objectstack/service-storage": patch +"@objectstack/service-messaging": patch +--- + +The storage store's by-id methods and the HTTP outbox's `redeliver` now pass the explicit system opt-in (`{ isSystem: true }`) on their data-engine calls. Until now they reached the engine with no principal and no opt-in, and the security middleware let that through only because of its principal-less hand-off. + +Clause-②: no + +- **service-storage.** `StorageMetadataStore.getFile`, `updateFile`, `deleteFile`, `getSession`, `updateSession` and `deleteSession` take the opt-in inside the store. Access stays by id, and the reads stay unscoped by organization, as before. On update and delete the acting organization still reaches the driver beside the opt-in, so a row stamped for another organization is still out of reach of these doors. The doors keep the authorization they already ran. +- **service-storage, the update payload.** `updateFile` and `updateSession` now send the caller's patch alone, where they used to send the whole row read back merged with it. The engine's read-only strip, which does not run for a system write, used to take `organization_id` and the four audit columns out of that row; now the store never sends them. The stored row is the same as before, and a column another writer changed between the read and the write is no longer reverted by it. +- **service-messaging.** `SqlHttpOutbox.redeliver` takes the opt-in on both of its reads and on its reset write. The caller's `tenantId` stays on every call as the driver-level scope, so a delivery in another organization is still not found. The reset write states `bypassTenantAudit: false`, so a redelivery from a caller with no organization is still reported by the driver's tenant audit. +- None of the gates the security middleware runs before its hand-off applies to these calls. ⛔ No new export on either package entry, and no new elevation API. diff --git a/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts b/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts index 94feb0323eb..ed38c078b0f 100644 --- a/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts +++ b/packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts @@ -70,7 +70,7 @@ */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { ObjectQL } from '@objectstack/objectql'; +import { ObjectQL, assertEngineFindOnePredicate, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { SqlHttpOutbox } from './sql-http-outbox.js'; import { SqlNotificationOutbox, DELIVERY_OBJECT } from './sql-outbox.js'; @@ -325,8 +325,11 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( expect(writes).toHaveLength(1); expect(writes[0].where).toMatchObject({ id: 'h_a', status: { $in: ['success', 'failed', 'dead'] } }); // ⛔ The forbidden implementation, named: a bypass here would silence - // the audit for an authenticated user's unscoped write. - expect(writes[0].options?.bypassTenantAudit).toBeUndefined(); + // the audit for an authenticated user's unscoped write. [#21908] The + // write now carries the explicit system opt-in, for which the engine + // fills in a bypass on this object unless one is stated — so the + // outbox states `false`, and that is what must reach the driver. + expect(writes[0].options?.bypassTenantAudit).toBe(false); // …and the remedy that replaces it, present. expect(writes[0].options?.tenantId).toBe('org_a'); // The line is absent BECAUSE the write is scoped — the two assertions @@ -373,6 +376,47 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( expect(rows.map((r) => `${r.id}:${r.status}`).sort()).toEqual(['h_a:pending', 'h_b:dead']); }); + it('[#21908] ⛔ under the explicit system opt-in the threaded tenant still walls a foreign row', async () => { + // The opt-in replaces the security middleware's principal-less + // hand-off on these calls; it must not replace the tenant scope. Read + // through a wrapper that records the context each engine call carried, + // so this is a reading of the opt-in path and of no other. + await seedDeadRow('h_a', 'org_a'); + await seedDeadRow('h_b', 'org_b'); + const contexts: Array<{ verb: string; context: unknown; tenantId: unknown }> = []; + const recorded: any = { + findOne: (o: string, q: any, opts: any) => { + assertEngineFindOnePredicate(o, q); + contexts.push({ verb: 'findOne', context: opts?.context, tenantId: q?.tenantId }); + return engine.findOne(o, q, opts); + }, + update: (o: string, d: any, opts: any) => { + assertEngineUpdateDispatch(d, opts); + contexts.push({ verb: 'update', context: opts?.context, tenantId: opts?.tenantId }); + return engine.update(o, d, opts); + }, + }; + const outbox = new SqlHttpOutbox(recorded, { partitionCount: 1 }); + + await expect(outbox.redeliver('h_b', { tenantId: 'org_a' })).rejects.toMatchObject({ + name: 'HttpRedeliverError', + code: 'RESOURCE_NOT_FOUND', + }); + expect(contexts).toEqual([{ verb: 'findOne', context: { isSystem: true }, tenantId: 'org_a' }]); + expect(driverUpdateManys.filter((u) => u.object === SYS_HTTP_DELIVERY)).toHaveLength(0); + + // The still-works half, on the same path: the caller's own row resets. + const replayed = await outbox.redeliver('h_a', { tenantId: 'org_a' }); + expect(`${replayed.id}:${replayed.status}:${replayed.attempts}`).toBe('h_a:pending:0'); + expect(contexts.slice(1)).toEqual([ + { verb: 'findOne', context: { isSystem: true }, tenantId: 'org_a' }, + { verb: 'update', context: { isSystem: true }, tenantId: 'org_a' }, + { verb: 'findOne', context: { isSystem: true }, tenantId: 'org_a' }, + ]); + const rows = (await engine.find(SYS_HTTP_DELIVERY, { where: {} })) as any[]; + expect(rows.map((r) => `${r.id}:${r.status}:${r.attempts}`).sort()).toEqual(['h_a:pending:0', 'h_b:dead:3']); + }); + it('a tenant-less caller is NOT silenced — the audit still reports the gap', async () => { // The honest half of `tenantId: string | undefined`. Passing // `undefined` leaves the write unscoped, and the finding is REPORTED @@ -389,7 +433,8 @@ describe('redeliver — the request-reachable site is SCOPED, never bypassed', ( // have consumed this op's one warning. const writes = driverUpdateManys.filter((u) => u.object === SYS_HTTP_DELIVERY); expect(writes).toHaveLength(1); - expect(writes[0].options?.bypassTenantAudit).toBeUndefined(); + // [#21908] Stated `false` under the opt-in — see the first test. + expect(writes[0].options?.bypassTenantAudit).toBe(false); expect(auditedUpdateMany(SYS_HTTP_DELIVERY)).toBe(true); }); }); diff --git a/packages/services/service-messaging/src/messaging-service.ts b/packages/services/service-messaging/src/messaging-service.ts index 448159c99ce..fd81bebe283 100644 --- a/packages/services/service-messaging/src/messaging-service.ts +++ b/packages/services/service-messaging/src/messaging-service.ts @@ -358,8 +358,10 @@ export class MessagingService { * authenticated user, and `sys_http_delivery` is tenant-scoped. The * outbox applies it to the rows it reads and the row it writes, so a * caller can only replay deliveries in its own organization; a row - * elsewhere is `RESOURCE_NOT_FOUND`. ⛔ There is no `bypassTenantAudit` + * elsewhere is `RESOURCE_NOT_FOUND`. ⛔ There is no tenant-audit bypass * anywhere on this path and there must not be — see `RedeliverOptions`. + * [#21908] The outbox's write states `bypassTenantAudit: false` outright, + * so the explicit system opt-in it now carries cannot fill one in either. */ async redeliverHttp(id: string, options: RedeliverOptions): Promise { if (!this.httpOutbox) { diff --git a/packages/services/service-messaging/src/outbox-dispatcher-scope.ts b/packages/services/service-messaging/src/outbox-dispatcher-scope.ts index 19acc8a5943..53764aa1c6d 100644 --- a/packages/services/service-messaging/src/outbox-dispatcher-scope.ts +++ b/packages/services/service-messaging/src/outbox-dispatcher-scope.ts @@ -95,7 +95,9 @@ export function dispatcherSweepOptions( * queue. * * ⛔ Never on `redeliver`: it is request-reachable and threads the caller's - * tenant, the line this file draws for `bypassTenantAudit` too. + * tenant, the line this file draws for `bypassTenantAudit` too. [#21908] It + * takes its own opt-in, {@link REDELIVER_SYSTEM_CONTEXT}, on a different + * warrant — the door's — and keeps that tenant beside it. */ export const DISPATCHER_SYSTEM_CONTEXT = { isSystem: true } as const; @@ -117,10 +119,44 @@ export const DISPATCHER_SYSTEM_CONTEXT = { isSystem: true } as const; * * ⛔ Never on `redeliver`: it is the one request-reachable call on these * objects, and it threads the caller's tenant (see {@link DISPATCHER_SYSTEM_CONTEXT}). + * [#21908] Its opt-in is {@link REDELIVER_SYSTEM_CONTEXT}, whose warrant is not + * this one: a caller exists there, and the door authorizes it. */ export const OUTBOX_SYSTEM_CONTEXT = { isSystem: true } as const; +/** + * [#21908] The execution context `SqlHttpOutbox.redeliver` runs under — its + * two reads and its reset write: the explicit system opt-in. + * + * The one request-reachable call on `sys_http_delivery` used to be the one + * outbox call kept OFF the opt-ins above, because it threads the caller's + * tenant and their warrants rest on there being no caller. Both still hold. + * What changed is that the security middleware's principal-less hand-off, + * which this call passed through with no principal and no opt-in, closes + * (ADR-0096 D5), and the caller's own principal is no replacement: no member + * grant exists on `sys_http_delivery`, so every member's redelivery would be + * refused. The maintainer's ruling on this card puts the explicit opt-in on it + * inside this service, on a warrant of its own: + * + * 1. **The door authorizes.** `POST /api/v1/webhooks/redeliver` admits an + * authenticated session only (else `401`), and the producer's veto + * (`RedeliverGuard`) runs before the write. + * 2. **The tenant stays the scope.** The caller's `tenantId` rides every + * call's options bag beside this context, as the driver-level scope it + * always was, so a row in another organization is still not found. + * 3. **The audit stays armed.** The reset write states + * `bypassTenantAudit: false` itself, because the engine fills in `true` for + * an `isSystem` write on an object outside the tenant-audit inventory — + * which would silence the line `RedeliverOptions` keeps for a caller with + * no tenant. + * + * ⛔ Never borrowed by the dispatcher or producer sites, and never the other + * way round: the three contexts are equal in value and differ in warrant. + */ +export const REDELIVER_SYSTEM_CONTEXT = { isSystem: true } as const; + + /** * The write options for a dispatcher **`ack`** — the single-record * (`multi: false`) write that records one delivery attempt's outcome on diff --git a/packages/services/service-messaging/src/sql-http-outbox.ts b/packages/services/service-messaging/src/sql-http-outbox.ts index 8fb4f6c6755..7fcc7954d33 100644 --- a/packages/services/service-messaging/src/sql-http-outbox.ts +++ b/packages/services/service-messaging/src/sql-http-outbox.ts @@ -2,11 +2,13 @@ import { randomUUID } from 'node:crypto'; import type { IDataEngine } from '@objectstack/spec/contracts'; +import type { EngineUpdateOptions } from '@objectstack/spec/data'; import { hashPartition } from './backoff.js'; import { toEpochMs } from './audit-timestamp.js'; import { DISPATCHER_SYSTEM_CONTEXT, OUTBOX_SYSTEM_CONTEXT, + REDELIVER_SYSTEM_CONTEXT, dispatcherAckCasOptions, dispatcherAckOptions, dispatcherSweepOptions, @@ -464,6 +466,26 @@ export class SqlHttpOutbox implements IHttpOutbox { * so the endpoint neither replays it nor confirms it exists. It is also * what keeps the guard honest — the producer veto never sees a row the * caller may not read. + * + * [#21908] Every engine call here also carries the explicit system opt-in + * ({@link REDELIVER_SYSTEM_CONTEXT}). Until now they reached the data engine + * with no principal and no opt-in and passed the security middleware only + * through its principal-less hand-off (ADR-0096 E1), which D5 closes; the + * caller's own principal is no answer, because no member grant exists on + * `sys_http_delivery`. The door keeps its authorization (an authenticated + * session, else `401`) and the producer's veto still runs before the write. + * Two things the opt-in would otherwise change are held where they were: + * + * - the REACH. `tenantId` stays on the bag of every call below, the + * driver-level scope it always was. The opt-in replaces the hand-off, + * not this scope, so a row in another organization is still not found. + * - the AUDIT. For an `isSystem` write the engine fills in + * `bypassTenantAudit: true` on an object outside the tenant-audit + * inventory, which `sys_http_delivery` is. That would silence exactly + * the line {@link RedeliverOptions} keeps for a caller with no tenant, so + * the write states `bypassTenantAudit: false` itself: an explicit value + * is never overwritten by that fill-in, and the driver still audits an + * unscoped redeliver. */ async redeliver(id: string, options: RedeliverOptions): Promise { // One options bag for every engine call below, so the read that decides @@ -473,7 +495,7 @@ export class SqlHttpOutbox implements IHttpOutbox { const current = (await this.engine.findOne(this.objectName, { where: { id }, ...scope, - })) as DeliveryRow | null; + }, { context: REDELIVER_SYSTEM_CONTEXT })) as DeliveryRow | null; if (!current) { throw new HttpRedeliverError(`Delivery row '${id}' not found`, 'RESOURCE_NOT_FOUND'); } @@ -493,6 +515,16 @@ export class SqlHttpOutbox implements IHttpOutbox { // included, so the reset lands only if the row is still terminal. // A miss writes 0 rows and the read-back below reports the refusal // (`DELIVERY_NOT_ELIGIBLE`) instead of a false success. + // [#21908] Typed with the two driver pass-through keys it carries: the + // caller's tenant as the statement's scope, and the tenant audit held + // armed explicitly — see the method docs. + const resetOptions: EngineUpdateOptions & { tenantId: string | undefined; bypassTenantAudit: false } = { + where: { id, status: { $in: ['success', 'failed', 'dead'] } }, + multi: true, + ...scope, + bypassTenantAudit: false, + context: REDELIVER_SYSTEM_CONTEXT, + }; await this.engine.update( this.objectName, { @@ -506,12 +538,12 @@ export class SqlHttpOutbox implements IHttpOutbox { response_body: null, error: null, }, - { where: { id, status: { $in: ['success', 'failed', 'dead'] } }, multi: true, ...scope }, + resetOptions, ); const after = (await this.engine.findOne(this.objectName, { where: { id }, ...scope, - })) as DeliveryRow | null; + }, { context: REDELIVER_SYSTEM_CONTEXT })) as DeliveryRow | null; if (!after || after.status !== 'pending') { throw new HttpRedeliverError(`Delivery row '${id}' state changed during redeliver`, 'DELIVERY_NOT_ELIGIBLE'); } diff --git a/packages/services/service-messaging/src/system-context.pin.test.ts b/packages/services/service-messaging/src/system-context.pin.test.ts index 8ce5d4654fd..c05b26410e0 100644 --- a/packages/services/service-messaging/src/system-context.pin.test.ts +++ b/packages/services/service-messaging/src/system-context.pin.test.ts @@ -420,3 +420,56 @@ describe('[#21908] rows 24 onward — the remaining fan-out and outbox calls car expectAllSystem(calls); }); }); + +describe('[#21908] SqlHttpOutbox.redeliver carries the explicit system opt-in beside its threaded tenant', () => { + /** A terminal, genuinely-attempted row on the first read; `pending` on the read-back. */ + function redeliverEngine() { + let reads = 0; + return recordingEngine((verb) => { + if (verb !== 'findOne') return []; + reads += 1; + return { + id: 'h1', source: 'webhook', ref_id: 'wh1', dedup_key: 'k1', url: 'https://example.test/hook', + payload_json: '{}', signature: 'sig', organization_id: 'org_a', partition_key: 0, + status: reads === 1 ? 'dead' : 'pending', attempts: reads === 1 ? 2 : 0, + created_at: NOW, updated_at: NOW, + }; + }); + } + + it('both reads and the reset write: isSystem, the caller’s tenantId on the bag, and the audit stated armed', async () => { + const { engine, calls } = redeliverEngine(); + const outbox = new SqlHttpOutbox(engine, { partitionCount: 1 }); + + const row = await outbox.redeliver('h1', { tenantId: 'org_a' }); + expect(row.status).toBe('pending'); + + expect(calls.map((c) => c.verb)).toEqual(['findOne', 'update', 'findOne']); + expect(calls.every((c) => c.object === 'sys_http_delivery')).toBe(true); + expectAllSystem(calls); + // The scope stays the driver-level tenant on every call, never moved + // into the context and never dropped. + expect(calls[0].query).toMatchObject({ where: { id: 'h1' }, tenantId: 'org_a' }); + expect(calls[2].query).toMatchObject({ where: { id: 'h1' }, tenantId: 'org_a' }); + expect(calls[1].options).toMatchObject({ + where: { id: 'h1', status: { $in: ['success', 'failed', 'dead'] } }, + multi: true, + tenantId: 'org_a', + bypassTenantAudit: false, + }); + }); + + it('⛔ a caller with no tenant gets no tenant invented, and still no audit bypass', async () => { + const { engine, calls } = redeliverEngine(); + const outbox = new SqlHttpOutbox(engine, { partitionCount: 1 }); + + await outbox.redeliver('h1', { tenantId: undefined }); + + expectAllSystem(calls); + for (const c of calls) { + const bag = c.verb === 'update' ? c.options : c.query; + expect(bag).toHaveProperty('tenantId', undefined); + } + expect(calls[1].options.bypassTenantAudit).toBe(false); + }); +}); diff --git a/packages/services/service-storage/src/metadata-store.ts b/packages/services/service-storage/src/metadata-store.ts index 382b09b2612..ff99779bb75 100644 --- a/packages/services/service-storage/src/metadata-store.ts +++ b/packages/services/service-storage/src/metadata-store.ts @@ -159,6 +159,73 @@ function systemInsertOptionsFor( return { context: { ...(tenant ?? {}), isSystem: true } }; } +/** + * [#21908] The engine options the four by-id WRITE doors of this store run + * under — {@link StorageMetadataStore.updateFile}, + * {@link StorageMetadataStore.deleteFile}, + * {@link StorageMetadataStore.updateSession} and + * {@link StorageMetadataStore.deleteSession}: the same explicit system opt-in + * the inserts take ({@link systemInsertOptionsFor}), taken inside this store, + * with the acting organization's `tenantId` beside it when there is one. + * + * The posture is the inserts', and so is the reason: each of these reached the + * data engine with a tenant-only context (or none), no principal and no + * opt-in, and passed the security middleware only through its principal-less + * hand-off (ADR-0096 E1), which D5 closes. Carrying the caller's principal is + * not open — no member grant exists on `sys_file` / `sys_upload_session` — and + * the doors keep the authorization they already run (session authentication, + * the download door's `authorizeDownload`, the chunk door's resume token). + * + * What the `tenantId` buys differs by verb ({@link StorageWriteContext}): here + * it is the REACH, not a stamp. The driver's `applyTenantScope` still composes + * `(organization_id = :tenantId OR organization_id IS NULL)` into the + * statement, so a row stamped for another organization stays out of reach of + * these doors exactly as before. ⛔ Never drop the `tenantId` here: under the + * opt-in the security middleware composes no tenant wall of its own, so the + * driver-level scope is the only one these writes have. + */ +const systemByIdWriteOptionsFor = systemInsertOptionsFor; + +/** + * [#21908] The engine options the two by-id READS of this store run under — + * {@link StorageMetadataStore.getFile} and + * {@link StorageMetadataStore.getSession}: the explicit system opt-in and + * nothing else. + * + * The read's reach is unchanged by it. Before, the read carried no context at + * all: no `tenantId` reached the driver, and the security middleware handed it + * through before any row or tenant filter (the principal-less hand-off, + * ADR-0096 E1). Under the opt-in the middleware short-circuits before the same + * filters, and still no `tenantId` reaches the driver. Access is by id, and the + * door decides what the caller is shown: the download doors run + * `authorizeDownload` on the row this read returns before they disclose + * anything, and the chunk door checks the row's resume token before it writes. + * ⛔ No `tenantId` is added here: scoping these reads would change which rows + * the doors find, and that is a door decision, not this store's. + */ +const SYSTEM_BY_ID_READ_OPTIONS = { context: { isSystem: true } } as const; + +/** + * [#21908] The columns a by-id UPDATE sends to the engine: the caller's patch, + * minus the address. + * + * Until the opt-in, `updateFile` / `updateSession` sent the whole read-back + * row merged with the patch, and the engine's update-side `readonly` strip took + * the platform-provisioned columns out of it — `organization_id` and the four + * audit columns (`created_at`, `updated_at`, `created_by`, `updated_by`). That + * strip does not run for an `isSystem` write, so the full row would now be + * written back, `organization_id` included, from a read that is not + * tenant-scoped. The store sends only what it means to change instead: the + * stored row ends up as it did before, those five columns stay the platform's + * ({@link FileRecord.organization_id} — the store never puts the tenant column + * in the engine payload), and a column another writer changed between the read + * and this write is no longer reverted by it. + */ +function changedColumns(patch: Partial): Partial { + const { id: _address, ...changes } = patch; + return changes as Partial; +} + /** * Persisted upload-session record (matches `sys_upload_session` object schema). */ @@ -372,8 +439,10 @@ export class StorageMetadataStore { async getFile(id: string): Promise { if (!this.engine) return this.files.get(id) ?? null; + // [#21908] The explicit system opt-in; access stays by id — see + // SYSTEM_BY_ID_READ_OPTIONS. const found = await this.engineOp('sys_file', 'findOne', FILE_READ_CONSEQUENCE, (engine) => - engine.findOne('sys_file', { where: { id } }), + engine.findOne('sys_file', { where: { id } }, SYSTEM_BY_ID_READ_OPTIONS), ); return (found as FileRecord | null | undefined) ?? null; } @@ -416,9 +485,14 @@ export class StorageMetadataStore { this.files.set(id, merged); return merged; } - const options = writeOptionsFor(context); + // [#21908] The explicit system opt-in with the organization kept beside it + // as the statement's scope (systemByIdWriteOptionsFor), carrying the patch + // alone (changedColumns). await this.engineOp('sys_file', 'update', FILE_UPDATE_CONSEQUENCE, (engine) => - engine.update('sys_file', merged as any, { where: { id }, ...options } as any), + engine.update('sys_file', changedColumns(patch), { + where: { id }, + ...systemByIdWriteOptionsFor(context), + }), ); return merged; } @@ -437,9 +511,10 @@ export class StorageMetadataStore { this.files.delete(id); return; } - const options = writeOptionsFor(context); + // [#21908] The explicit system opt-in, the organization kept beside it as + // the statement's scope — see systemByIdWriteOptionsFor. await this.engineOp('sys_file', 'delete', FILE_DELETE_CONSEQUENCE, (engine) => - engine.delete('sys_file', { where: { id }, ...options } as any), + engine.delete('sys_file', { where: { id }, ...systemByIdWriteOptionsFor(context) }), ); } @@ -505,7 +580,9 @@ export class StorageMetadataStore { 'sys_upload_session', 'findOne', SESSION_READ_CONSEQUENCE, - (engine) => engine.findOne('sys_upload_session', { where: { id } }), + // [#21908] The explicit system opt-in; access stays by id — see + // SYSTEM_BY_ID_READ_OPTIONS. + (engine) => engine.findOne('sys_upload_session', { where: { id } }, SYSTEM_BY_ID_READ_OPTIONS), ); return (found as UploadSessionRecord | null | undefined) ?? null; } @@ -545,9 +622,12 @@ export class StorageMetadataStore { this.sessions.set(id, merged); return merged; } - const options = writeOptionsFor(context); + // [#21908] Same opt-in, same scope and same payload as updateFile. await this.engineOp('sys_upload_session', 'update', SESSION_UPDATE_CONSEQUENCE, (engine) => - engine.update('sys_upload_session', merged as any, { where: { id }, ...options } as any), + engine.update('sys_upload_session', changedColumns(patch), { + where: { id }, + ...systemByIdWriteOptionsFor(context), + }), ); return merged; } @@ -565,9 +645,10 @@ export class StorageMetadataStore { this.sessions.delete(id); return; } - const options = writeOptionsFor(context); + // [#21908] The explicit system opt-in, the organization kept beside it as + // the statement's scope — see systemByIdWriteOptionsFor. await this.engineOp('sys_upload_session', 'delete', SESSION_DELETE_CONSEQUENCE, (engine) => - engine.delete('sys_upload_session', { where: { id }, ...options } as any), + engine.delete('sys_upload_session', { where: { id }, ...systemByIdWriteOptionsFor(context) }), ); } } diff --git a/packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts b/packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts index dffd590803e..63ba4fb1b10 100644 --- a/packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts +++ b/packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts @@ -111,13 +111,17 @@ function createRecordingEngine(seed: Array<{ object: string; data: any }> = []) const rows = seed.map((s) => ({ ...s, data: { ...s.data } })); const updates: Array<{ object: string; data: any; options: any }> = []; const deletes: Array<{ object: string; options: any }> = []; + // [#21908] The by-id reads' trailing options bag, where the store now puts + // its explicit system opt-in. + const reads: Array<{ object: string; query: any; options: any }> = []; const engine: any = { async insert(object: string, data: any) { rows.push({ object, data: { ...data } }); return { ...data }; }, - async findOne(object: string, query?: any) { + async findOne(object: string, query?: any, options?: any) { assertEngineFindOnePredicate(object, query); + reads.push({ object, query, options }); const hit = rows.find( (r) => r.object === object && String(r.data.id) === String(query?.where?.id), ); @@ -144,6 +148,7 @@ function createRecordingEngine(seed: Array<{ object: string; data: any }> = []) }, _updates: updates, _deletes: deletes, + _reads: reads, }; return engine; } @@ -160,9 +165,10 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga // `where` keeps ONLY the id — the tenant term is `applyTenantScope`'s to // compose, not this store's (composing it here would re-decide whether the // object has a tenant column, one package away from the schema). + // [#21908] …beside the explicit system opt-in. expect(engine._updates[0].options).toEqual({ where: { id: 'f1' }, - context: { tenantId: 'org_A' }, + context: { tenantId: 'org_A', isSystem: true }, }); }); @@ -173,7 +179,7 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga await store.deleteFile('f1', { organizationId: 'org_A' }); expect(engine._deletes).toEqual([ - { object: 'sys_file', options: { where: { id: 'f1' }, context: { tenantId: 'org_A' } } }, + { object: 'sys_file', options: { where: { id: 'f1' }, context: { tenantId: 'org_A', isSystem: true } } }, ]); }); @@ -189,7 +195,7 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga expect(engine._updates[0].object).toBe('sys_upload_session'); expect(engine._updates[0].options).toEqual({ where: { id: 's1' }, - context: { tenantId: 'org_A' }, + context: { tenantId: 'org_A', isSystem: true }, }); }); @@ -202,7 +208,7 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga expect(engine._deletes).toEqual([ { object: 'sys_upload_session', - options: { where: { id: 's1' }, context: { tenantId: 'org_A' } }, + options: { where: { id: 's1' }, context: { tenantId: 'org_A', isSystem: true } }, }, ]); }); @@ -221,7 +227,7 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga expect(engine._updates[0].data).not.toHaveProperty('organization_id'); }); - it('a caller with no organization produces the pre-#13178 call shape exactly', async () => { + it('a caller with no organization carries the opt-in and NO tenant — ⛔ none is invented', async () => { const engine = createRecordingEngine([ { object: 'sys_file', data: fileRec('f1') }, { object: 'sys_upload_session', data: sessionRec('s1') }, @@ -236,11 +242,11 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga await store.deleteFile('f1'); await store.deleteSession('s1', { organizationId: '' }); - // ⛔ No `context` key at all, not `context: {}`. An empty context is still a - // context and changes what every other option resolver on the call sees — - // the reason `writeOptionsFor` answers `undefined` rather than an empty bag. - for (const u of engine._updates) expect(u.options).not.toHaveProperty('context'); - for (const d of engine._deletes) expect(d.options).not.toHaveProperty('context'); + // [#21908] Every by-id write now carries the explicit system opt-in, and + // still no `tenantId` where the caller has no organization: the opt-in is + // the only key, never a guessed organization. + for (const u of engine._updates) expect(u.options.context).toEqual({ isSystem: true }); + for (const d of engine._deletes) expect(d.options.context).toEqual({ isSystem: true }); expect(engine._updates.map((u: any) => u.options.where)).toEqual([ { id: 'f1' }, { id: 'f1' }, @@ -248,9 +254,9 @@ describe('[#13178] the store: the four half-repaired sites carry the acting orga { id: 'f1' }, { id: 's1' }, ]); - expect(engine._deletes.map((d: any) => d.options)).toEqual([ - { where: { id: 'f1' } }, - { where: { id: 's1' } }, + expect(engine._deletes.map((d: any) => d.options.where)).toEqual([ + { id: 'f1' }, + { id: 's1' }, ]); }); }); @@ -359,7 +365,7 @@ describe('[#13178] the routes: the upload doors pass the session organization on const update = engine._updates.find((u: any) => u.object === 'sys_file'); expect(update).toBeTruthy(); - expect(update.options).toEqual({ where: { id: 'f1' }, context: { tenantId: 'org_A' } }); + expect(update.options).toEqual({ where: { id: 'f1' }, context: { tenantId: 'org_A', isSystem: true } }); }); it('the chunked completion scopes BOTH of its writes from the same session value', async () => { @@ -398,8 +404,8 @@ describe('[#13178] the routes: the upload doors pass the session organization on // from ONE resolved organization. const file = engine._updates.find((u: any) => u.object === 'sys_file'); const session = engine._updates.find((u: any) => u.object === 'sys_upload_session'); - expect(file.options.context).toEqual({ tenantId: 'org_A' }); - expect(session.options.context).toEqual({ tenantId: 'org_A' }); + expect(file.options.context).toEqual({ tenantId: 'org_A', isSystem: true }); + expect(session.options.context).toEqual({ tenantId: 'org_A', isSystem: true }); expect(engine._updates.every((u: any) => u.options.context?.tenantId === 'org_A')).toBe(true); }); @@ -423,7 +429,7 @@ describe('[#13178] the routes: the upload doors pass the session organization on const update = engine._updates.find((u: any) => u.object === 'sys_upload_session'); expect(update).toBeTruthy(); - expect(update.options).toEqual({ where: { id: 's1' }, context: { tenantId: 'org_A' } }); + expect(update.options).toEqual({ where: { id: 's1' }, context: { tenantId: 'org_A', isSystem: true } }); }); it('the progress door — which WRITES when it expires a row — scopes that write too', async () => { @@ -451,7 +457,7 @@ describe('[#13178] the routes: the upload doors pass the session organization on const update = engine._updates.find((u: any) => u.object === 'sys_upload_session'); expect(update).toBeTruthy(); expect(update.data.status).toBe('expired'); - expect(update.options.context).toEqual({ tenantId: 'org_A' }); + expect(update.options.context).toEqual({ tenantId: 'org_A', isSystem: true }); }); it('a session with no active organization still stamps nothing — ⛔ no guess is substituted', async () => { @@ -469,7 +475,8 @@ describe('[#13178] the routes: the upload doors pass the session organization on ); const update = engine._updates.find((u: any) => u.object === 'sys_file'); - expect(update.options).toEqual({ where: { id: 'f1' } }); + // [#21908] The opt-in rides alone: no organization is substituted. + expect(update.options).toEqual({ where: { id: 'f1' }, context: { isSystem: true } }); }); }); @@ -723,3 +730,240 @@ describe('[#13178] the driver leg: what the threaded tenantId actually buys', () expect(await readStatus('legacy')).toBeNull(); }); }); + +// --------------------------------------------------------------------------- +// [#21908] D. THE OPT-IN — the six by-id methods run under the explicit system +// opt-in, and the door's organization stays the write statement's reach. +// +// The maintainer's ruling on that card (letter A) put the opt-in on these six +// INSIDE this store, the posture `createFile` already had: access by id, with +// the door-derived tenant kept as the driver-level scope on update and delete. +// Three facts are pinned, at the altitude each one lives at: +// +// D1. the store: each by-id READ carries the opt-in and no tenant; each +// by-id WRITE carries the opt-in beside the door's tenant (above, in A +// and B) and sends the caller's patch alone; +// D2. ⛔ the negative, through the REAL ObjectQL over a REAL SqlDriver: under +// the opt-in a door's tenant still cannot update or delete a row stamped +// for another organization. The opt-in replaces the security +// middleware's principal-less hand-off; it must not replace the +// driver's scope, which is the only one these writes have. +// --------------------------------------------------------------------------- + +describe('[#21908] D1. the store: the by-id reads carry the opt-in, and the writes send the patch alone', () => { + it('getFile and getSession read with { isSystem: true } and NO tenant — access stays by id', async () => { + const engine = createRecordingEngine([ + { object: 'sys_file', data: fileRec('f1') }, + { object: 'sys_upload_session', data: sessionRec('s1') }, + ]); + const store = new StorageMetadataStore(engine); + + expect((await store.getFile('f1'))?.id).toBe('f1'); + expect((await store.getSession('s1'))?.id).toBe('s1'); + + expect(engine._reads.map((r: any) => [r.object, r.query, r.options])).toEqual([ + ['sys_file', { where: { id: 'f1' } }, { context: { isSystem: true } }], + ['sys_upload_session', { where: { id: 's1' } }, { context: { isSystem: true } }], + ]); + }); + + it('the read behind an update is that same read; the write alone carries the door tenant', async () => { + const engine = createRecordingEngine([ + { object: 'sys_file', data: fileRec('f1') }, + { object: 'sys_upload_session', data: sessionRec('s1') }, + ]); + const store = new StorageMetadataStore(engine); + + await store.updateFile('f1', { status: 'committed' }, { organizationId: 'org_A' }); + await store.updateSession('s1', { status: 'completed' }, { organizationId: 'org_A' }); + + for (const r of engine._reads) expect(r.options).toEqual({ context: { isSystem: true } }); + for (const u of engine._updates) expect(u.options.context).toEqual({ tenantId: 'org_A', isSystem: true }); + }); + + it('⛔ the provisioned columns of the row read back never reach an update payload', async () => { + // The row as a walled install stores it: stamped, audited, owned. Under the + // opt-in the engine's `readonly` strip no longer runs on this write, so the + // payload itself is what keeps these columns the platform's. + const provisioned = { + organization_id: 'org_B', + created_at: '2026-01-01T00:00:00.000Z', + updated_at: '2026-01-01T00:00:00.000Z', + created_by: 'u0', + updated_by: 'u0', + }; + const engine = createRecordingEngine([ + { object: 'sys_file', data: { ...fileRec('f1'), owner_id: 'u0', ...provisioned } }, + { object: 'sys_upload_session', data: { ...sessionRec('s1'), ...provisioned } }, + ]); + const store = new StorageMetadataStore(engine); + + const file = await store.updateFile('f1', { status: 'committed', etag: 'e1' }, { organizationId: 'org_A' }); + const session = await store.updateSession('s1', { status: 'completing' }, { organizationId: 'org_A' }); + + expect(engine._updates.map((u: any) => u.data)).toEqual([ + { status: 'committed', etag: 'e1' }, + { status: 'completing' }, + ]); + // What the caller is handed back is unchanged: the row as read, patched. + expect(file).toMatchObject({ id: 'f1', status: 'committed', etag: 'e1', owner_id: 'u0', organization_id: 'org_B' }); + expect(session).toMatchObject({ id: 's1', status: 'completing', organization_id: 'org_B' }); + }); +}); + +describe('[#21908] D1b. the opt-in leaves these writes audited — real ObjectQL, recording driver', () => { + it('with no organization: no tenantId reaches the driver, and no tenant-audit bypass is filled in', async () => { + // Both objects are tenant-scoped in the platform's tenancy inventory, so the + // engine fills in no `bypassTenantAudit` for an `isSystem` write on them: an + // unscoped by-id write is still reported on a walled deployment. + const { engine, calls } = await makeEngine(); + const store = new StorageMetadataStore(engine as any); + + await store.updateFile('f1', { status: 'committed' }); + await store.deleteSession('s1'); + + const writes = calls.filter((x) => x.method === 'update' || x.method === 'delete'); + expect(writes.map((w) => w.method)).toEqual(['update', 'delete']); + for (const w of writes) { + expect(w.options?.tenantId ?? undefined).toBeUndefined(); + expect(w.options?.bypassTenantAudit ?? undefined).toBeUndefined(); + } + }); +}); + +describe('[#21908] D2. ⛔ under the opt-in, a door tenant still cannot reach another organization’s row', () => { + const OLD = process.env.OS_TENANCY_POSTURE; + let ql: ObjectQL; + let sql: SqlDriver; + let driverWrites: Array<{ verb: string; object: string; id: unknown; options: any }>; + let engineContexts: Array<{ verb: string; object: string; context: any }>; + let store: StorageMetadataStore; + + const SYS = { isSystem: true } as const; + const stored = async (object: string, id: string) => + (await ql.findOne(object, { where: { id } }, { context: SYS })) as Record | null; + + beforeEach(async () => { + // Seeded under the default posture: an organization-less row is exactly + // what the walled posture refuses to WRITE now, and exactly the legacy + // population the driver's `OR … IS NULL` arm keeps reachable. The doors + // then run walled. + delete process.env.OS_TENANCY_POSTURE; + sql = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + driverWrites = []; + engineContexts = []; + const realUpdate = (sql as any).update.bind(sql); + (sql as any).update = async (object: string, id: unknown, data: any, options: any) => { + driverWrites.push({ verb: 'update', object, id, options }); + return realUpdate(object, id, data, options); + }; + const realDelete = (sql as any).delete.bind(sql); + (sql as any).delete = async (object: string, id: unknown, options: any) => { + driverWrites.push({ verb: 'delete', object, id, options }); + return realDelete(object, id, options); + }; + ql = new ObjectQL(); + ql.registerDriver(sql, true); + await ql.init(); + ql.registry.registerObject(SystemFile as any, 'com.objectstack.storage'); + ql.registry.registerObject(SystemUploadSession as any, 'com.objectstack.storage'); + await ql.syncSchemas(); + for (const [id, org] of [['own', 'org_A'], ['other', 'org_B'], ['legacy', null]] as const) { + await ql.insert('sys_file', { ...fileRec(id), organization_id: org }, { context: SYS }); + await ql.insert('sys_upload_session', { ...sessionRec(id), organization_id: org }, { context: SYS }); + } + process.env.OS_TENANCY_POSTURE = 'isolated'; + // The store reaches the REAL engine; this wrapper only records the context + // each call carried, so the pin is about the opt-in path and no other. Each + // verb still opens with the engine's own dispatch predicate, as the double + // above does, so the wrapper can never accept a shape the engine refuses. + const recorded: any = { + findOne: (o: string, q: any, opts: any) => { + assertEngineFindOnePredicate(o, q); + engineContexts.push({ verb: 'findOne', object: o, context: opts?.context ?? q?.context }); + return ql.findOne(o, q, opts); + }, + update: (o: string, d: any, opts: any) => { + assertEngineUpdateDispatch(d, opts); + engineContexts.push({ verb: 'update', object: o, context: opts?.context }); + return ql.update(o, d, opts); + }, + delete: (o: string, opts: any) => { + assertEngineDeleteDispatch(opts); + engineContexts.push({ verb: 'delete', object: o, context: opts?.context }); + return ql.delete(o, opts); + }, + }; + store = new StorageMetadataStore(recorded); + }); + + afterEach(async () => { + try { + await ql?.destroy(); + } catch { + /* noop */ + } + if (OLD === undefined) delete process.env.OS_TENANCY_POSTURE; + else process.env.OS_TENANCY_POSTURE = OLD; + }); + + /** The refusal a foreign row gets: the engine finds no row in the door's reach. */ + const notFound = (objectName: string, operation: 'update' | 'delete') => ({ + name: 'StorageMetadataStoreError', + objectName, + operation, + cause: { code: 'RECORD_NOT_FOUND' }, + }); + + it('updateFile and updateSession on a foreign row are refused, and change nothing', async () => { + await expect( + store.updateFile('other', { status: 'committed' }, { organizationId: 'org_A' }), + ).rejects.toMatchObject(notFound('sys_file', 'update')); + await expect( + store.updateSession('other', { status: 'completed' }, { organizationId: 'org_A' }), + ).rejects.toMatchObject(notFound('sys_upload_session', 'update')); + + expect((await stored('sys_file', 'other'))?.status).toBe('pending'); + expect((await stored('sys_upload_session', 'other'))?.status).toBe('in_progress'); + // The writes really ran on the opt-in path, scoped by the door's tenant. + expect(engineContexts.filter((c) => c.verb === 'update').map((c) => c.context)).toEqual([ + { tenantId: 'org_A', isSystem: true }, + { tenantId: 'org_A', isSystem: true }, + ]); + for (const w of driverWrites) expect(w.options?.tenantId).toBe('org_A'); + }); + + it('deleteFile and deleteSession on a foreign row are refused, and delete nothing', async () => { + await expect(store.deleteFile('other', { organizationId: 'org_A' })).rejects.toMatchObject( + notFound('sys_file', 'delete'), + ); + await expect(store.deleteSession('other', { organizationId: 'org_A' })).rejects.toMatchObject( + notFound('sys_upload_session', 'delete'), + ); + + expect(await stored('sys_file', 'other')).not.toBeNull(); + expect(await stored('sys_upload_session', 'other')).not.toBeNull(); + expect(engineContexts.filter((c) => c.verb === 'delete').map((c) => c.context)).toEqual([ + { tenantId: 'org_A', isSystem: true }, + { tenantId: 'org_A', isSystem: true }, + ]); + for (const w of driverWrites) expect(w.options?.tenantId).toBe('org_A'); + }); + + it('still works: the caller’s own row and an organization-less row are reached', async () => { + // A refusal that refused everything would score green above; this is the + // other half. + expect(await store.updateFile('own', { status: 'committed' }, { organizationId: 'org_A' })).toBeTruthy(); + expect(await store.updateSession('legacy', { status: 'completed' }, { organizationId: 'org_A' })).toBeTruthy(); + await store.deleteFile('legacy', { organizationId: 'org_A' }); + await store.deleteSession('own', { organizationId: 'org_A' }); + + expect((await stored('sys_file', 'own'))?.status).toBe('committed'); + expect((await stored('sys_upload_session', 'legacy'))?.status).toBe('completed'); + expect(await stored('sys_file', 'legacy')).toBeNull(); + expect(await stored('sys_upload_session', 'own')).toBeNull(); + // …and the stored tenant column is the one the row already had. + expect((await stored('sys_file', 'own'))?.organization_id).toBe('org_A'); + expect((await stored('sys_upload_session', 'legacy'))?.organization_id ?? null).toBeNull(); + }); +}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 15ed413e19f..1388536ef22 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -4419,17 +4419,17 @@ { "file": "packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts", "verb": "delete", - "pinned": 1 + "pinned": 2 }, { "file": "packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts", "verb": "findOne", - "pinned": 1 + "pinned": 2 }, { "file": "packages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.ts", "verb": "update", - "pinned": 1 + "pinned": 2 }, { "file": "packages/services/service-storage/src/tombstone-download-live-reference.test.ts",