diff --git a/.changeset/sec-r17-attach-on-upload-complete.md b/.changeset/sec-r17-attach-on-upload-complete.md new file mode 100644 index 00000000000..e12e45b2c1f --- /dev/null +++ b/.changeset/sec-r17-attach-on-upload-complete.md @@ -0,0 +1,24 @@ +--- +'@objectstack/spec': minor +'@objectstack/core': patch +'@objectstack/objectql': patch +'@objectstack/service-storage': patch +'@object-ui/data-objectstack': patch +'@object-ui/app-shell': patch +--- + +fix(service-storage): a member attaches a file from the browser again — the upload's own completion attaches it as them, restoring the Attachments panel after #31031 + +Clause-②: yes (widening) +Metadata-protocol: added +Skills-docs: updated here — `content/docs/permissions/attachments-access.mdx` and `content/docs/api/plugin-endpoints.mdx` state the `attachTo` key of the upload-complete request and its refusals; no file under `skills/**` teaches the upload-complete request (a `git grep` of `skills/` finds no `upload/complete` and no `CompleteUploadRequest`). + +A signed-in member who may attach files, and holds no read of `sys_file`, can attach a file from a record's Attachments panel again. Since the write-time reference check began judging a `lookup` value through the caller's own read door, the panel's second step — `POST /api/v1/data/sys_attachment` naming the file it had just uploaded — was refused `400 VALIDATION_FAILED` (`file_id`, `reference_not_found`) for such a member. + +The rule now: the upload-complete door (`POST /api/v1/storage/upload/complete`) takes an optional `attachTo: { parentObject, parentId }`. With it, the door runs every gate it already asked — the session, the upload ownership rule, the starting organization, the held bytes — and then hands the upload to the file-attachments service's `attachUploadAs`, which runs as the caller. That verb vouches for one `sys_file` only: a `pending` upload in the `attachments` scope that the caller uploaded, that no record holds yet, in the caller's write reach and in the parent's organization. It asks the attach authorization first (the permission to create an attachment, the parent-edit attach rule, `enable.files`), then writes the `sys_attachment` row and commits the file as one unit. The answer is the committed file with its new `attachmentId`. Another user's upload id, an id that names no file, a committed or already attached upload, and a record the caller may not attach to are refused, and the upload stays `pending`. + +The vouch is spent on that one write: a hook that runs under it — as the user or as the system — writes without the mark (`withoutVouchedReferences`, `@objectstack/core`), so a hook naming the same `file_id` is judged through the caller's read door. + +Unchanged: no caller gains a read of `sys_file` or of anything else. The generic data door keeps its rule — a `sys_attachment` create through `POST /api/v1/data/sys_attachment` naming a `sys_file` the caller cannot read is still refused `reference_not_found`. A completion without `attachTo` commits exactly as before. A mount with no file-attachments service bound refuses `attachTo` with `501 NOT_IMPLEMENTED`. + +The console's Attachments panel (`@object-ui/app-shell`) now completes its upload with `attachTo` and makes no second call, keeping its refusal messages; the upload adapter (`@object-ui/data-objectstack`) gains an `attachTo` option and returns the `attachmentId` it receives. diff --git a/content/docs/api/plugin-endpoints.mdx b/content/docs/api/plugin-endpoints.mdx index 24befc39509..e4834dce92a 100644 --- a/content/docs/api/plugin-endpoints.mdx +++ b/content/docs/api/plugin-endpoints.mdx @@ -154,7 +154,7 @@ other adapter. | Method | Endpoint | SDK | Description | |:-------|:---------|:----|:------------| | POST | `/storage/upload/presigned` | `storage.getPresignedUrl` | Step 1 — mint an upload target for a new `sys_file` row | -| POST | `/storage/upload/complete` | `storage.upload` | Step 3 — commit the uploaded bytes | +| POST | `/storage/upload/complete` | `storage.upload` | Step 3 — commit the uploaded bytes; with `attachTo`, also attach the file to a record as the caller | | POST | `/storage/upload/chunked` | `storage.initChunkedUpload` | Open a chunked / resumable session | | PUT | `/storage/upload/chunked/:uploadId/chunk/:chunkIndex` | `storage.uploadPart` | Send one chunk | | POST | `/storage/upload/chunked/:uploadId/complete` | `storage.completeChunkedUpload` | Assemble the parts | @@ -167,6 +167,13 @@ other adapter. so for a file whose type the browser does not know (`file.type === ''`) the helper sends `application/octet-stream`; a typed file keeps its own type. +To attach the upload to a record's Attachments, send `attachTo: { parentObject, +parentId }` with step 3 (and start the upload with `scope: 'attachments'`): the +commit and the `sys_attachment` row are one unit, written as the signed-in +caller, and the answer carries the `attachmentId` +([attachment access](/docs/permissions/attachments-access)). A refused attach +leaves the upload pending. + A file field takes the `fileId` only after step 3. A signed-in record write that names a file whose upload was never completed is refused with `400 VALIDATION_FAILED`, one `invalid_value` entry naming the field and the file, diff --git a/content/docs/permissions/attachments-access.mdx b/content/docs/permissions/attachments-access.mdx index 6760ee3ea6b..1f72d8bf5ba 100644 --- a/content/docs/permissions/attachments-access.mdx +++ b/content/docs/permissions/attachments-access.mdx @@ -72,6 +72,39 @@ drops a non-system caller's `uploaded_by` and reports it in `droppedFields`, and the rest of the write lands. System-context writes keep the value they supply. +### Attaching an upload from the browser + +The Attachments panel attaches a file in the call that completes its upload: +the presigned upload's `POST /api/v1/storage/upload/complete` carries +`attachTo: { parentObject, parentId }`, and the server commits the file and +writes its `sys_attachment` row as one unit, **as the signed-in user**. Every +check above judges that user: the permission to create an attachment, edit +access to the record, the record object's `enable.files`, and the file +reference itself. The answer carries the new `attachmentId`. A refused attach +leaves the upload `pending` and attached to nothing, and the pending-upload +retention reclaims it. + +The file must be the caller's own upload, still `pending`, in the +`attachments` scope, attached to nothing and owned by no record field, with its +bytes stored. Any other file id — another user's upload, an id that names no +file — is refused before anything is written: + +| Code | Status | When | +| --- | --- | --- | +| `PERMISSION_DENIED` | 403 | The file is another user's upload (the commit door's uploader rule) | +| `FILE_NOT_FOUND` | 404 | The id names no file | +| `RESOURCE_CONFLICT` | 409 | The upload is already completed, already attached or owned by a field, or its bytes are not stored | +| `INVALID_REQUEST` | 400 | `attachTo` is malformed, or the upload was not started in the `attachments` scope | +| `NOT_IMPLEMENTED` | 501 | The deployment composes no auth service or no data engine, so there is no caller to attach as | + +This is how a member who may attach files but holds no read on `sys_file` +attaches one: a `sys_attachment` create through the generic data API +(`POST /api/v1/data/sys_attachment`) names its `file_id` as any other +`lookup` value, which the caller must be able to read, and a file they cannot +read is refused `400 VALIDATION_FAILED` (`reference_not_found` on `file_id`). +The attach on completion vouches for that one file, in that one call, and +grants no read of `sys_file` or of anything else. + | Code | Status | When | | --- | --- | --- | | `RECORD_NOT_ACCESSIBLE` | 403 | The session names no signed-in user to record as the uploader | @@ -224,6 +257,7 @@ can be shared across records). Reclamation is handled by the platform LifecycleS | Operation | Requirement | Deny code | | --- | --- | --- | | Attach (create) | parent object opts in (`enable.files`) | `FILES_DISABLED` (403) | +| Attach on upload completion (`attachTo`) | the caller's own pending, unattached `attachments`-scope upload, then every create check | `PERMISSION_DENIED` (403) / `RESOURCE_CONFLICT` (409), then the create codes | | Attach (create), re-point | a by-id update of the parent record by the caller would be admitted | `ATTACHMENT_PARENT_ACCESS` (403); `OBJECT_NOT_FOUND` (404) when the parent object is not registered | | List / read | inherits parent read visibility | *(filtered out)* | | Delete | uploader or parent editor (+ RBAC delete grant) | `ATTACHMENT_DELETE_DENIED` (403); `PERMISSION_DENIED` (403) when the parent is not readable or no delete grant is held | diff --git a/content/docs/permissions/tenant-audit-census.mdx b/content/docs/permissions/tenant-audit-census.mdx index 0302c17a734..499deba0a13 100644 --- a/content/docs/permissions/tenant-audit-census.mdx +++ b/content/docs/permissions/tenant-audit-census.mdx @@ -93,7 +93,7 @@ what moved this page's population from 225 to 227; nothing about the two sites changed, only whether this instrument could see them. **The expensive failure direction is a keyword.** Sites whose receiver the author -typed `any` have no type to read, and there are 62 of them — just over a fifth +typed `any` have no type to read, and there are 63 of them — just over a fifth of the population, concentrated in exactly the seed and bootstrap paths this control exists for. Scoring an unreadable receiver as "not an engine" would have dropped every one of them silently, with a clean exit and a smaller number that @@ -132,7 +132,7 @@ are reported as `undecidable` rather than assumed either way. The same holds twice over for the context. An options argument spelled as a literal can be read; one spelled `options`, `{ ...opts }`, or handed through a -forwarding shim cannot, and **55 of the 265 sites are spelled that way**. A +forwarding shim cannot, and **55 of the 266 sites are spelled that way**. A context resolved from an inline literal or a local `const` can be tested for `isSystem`; one arriving from a helper call cannot. @@ -197,10 +197,10 @@ reproduce them. Where it disagrees, it disagrees on the page: | carried figure | where it survives | this census | | :--- | :--- | ---: | -| 175 write call sites | quoted in the merged changeset | **265** | +| 175 write call sites | quoted in the merged changeset | **266** | | 24 carrying no tenant context | quoted in the merged changeset | **2** provable and tenancy-enabled; **29** more whose options argument is unreadable | -| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **188 of 265** decidable, **77** undecidable | -| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 148 decidably elevated, 0 decidably not, 109 undecidable | +| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **189 of 266** decidable, **77** undecidable | +| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 148 decidably elevated, 0 decidably not, 110 undecidable | | 141 and 132, two independent re-derivations | the card that filed this work | — | **The differences are not reconciled, and deliberately so.** The old census's @@ -210,18 +210,18 @@ be stated is what this instrument counts, which is written above and re-runnable at any commit. Two structural facts do plausibly widen this reading against any hand or regex -one, and both are counted in the generated tables below: the 62 sites reached +one, and both are counted in the generated tables below: the 63 sites reached through an erased (`any`) receiver, and the 69 that name their object through a `const` rather than inline. An instrument that read either the way a person does would report a smaller number and would not say so. The fourth row is the one worth flagging to anyone citing it. **The 135 / 77% figure has no surviving corroboration anywhere in the tree.** This census reads -148 of 265 (56%) as decidably elevated, with 109 more whose elevation is a +148 of 266 (56%) as decidably elevated, with 110 more whose elevation is a run-time fact — so the claim is neither confirmed nor refuted, and the honest answer is that a static reading cannot settle it. -⇒ **Cite `2 / 265`, and say what it is**: the sites whose options argument was +⇒ **Cite `2 / 266`, and say what it is**: the sites whose options argument was READ and holds no tenant context, against a decidably tenancy-enabled object. That is the control's provable yield surface. ⛔ Do not cite it as "the sites without tenant context" — **29 further sites** have an options argument this @@ -233,28 +233,28 @@ cannot read, and they are neither in nor out. | what | count | | :--- | ---: | -| write call sites on the application surface | **265** | -| …whose object name is statically decidable | 188 | +| write call sites on the application surface | **266** | +| …whose object name is statically decidable | 189 | | …whose object name is chosen at run time | 77 | -| …against an object with tenancy ENABLED | 182 | +| …against an object with tenancy ENABLED | 183 | | …against an object that declares tenancy off | 6 | -| threading a tenant context | 202 | +| threading a tenant context | 203 | | PROVABLY carrying none (options read, no context key) | **8** | | …of those, against a decidably tenancy-enabled object | **2** | | options argument UNREADABLE — may or may not carry one | 55 | | …of those, against a decidably tenancy-enabled object | 29 | | threading a decidably ELEVATED (`isSystem`) context | 148 | | threading a context that is decidably NOT elevated | 0 | -| threading a context whose elevation is a run-time fact | 109 | +| threading a context whose elevation is a run-time fact | 110 | | how the instrument reached the site | count | | :--- | ---: | | receiver carried a readable engine type | 203 | -| receiver erased, placed by the object NAME | 52 | +| receiver erased, placed by the object NAME | 53 | | receiver erased, placed by an `object: string` PARAMETER | 5 | | receiver erased, placed by an `UNTYPED_RECEIVERS` row | 5 | -| object name spelled inline | 119 | +| object name spelled inline | 120 | | object name spelled through a `const` | 69 | | object name is an `object: string` parameter | 7 | | object name is some other run-time expression | 70 | @@ -307,7 +307,7 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-11-06 at `4addb47fd`. +Measured on 2026-11-07 at `ce0325138`. | corpus scale (not enforced) | count | | :--- | ---: | diff --git a/content/docs/references/api/storage.mdx b/content/docs/references/api/storage.mdx index f34c17c4b44..e2916eb0d1a 100644 --- a/content/docs/references/api/storage.mdx +++ b/content/docs/references/api/storage.mdx @@ -103,6 +103,14 @@ const result = CompleteChunkedUploadRequestSchema.parse(data); | :--- | :--- | :--- | :--- | | **fileId** | `string` | ✅ | File ID returned from presigned request | | **eTag** | `string` | optional | ETag the client reports for the uploaded object. Stored on the file record as sent and not compared with anything: no storage adapter reports a stored object's ETag. Before committing, the door checks that the storage backend holds bytes under the file's key and, when the upload declared a size, that the held size equals it. | +| **attachTo** | `{ parentObject: string; parentId: string }` | optional | Attach the uploaded file to this record in the same call, as the signed-in caller: the commit and the attachment row are one unit, and the answer carries the new attachmentId. Only the uploader's own pending upload in the attachments scope can be attached this way. The attachment is judged as the caller's own create: the permission to create an attachment, edit access to the record, and the record object's enable.files opt-in. A refused attach leaves the upload pending and uncommitted. | + +### Nested Shape: `CompleteUploadRequest.attachTo` + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **parentObject** | `string` | ✅ | Object of the record the file is attached to (enable.files: true) | +| **parentId** | `string` | ✅ | Id of the record the file is attached to | --- @@ -211,6 +219,7 @@ const result = CompleteChunkedUploadRequestSchema.parse(data); | **created** | `string` | ✅ | Creation timestamp | | **etag** | `string` | optional | Entity tag | | **fileId** | `string` | optional | Opaque sys_file id (ADR-0104 D3 file-as-reference) | +| **attachmentId** | `string` | optional | Id of the sys_attachment row the commit created, when the request carried attachTo | --- diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.counts/api.md b/docs/audits/2026-07-unknown-key-strictness-ledger.counts/api.md index 3ba6578f2de..12717707e6a 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.counts/api.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.counts/api.md @@ -19,4 +19,4 @@ hand-patch a number here** — fix the code or the verdict and regenerate. | Dir | Sites | |---|---| -| `api/` | 500 | +| `api/` | 501 | diff --git a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md index 2f57b162912..6d952f354cc 100644 --- a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md +++ b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md @@ -33,19 +33,19 @@ silent, and `node scripts/tenant-audit-census.mjs --write` is the resolution. | Measure | Value | |---|---:| -| Write call sites | 265 | -| Object name statically decidable | 188 | +| Write call sites | 266 | +| Object name statically decidable | 189 | | Object name chosen at run time | 77 | -| Against a tenancy-enabled object | 182 | +| Against a tenancy-enabled object | 183 | | Against an object declaring tenancy off | 6 | -| Threading a tenant context | 202 | +| Threading a tenant context | 203 | | Provably carrying none | 8 | | …and decidably tenancy-enabled | 2 | | Options argument unreadable | 55 | | …and decidably tenancy-enabled | 29 | | Threading a decidably elevated context | 148 | | Threading a decidably non-elevated context | 0 | -| Threading a context of undecidable elevation | 109 | +| Threading a context of undecidable elevation | 110 | ## Subtractions the census could NOT defend — enforced @@ -90,7 +90,7 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-11-06 at `4addb47fd`. +Measured on 2026-11-07 at `ce0325138`. | corpus scale (not enforced) | count | | :--- | ---: | @@ -270,7 +270,7 @@ Measured on 2026-11-06 at `4addb47fd`. | `packages/services/service-storage/src/backfill-sys-file-organizations.ts` | `update` | `sys_file` | enabled | context, elevation undecidable | 1 | | `packages/services/service-storage/src/backfill-sys-file-public-scope.ts` | `update` | `sys_file` | enabled | context, elevation undecidable | 1 | | `packages/services/service-storage/src/file-attachment-service.ts` | `delete` | `sys_attachment` | enabled | context, elevation undecidable | 1 | -| `packages/services/service-storage/src/file-attachment-service.ts` | `insert` | `sys_attachment` | enabled | context, elevation undecidable | 1 | +| `packages/services/service-storage/src/file-attachment-service.ts` | `insert` | `sys_attachment` | enabled | context, elevation undecidable | 2 | | `packages/services/service-storage/src/file-reference-lifecycle.ts` | `insert` | `sys_file` | enabled | context, elevation undecidable | 1 | | `packages/services/service-storage/src/file-reference-lifecycle.ts` | `update` | `sys_file` | enabled | elevated | 2 | | `packages/services/service-storage/src/metadata-store.ts` | `delete` | `sys_file` | enabled | options unreadable | 1 | diff --git a/packages/core/src/security/index.ts b/packages/core/src/security/index.ts index 5095f0d2da8..16dd237b8f2 100644 --- a/packages/core/src/security/index.ts +++ b/packages/core/src/security/index.ts @@ -544,6 +544,7 @@ export { VOUCHED_REFERENCES, vouchForReference, isVouchedReference, + withoutVouchedReferences, type VouchedReference, } from './vouched-reference.js'; diff --git a/packages/core/src/security/vouched-reference.ts b/packages/core/src/security/vouched-reference.ts index 65d934a2d80..66a21a5d099 100644 --- a/packages/core/src/security/vouched-reference.ts +++ b/packages/core/src/security/vouched-reference.ts @@ -46,9 +46,18 @@ * string, stored row or flow definition can spell a symbol, so no author can * set it; it is in no schema; `Symbol.for` keeps it one key across the ESM and * CJS builds; and, being undeclared, it does not survive - * `ExecutionContextSchema.parse`. It rides a spread of the context as any own - * property does, so a write a hook sets off under the vouched write carries it - * too, and is admitted only for the same object, field and id. + * `ExecutionContextSchema.parse`. + * + * ## It is spent on the verb's own write — no hook inherits it + * + * The mark rides a spread of the context as any own property does, and every + * context a hook writes through is derived from the write that fired it. So + * the engine strips it where it builds a hook's api from the write's context + * (`buildHookApi`, objectql, via {@link withoutVouchedReferences}): a hook — + * app-authored or platform, `runAs: 'user'` or inherited — that writes the same + * object, field and id under the vouched write is judged through its caller's + * read door like any other write. The mark admits the one write the verb + * makes, and no write that write sets off. */ /** The context key of the mark. See the module header. */ @@ -98,3 +107,16 @@ export function isVouchedReference(context: unknown, object: unknown, field: unk && (r as VouchedReference).id === value, ); } + +/** + * `context` without the mark: the same object when it carries none, else a + * fresh copy with every other own property. The engine calls it where it hands + * a hook the write's context (see the module header), so the vouch is spent + * on the verb's own write. + */ +export function withoutVouchedReferences(context: T): T { + if (context === null || typeof context !== 'object') return context; + if (!Object.prototype.hasOwnProperty.call(context, VOUCHED_REFERENCES)) return context; + const { [VOUCHED_REFERENCES]: _spent, ...rest } = context as T & { [VOUCHED_REFERENCES]?: unknown }; + return rest as T; +} diff --git a/packages/objectql/src/engine-defaults-and-autonumbers.ts b/packages/objectql/src/engine-defaults-and-autonumbers.ts index 28fa8c99bcf..db8fdee18c2 100644 --- a/packages/objectql/src/engine-defaults-and-autonumbers.ts +++ b/packages/objectql/src/engine-defaults-and-autonumbers.ts @@ -37,6 +37,7 @@ import { isCurrentUserDefaultToken, isNowDefaultToken, missingFieldValues, parseAutonumberFormat, renderAutonumber, resolveAutonumberFormat } from '@objectstack/spec/data'; import type { EngineQueryOptions } from '@objectstack/spec/data'; import type { IDataEngine } from '@objectstack/core'; +import { withoutVouchedReferences } from '@objectstack/core'; import { isUniqueViolationError, keysetWalk, uniqueViolationColumn } from '@objectstack/types'; import type { KeysetPageQuery } from '@objectstack/types'; import type { ExecutionContext } from '@objectstack/spec/kernel'; @@ -202,7 +203,11 @@ export class DefaultsAndAutonumbersFacet { * is supplied (e.g. system-triggered hooks). */ static buildHookApi(this: Pick, execCtx?: ExecutionContext): ScopedContext { - const safeCtx: ExecutionContext = execCtx ?? ({ isSystem: true } as any); + // A reference a service verb vouched for on the write that fired this hook + // is spent on that write: no context a hook writes through carries it + // (`withoutVouchedReferences`, @objectstack/core). A context with no mark + // is handed through as the same object. + const safeCtx: ExecutionContext = withoutVouchedReferences(execCtx ?? ({ isSystem: true } as any)); return new ScopedContext(safeCtx, this.facade as unknown as IDataEngine); } diff --git a/packages/objectql/src/engine-unreadable-reference-target.test.ts b/packages/objectql/src/engine-unreadable-reference-target.test.ts index 7a2299eee80..30d50c6094e 100644 --- a/packages/objectql/src/engine-unreadable-reference-target.test.ts +++ b/packages/objectql/src/engine-unreadable-reference-target.test.ts @@ -562,6 +562,55 @@ describe('a reference a service verb vouches for on its own write: that one valu expect(items()).toEqual([]); }); + describe('the mark is spent on the vouched write: a hook that runs as the user under it is judged as any write is', () => { + /** + * An afterInsert hook on the task that, under the vouched insert, inserts + * a second task naming the SAME vouched id in the SAME field — through the + * api it is handed (the triggering user's context), or through + * `withRunAs('user')`, as an app-authored `runAs: 'user'` hook does. Its + * refusal is captured, and the outer write is left to finish. + */ + const armEchoHook = (via: 'api' | 'runAs-user') => { + const refusals: unknown[] = []; + engine.registerHook('afterInsert', async (ctx: any) => { + if (ctx.input?.data?.title !== 'outer') return; + const api = via === 'api' ? ctx.api : ctx.api.withRunAs('user', { hook: 'echo', object: ITEM, event: 'afterInsert' }); + try { + await api.object(ITEM).insert({ title: 'echo', duty: UNREADABLE }); + } catch (err) { + refusals.push(err); + } + }, { object: ITEM }); + return refusals; + }; + + for (const via of ['api', 'runAs-user'] as const) { + it(`a hook writing through ${via === 'api' ? 'the api it is handed' : "withRunAs('user')"} that names the vouched id again is refused reference_not_found`, async () => { + const refusals = armEchoHook(via); + await insert(ITEM, { title: 'outer', duty: UNREADABLE }, vouched(UNREADABLE)); + expect(refusals).toHaveLength(1); + expect(envelopeOf(refusals[0])).toEqual(notFound('duty')); + // The verb's own write landed; the hook's did not. + expect(items().map((r) => r.title)).toEqual(['outer']); + }); + } + + it('control: the same hook under an unmarked system write lands its echo (the hook\'s refusal above is the mark being spent, not the hook failing)', async () => { + const refusals: unknown[] = []; + engine.registerHook('afterInsert', async (ctx: any) => { + if (ctx.input?.data?.title !== 'outer') return; + try { + await ctx.api.object(ITEM).insert({ title: 'echo', duty: UNREADABLE }); + } catch (err) { + refusals.push(err); + } + }, { object: ITEM }); + await engine.insert(ITEM, { title: 'outer', duty: UNREADABLE }, { context: { isSystem: true } }); + expect(refusals).toEqual([]); + expect(items().map((r) => r.title).sort()).toEqual(['echo', 'outer']); + }); + }); + it('control: a caller whose read of the target object is refused outright is admitted for the vouched value only', async () => { await insert(ITEM, { duty: 't_mine' }, vouched('t_mine', 'duty', ITEM, CAROL)); expect(items().some((r) => r.duty === 't_mine')).toBe(true); diff --git a/packages/qa/dogfood/test/attachments-browser-attach.dogfood.test.ts b/packages/qa/dogfood/test/attachments-browser-attach.dogfood.test.ts new file mode 100644 index 00000000000..b99805f625d --- /dev/null +++ b/packages/qa/dogfood/test/attachments-browser-attach.dogfood.test.ts @@ -0,0 +1,225 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The console Attachments panel's flow, at the HTTP level, on the showcase +// boot: presigned upload → raw PUT → the upload-complete door with `attachTo`, +// which commits the file and writes its `sys_attachment` row as one unit, as +// the signed-in member (`FileAttachmentService.attachUploadAs`). +// +// The member holds what a real app grants a member who may attach files: the +// app's own `showcase_contributor` (they read and edit their own invoices) and +// a set with `sys_attachment` read and create, and nothing on `sys_file`. A +// `sys_attachment` create through the generic data door names `file_id` as +// any `lookup` value, which the caller must be able to read, so that member's +// two-step attach (complete, then `POST /data/sys_attachment`) is refused +// `reference_not_found`. The attach on completion vouches for the one upload +// it commits, and is what the panel calls. +// +// Controls: another user's upload id, an id that names no file, a record the +// member may not attach to, the generic door's direct create (still refused), +// and a member whose set reads every object (`'*'`), for whom both flows work. +// +// `showcase_invoice` declares `enable.files`; its select RLS +// (`owner == current_user.email`) hides another user's invoice from a +// contributor. + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { VerifyStack } from '@objectstack/verify'; +import type { IDataEngine } from '@objectstack/spec/contracts'; +import { StorageServicePlugin } from '@objectstack/service-storage'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { SecurityPlugin, appSecurityPluginOptions, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootShowcase } from './showcase-boot.js'; + +const SYS = { isSystem: true } as const; +const BYTES = 'hello'; + +/** The grant an app ships to let members attach files: `sys_attachment` only, nothing on `sys_file`. */ +const attacherSet = PermissionSetSchema.parse({ + name: 'browser_attach_dogfood_attacher', + label: 'Browser attach dogfood: attach files', + objects: { sys_attachment: { allowRead: true, allowCreate: true } }, +}); +/** A set that reads every object, `sys_file` included — the posture the permission-matrix fixture grants. */ +const wildcardReaderSet = PermissionSetSchema.parse({ + name: 'browser_attach_dogfood_wildcard_reader', + label: 'Browser attach dogfood: read everything', + objects: { '*': { allowRead: true }, sys_attachment: { allowRead: true, allowCreate: true } }, +}); + +describe('the Attachments panel’s upload flow on a real showcase boot: the upload’s own completion attaches, as the member', () => { + let stack: VerifyStack; + let rootDir: string; + let ql: IDataEngine; + let attacherToken: string; + let otherToken: string; + let wildcardToken: string; + let attacherId: string; + let attacherInvoice: string; + let othersInvoice: string; + let wildcardInvoice: string; + + beforeAll(async () => { + rootDir = mkdtempSync(join(tmpdir(), 'browser-attach-dogfood-')); + stack = await bootShowcase({ + security: new SecurityPlugin({ + ...appSecurityPluginOptions(showcaseStack), + defaultPermissionSets: [...securityDefaultPermissionSets, attacherSet, wildcardReaderSet], + }), + extraPlugins: [ + new StorageServicePlugin({ adapter: 'local', local: { rootDir }, bindToSettings: false }), + new AuditPlugin(), + ], + }); + ql = stack.kernel.getService('objectql'); + const member = async (email: string, sets: string[]) => { + const token = await stack.signUp(email); + const user = await ql.findOne('sys_user', { where: { email } }, { context: SYS }); + for (const permission_set of sets) { + await ql.insert('sys_user_permission_set', { user_id: user!.id, permission_set }, { context: SYS }); + } + return { token, id: String(user!.id) }; + }; + const attacher = await member('browser-attacher@dogfood.test', ['showcase_contributor', attacherSet.name]); + attacherToken = attacher.token; + attacherId = attacher.id; + otherToken = (await member('browser-other@dogfood.test', ['showcase_contributor', attacherSet.name])).token; + wildcardToken = (await member('browser-wildcard@dogfood.test', ['showcase_contributor', wildcardReaderSet.name])).token; + const [seeded] = await ql.find('showcase_invoice', { limit: 1 }, { context: SYS }); + const invoice = async (name: string, owner: string) => + String((await ql.insert('showcase_invoice', { name, owner, account: seeded.account, status: 'draft' }, { context: SYS })).id); + attacherInvoice = await invoice('INV-BROWSER-ATTACHER', 'browser-attacher@dogfood.test'); + othersInvoice = await invoice('INV-BROWSER-OTHER', 'someone-else@dogfood.test'); + wildcardInvoice = await invoice('INV-BROWSER-WILDCARD', 'browser-wildcard@dogfood.test'); + }, 180_000); + + afterAll(async () => { + await stack?.stop(); + rmSync(rootDir, { recursive: true, force: true }); + }); + + const json = { 'Content-Type': 'application/json' }; + const bearer = (token: string) => ({ Authorization: `Bearer ${token}` }); + + /** Steps 1 and 2 of the panel's flow: presign in the attachments scope, then PUT the bytes. */ + async function presignAndPut(token: string): Promise { + const presign = await stack.api('/storage/upload/presigned', { + method: 'POST', + headers: { ...json, ...bearer(token) }, + body: JSON.stringify({ filename: 'notes.txt', mimeType: 'text/plain', size: BYTES.length, scope: 'attachments' }), + }); + expect(presign.status, 'presign').toBe(200); + const { data } = (await presign.json()) as any; + const put = await stack.raw(String(data.uploadUrl).replace(/^https?:\/\/[^/]+/, ''), { + method: 'PUT', + headers: data.headers ?? { 'content-type': 'text/plain' }, + body: BYTES, + }); + expect(put.status, 'raw PUT').toBeLessThan(300); + return String(data.fileId); + } + + /** Step 3: the upload-complete door, with or without `attachTo`. */ + async function complete(token: string, fileId: string, attachTo?: { parentObject: string; parentId: string }) { + const res = await stack.api('/storage/upload/complete', { + method: 'POST', + headers: { ...json, ...bearer(token) }, + body: JSON.stringify(attachTo ? { fileId, attachTo } : { fileId }), + }); + return { status: res.status, body: (await res.json()) as any }; + } + + const onInvoice = (parentId: string) => ({ parentObject: 'showcase_invoice', parentId }); + const fileRow = (id: string) => ql.findOne('sys_file', { where: { id } }, { context: SYS }) as Promise; + const linksOf = (fileId: string) => + ql.find('sys_attachment', { where: { file_id: fileId } }, { context: SYS }) as Promise; + + it('precondition: the member cannot read sys_file through the data door', async () => { + const res = await stack.apiAs(attacherToken, 'GET', '/data/sys_file'); + expect(res.status).toBe(403); + }); + + it('the member attaches end to end: presign 200, PUT, complete with attachTo 200 — one committed file, one attachment in their name', async () => { + const fileId = await presignAndPut(attacherToken); + const out = await complete(attacherToken, fileId, onInvoice(attacherInvoice)); + expect(out.status, JSON.stringify(out.body)).toBe(200); + expect(out.body.data).toMatchObject({ fileId, name: 'notes.txt', attachmentId: expect.any(String) }); + expect(await fileRow(fileId)).toMatchObject({ status: 'committed', scope: 'attachments', owner_id: attacherId }); + const links = await linksOf(fileId); + expect(links).toHaveLength(1); + expect(links[0]).toMatchObject({ + id: out.body.data.attachmentId, parent_object: 'showcase_invoice', parent_id: attacherInvoice, uploaded_by: attacherId, + }); + // The panel's list read sees it, and the member downloads it. + const list = await stack.apiAs(attacherToken, 'GET', `/data/sys_attachment?parent_id=${attacherInvoice}`); + expect(list.status).toBe(200); + expect(JSON.stringify(await list.json())).toContain(out.body.data.attachmentId); + const url = await stack.api(`/storage/files/${fileId}/url`, { headers: bearer(attacherToken) }); + expect(url.status).toBe(200); + // No wider read: sys_file stays refused to them. + expect((await stack.apiAs(attacherToken, 'GET', `/data/sys_file/${fileId}`)).status).toBe(403); + }); + + it('control: the generic data door keeps its rule — the member’s two-step attach naming their own completed upload is refused reference_not_found', async () => { + const fileId = await presignAndPut(attacherToken); + expect((await complete(attacherToken, fileId)).status).toBe(200); + const res = await stack.apiAs(attacherToken, 'POST', '/data/sys_attachment', { + parent_object: 'showcase_invoice', parent_id: attacherInvoice, file_id: fileId, file_name: 'notes.txt', + }); + expect(res.status).toBe(400); + const body = JSON.stringify(await res.json()); + expect(body).toContain('VALIDATION_FAILED'); + expect(body).toContain('reference_not_found'); + expect(await linksOf(fileId)).toEqual([]); + }); + + it('control: another user’s upload id is refused, and their upload stays pending and unattached', async () => { + const othersFile = await presignAndPut(otherToken); + const out = await complete(attacherToken, othersFile, onInvoice(attacherInvoice)); + expect([out.status, out.body.error?.code]).toEqual([403, 'PERMISSION_DENIED']); + expect((await fileRow(othersFile)).status).toBe('pending'); + expect(await linksOf(othersFile)).toEqual([]); + }); + + it('control: an id that names no file is refused FILE_NOT_FOUND / 404', async () => { + const out = await complete(attacherToken, 'no-such-upload', onInvoice(attacherInvoice)); + expect([out.status, out.body.error?.code]).toEqual([404, 'FILE_NOT_FOUND']); + }); + + it('control: a record the member may not attach to (another user’s invoice) is refused, and the upload stays pending and unattached', async () => { + const fileId = await presignAndPut(attacherToken); + const out = await complete(attacherToken, fileId, onInvoice(othersInvoice)); + expect(out.status).toBeGreaterThanOrEqual(403); + expect(out.status).toBeLessThan(500); + expect(['ATTACHMENT_PARENT_ACCESS', 'RECORD_NOT_FOUND']).toContain(out.body.error?.code); + expect((await fileRow(fileId)).status).toBe('pending'); + expect(await linksOf(fileId)).toEqual([]); + }); + + it('control: an upload already attached cannot be attached again on completion', async () => { + const fileId = await presignAndPut(attacherToken); + expect((await complete(attacherToken, fileId, onInvoice(attacherInvoice))).status).toBe(200); + const again = await complete(attacherToken, fileId, onInvoice(attacherInvoice)); + expect([again.status, again.body.error?.code]).toEqual([409, 'RESOURCE_CONFLICT']); + expect(await linksOf(fileId)).toHaveLength(1); + }); + + it('control: a member whose set reads every object keeps working — through the attach on completion and through the generic two-step', async () => { + const viaComplete = await presignAndPut(wildcardToken); + const out = await complete(wildcardToken, viaComplete, onInvoice(wildcardInvoice)); + expect(out.status, JSON.stringify(out.body)).toBe(200); + expect(await linksOf(viaComplete)).toHaveLength(1); + + const viaDataDoor = await presignAndPut(wildcardToken); + expect((await complete(wildcardToken, viaDataDoor)).status).toBe(200); + const res = await stack.apiAs(wildcardToken, 'POST', '/data/sys_attachment', { + parent_object: 'showcase_invoice', parent_id: wildcardInvoice, file_id: viaDataDoor, file_name: 'notes.txt', + }); + expect(res.status, JSON.stringify(await res.clone().json())).toBeLessThan(300); + expect(await linksOf(viaDataDoor)).toHaveLength(1); + }); +}); diff --git a/packages/services/service-storage/README.md b/packages/services/service-storage/README.md index b40cee4e39d..386ce4d1392 100644 --- a/packages/services/service-storage/README.md +++ b/packages/services/service-storage/README.md @@ -129,7 +129,7 @@ All routes are mounted at `/api/v1/storage` (configurable via `basePath`). | Method | Path | Description | |--------|------|-------------| | POST | `/upload/presigned` | Get presigned upload URL | -| POST | `/upload/complete` | Mark upload as committed | +| POST | `/upload/complete` | Mark upload as committed; with `attachTo: { parentObject, parentId }`, attach the caller's own pending upload to that record as the caller, in the same unit | | POST | `/upload/chunked` | Initiate chunked upload | | PUT | `/upload/chunked/:uploadId/chunk/:chunkIndex` | Upload a chunk | | POST | `/upload/chunked/:uploadId/complete` | Complete chunked upload | diff --git a/packages/services/service-storage/src/file-attachment-service.test.ts b/packages/services/service-storage/src/file-attachment-service.test.ts index e57891523c1..86c6706fde5 100644 --- a/packages/services/service-storage/src/file-attachment-service.test.ts +++ b/packages/services/service-storage/src/file-attachment-service.test.ts @@ -638,3 +638,187 @@ describe('attachFileAs for a member who cannot read sys_file: the verb vouches f expect(await rows(ql, 'sys_attachment')).toHaveLength(1); }); }); + +describe('attachUploadAs: a member attaches their own pending browser upload, and the commit is the attach', () => { + // The same member posture as above: the create grant on `sys_attachment`, + // nothing on `sys_file` (a non-system read of it is refused outright), and + // Bob cannot read the parent object. The upload is seeded as the presigned + // door leaves one: a `pending` row owned by the uploader in the + // `attachments` scope, and its bytes at its key. + const ALICE = { userId: 'usr_alice', tenantId: 'org_a', positions: ['org_member'] }; + const BOB = { userId: 'usr_bob', tenantId: 'org_a', positions: ['org_member'] }; + const asCaller = (a: IFileAttachmentService) => a as IFileAttachmentService & ICallerFileAttachmentService; + const READS = new Set(['find', 'findOne', 'count', 'aggregate']); + const readRefusal = (object: string) => + Object.assign(new Error(`[Security] Access denied: read on ${object}`), { + name: 'PermissionDeniedError', code: 'PERMISSION_DENIED', status: 403, statusCode: 403, + }); + + async function bootMember(opts: { sharing?: unknown; security?: unknown } = {}) { + const booted = await boot(opts); + booted.ql.registerMiddleware(async (ctx: any, next: () => Promise) => { + if (READS.has(ctx.operation) && ctx.context?.isSystem !== true) { + if (ctx.object === 'sys_file') throw readRefusal('sys_file'); + if (ctx.object === 'att_case' && ctx.context?.userId === BOB.userId) throw readRefusal('att_case'); + } + await next(); + }); + return booted; + } + + const BYTES = Buffer.from('hello'); + /** A pending upload, as the presigned door and the raw PUT leave one. */ + async function seedUpload( + ql: ObjectQL, + storage: IStorageService, + over: { id?: string; owner?: string; scope?: string; status?: string; bytes?: boolean } = {}, + ) { + const id = over.id ?? 'upl_alice'; + const scope = over.scope ?? 'attachments'; + const key = `${scope}/${id}.txt`; + await ql.insert('sys_file', { + id, key, name: 'notes.txt', mime_type: 'text/plain', size: BYTES.byteLength, scope, acl: 'private', + status: over.status ?? 'pending', owner_id: over.owner ?? ALICE.userId, organization_id: 'org_a', + }, SYS); + if (over.bytes !== false) await storage.upload(key, BYTES, { contentType: 'text/plain' }); + return id; + } + const target = (fileId: string, over: Record = {}) => ({ fileId, parentObject: 'att_case', parentId: 'case_a', ...over }); + const answer = (err: { code?: string; status?: number; message?: string }) => ({ code: err.code, status: err.status }); + const fileRow = async (ql: ObjectQL, id: string) => (await rows(ql, 'sys_file')).find((f) => f.id === id); + + /** A refused attach leaves the upload as it was: pending, bytes stored, nothing attached. */ + async function expectUploadUntouched(ql: ObjectQL, storage: IStorageService, id: string) { + const file = await fileRow(ql, id); + expect(file?.status).toBe('pending'); + expect(await storedKeys(storage)).toContain(file!.key); + expect(await rows(ql, 'sys_attachment')).toEqual([]); + } + + it('attaches as the member: the upload is committed and attached in one call, uploaded_by them, and sys_file stays unreadable to them', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage); + const out = await asCaller(attachments).attachUploadAs(target(fileId, { eTag: 'etag-1' }), ALICE as never); + expect(out).toEqual({ fileId, attachmentId: expect.any(String), size: BYTES.byteLength }); + expect(await fileRow(ql, fileId)).toMatchObject({ status: 'committed', etag: 'etag-1', owner_id: ALICE.userId }); + const links = await rows(ql, 'sys_attachment'); + expect(links).toHaveLength(1); + expect(links[0]).toMatchObject({ + id: out.attachmentId, parent_object: 'att_case', parent_id: 'case_a', file_id: fileId, + file_name: 'notes.txt', uploaded_by: ALICE.userId, organization_id: 'org_a', + }); + const err = await refusalOf(ql.findOne('sys_file', { where: { id: fileId }, context: ALICE })); + expect(answer(err)).toEqual({ code: 'PERMISSION_DENIED', status: 403 }); + }); + + it('control: another user’s pending upload is refused with the upload ownership rule’s one answer, and is left as it was', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage, { id: 'upl_bob', owner: BOB.userId }); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'PERMISSION_DENIED', status: 403 }); + await expectUploadUntouched(ql, storage, fileId); + }); + + it('control: an id that names no file gets exactly the answer another user’s upload gets', async () => { + const { ql, attachments, storage } = await bootMember(); + const others = await seedUpload(ql, storage, { id: 'upl_bob', owner: BOB.userId }); + const a = await refusalOf(asCaller(attachments).attachUploadAs(target(others), ALICE as never)); + const b = await refusalOf(asCaller(attachments).attachUploadAs(target('upl_nowhere'), ALICE as never)); + expect([b.code, b.status, (b as Error).message]).toEqual([a.code, a.status, (a as Error).message]); + expect(await rows(ql, 'sys_attachment')).toEqual([]); + }); + + it('control: a committed upload is refused RESOURCE_CONFLICT / 409 — only the upload this call commits is vouched for', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage, { status: 'committed' }); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'RESOURCE_CONFLICT', status: 409 }); + expect(await rows(ql, 'sys_attachment')).toEqual([]); + }); + + it('control: a pending upload something already holds is refused RESOURCE_CONFLICT / 409, and no second attachment is written', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage); + await ql.insert('sys_attachment', { parent_object: 'att_case', parent_id: 'case_a', file_id: fileId, file_name: 'n.txt', organization_id: 'org_a' }, SYS); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'RESOURCE_CONFLICT', status: 409 }); + expect(await rows(ql, 'sys_attachment')).toHaveLength(1); + expect((await fileRow(ql, fileId))?.status).toBe('pending'); + }); + + it('control: an upload outside the attachments scope is refused INVALID_REQUEST / 400', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage, { scope: 'user' }); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'INVALID_REQUEST', status: 400 }); + expect(await rows(ql, 'sys_attachment')).toEqual([]); + }); + + it('control: an upload whose bytes the backend does not hold is refused RESOURCE_CONFLICT / 409 and stays pending', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage, { bytes: false }); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'RESOURCE_CONFLICT', status: 409 }); + expect((await fileRow(ql, fileId))?.status).toBe('pending'); + expect(await rows(ql, 'sys_attachment')).toEqual([]); + }); + + it('control: a member the attach rule refuses is refused ATTACHMENT_PARENT_ACCESS / 403, and the upload is left as it was', async () => { + const sharing = { checkEdit: vi.fn(async () => 'deny') }; + const { ql, attachments, storage } = await bootMember({ sharing }); + const fileId = await seedUpload(ql, storage); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 }); + await expectUploadUntouched(ql, storage, fileId); + }); + + it('control: a member without the permission to create an attachment is refused PERMISSION_DENIED / 403, and the upload is left as it was', async () => { + const security = { explain: vi.fn(async () => ({ allowed: false })) }; + const { ql, attachments, storage } = await bootMember({ security }); + const fileId = await seedUpload(ql, storage); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), ALICE as never)); + expect(answer(err)).toEqual({ code: 'PERMISSION_DENIED', status: 403 }); + expect(security.explain).toHaveBeenCalledWith({ object: 'sys_attachment', operation: 'create' }, ALICE); + await expectUploadUntouched(ql, storage, fileId); + }); + + it('control: a parent object without enable.files is refused FILES_DISABLED / 403 by the insert’s own gate, and the upload is left as it was', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId, { parentObject: 'att_memo', parentId: 'memo_a' }), ALICE as never)); + expect(answer(err)).toEqual({ code: 'FILES_DISABLED', status: 403 }); + await expectUploadUntouched(ql, storage, fileId); + }); + + it('control: a parent the member cannot read is refused, and the upload is left as it was', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage, { id: 'upl_bob', owner: BOB.userId }); + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), BOB as never)); + expect(err.status).toBe(err.code === 'RECORD_NOT_FOUND' ? 404 : 403); + expect(['RECORD_NOT_FOUND', 'ATTACHMENT_PARENT_ACCESS']).toContain(err.code); + await expectUploadUntouched(ql, storage, fileId); + }); + + it('the vouch is the verb’s call alone: after attachUploadAs, the member’s own insert naming the same file is refused reference_not_found', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage); + await asCaller(attachments).attachUploadAs(target(fileId), ALICE as never); + const err = await refusalOf( + ql.insert('sys_attachment', { parent_object: 'att_case', parent_id: 'case_a', file_id: fileId, file_name: 'direct.txt' }, { context: ALICE }), + ); + const envelope = resolveThrownHttpError(err); + expect([envelope.code, envelope.status]).toEqual(['VALIDATION_FAILED', 400]); + expect(((err as any).fields ?? []).map((f: any) => [f.field, f.code])).toEqual([['file_id', 'reference_not_found']]); + expect(await rows(ql, 'sys_attachment')).toHaveLength(1); + }); + + it('control: the system context and a context without a user are refused PERMISSION_DENIED / 403', async () => { + const { ql, attachments, storage } = await bootMember(); + const fileId = await seedUpload(ql, storage); + for (const context of [{ isSystem: true }, { tenantId: 'org_a' }]) { + const err = await refusalOf(asCaller(attachments).attachUploadAs(target(fileId), context as never)); + expect(answer(err)).toEqual({ code: 'PERMISSION_DENIED', status: 403 }); + } + await expectUploadUntouched(ql, storage, fileId); + }); +}); diff --git a/packages/services/service-storage/src/file-attachment-service.ts b/packages/services/service-storage/src/file-attachment-service.ts index c8ea9362d3a..24e738f31e2 100644 --- a/packages/services/service-storage/src/file-attachment-service.ts +++ b/packages/services/service-storage/src/file-attachment-service.ts @@ -15,7 +15,13 @@ import type { ExecutionContext } from '@objectstack/spec/kernel'; // The one reference this verb's caller-scoped insert vouches for: the file it // created in the same call (see step ⑤). import { vouchForReference } from '@objectstack/core'; -import { StorageMetadataStore, type StorageWriteContext } from './metadata-store.js'; +import { StorageMetadataStore, organizationOutOfWriteReach, type StorageWriteContext } from './metadata-store.js'; +// The upload ownership rule, asked of the upload the attach-on-commit verb +// names — the commit door's own rule and its one refusal. +import { isFileUploader, NOT_UPLOADER_CODE, NOT_UPLOADER_MESSAGE, NOT_UPLOADER_STATUS } from './upload-ownership.js'; +// The one definition of "is anything holding this file?" — the reap guard's — +// asked so the attach-on-commit verb vouches only for an upload nothing holds. +import { findFileHolder } from './attachment-lifecycle.js'; // The attach rule's one verdict (parent EDIT), asked here before a producer // spends work on a file it could not attach. import { @@ -35,8 +41,9 @@ import { UPLOAD_TOO_LARGE_STATUS, type StorageLimitsSnapshot, } from './storage-limits.js'; -// The upload doors' own storage-key layout. -import { buildKey } from './storage-routes.js'; +// The upload doors' own storage-key layout, and the commit door's question +// of whether the backend holds an upload's bytes. +import { buildKey, heldBytesRefusal } from './storage-routes.js'; /** * [#23306] The server-side attach verb — `IFileAttachmentService`, registered @@ -157,6 +164,46 @@ export interface ICallerFileAttachmentService { * `403`: the system has `attachFile`. */ attachFileAs(input: AttachFileInput, context: ExecutionContext): Promise; + /** + * Attach the caller's own browser upload to a record, and commit it, in one + * unit, as `context`'s user: the attach verb of the record attachments + * surface, asked by the upload-complete door when its request carries + * `attachTo` (`POST /api/v1/storage/upload/complete`). + * + * The file must be the upload the caller is completing: a `sys_file` row + * whose `owner_id` is the caller (the upload doors stamp it from the + * session), still `pending`, in the `attachments` scope, in the caller's + * organization, held by nothing yet (no `sys_attachment` names it and no + * record field owns it — `findFileHolder`), with its bytes held by the + * storage backend. That is the whole premise of the vouch: the one file this + * call turns from the caller's unattached upload into an attachment, and + * nothing else. Any other id — + * another user's upload, an id that names no file — is refused with the + * upload ownership rule's one answer (`PERMISSION_DENIED` / `403`, the same + * body for both); a committed or already-held file `RESOURCE_CONFLICT` / + * `409`; another scope `INVALID_REQUEST` / `400`. + * + * Then the attach authorization, before any write: {@link assertMayAttach} + * (the permission to create an attachment, and the parent-EDIT attach rule), + * and the parent read as the caller (`RECORD_NOT_FOUND` / `404`, as + * {@link attachFileAs} answers). The `sys_attachment` insert runs as the + * caller, so every gate on it judges the caller, and it vouches for this + * one file id and for nothing else (`vouchForReference`, `@objectstack/core`). + * A refusal leaves the upload `pending`, its bytes stored, and no attachment. + */ + attachUploadAs(input: AttachUploadInput, context: ExecutionContext): Promise; +} + +/** What {@link ICallerFileAttachmentService.attachUploadAs} attaches: an upload, and the record it is attached to. */ +export interface AttachUploadInput { + /** The `sys_file` id the upload-start door returned to the caller. */ + fileId: string; + /** The object of the record the file is attached to. */ + parentObject: string; + /** The id of the record the file is attached to. */ + parentId: string; + /** The ETag the client reports for the uploaded bytes, stored on the file as the commit door stores it. */ + eTag?: string; } /** The upload scope of every file this verb writes: the attachments surface. */ @@ -213,6 +260,17 @@ function validateAttachFileInput(input: AttachFileInput): AttachFileInput { return input; } +/** {@link validateAttachFileInput}'s counterpart for {@link AttachUploadInput}. */ +function validateAttachUploadInput(input: AttachUploadInput): AttachUploadInput { + const fail = (message: string): never => refuse('VALIDATION_ERROR', 400, `attachUploadAs: ${message}`); + if (!input || typeof input !== 'object') fail('the input must be an object'); + for (const field of ['fileId', 'parentObject', 'parentId'] as const) { + if (!nonEmptyString(input[field])) fail(`'${field}' must be a non-empty string`); + } + if (input.eTag !== undefined && typeof input.eTag !== 'string') fail("'eTag' must be a string when it is given"); + return input; +} + export class FileAttachmentService implements IFileAttachmentService, ICallerFileAttachmentService { constructor(private readonly deps: FileAttachmentServiceDeps) {} @@ -224,6 +282,127 @@ export class FileAttachmentService implements IFileAttachmentService, ICallerFil return this.attach(input, requireUserContext(context)); } + async attachUploadAs(input: AttachUploadInput, context: ExecutionContext): Promise { + const caller = requireUserContext(context); + const request = validateAttachUploadInput(input); + const engine = this.deps.getEngine(); + const storage = this.deps.getStorage(); + if (!engine || !storage) refuse('SERVICE_UNAVAILABLE', 503, NOT_READY_MESSAGE); + const store = new StorageMetadataStore(engine); + + // ① The upload — the caller's own, as the store stamped it. Read as the + // system, by id, as the commit door reads it; nothing about the row is + // answered before the ownership rule has passed, and a missing row gets + // the same answer as another user's. + const file = await store.getFile(request.fileId); + if (!file || !isFileUploader(caller.userId, file)) { + refuse(NOT_UPLOADER_CODE, NOT_UPLOADER_STATUS, NOT_UPLOADER_MESSAGE); + } + if (file.status !== 'pending') { + refuse( + 'RESOURCE_CONFLICT', + 409, + 'This upload is already completed, so it cannot be attached on completion. An upload is attached in the ' + + 'call that completes it: start a new upload and complete it with attachTo.', + ); + } + if (file.scope !== ATTACHMENTS_SCOPE) { + refuse( + 'INVALID_REQUEST', + 400, + `This upload was started in the '${String(file.scope ?? 'user')}' scope, and only an upload in the ` + + `'${ATTACHMENTS_SCOPE}' scope is attached to a record. Start the upload with scope '${ATTACHMENTS_SCOPE}'.`, + ); + } + const callerOrganization = nonEmptyString(caller.tenantId) ? caller.tenantId : null; + if (organizationOutOfWriteReach(store, file, { organizationId: callerOrganization }) !== null) { + refuse( + 'PERMISSION_DENIED', + 403, + 'attachUploadAs: this upload was started in another organization than the one the caller acts in; ' + + 'switch back to that organization to attach it.', + ); + } + if ((await findFileHolder(engine, file.id, file as unknown as Record)) !== null) { + refuse( + 'RESOURCE_CONFLICT', + 409, + 'This upload is already attached to a record or owned by a record field, so it cannot be attached on ' + + 'completion. Start a new upload and complete it with attachTo.', + ); + } + const notHeld = await heldBytesRefusal(storage, file); + if (notHeld !== null) refuse('RESOURCE_CONFLICT', 409, notHeld); + + // ② The attach authorization, before any write: the permission to create + // an attachment and the attach rule (parent EDIT), then the parent read as + // the caller and the organization the attachment is written in. + await this.assertMayAttach(request.parentObject, request.parentId, caller); + const organizationId = await this.organizationOfParent( + engine, + { parentObject: request.parentObject, parentId: request.parentId, organizationId: callerOrganization }, + caller, + ); + const fileOrganization = nonEmptyString(file.organization_id) ? file.organization_id : null; + if (fileOrganization !== null && fileOrganization !== organizationId) { + refuse( + 'RECORD_NOT_FOUND', + 404, + `Cannot attach a file to ${request.parentObject}/${request.parentId}: the record does not exist in the ` + + 'organization the upload was started in.', + ); + } + const writeContext: StorageWriteContext = { organizationId }; + + // ③ The attachment and the commit — one unit, as the caller. + let attachmentId: string | undefined; + let ownedUnit = false; + try { + await this.inOneUnit(engine, organizationId, caller, async (unitContext, owned) => { + ownedUnit = owned; + const row = (await engine.insert( + 'sys_attachment', + { + parent_object: request.parentObject, + parent_id: request.parentId, + file_id: file.id, + file_name: file.name, + mime_type: file.mime_type, + size: file.size, + }, + // As in `attachFileAs`: the call vouches for the one file it judged + // in ① — the caller's own pending upload, which this call commits — + // and for no other value. The write-time reference check does not + // ask the caller's read door for that id, and still probes that it + // exists; every other gate on the insert judges the caller. + { context: vouchForReference(unitContext as ExecutionContext, { object: 'sys_attachment', field: 'file_id', id: file.id }) }, + )) as { id?: unknown } | null | undefined; + if (row?.id === undefined || row.id === null || row.id === '') { + throw new Error('attachUploadAs: the sys_attachment insert answered no row id, so the attachment cannot be named'); + } + attachmentId = String(row.id); + const committed = await store.updateFile( + file.id, + { status: 'committed', ...(request.eTag !== undefined ? { etag: request.eTag } : {}) }, + writeContext, + ); + if (!committed) { + throw new Error(`attachUploadAs: the sys_file row ${file.id} was gone before it could be committed`); + } + }); + } catch (err) { + // An owned transaction rolled the attachment back with the commit. A + // unit this call does not own keeps an inserted row, so it is removed + // here. The upload itself is the caller's and stays as it was — + // `pending`, its bytes stored — so completing again can still commit it. + if (!ownedUnit && attachmentId !== undefined) { + await this.removeAttachment(engine, attachmentId, file.id, writeContext); + } + throw err; + } + return { fileId: file.id, attachmentId: attachmentId!, size: Number(file.size ?? 0) }; + } + async assertMayAttach(parentObject: string, parentId: string, context: ExecutionContext): Promise { const caller = requireUserContext(context); const engine = this.deps.getEngine(); @@ -393,7 +572,7 @@ export class FileAttachmentService implements IFileAttachmentService, ICallerFil */ private async organizationOfParent( engine: FileAttachmentEngine, - request: AttachFileInput, + request: Pick, caller: ExecutionContext | null, ): Promise { let parent: Record | null; @@ -453,6 +632,31 @@ export class FileAttachmentService implements IFileAttachmentService, ICallerFil await engine.transaction((trxCtx, info) => work(trxCtx as Record, info?.owned === true), base); } + /** + * Remove the `sys_attachment` row a refused call inserted outside a + * transaction it owns. Best-effort: the call's refusal is what is thrown. + */ + private async removeAttachment( + engine: FileAttachmentEngine, + attachmentId: string, + fileId: string, + writeContext: StorageWriteContext, + ): Promise { + try { + const tenant = writeContext.organizationId; + await engine.delete('sys_attachment', { + where: { id: attachmentId }, + context: tenant ? { isSystem: true, tenantId: tenant } : { isSystem: true }, + }); + } catch (err) { + this.deps.logger.warn( + `[storage] attachFile: a refused call could not remove its sys_attachment row ${attachmentId} ` + + `(${(err as Error)?.message ?? err}). It points at the pending sys_file ${fileId}; delete the ` + + 'attachment row by hand.', + ); + } + } + /** * Undo a refused call: the attachment row it still holds (if any), then the * bytes, then the pending row. Best-effort, and the caller's refusal is what @@ -468,19 +672,7 @@ export class FileAttachmentService implements IFileAttachmentService, ICallerFil ): Promise { const { logger } = this.deps; if (call.attachmentId !== undefined) { - try { - const tenant = call.writeContext.organizationId; - await engine.delete('sys_attachment', { - where: { id: call.attachmentId }, - context: tenant ? { isSystem: true, tenantId: tenant } : { isSystem: true }, - }); - } catch (err) { - logger.warn( - `[storage] attachFile: a refused call could not remove its sys_attachment row ${call.attachmentId} ` + - `(${(err as Error)?.message ?? err}). It points at the pending sys_file ${call.fileId}; delete the ` + - 'attachment row by hand.', - ); - } + await this.removeAttachment(engine, call.attachmentId, call.fileId, call.writeContext); } try { await storage.delete(call.key); diff --git a/packages/services/service-storage/src/storage-route-ledger.ts b/packages/services/service-storage/src/storage-route-ledger.ts index 7857e81c281..91a5eeb1671 100644 --- a/packages/services/service-storage/src/storage-route-ledger.ts +++ b/packages/services/service-storage/src/storage-route-ledger.ts @@ -99,7 +99,7 @@ export const STORAGE_ROUTE_LEDGER: readonly StorageRouteLedgerEntry[] = [ { route: 'POST /api/v1/storage/upload/presigned', family: 'upload-presigned', disposition: 'sdk', client: 'storage.getPresignedUrl', note: 'also step 1 of the composite storage.upload' }, { route: 'POST /api/v1/storage/upload/complete', family: 'upload-presigned', disposition: 'sdk', client: 'storage.upload', - note: 'step 3 of the composite upload; no standalone client method mints the commit on its own' }, + note: 'step 3 of the composite upload; no standalone client method mints the commit on its own. With attachTo it also attaches the upload to a record as the caller (the console attachments panel); the SDK sends none' }, // ── chunked / resumable upload ──────────────────────────────────────────── { route: 'POST /api/v1/storage/upload/chunked', family: 'upload-chunked', disposition: 'sdk', client: 'storage.initChunkedUpload' }, diff --git a/packages/services/service-storage/src/storage-routes.complete-attach.test.ts b/packages/services/service-storage/src/storage-routes.complete-attach.test.ts new file mode 100644 index 00000000000..04548f39bf5 --- /dev/null +++ b/packages/services/service-storage/src/storage-routes.complete-attach.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The commit door's `attachTo` arm (`POST …/upload/complete`): the record +// attachments surface's attach verb. With `attachTo`, the door hands the +// caller's own upload to the attacher `composeStorageRoutes` binds (the +// file-attachments service's `attachUploadAs`, as the caller) instead of +// committing it itself, and answers the committed file with its new +// `attachmentId`. +// +// Pinned here at the door: every gate the door already asked runs FIRST — +// the session, a malformed `attachTo`, a file that does not exist, the upload +// ownership rule, the bytes — and a refused request never reaches the +// attacher; the attacher's declared refusals are relayed with their code, +// status and details; a mount with no attacher bound refuses `attachTo` with +// `501`; and a completion without `attachTo` is unchanged. The attach verb's +// own checks and its one-file vouch are pinned in +// `file-attachment-service.test.ts`, and the whole flow on a real boot in the +// `attachments-browser-attach` dogfood. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { LocalStorageAdapter } from './local-storage-adapter.js'; +import { StorageMetadataStore } from './metadata-store.js'; +import { + registerStorageFamily, + registerStorageRoutes, + type StorageUploadAttacher, + type StorageUploadSession, +} from './storage-routes.js'; + +const BASE = '/api/v1/storage'; +const COMMIT = `${BASE}/upload/complete`; + +type Res = IHttpResponse & { _status: number; _json: any }; + +function createMockHttpServer() { + const routes = new Map(); + return { + get: vi.fn((path: string, handler: RouteHandler) => { routes.set(`GET:${path}`, handler); }), + post: vi.fn((path: string, handler: RouteHandler) => { routes.set(`POST:${path}`, handler); }), + put: vi.fn((path: string, handler: RouteHandler) => { routes.set(`PUT:${path}`, handler); }), + delete: vi.fn(), + patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + handler(method: string, path: string): RouteHandler { + const h = routes.get(`${method}:${path}`); + if (!h) throw new Error(`no route ${method} ${path}`); + return h; + }, + }; +} + +function createMockRes(): Res { + const res: any = { + _status: 200, + _json: null, + json(data: any) { res._json = data; }, + send(data: any) { res._sent = data; }, + status(code: number) { res._status = code; return res; }, + header() { return res; }, + }; + return res; +} + +const ALICE: StorageUploadSession = { userId: 'usr_alice' }; +const BOB: StorageUploadSession = { userId: 'usr_bob' }; +const BYTES = Buffer.from('hello'); + +let rootDir: string; +let storage: LocalStorageAdapter; +let store: StorageMetadataStore; +let server: ReturnType; +let attacher: ReturnType; + +/** A session per request, read off a test header — the door's `resolveSession`. */ +const resolveSession = async (req: { headers?: unknown }) => { + const who = (req.headers as Record | undefined)?.['x-test-user']; + return who === 'alice' ? ALICE : who === 'bob' ? BOB : null; +}; + +function mount(attach: StorageUploadAttacher | undefined) { + server = createMockHttpServer(); + registerStorageFamily(server as any, storage, store, { basePath: BASE, resolveSession }, undefined, attach); +} + +/** A pending upload of Alice's, its bytes held — what the presigned door and the raw PUT leave. */ +async function seedUpload(over: { id?: string; owner?: string; bytes?: boolean } = {}) { + const id = over.id ?? 'upl_alice'; + const key = `attachments/${id}.txt`; + await store.createFile({ + id, key, name: 'notes.txt', mime_type: 'text/plain', size: BYTES.byteLength, scope: 'attachments', + acl: 'private', status: 'pending', owner_id: over.owner ?? ALICE.userId, + }); + if (over.bytes !== false) await storage.upload(key, BYTES, { contentType: 'text/plain' }); + return id; +} + +async function complete(who: 'alice' | 'bob' | null, body: Record): Promise { + const res = createMockRes(); + const req = { params: {}, query: {}, body, headers: who ? { 'x-test-user': who } : {}, method: 'POST', path: COMMIT } as unknown as IHttpRequest; + await server.handler('POST', COMMIT)(req, res); + return res; +} + +const ATTACH_TO = { parentObject: 'att_case', parentId: 'case_a' }; + +beforeEach(async () => { + rootDir = join(tmpdir(), `os-complete-attach-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + storage = new LocalStorageAdapter({ rootDir, signingSecret: 'complete-attach' }); + store = new StorageMetadataStore(null); + attacher = vi.fn(async () => ({ attachmentId: 'att_1' })); + mount(attacher as unknown as StorageUploadAttacher); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(rootDir, { recursive: true, force: true }); +}); + +describe('POST /storage/upload/complete with attachTo — the attach verb at the door', () => { + it('hands the caller’s own upload to the attacher, does not commit it itself, and answers the file with its attachmentId', async () => { + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, eTag: 'etag-1', attachTo: ATTACH_TO }); + expect(res._status, JSON.stringify(res._json)).toBe(200); + expect(attacher).toHaveBeenCalledTimes(1); + expect(attacher.mock.calls[0][1]).toEqual({ fileId, ...ATTACH_TO, eTag: 'etag-1' }); + expect(res._json).toMatchObject({ success: true, data: { fileId, name: 'notes.txt', attachmentId: 'att_1' } }); + // The commit is the attacher's (one unit with the insert): this stand-in + // committed nothing, and the door wrote nothing of its own. + expect((await store.getFile(fileId))?.status).toBe('pending'); + }); + + it('control: another user’s upload is refused by the upload ownership rule before the attacher is asked', async () => { + const fileId = await seedUpload({ owner: BOB.userId }); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([403, 'PERMISSION_DENIED']); + expect(attacher).not.toHaveBeenCalled(); + expect((await store.getFile(fileId))?.status).toBe('pending'); + }); + + it('control: an id that names no file is refused FILE_NOT_FOUND / 404 before the attacher is asked', async () => { + const res = await complete('alice', { fileId: 'upl_nowhere', attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([404, 'FILE_NOT_FOUND']); + expect(attacher).not.toHaveBeenCalled(); + }); + + it('control: an anonymous request is refused AUTH_REQUIRED / 401 before anything is read', async () => { + const fileId = await seedUpload(); + const res = await complete(null, { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([401, 'AUTH_REQUIRED']); + expect(attacher).not.toHaveBeenCalled(); + }); + + it('control: an upload whose bytes are not held is refused RESOURCE_CONFLICT / 409 before the attacher is asked', async () => { + const fileId = await seedUpload({ bytes: false }); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([409, 'RESOURCE_CONFLICT']); + expect(attacher).not.toHaveBeenCalled(); + }); + + for (const [what, attachTo] of [ + ['a missing parentId', { parentObject: 'att_case' }], + ['an empty parentObject', { parentObject: '', parentId: 'case_a' }], + ['a key attachTo does not declare', { ...ATTACH_TO, file_id: 'other' }], + ['an array', [ATTACH_TO]], + ['null', null], + ] as const) { + it(`control: attachTo with ${what} is refused INVALID_REQUEST / 400, and nothing is read, attached or committed`, async () => { + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, attachTo }); + expect([res._status, res._json?.error?.code]).toEqual([400, 'INVALID_REQUEST']); + expect(attacher).not.toHaveBeenCalled(); + expect((await store.getFile(fileId))?.status).toBe('pending'); + }); + } + + it('relays the attacher’s declared refusal with its code and status, and the upload is left pending', async () => { + attacher.mockRejectedValueOnce(Object.assign(new Error('Cannot attach to att_case/case_a'), { code: 'ATTACHMENT_PARENT_ACCESS', status: 403 })); + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([403, 'ATTACHMENT_PARENT_ACCESS']); + expect(res._json.error.message).toBe('Cannot attach to att_case/case_a'); + expect((await store.getFile(fileId))?.status).toBe('pending'); + }); + + it('relays a validation refusal from the insert as VALIDATION_FAILED / 400 with its field details', async () => { + const validation = Object.assign(new Error('file_id: reference not found'), { + name: 'ValidationError', + fields: [{ field: 'file_id', code: 'reference_not_found', message: 'reference not found' }], + }); + attacher.mockRejectedValueOnce(validation); + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([400, 'VALIDATION_FAILED']); + }); + + it('an undeclared fault is a 500 with the door’s generic text, never the thrown message', async () => { + attacher.mockRejectedValueOnce(new Error('engine said something about the host')); + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect(res._status).toBe(500); + expect(res._json.error.message).not.toContain('engine said'); + }); + + it('a mount with no attacher bound (the public registrar, a bare kernel) refuses attachTo NOT_IMPLEMENTED / 501 and commits nothing', async () => { + server = createMockHttpServer(); + registerStorageRoutes(server as any, storage, store, { basePath: BASE, resolveSession }); + const fileId = await seedUpload(); + const res = await complete('alice', { fileId, attachTo: ATTACH_TO }); + expect([res._status, res._json?.error?.code]).toEqual([501, 'NOT_IMPLEMENTED']); + expect((await store.getFile(fileId))?.status).toBe('pending'); + }); + + it('control: a completion without attachTo commits as before and never asks the attacher', async () => { + const fileId = await seedUpload(); + const res = await complete('alice', { fileId }); + expect(res._status).toBe(200); + expect(res._json.data.attachmentId).toBeUndefined(); + expect((await store.getFile(fileId))?.status).toBe('committed'); + expect(attacher).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 90cd9a7c352..8bacdb79bf9 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -12,6 +12,9 @@ import { InitiateChunkedUploadRequestSchema, UploadScopeSchema } from '@objectst // [#23174] A catch-all 5xx answers through `sendServerFault`: the route's // generic text on the wire, the caught error with its stack in the fault log. import { sendOk, sendError, sendServerFault } from '@objectstack/types'; +// The commit door's attach arm relays the attach verb's declared refusals as +// the print door relays its service's: the producer's code and status. +import { declaredRefusalMessage, demotedDeclaredCode, resolveThrownHttpError } from '@objectstack/types'; // [#15999] The BRAND predicate, never `instanceof` — this error crosses package // boundaries and a monorepo resolves the same module through more than one path // (`src` under vitest aliases, `dist` under the published `exports`), so two @@ -435,7 +438,8 @@ export interface StorageRoutesOptions { * * Routes: * - POST /storage/upload/presigned → get presigned upload URL - * - POST /storage/upload/complete → mark upload as committed + * - POST /storage/upload/complete → mark upload as committed (and, with + * `attachTo`, attach it to a record as the caller) * - POST /storage/upload/chunked → initiate chunked upload * - PUT /storage/upload/chunked/:uploadId/chunk/:chunkIndex → upload a chunk * - POST /storage/upload/chunked/:uploadId/complete → complete chunked @@ -468,6 +472,60 @@ export function registerStorageRoutes( */ export type StorageGatedSessionReader = (req: IHttpRequest) => Promise; +/** What the commit door's `attachTo` names: the upload, and the record it is attached to. */ +export interface StorageUploadAttachment { + fileId: string; + parentObject: string; + parentId: string; + eTag?: string; +} + +/** + * Package-internal: attach the request's caller's own pending upload to a + * record and commit it, as that caller — the commit door's `attachTo` arm. + * Resolves the request's FULL execution context (the one request-to-context + * composition, as the download authorizer does) and hands it to the + * file-attachments service's caller-scoped `attachUploadAs`, which asks every + * check and vouches for that one file. It throws what that verb throws, with + * its declared `code` and `status`. + * + * Bound by `composeStorageRoutes` alone, like every other storage gate, and a + * parameter of {@link registerStorageFamily} rather than a + * `StorageRoutesOptions` key, so no exported declaration moves and no host can + * hand the door an attacher of its own. `undefined` (no `auth` service, or no + * data engine): the door refuses an `attachTo` with `501`, and a completion + * without one is unchanged. + */ +export type StorageUploadAttacher = ( + req: IHttpRequest, + upload: StorageUploadAttachment, +) => Promise<{ attachmentId: string }>; + +/** The body keys of `attachTo` — `CompleteUploadRequestSchema.attachTo` (`@objectstack/spec/api`). */ +const ATTACH_TO_FIELDS = ['parentObject', 'parentId'] as const; + +/** + * The commit door's `attachTo`: `null` when the request carries none, the + * target when it is well formed, or the sentence a `400` answers. + */ +function attachTargetOf(value: unknown): { parentObject: string; parentId: string } | null | string { + if (value === undefined) return null; + const shape = + `attachTo is an object with the fields ${ATTACH_TO_FIELDS.join(' and ')}, each a non-empty string: ` + + 'the record the upload is attached to.'; + if (value === null || typeof value !== 'object' || Array.isArray(value)) return shape; + const fields = value as Record; + const known = new Set(ATTACH_TO_FIELDS); + const unknown = Object.keys(fields).filter((key) => !known.has(key)); + if (unknown.length > 0) { + return `${unknown.map((k) => `'${k}'`).join(', ')} ${unknown.length === 1 ? 'is not a field' : 'are not fields'} of attachTo. ${shape}`; + } + const { parentObject, parentId } = fields; + if (typeof parentObject !== 'string' || parentObject.length === 0) return shape; + if (typeof parentId !== 'string' || parentId.length === 0) return shape; + return { parentObject, parentId }; +} + /** * Package-internal: register the storage family through ONE door, which asks * the ADR-0069 gate before any route runs (see `mountFamilyRoute` below). @@ -485,6 +543,7 @@ export function registerStorageFamily( store: StorageMetadataStore, opts: StorageRoutesOptions, readGatedSessionUser: StorageGatedSessionReader | undefined, + attachUpload: StorageUploadAttacher | undefined = undefined, ): void { const basePath = opts.basePath ?? '/api/v1/storage'; const downloadTtl = opts.downloadTtl ?? 300; @@ -1053,11 +1112,17 @@ export function registerStorageFamily( // statement unscoped and raising `[tenant-audit]`. const session = await requireUploadSession(req, res); if (session === false) return; - const { fileId, eTag } = req.body ?? {}; + const { fileId, eTag, attachTo } = req.body ?? {}; if (!fileId) { sendError(res, 400, 'INVALID_REQUEST', 'fileId is required'); return; } + // The attach arm's target, judged before anything is read. + const attach = attachTargetOf(attachTo); + if (typeof attach === 'string') { + sendError(res, 400, 'INVALID_REQUEST', attach); + return; + } const file = await store.getFile(fileId); if (!file) { @@ -1082,6 +1147,43 @@ export function registerStorageFamily( return; } + // The attach arm: the record attachments surface's attach verb. The + // commit and the `sys_attachment` insert are one unit, run as the caller + // by the file-attachments service (`attachUploadAs`), which asks the + // ownership rule again, the upload's state and scope, the permission to + // create an attachment, the parent-EDIT attach rule and the parent read, + // and vouches for this one file on its own insert — so a member who may + // attach need not read `sys_file`. ⛔ The generic data door gets no such + // mark: a `sys_attachment` create through it naming a file the caller + // cannot read is still refused `reference_not_found`. + if (attach) { + if (!attachUpload || session === null) { + sendError( + res, + 501, + 'NOT_IMPLEMENTED', + 'This storage mount cannot attach an upload as its caller: the kernel composes no auth service or no ' + + 'data engine. Complete the upload without attachTo.', + ); + return; + } + let attached: { attachmentId: string }; + try { + attached = await attachUpload(req, { + fileId: String(fileId), + parentObject: attach.parentObject, + parentId: attach.parentId, + ...(eTag !== undefined && eTag !== null ? { eTag } : {}), + }); + } catch (err) { + relayAttachFailure(res, err, `${basePath}/upload/complete`); + return; + } + const attachedFile = (await store.getFile(fileId)) ?? file; + sendOk(res, { ...committedFileMetadata(attachedFile, fileId), attachmentId: attached.attachmentId }); + return; + } + const updated = await store.updateFile( fileId, { @@ -1091,19 +1193,7 @@ export function registerStorageFamily( writeContext, ); - sendOk(res, { - // The opaque sys_file id — the value a file field stores as a - // reference (ADR-0104 D3). Previously omitted, so a caller could not - // learn the id to persist after committing an upload. - fileId: updated!.id ?? fileId, - path: updated!.key, - name: updated!.name, - size: updated!.size ?? 0, - mimeType: updated!.mime_type ?? 'application/octet-stream', - lastModified: updated!.updated_at ?? new Date().toISOString(), - created: updated!.created_at ?? new Date().toISOString(), - etag: updated!.etag, - }); + sendOk(res, committedFileMetadata(updated!, fileId)); } catch (err: any) { sendServerFault(res, 500, 'INTERNAL', 'Internal error', err); } @@ -1972,8 +2062,12 @@ function judgeCompletion(session: UploadSessionRecord, listed: ReadonlyArray { +export async function heldBytesRefusal(storage: IStorageService, file: FileRecord): Promise { const remedy = 'Send the file to its upload URL (start a new upload if that URL has expired), then complete again. ' + 'The file stays pending until then.'; @@ -1991,6 +2085,54 @@ async function heldBytesRefusal(storage: IStorageService, file: FileRecord): Pro ); } +/** + * The commit door's answer for a committed file — `FileUploadResponseSchema`'s + * `data` (`@objectstack/spec/api`). + */ +function committedFileMetadata(file: FileRecord, fileId: string): Record { + return { + // The opaque sys_file id — the value a file field stores as a + // reference (ADR-0104 D3). Previously omitted, so a caller could not + // learn the id to persist after committing an upload. + fileId: file.id ?? fileId, + path: file.key, + name: file.name, + size: file.size ?? 0, + mimeType: file.mime_type ?? 'application/octet-stream', + lastModified: file.updated_at ?? new Date().toISOString(), + created: file.created_at ?? new Date().toISOString(), + etag: file.etag, + }; +} + +/** + * The commit door's relay for a refused or failed attach: a declared `4xx` — + * the attach verb's own refusals, the `sys_attachment` insert's gates (the + * permission to create, `ATTACHMENT_PARENT_ACCESS`, `FILES_DISABLED`, a + * `VALIDATION_FAILED` with its field details) — keeps its code, status, + * message and details; a declared `5xx` keeps its code and status with this + * door's generic text unless the producer declared the text for its caller; + * anything else is a fault. + */ +function relayAttachFailure(res: IHttpResponse, err: unknown, path: string): void { + const thrown = resolveThrownHttpError(err, 500); + if (thrown.status < 500) { + const declaredCode = demotedDeclaredCode(thrown); + sendError(res, thrown.status, thrown.code, thrown.message, { + ...(thrown.details ? { details: thrown.details } : {}), + ...(declaredCode !== undefined ? { declaredCode } : {}), + ...(thrown.userMessage !== undefined ? { userMessage: thrown.userMessage } : {}), + }); + return; + } + const refusal = declaredRefusalMessage(err); + if (refusal !== undefined) { + sendError(res, thrown.status, thrown.code, refusal); + return; + } + sendServerFault(res, thrown.status, thrown.code, 'Internal error', err, { method: 'POST', path }); +} + /** * [#24548] Whether a local adapter read failed because no file is stored under * the key: Node's `ENOENT`, which `LocalStorageAdapter.download` passes through diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 59063b6c259..c01dbb103b0 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -60,7 +60,12 @@ import { StorageMetadataStore, releasingRecordOf } from './metadata-store.js'; import type { FileRecord } from './metadata-store.js'; import type { TenancyPosture } from '@objectstack/spec/security'; import { registerStorageFamily } from './storage-routes.js'; -import type { FileReadVerdict, StorageGatedSessionReader, StorageUploadSession } from './storage-routes.js'; +import type { + FileReadVerdict, + StorageGatedSessionReader, + StorageUploadAttacher, + StorageUploadSession, +} from './storage-routes.js'; import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder, findHeldFiles } from './attachment-lifecycle.js'; import { installFileReferenceHooks, type FileReferenceEngine } from './file-reference-lifecycle.js'; import { installSysFileStorageOwnedColumnsGuard } from './sys-file-storage-owned-columns.js'; @@ -71,7 +76,11 @@ import { installAttachmentReadVisibility, } from './attachment-access-hooks.js'; import { contributeAttachmentDeleteFloorAlternate } from './attachment-delete-floor-alternate.js'; -import { FileAttachmentService, type FileAttachmentEngine } from './file-attachment-service.js'; +import { + FileAttachmentService, + type FileAttachmentEngine, + type ICallerFileAttachmentService, +} from './file-attachment-service.js'; import { FieldFileService } from './field-file-service.js'; import { SystemFile, SystemUploadSession } from './objects/index.js'; // ADR-0052 §3 ownership: `sys_attachment` (a file↔record link) belongs with the @@ -1183,6 +1192,7 @@ export function composeStorageRoutes( const resolveSession = buildAuthSessionResolver(registry); const readGatedSessionUser = buildAuthGateSessionReader(registry, engine); const authorizeFileRead = buildFileReadAuthorizer(registry, engine); + const attachUpload = buildUploadAttacher(registry, engine); const resolveFileHolder = engine && typeof (engine as any).find === 'function' ? (file: FileRecord) => findFileHolder(engine as any, file.id, file as any) @@ -1215,6 +1225,8 @@ export function composeStorageRoutes( }, // ADR-0069 — bound here with the other gates, so both mounts carry it. readGatedSessionUser, + // The commit door's `attachTo` arm — bound here with the other gates. + attachUpload, ); return { @@ -1606,6 +1618,75 @@ function buildFileReadAuthorizer( }; } +/** + * The commit door's `attachTo` arm (`POST …/upload/complete`): attach the + * caller's own pending upload to a record and commit it, AS THE CALLER — the + * record attachments surface's attach verb, which the browser panel calls + * instead of a `sys_attachment` create through the generic data door. + * + * The caller is the request's FULL execution context, resolved through the one + * request-to-context composition with the download authorizer's parameters + * (the attach is a write judged by the caller's grants, RLS and sharing, so a + * bare `{ userId }` would not do), and the verb is the file-attachments + * service's caller-scoped `attachUploadAs`, looked up per request: it asks + * every check, and its `sys_attachment` insert vouches for that one file id + * and for nothing else. A request that resolves no user is refused + * `AUTH_REQUIRED` / `401`; a kernel whose file-attachments service is not + * registered, `SERVICE_UNAVAILABLE` / `503`. Everything else is the verb's own + * answer, thrown with its declared `code` and `status` for the door to relay. + * + * `undefined` — the door then refuses `attachTo` with `501` — when the kernel + * has no `auth` service or no data engine: there is no caller to attach as. + */ +function buildUploadAttacher( + registry: StorageGateRegistry, + engine: IDataEngine | null, +): StorageUploadAttacher | undefined { + const authService = resolveGateAuthService(registry); + if (!authService || !engine) return undefined; + // Declared refusals (`refusal: true`): their text is written for the caller. + const refusal = (status: number, code: string, message: string) => + Object.assign(new Error(message), { status, code, refusal: true }); + return async (req, upload) => { + const headers = toWebHeaders(req); + // The download authorizer's parameters (see `buildFileReadAuthorizer`): + // the family's door has already asked the ADR-0069 gate on this request. + const context = headers + ? await resolveRequestExecutionContext({ + headers, + ql: engine, + authService, + tenancy: tenancyServiceResolver(registry), + anonymous: 'fail-closed', + authGate: 'resolved', + credentials: 'api-key-and-session', + sessionApi: 'optional', + session: 'request', + localization: 'withhold', + requestLocale: undefined, + accessToken: 'carry', + oauth: undefined, + onAdmission: undefined, + }) + : undefined; + if (!context?.userId) throw refusal(401, 'AUTH_REQUIRED', 'Authentication required to attach a file'); + let service: Partial | undefined; + try { + service = registry.getService>('file-attachments'); + } catch { + service = undefined; + } + if (!service || typeof service.attachUploadAs !== 'function') { + throw refusal( + 503, + 'SERVICE_UNAVAILABLE', + 'The file-attachments service is not registered in this kernel, so an upload cannot be attached.', + ); + } + return service.attachUploadAs(upload, context); + }; +} + /** * The download door's verdict on ONE file for a caller it has already * resolved — every arm after the door's own `'unauthenticated'` answer. Read diff --git a/packages/spec/authorable-surface/api.json b/packages/spec/authorable-surface/api.json index 966f6a767ec..1100abac1e8 100644 --- a/packages/spec/authorable-surface/api.json +++ b/packages/spec/authorable-surface/api.json @@ -351,6 +351,7 @@ "api/CompleteChunkedUploadResponse:error", "api/CompleteChunkedUploadResponse:meta", "api/CompleteChunkedUploadResponse:success", + "api/CompleteUploadRequest:attachTo", "api/CompleteUploadRequest:eTag", "api/CompleteUploadRequest:fileId", "api/ConceptListResponse:data", diff --git a/packages/spec/src/api/storage.zod.ts b/packages/spec/src/api/storage.zod.ts index 8e3603c8edd..bbe8536daf8 100644 --- a/packages/spec/src/api/storage.zod.ts +++ b/packages/spec/src/api/storage.zod.ts @@ -82,6 +82,19 @@ export const CompleteUploadRequestSchema = lazySchema(() => z.object({ + 'that the storage backend holds bytes under the file\'s key and, when the upload declared a size, ' + 'that the held size equals it.', ), + // The attach verb of the record attachments surface: the commit and the + // `sys_attachment` insert run as one unit, as the caller, in this call — + // `FileAttachmentService.attachUploadAs` (`@objectstack/service-storage`). + attachTo: z.object({ + parentObject: z.string().min(1).describe('Object of the record the file is attached to (enable.files: true)'), + parentId: z.string().min(1).describe('Id of the record the file is attached to'), + }).optional().describe( + 'Attach the uploaded file to this record in the same call, as the signed-in caller: the commit and the ' + + 'attachment row are one unit, and the answer carries the new attachmentId. Only the uploader\'s own ' + + 'pending upload in the attachments scope can be attached this way. The attachment is judged as the ' + + 'caller\'s own create: the permission to create an attachment, edit access to the record, and the ' + + 'record object\'s enable.files opt-in. A refused attach leaves the upload pending and uncommitted.', + ), })); // ========================================== @@ -107,7 +120,11 @@ export const PresignedUrlResponseSchema = lazySchema(() => BaseResponseSchema.ex })); export const FileUploadResponseSchema = lazySchema(() => BaseResponseSchema.extend({ - data: FileMetadataSchema.describe('Uploaded file metadata'), + data: FileMetadataSchema.extend({ + attachmentId: z.string().optional().describe( + 'Id of the sys_attachment row the commit created, when the request carried attachTo', + ), + }).describe('Uploaded file metadata'), })); /** diff --git a/ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx b/ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx index 357174d5f02..fdaad5c44a4 100644 --- a/ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx +++ b/ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx @@ -46,7 +46,11 @@ import { RECORD_PAGE_ATTACHMENT_ROWS, recordAttachmentsCountField } from './reco * * Storage model: one `sys_file` row per uploaded blob (three-step * presigned upload via @object-ui/data-objectstack's ObjectStack adapter), one - * `sys_attachment` join row linking it to `(parent_object, parent_id)`. + * `sys_attachment` join row linking it to `(parent_object, parent_id)`. The + * join row is written by the upload's own completion (`attachTo`), as the + * signed-in user: the server's attach verb, never a `sys_attachment` create + * through the generic data door, which a member who may attach files but not + * read `sys_file` is refused (`reference_not_found` on `file_id`). * Downloads fetch a short-lived signed URL from `/storage/files/:fileId/url` * with the console's Bearer token (the endpoint requires an authenticated * session for attachments-scope files, objectstack-ai/objectstack#2970), then open it. @@ -65,9 +69,9 @@ interface AttachmentRow { export interface RecordAttachmentsPanelProps { objectName: string; recordId: string; - /** ObjectUI DataSource (sys_attachment CRUD goes through the generic data path). */ + /** ObjectUI DataSource (the list read and the delete go through the generic data path; the attach does not). */ dataSource: any; - /** Current user id, stamped on `uploaded_by`. */ + /** Current user id. The server stamps `uploaded_by` from the session; the panel no longer sends it. */ currentUserId?: string | null; className?: string; } @@ -175,7 +179,6 @@ export const RecordAttachmentsPanel: React.FC = ({ objectName, recordId, dataSource, - currentUserId, className, }) => { const { t } = useObjectTranslation(); @@ -216,10 +219,10 @@ export const RecordAttachmentsPanel: React.FC = ({ * and a delete go through the storage route, not `sys_attachment`'s generic * data door, so that door's operation set says nothing about them. * - * Upload needs `create`. Without it the three-step presigned flow below - * commits a `sys_file` and only the final `sys_attachment` insert is - * refused: the user meets the error after the bytes are stored, and the file - * stays committed with nothing attached to it. + * Upload needs `create`. Without it the attach at the end of the upload + * below is refused after the bytes are stored (the server leaves the file + * pending, and its retention reclaims it); hiding Upload spares the user + * that round trip. * * Delete needs `delete`, and being the uploader does not change that. The * server asks the object grant first (plugin-security's CRUD check wraps the @@ -252,9 +255,17 @@ export const RecordAttachmentsPanel: React.FC = ({ // upload adapter and the download-URL fetch — the storage routes require a // session and there is no cookie for `credentials: 'include'` to carry. const authFetch = React.useMemo(() => createAuthenticatedFetch(), []); + // Every upload from this panel is attached to this record by the call that + // completes it (`attachTo`): the server's attach verb, run as the user. const adapter = React.useMemo( - () => createObjectStackUploadAdapter({ baseUrl, scope: 'attachments', fetchImpl: authFetch }), - [baseUrl, authFetch], + () => + createObjectStackUploadAdapter({ + baseUrl, + scope: 'attachments', + fetchImpl: authFetch, + ...(objectName && recordId ? { attachTo: { parentObject: objectName, parentId: recordId } } : {}), + }), + [baseUrl, authFetch, objectName, recordId], ); /** Map the server's fail-closed 40x codes (objectstack-ai/objectstack#2755, objectstack-ai/objectstack#2970) to friendly copy. */ @@ -390,27 +401,18 @@ export const RecordAttachmentsPanel: React.FC = ({ const handleFiles = React.useCallback( async (files: FileList | null) => { - if (!files || files.length === 0 || !dataSource) return; + if (!files || files.length === 0 || !dataSource || !objectName || !recordId) return; setUploading(true); setError(null); try { for (const file of Array.from(files)) { - // 1) blob → sys_file via the canonical presigned three-step flow + // blob → sys_file → sys_attachment: the presigned three-step flow, + // whose completion attaches the file to this record as the user + // (the server checks the create grant, edit access to the record, + // `enable.files`, and that the file is the user's own upload). const uploaded = await adapter.upload(file); - const fileId = (uploaded.meta as any)?.fileId as string | undefined; - if (!fileId) throw new Error('Upload did not return a fileId'); - // 2) join row → sys_attachment (server enforces enable.files) - await dataSource.create('sys_attachment', { - parent_object: objectName, - parent_id: recordId, - file_id: fileId, - file_name: uploaded.name ?? file.name, - mime_type: uploaded.mimeType ?? file.type, - size: uploaded.size ?? file.size, - // Back-compat with pre-objectstack-ai/objectstack#2755 servers; a current server stamps - // `uploaded_by` from the session and ignores this value. - ...(currentUserId ? { uploaded_by: currentUserId } : {}), - }); + const attachmentId = (uploaded.meta as any)?.attachmentId as string | undefined; + if (!attachmentId) throw new Error('The server completed the upload but did not attach it to this record'); } await refresh(); } catch (err: any) { @@ -420,7 +422,7 @@ export const RecordAttachmentsPanel: React.FC = ({ if (inputRef.current) inputRef.current.value = ''; } }, - [adapter, dataSource, objectName, recordId, currentUserId, refresh], + [adapter, dataSource, objectName, recordId, refresh], ); /** diff --git a/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.affordanceGrant-12047.test.tsx b/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.affordanceGrant-12047.test.tsx index 46bf4020a2a..f103a56c512 100644 --- a/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.affordanceGrant-12047.test.tsx +++ b/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.affordanceGrant-12047.test.tsx @@ -36,12 +36,17 @@ import { render, screen, waitFor, within, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { MePermissionsProvider, type MePermissionsAuthenticatedResponse } from '@object-ui/permissions'; -/** The fake presigned adapter: `upload` resolving means a `sys_file` was committed. */ +/** The fake presigned adapter: `upload` resolving means the file was committed and attached. */ const adapterUpload = vi.fn(); +/** The options the panel built its adapter with — `attachTo` names the record. */ +const adapterOptions = vi.fn(); vi.mock('@object-ui/data-objectstack', async (importOriginal) => ({ ...(await importOriginal>()), - createObjectStackUploadAdapter: () => ({ upload: adapterUpload }), + createObjectStackUploadAdapter: (opts: unknown) => { + adapterOptions(opts); + return { upload: adapterUpload }; + }, })); import { RecordAttachmentsPanel } from '../RecordAttachmentsPanel'; @@ -174,20 +179,15 @@ describe('RecordAttachmentsPanel — write affordances follow the sys_attachment }); it('CONTROL — with Upload shown and the server refusing the attach, the refusal message still appears', async () => { - adapterUpload.mockResolvedValue({ - name: 'quote.pdf', - mimeType: 'application/pdf', - size: 10, - meta: { fileId: 'f-new' }, - }); - const dataSource = makeDataSource({ - create: vi.fn(async () => { - throw Object.assign(new Error("[Security] Access denied: operation 'insert' on sys_attachment"), { - httpStatus: 403, - code: 'PERMISSION_DENIED', - }); + // The attach is the upload's own completion (`attachTo`), so the server's + // refusal is the upload's. + adapterUpload.mockRejectedValue( + Object.assign(new Error('Upload completion failed (403): {"success":false,"error":{"code":"PERMISSION_DENIED"}}'), { + status: 403, + code: 'PERMISSION_DENIED', }), - }); + ); + const dataSource = makeDataSource(); const { container } = renderWithGrant({ allowCreate: true }, dataSource); await rowsLoaded(); expect(screen.getByRole('button', { name: 'Upload' })).toBeInTheDocument(); @@ -200,10 +200,11 @@ describe('RecordAttachmentsPanel — write affordances follow the sys_attachment expect(screen.getByRole('alert')).toHaveTextContent("You don't have permission to do that."), ); expect(adapterUpload).toHaveBeenCalledTimes(1); - expect(dataSource.create).toHaveBeenCalledWith( - 'sys_attachment', - expect.objectContaining({ parent_object: 'att_case', parent_id: 'r1', file_id: 'f-new' }), + expect(adapterOptions).toHaveBeenLastCalledWith( + expect.objectContaining({ scope: 'attachments', attachTo: { parentObject: 'att_case', parentId: 'r1' } }), ); + // No `sys_attachment` create through the generic data door. + expect(dataSource.create).not.toHaveBeenCalled(); }); it('no flash while permissions load: a read-only answer never shows Upload, not even before it arrives', async () => { diff --git a/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx b/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx index b57e023de4c..9b8148587a3 100644 --- a/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx +++ b/ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx @@ -846,3 +846,81 @@ describe('RecordAttachmentsPanel — mobile shell openFile', () => { expect(host.openFile).not.toHaveBeenCalled(); }); }); + +/** + * The upload attaches through the server's attach verb: the presigned upload's + * own completion carries `attachTo` (`POST /api/v1/storage/upload/complete`), + * and the server commits the file and writes its `sys_attachment` row as the + * signed-in user. The panel never creates `sys_attachment` through the generic + * data door, which refuses a member who may attach but cannot read `sys_file`. + * Driven through the REAL ObjectStack upload adapter over a stubbed `fetch`. + */ +describe('RecordAttachmentsPanel — upload attaches through the upload’s own completion', () => { + const ok = (data: unknown) => + new Response(JSON.stringify({ success: true, data }), { status: 200, headers: { 'content-type': 'application/json' } }); + + function stubStorage(complete: () => Response) { + const calls: Array<{ url: string; method: string; body: unknown }> = []; + const spy = vi.spyOn(globalThis, 'fetch').mockImplementation(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + const method = init?.method ?? 'GET'; + const body = typeof init?.body === 'string' ? JSON.parse(init.body) : init?.body; + calls.push({ url, method, body }); + if (url.endsWith('/api/v1/storage/upload/presigned')) { + return ok({ uploadUrl: '/api/v1/storage/_local/raw/tok', method: 'PUT', fileId: 'f-new', headers: {} }); + } + if (url.endsWith('/api/v1/storage/_local/raw/tok')) return ok({ key: 'attachments/f-new.pdf' }); + if (url.endsWith('/api/v1/storage/upload/complete')) return complete(); + throw new Error(`unexpected fetch ${method} ${url}`); + }); + return { spy, calls }; + } + + async function uploadOne() { + const dataSource = makeDataSource(); + const { container } = render( + , + ); + await waitFor(() => expect(screen.getByText('report.pdf')).toBeInTheDocument()); + const input = container.querySelector('input[type="file"]') as HTMLInputElement; + await userEvent.setup().upload(input, new File(['%PDF'], 'quote.pdf', { type: 'application/pdf' })); + return dataSource; + } + + afterEach(() => vi.restoreAllMocks()); + + it('completes the upload with attachTo naming this record, and creates no sys_attachment through the data door', async () => { + const { calls } = stubStorage(() => ok({ fileId: 'f-new', name: 'quote.pdf', attachmentId: 'a-new' })); + const dataSource = await uploadOne(); + await waitFor(() => expect(dataSource.find).toHaveBeenCalledTimes(2)); // the list re-read after the attach + const complete = calls.find((c) => c.url.endsWith('/upload/complete')); + expect(complete).toMatchObject({ + method: 'POST', + body: { fileId: 'f-new', attachTo: { parentObject: 'att_case', parentId: 'r1' } }, + }); + expect(calls.find((c) => c.url.endsWith('/upload/presigned'))?.body).toMatchObject({ scope: 'attachments' }); + expect(dataSource.create).not.toHaveBeenCalled(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('maps the attach verb’s ATTACHMENT_PARENT_ACCESS refusal to the friendly copy', async () => { + stubStorage( + () => + new Response( + JSON.stringify({ success: false, error: { code: 'ATTACHMENT_PARENT_ACCESS', message: 'Cannot attach to att_case/r1' } }), + { status: 403, headers: { 'content-type': 'application/json' } }, + ), + ); + const dataSource = await uploadOne(); + await waitFor(() => + expect(screen.getByRole('alert')).toHaveTextContent("You don't have access to attach files to this record."), + ); + expect(dataSource.create).not.toHaveBeenCalled(); + }); + + it('a completion that attached nothing is an error, never a silent success', async () => { + stubStorage(() => ok({ fileId: 'f-new', name: 'quote.pdf' })); + await uploadOne(); + await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('did not attach it to this record')); + }); +}); diff --git a/ui/packages/data-objectstack/src/upload-adapter.ts b/ui/packages/data-objectstack/src/upload-adapter.ts index 4cb6f33668c..46bb834e63b 100644 --- a/ui/packages/data-objectstack/src/upload-adapter.ts +++ b/ui/packages/data-objectstack/src/upload-adapter.ts @@ -45,6 +45,18 @@ export interface ObjectStackUploadAdapterOptions { * declares (objectui#12055). */ scope?: API.UploadScope; + /** + * Attach every upload from this adapter to this record, in the call that + * completes it: the completion request carries `attachTo`, and the server + * commits the file and writes its `sys_attachment` row as one unit, as the + * signed-in caller (the record attachments surface's attach verb). The + * result's `meta.attachmentId` names the new attachment. The server attaches + * only the caller's own pending upload in the `attachments` scope, so set + * `scope: 'attachments'` with it. A refused attach fails the upload with the + * server's refusal and leaves the file pending, never committed and + * unattached. + */ + attachTo?: { parentObject: string; parentId: string }; /** Override `fetch` (tests, ServiceWorker queues). */ fetchImpl?: typeof fetch; /** @@ -64,7 +76,8 @@ export interface ObjectStackUploadAdapterOptions { * 1. POST `/upload/presigned` → `{ uploadUrl, fileId, downloadUrl }` * 2. PUT `uploadUrl` → uploads the raw bytes (HMAC token) * 3. POST `/upload/complete` → marks the file ready and registers - * the `sys_file` record + * the `sys_file` record — and, with + * `attachTo`, attaches it to that record * * Falls back to a canonical (non-signed) download URL so the returned * `UploadResult.url` is stable across signed-URL expiry — components @@ -136,14 +149,27 @@ export function createObjectStackUploadAdapter( credentials, headers: { 'Content-Type': 'application/json' }, signal: options.signal, - body: JSON.stringify({ fileId }), + body: JSON.stringify(opts.attachTo ? { fileId, attachTo: opts.attachTo } : { fileId }), }); if (!completeRes.ok) { const text = await completeRes.text().catch(() => ''); - throw new Error( - `Upload completion failed (${completeRes.status}): ${text || completeRes.statusText}`, + // The server's refusal code and status ride on the error, so a caller + // maps a refused attach (`ATTACHMENT_PARENT_ACCESS`, `FILES_DISABLED`, + // `PERMISSION_DENIED`…) without parsing the message. + let code: string | undefined; + try { + const parsed = JSON.parse(text); + if (typeof parsed?.error?.code === 'string') code = parsed.error.code; + } catch { + // Not the declared envelope — the message keeps the raw text. + } + throw Object.assign( + new Error(`Upload completion failed (${completeRes.status}): ${text || completeRes.statusText}`), + { status: completeRes.status, ...(code ? { code } : {}) }, ); } + const completeBody = opts.attachTo ? await completeRes.json().catch(() => null) : null; + const attachmentId = (completeBody?.data ?? completeBody)?.attachmentId as string | undefined; // Use the canonical (stable) download endpoint as the field value // so a saved record keeps working past signed-URL expiry. This @@ -156,7 +182,7 @@ export function createObjectStackUploadAdapter( name, size: file.size, mimeType, - meta: { fileId, scope: opts.scope }, + meta: { fileId, scope: opts.scope, ...(attachmentId ? { attachmentId } : {}) }, }; }, }; diff --git a/ui/packages/plugin-form/src/affordanceGrantMap-12082.test.tsx b/ui/packages/plugin-form/src/affordanceGrantMap-12082.test.tsx index d162fd0130b..6f802569d4c 100644 --- a/ui/packages/plugin-form/src/affordanceGrantMap-12082.test.tsx +++ b/ui/packages/plugin-form/src/affordanceGrantMap-12082.test.tsx @@ -680,8 +680,10 @@ const WRITE_SITES: Record = { outOfFamily("the language switcher changes the session's language locally and persists the caller's OWN sys_user.locale best-effort, a refusal surfacing as a toast"), 'packages/app-shell/src/hooks/useUserLocale.ts :: saveSignInLanguagePick :: update': outOfFamily("the sign-in page saves the language picked on it as the caller's OWN sys_user.locale, only while that column is unset; a refusal leaves the pick on this device and does not hold the sign-in (no CRUD visibility door)"), - 'packages/app-shell/src/views/RecordAttachmentsPanel.tsx :: RecordAttachmentsPanel > handleFiles :: create': - mapped(['attachmentUpload'], 'packages/app-shell/src/views/RecordAttachmentsPanel.tsx'), + // The attachments panel's upload has no data-source write site: the upload's + // own completion attaches it (the upload adapter's `attachTo`), so the + // `attachmentUpload` row is read where the upload is offered and no + // `create` is called. Its delete is still a data-source write. 'packages/app-shell/src/views/RecordAttachmentsPanel.tsx :: RecordAttachmentsPanel > handleDelete :: delete': mapped(['attachmentDelete'], 'packages/app-shell/src/views/RecordAttachmentsPanel.tsx'), 'packages/app-shell/src/views/RecordDetailView.tsx :: RecordDetailView > apiHandler :: update':