diff --git a/integrationTests/security/fixtures/record-row-allowwrite/config.yaml b/integrationTests/security/fixtures/record-row-allowwrite/config.yaml new file mode 100644 index 000000000..66db5f77e --- /dev/null +++ b/integrationTests/security/fixtures/record-row-allowwrite/config.yaml @@ -0,0 +1,6 @@ +# Fixture for record-scoped WRITE authorization (write-side corollary to #1786). +graphqlSchema: + files: '*.graphql' +jsResource: + files: resources.js +rest: true diff --git a/integrationTests/security/fixtures/record-row-allowwrite/resources.js b/integrationTests/security/fixtures/record-row-allowwrite/resources.js new file mode 100644 index 000000000..b1aadac41 --- /dev/null +++ b/integrationTests/security/fixtures/record-row-allowwrite/resources.js @@ -0,0 +1,42 @@ +// Fixture for record-scoped WRITE authorization (write-side corollary to #1786). +// +// The recommended composition pattern for record-scoped write overrides: +// - Gate on the base table/RBAC grant via `super.allow*(...)` first, so a user who loses the +// role's table write permission is denied regardless of ownership. +// - Per record (`this` = a per-row resource for delete / the element's resource for array puts, +// with schema properties readable through it), additionally require the requesting user to +// own the row. For a create there is no existing row: the incoming record is the parameter. +// +// Super users always pass so that seed writes and setup ops succeed. + +function isSuper(user) { + return !!user?.role?.permission?.super_user; +} + +export class Vault extends tables.Vault { + allowDelete(user, target, context) { + if (isSuper(user)) return true; + if (!super.allowDelete(user, target, context)) return false; + const owner = this?.owner; + // No loaded record (a single-record entry check on a missing row): RBAC passed, allow. + if (owner === undefined || owner === null) return true; + // Per-row: only the owner may delete this specific record. + return owner === user?.username; + } + + allowUpdate(user, _record, _context) { + if (isSuper(user)) return true; + // `this` = the resource with the EXISTING record loaded; only the owner may overwrite it. + return this?.owner === user?.username; + } + + allowCreate(user, record, _context) { + if (isSuper(user)) return true; + // New record: it must be created under the requesting user's own ownership. + return record?.owner === user?.username; + } + + allowRead() { + return true; // reads are intentionally open so the test can verify surviving rows per user + } +} diff --git a/integrationTests/security/fixtures/record-row-allowwrite/schema.graphql b/integrationTests/security/fixtures/record-row-allowwrite/schema.graphql new file mode 100644 index 000000000..5768e4a5d --- /dev/null +++ b/integrationTests/security/fixtures/record-row-allowwrite/schema.graphql @@ -0,0 +1,12 @@ +# Fixture for record-scoped WRITE authorization (write-side corollary to #1786). +# +# Vault rows are owned by a specific user (`owner` field). The allow* overrides in resources.js +# permit a write/delete only when the record's owner matches the requesting user. The test +# verifies that a conditional REST DELETE only removes the caller's own rows (filter semantics) +# and that an array PUT containing a foreign element fails atomically. +type Vault @table @export { + id: ID @primaryKey + owner: String + kind: String @indexed + secret: String +} diff --git a/integrationTests/security/record-row-allowwrite.test.ts b/integrationTests/security/record-row-allowwrite.test.ts new file mode 100644 index 000000000..4dc787c48 --- /dev/null +++ b/integrationTests/security/record-row-allowwrite.test.ts @@ -0,0 +1,149 @@ +/** + * Record-scoped WRITE authorization (write-side corollary to #1786's record-scoped allowRead). + * + * An application-overridden allowDelete on a query-shaped REST DELETE is evaluated once per + * matching record (FILTER semantics: the caller's own rows are deleted, everyone else's + * survive — a 200, not a blanket 403). An array PUT is authorized per element (create-vs-update + * chosen by that element's existence) with FAIL semantics: one denied element rejects the whole + * request and the aborted transaction commits nothing. + * + * Fixture: `record-row-allowwrite` — Vault table with owner-scoped allowDelete/allowUpdate/ + * allowCreate composing the base RBAC grant via `super.allowDelete`. + * + * Reproduction: + * npm run test:integration -- "integrationTests/security/record-row-allowwrite.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { deepStrictEqual, ok, strictEqual } from 'node:assert'; +import { resolve } from 'node:path'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient, createHeaders } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/record-row-allowwrite'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +const ALICE = { username: 'write_auth_alice', password: 'Alice-pw-write!' }; +const BOB = { username: 'write_auth_bob', password: 'Bobby-pw-write!' }; +const ROLE = 'write_auth_role'; + +suite('record-scoped write authorization over REST', { skip: skipSuite }, (ctx: ContextWithHarper) => { + let client: ReturnType; + let restURL = ''; + let aliceHeaders: Record; + + async function seed(rows: any[]) { + await client.req().send({ operation: 'insert', schema: 'data', table: 'Vault', records: rows }).expect(200); + } + async function idsOf(kind: string): Promise { + const resp = await fetch(`${restURL}/Vault/?kind=${kind}`, { headers: aliceHeaders }); + strictEqual(resp.status, 200); + const rows = await resp.json(); + return rows.map((row: any) => row.id).sort(); + } + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + restURL = ctx.harper.httpURL; + + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + try { + const probe = await client.reqRest('/Vault/').timeout(3000); + if (probe.status !== 404) break; + } catch { + /* not ready */ + } + await new Promise((r) => setTimeout(r, 250)); + } + + await client + .req() + .send({ + operation: 'add_role', + role: ROLE, + permission: { + super_user: false, + data: { + tables: { + Vault: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + + for (const u of [ALICE, BOB]) { + await client + .req() + .send({ operation: 'add_user', role: ROLE, username: u.username, password: u.password, active: true }) + .expect(200); + } + + aliceHeaders = createHeaders(ALICE.username, ALICE.password); + }); + + after(async () => { + await teardownHarper(ctx); + }); + + test('conditional DELETE filters per record: only the caller-owned rows are deleted (200)', async () => { + await seed([ + { id: 'del-a1', owner: ALICE.username, kind: 'del', secret: 'a1' }, + { id: 'del-a2', owner: ALICE.username, kind: 'del', secret: 'a2' }, + { id: 'del-b1', owner: BOB.username, kind: 'del', secret: 'b1' }, + { id: 'del-b2', owner: BOB.username, kind: 'del', secret: 'b2' }, + ]); + const resp = await fetch(`${restURL}/Vault/?kind=del`, { method: 'DELETE', headers: aliceHeaders }); + ok(resp.status < 300, `conditional DELETE should succeed with filter semantics, got ${resp.status}`); + deepStrictEqual(await idsOf('del'), ['del-b1', 'del-b2'], "bob's rows must survive alice's query delete"); + }); + + test('array PUT with a foreign element fails whole (403) and commits nothing', async () => { + await seed([ + { id: 'put-a1', owner: ALICE.username, kind: 'put', secret: 'original' }, + { id: 'put-b1', owner: BOB.username, kind: 'put', secret: 'original' }, + ]); + const resp = await fetch(`${restURL}/Vault/`, { + method: 'PUT', + headers: { ...aliceHeaders, 'Content-Type': 'application/json' }, + body: JSON.stringify([ + { id: 'put-a1', owner: ALICE.username, kind: 'put', secret: 'updated' }, + { id: 'put-b1', owner: BOB.username, kind: 'put', secret: 'updated' }, + ]), + }); + strictEqual(resp.status, 403, `a denied element must fail the whole array PUT, got ${resp.status}`); + const rows = await (await fetch(`${restURL}/Vault/?kind=put`, { headers: aliceHeaders })).json(); + strictEqual(rows.length, 2, 'both seeded rows must still exist'); + ok( + rows.every((row: any) => row.secret === 'original'), + `the aborted transaction must not commit ANY element (including the allowed one): ${JSON.stringify(rows)}` + ); + }); + + test('all-owned array PUT succeeds per element, including an owned create', async () => { + await seed([{ id: 'ok-a1', owner: ALICE.username, kind: 'ok', secret: 'original' }]); + const resp = await fetch(`${restURL}/Vault/`, { + method: 'PUT', + headers: { ...aliceHeaders, 'Content-Type': 'application/json' }, + body: JSON.stringify([ + { id: 'ok-a1', owner: ALICE.username, kind: 'ok', secret: 'updated' }, // existing → allowUpdate + { id: 'ok-a2', owner: ALICE.username, kind: 'ok', secret: 'new' }, // new → allowCreate + ]), + }); + ok(resp.status < 300, `an all-owned array PUT must succeed, got ${resp.status}`); + deepStrictEqual(await idsOf('ok'), ['ok-a1', 'ok-a2']); + }); + + test('array PUT claiming foreign ownership on a NEW row is denied by allowCreate', async () => { + const resp = await fetch(`${restURL}/Vault/`, { + method: 'PUT', + headers: { ...aliceHeaders, 'Content-Type': 'application/json' }, + body: JSON.stringify([{ id: 'new-b1', owner: BOB.username, kind: 'new', secret: 'forged' }]), + }); + strictEqual(resp.status, 403, `creating a row under someone else's ownership must be denied, got ${resp.status}`); + deepStrictEqual(await idsOf('new'), [], 'the denied create must not exist'); + }); +}); diff --git a/resources/DESIGN.md b/resources/DESIGN.md index 49e52838b..61d4b22f0 100644 --- a/resources/DESIGN.md +++ b/resources/DESIGN.md @@ -78,23 +78,24 @@ One giant `makeTable()` factory that returns a `TableResource extends Resource` ## "Where is X" cheat sheet -| Question | Where | -| --------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| How is a CRUD request authorized? | `Table.ts → #section: authz-hooks`; defaults in `Resource.ts` (`allowRead` etc.) | -| Where does versioning / conflict resolution happen? | `Table.ts → _writeUpdate` (`#section: write-path-internals`) | -| How does `search()` choose an index? | `Table.ts → search` (`#section: search-query`) | -| How are subscriptions replayed? | `Table.ts → subscribe` (`#section: pub-sub`) | -| How is the response body shaped (select clause)? | `Table.ts → transformEntryForSelect` (`#section: search-query`) | -| Where is record-level TTL evaluated? | `Table.ts → setTTLExpiration` (`#section: lifecycle-admin`); `Updatable.getExpiresAt` (`#section: setup-and-factory`) | -| Why does `search()` hide a row that's past its TTL but not yet swept? | `Table.ts → transformEntryForSelect` unconditionally treats `entry.expiresAt < Date.now()` as gone (lazy eviction on read) — correct for a SELECT, but a mutation locating rows to overwrite needs the opposite: pass `target.includeExpired = true` (read by the SQL engine's `runUpdate`/`runDelete` via `SqlEngineContext.includeExpiredRows`) to treat such a row as a live match, matching the leniency a direct by-id `put`/`patch` already has (they skip this check entirely, since `Resource.patch`'s static options don't request `ensureLoaded`). | -| How are residencies enforced (replication)? | `Table.ts → #section: lifecycle-admin` (residency block: `getResidencyRecord`, `setResidency`, `setResidencyById`, `getResidency`) | -| How is the RecordObject prototype applied? | `RecordEncoder.ts` (see `../DESIGN.md`) | -| Where is the per-request transaction stored? | `transaction.ts` + `contextStorage` (AsyncLocalStorage) | -| How does a query opt out of a read snapshot? | Pass `snapshot: false` on the search request (e.g. `get_analytics`). `Table.ts → search` calls `txn.useReadTxn(snapshot === false)`; on RocksDB `DatabaseTransaction.getReadTxn` then builds the read txn with `{ disableSnapshot: true }` so a long scan reads latest without pinning a snapshot. No-op on LMDB (`LMDBTransaction.useReadTxn`). | -| How does a URL path map to a Resource? | `Resources.ts → getMatch` (exact/prefix fast path) then `matchParamRoute` (parameterised routes); see "Path routing" below | -| How does HNSW keep the graph connected on delete? | `indexes/HierarchicalNavigableSmallWorld.ts → index()` delete path: zero-degree orphans reindexed via `needsReindexing`; severed multi-node islands detected and reconnected by `repairSeveredNeighbors` (#1712) | -| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions + a request `vectorFilter` + a record-scoped `allowRead` override into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, filter)`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | -| How is row-level read access control enforced? | Unified `allowRead` (#1422 gap 2 / #1241): an application-OVERRIDDEN `allowRead` (detected via the `isDefaultAllowRead` marker on the framework defaults) is record-scoped — evaluated once per record with `this` = the (frozen) record during query execution, fail-closed on throw, dispatched via the resolved method (never `record.allowRead` lookup — data shadowing). The authorize wrapper (`Resource.ts → authorizeActionOnResource`) defers collection reads on tables (`supportsRowLevelAllowRead`) to this per-record path; single-record `get` keeps the entry check (record loaded, proxied reads work). Records also expose a non-enumerable `allowRead` delegate on the per-table `structPrototype`. | +| Question | Where | +| --------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| How is a CRUD request authorized? | `Table.ts → #section: authz-hooks`; defaults in `Resource.ts` (`allowRead` etc.) | +| Where does versioning / conflict resolution happen? | `Table.ts → _writeUpdate` (`#section: write-path-internals`) | +| How does `search()` choose an index? | `Table.ts → search` (`#section: search-query`) | +| How are subscriptions replayed? | `Table.ts → subscribe` (`#section: pub-sub`) | +| How is the response body shaped (select clause)? | `Table.ts → transformEntryForSelect` (`#section: search-query`) | +| Where is record-level TTL evaluated? | `Table.ts → setTTLExpiration` (`#section: lifecycle-admin`); `Updatable.getExpiresAt` (`#section: setup-and-factory`) | +| Why does `search()` hide a row that's past its TTL but not yet swept? | `Table.ts → transformEntryForSelect` unconditionally treats `entry.expiresAt < Date.now()` as gone (lazy eviction on read) — correct for a SELECT, but a mutation locating rows to overwrite needs the opposite: pass `target.includeExpired = true` (read by the SQL engine's `runUpdate`/`runDelete` via `SqlEngineContext.includeExpiredRows`) to treat such a row as a live match, matching the leniency a direct by-id `put`/`patch` already has (they skip this check entirely, since `Resource.patch`'s static options don't request `ensureLoaded`). | +| How are residencies enforced (replication)? | `Table.ts → #section: lifecycle-admin` (residency block: `getResidencyRecord`, `setResidency`, `setResidencyById`, `getResidency`) | +| How is the RecordObject prototype applied? | `RecordEncoder.ts` (see `../DESIGN.md`) | +| Where is the per-request transaction stored? | `transaction.ts` + `contextStorage` (AsyncLocalStorage) | +| How does a query opt out of a read snapshot? | Pass `snapshot: false` on the search request (e.g. `get_analytics`). `Table.ts → search` calls `txn.useReadTxn(snapshot === false)`; on RocksDB `DatabaseTransaction.getReadTxn` then builds the read txn with `{ disableSnapshot: true }` so a long scan reads latest without pinning a snapshot. No-op on LMDB (`LMDBTransaction.useReadTxn`). | +| How does a URL path map to a Resource? | `Resources.ts → getMatch` (exact/prefix fast path) then `matchParamRoute` (parameterised routes); see "Path routing" below | +| How does HNSW keep the graph connected on delete? | `indexes/HierarchicalNavigableSmallWorld.ts → index()` delete path: zero-degree orphans reindexed via `needsReindexing`; severed multi-node islands detected and reconnected by `repairSeveredNeighbors` (#1712) | +| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions + a request `vectorFilter` + a record-scoped `allowRead` override into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, filter)`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | +| How is row-level read access control enforced? | Unified `allowRead` (#1422 gap 2 / #1241): an application-OVERRIDDEN `allowRead` (detected via the `isDefaultAllowRead` marker on the framework defaults) is record-scoped — evaluated once per record with `this` = the (frozen) record during query execution, fail-closed on throw, dispatched via the resolved method (never `record.allowRead` lookup — data shadowing). The authorize wrapper (`Resource.ts → authorizeActionOnResource`) defers collection reads on tables (`supportsRowLevelAllowRead`) to this per-record path; single-record `get` keeps the entry check (record loaded, proxied reads work). Records also expose a non-enumerable `allowRead` delegate on the per-table `structPrototype`. | +| How is row-level WRITE access control enforced? | Write-side corollary to the record-scoped `allowRead`. Mutating `allow*` hooks stay on the RESOURCE (no record delegate); per-record evaluation binds `this` to a per-row resource instance (record loaded, so `this.ownerId` and `super.allow*` both work), and since the write paths are async, sync AND async overrides participate. Query-shaped delete: an overridden `allowDelete` (marker `isDefaultAllowDelete`) defers its entry check (wrapper gates on `TableResource.prototype.delete.enforcesCheckPermission` — a subclass overriding `delete()` keeps the entry check) into `Table.ts → #deleteWithRecordScopedCheck`, FILTER semantics (denied/throwing rows are skipped; limit/offset count allowed rows). Array put: with an overridden `allowUpdate`/`allowCreate` (markers `isDefaultAllowUpdate`/`isDefaultAllowCreate`), the entry check defers into the static `Resource.put` action (which sees the resolved body) and each element is checked individually — create-vs-update by that element's existence — FAIL semantics: one denial throws `AccessViolation`, aborting the whole transaction. Default-RBAC tables keep the single collection-entry check (insert operand), unchanged. Both write deferrals gate on the class-level `supportsRecordScopedWriteAuth` marker (write-side analogue of `supportsRowLevelAllowRead`) — a plain `Resource` subclass has no records to scope to and keeps the single entry check its overrides were written against. Operations-API/SQL writes never arm `checkPermission` and stay on operation-level RBAC (by design — raw tables have no override surface). | --- diff --git a/resources/DatabaseTransaction.ts b/resources/DatabaseTransaction.ts index 70056829a..2a70101b4 100644 --- a/resources/DatabaseTransaction.ts +++ b/resources/DatabaseTransaction.ts @@ -18,7 +18,7 @@ const trackedTxns = new Set(); const MAX_OUTSTANDING_TXN_DURATION = convertToMS(envMngr.get(CONFIG_PARAMS.STORAGE_MAXTRANSACTIONQUEUETIME)) || 45000; // Allow write transactions to be queued for up to 25 seconds before we start rejecting them const DEBUG_LONG_TXNS = envMngr.get(CONFIG_PARAMS.STORAGE_DEBUGLONGTRANSACTIONS); export const TRANSACTION_STATE = { - CLOSED: 0, // the transaction has been committed or aborted and can no longer be used for writes (if read txn is active, it can be used for reads) + CLOSED: 0, // the transaction completed; `aborted` distinguishes terminal failure from a successful close OPEN: 1, // the transaction is open and can be used for reads and writes LINGERING: 2, // the transaction has completed a read, but can be used for immediate writes }; @@ -52,6 +52,10 @@ export function transactionOpenTooLongError(): ServerError { ); } +export function transactionAbortedError(): ServerError { + return new ServerError('Can not write to a transaction that has been aborted'); +} + type MaybePromise = T | Promise; export type CommitOptions = { @@ -119,6 +123,8 @@ export class DatabaseTransaction implements Transaction { declare stackTraces?: StartedTransaction[]; overloadChecked: boolean; open = TRANSACTION_STATE.OPEN; + // A successful CLOSED transaction may support a later immediate write; an aborted one must never do so. + aborted = false; replicatedConfirmation: number; // Set when this transaction is applying data from a canonical source of truth (replication peer // or external caching source); its commits retry transient conflicts without the request-path @@ -204,6 +210,7 @@ export class DatabaseTransaction implements Transaction { addWrite(operation: TransactionWrite) { if (this.timedOut) throw transactionOpenTooLongError(); + this.assertNotAborted(); this.writes.push(operation); if (!operation.deferSave) { // Setting saved to false means to defer saving @@ -219,6 +226,8 @@ export class DatabaseTransaction implements Transaction { } save(operation: TransactionWrite, transaction?: RocksTransaction, reloadEntry = false, options?: CommitOptions) { + if (this.timedOut) throw transactionOpenTooLongError(); + this.assertNotAborted(); let txnTime = this.timestamp; transaction ??= this.transaction; let immediateCommit = false; @@ -279,6 +288,7 @@ export class DatabaseTransaction implements Transaction { */ commit(options: CommitOptions = {}): MaybePromise { if (this.timedOut) throw transactionOpenTooLongError(); + this.assertNotAborted(); let transaction = options.transaction ?? this.transaction; // we need to preserve this transaction as we might to resurrect it if we have to retry for (let i = 0; i < this.writes.length; i++) { let operation = this.writes[i]; @@ -547,6 +557,7 @@ export class DatabaseTransaction implements Transaction { ); } abort(): void { + this.aborted = true; while (this.readTxnsUsed > 0) this.doneReadTxn(); // release the read snapshot when we abort, we assume we don't need it this.open = TRANSACTION_STATE.CLOSED; for (const write of this.writes) { @@ -556,6 +567,10 @@ export class DatabaseTransaction implements Transaction { // reset the transaction this.writes = []; if (this.#context?.resourceCache) this.#context.resourceCache = null; + this.next?.abort(); + } + assertNotAborted(): void { + if (this.aborted) throw transactionAbortedError(); } /** * True if this transaction — or any database in its multi-store `next` chain — has writes accumulated diff --git a/resources/LMDBTransaction.ts b/resources/LMDBTransaction.ts index 2c0218847..902ea51da 100644 --- a/resources/LMDBTransaction.ts +++ b/resources/LMDBTransaction.ts @@ -15,7 +15,7 @@ import type { RootDatabaseKind } from './databases.ts'; const MAX_OPTIMISTIC_SIZE = 100; const trackedTxns = new Set(); export const TRANSACTION_STATE = { - CLOSED: 0, // the transaction has been committed or aborted and can no longer be used for writes (if read txn is active, it can be used for reads) + CLOSED: 0, // the transaction completed; inherited `aborted` distinguishes failure from a successful close OPEN: 1, // the transaction is open and can be used for reads and writes LINGERING: 2, // the transaction has completed a read, but can be used for immediate writes }; @@ -89,6 +89,7 @@ export class LMDBTransaction extends DatabaseTransaction { addWrite(operation: TransactionWrite): any { if (this.timedOut) throw transactionOpenTooLongError(); + this.assertNotAborted(); if (this.open === TRANSACTION_STATE.CLOSED) { throw new Error('Can not use a transaction that is no longer open'); } @@ -119,6 +120,7 @@ export class LMDBTransaction extends DatabaseTransaction { */ commit(options: CommitOptions = {}): any { if (this.timedOut) throw transactionOpenTooLongError(); + this.assertNotAborted(); options = options || {}; let txnTime = this.timestamp; if (!txnTime) txnTime = this.timestamp = options.timestamp || getNextMonotonicTime(); @@ -309,6 +311,7 @@ export class LMDBTransaction extends DatabaseTransaction { return txnResolution; } abort(): void { + this.aborted = true; while (this.readTxnsUsed > 0) this.doneReadTxn(); // release the read snapshot when we abort, we assume we don't need it this.open = TRANSACTION_STATE.CLOSED; // any blobs that were pre-saved as part of these writes will never be referenced; schedule deletion @@ -320,8 +323,10 @@ export class LMDBTransaction extends DatabaseTransaction { } // reset the transaction this.writes = []; + this.next?.abort(); } save(..._args: any[]): any { + this.assertNotAborted(); // noop for LMDB } } diff --git a/resources/Resource.ts b/resources/Resource.ts index d666b949d..1d962a3ea 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -138,8 +138,40 @@ export class Resource implements ResourceInterface< */ static put = transactional( function (resource: any, query: RequestTarget, request: Context, data: any) { + // A collection put's entry check is deferred to here by the authorization wrapper for + // tables with an overridden allow* hook (the entry only sees the possibly-unresolved + // body; `data` is resolved by now). An armed query.checkPermission is consumed by these + // checks; for other classes the wrapper ran the entry check and cleared it. if (Array.isArray(data) && resource.#isCollection && resource.constructor.loadAsInstance !== false) { - const results = []; + // Each element is authorized individually — create-vs-update chosen by that element's + // existence, `this` = the element's resource with its record loaded — and any denial + // fails the whole write: the thrown AccessViolation aborts the transaction, so no + // partial batch is committed. + const checkPermission = query.checkPermission; + if (checkPermission) query.checkPermission = false; + const authorizeElement = (elementResource: any, element: any) => { + if (!checkPermission) return true; + let allowed; + try { + allowed = + elementResource.doesExist?.() === false + ? elementResource.allowCreate(request.user, element, request) + : elementResource.allowUpdate(request.user, element, request); + } catch { + throw new AccessViolation(request.user); + } + return when( + allowed, + (allowed) => { + if (!allowed) throw new AccessViolation(request.user); + return true; + }, + () => { + throw new AccessViolation(request.user); + } + ); + }; + const authorizations = []; for (const element of data) { const resourceClass = resource.constructor; const id = element[resourceClass.primaryKey]; @@ -148,16 +180,61 @@ export class Resource implements ResourceInterface< const elementResource = resourceClass.getResource(target, request, { async: true, }); - if (elementResource.then) results.push(elementResource.then((resource) => resource.put(element, request))); - else results.push(elementResource.put(element, query)); + // Both branches put with the QUERY as the (legacy-position) second argument. The async + // branch used to pass `request` (the context) — the context is not a URLSearchParams, + // so put()'s legacy argument shift never engaged and the CONTEXT was written as the + // record body whenever an element resource resolved asynchronously (cyclic object → + // encoder stack overflow). typeof-check: a record attribute named `then` reads through + // the resource's attribute proxy, so truthiness alone could misroute a sync resource. + if (typeof elementResource.then === 'function') + authorizations.push( + elementResource.then((resource) => + when(authorizeElement(resource, element), () => ({ resource, element })) + ) + ); + else + authorizations.push( + when(authorizeElement(elementResource, element), () => ({ resource: elementResource, element })) + ); } - return Promise.all(results); + // Authorization is deliberately a separate phase: Promise.all rejects before any write starts, + // so a slower grant cannot save after a sibling denial aborts and closes the transaction. + return Promise.all(authorizations).then((authorizedElements) => + Promise.all(authorizedElements.map(({ resource, element }) => resource.put(element, query))) + ); + } + if (query.checkPermission && resource.constructor.loadAsInstance !== false) { + // Deferred entry check for a non-array body: the same single check the entry would have + // run, now against the resolved body. + query.checkPermission = false; + let allowed; + try { + allowed = + resource.doesExist?.() === false + ? resource.allowCreate(request.user, data, request) + : resource.allowUpdate(request.user, data, request); + } catch { + throw new AccessViolation(request.user); + } + return when( + allowed, + (allowed) => { + if (!allowed) throw new AccessViolation(request.user); + return doPut(); + }, + () => { + throw new AccessViolation(request.user); + } + ); + } + return doPut(); + function doPut() { + return resource.put + ? resource.constructor.loadAsInstance === false + ? resource.put(query, data) + : resource.put(data, query) + : missingMethod(resource, 'put'); } - return resource.put - ? resource.constructor.loadAsInstance === false - ? resource.put(query, data) - : resource.put(data, query) - : missingMethod(resource, 'put'); }, { hasContent: true, type: 'update', method: 'put' } ); @@ -545,6 +622,31 @@ function warnAsyncAllowReadOnce(tableName: string | undefined): void { // execution (#1422 gap 2 / #1241) — with `this` being each record — instead of once at collection // entry where `this` has no record to inspect. Table.ts marks its table-level default the same way. (Resource.prototype.allowRead as any).isDefaultAllowRead = true; +// Same for the write hooks: an application override on a query-shaped delete is evaluated per +// matching record inside Table.delete (with `this` a per-row resource instance), and an override +// of allowUpdate/allowCreate on a collection put is evaluated per element in the static put +// action — instead of once at collection entry where `this` has no record to inspect. The write +// paths are async, so unlike the record-scoped allowRead, async overrides participate in +// per-record evaluation too. Framework defaults (marked here and in Table.ts) keep the single +// entry check, so non-overriding tables see zero change. +(Resource.prototype.allowDelete as any).isDefaultAllowDelete = true; +(Resource.prototype.allowUpdate as any).isDefaultAllowUpdate = true; +(Resource.prototype.allowCreate as any).isDefaultAllowCreate = true; + +// Warn once per class when a query-shaped delete carries a record-scoped (overridden) allowDelete +// but the class ALSO overrides delete() — the override loses the framework delete's +// enforcesCheckPermission marker, so per-record enforcement silently reverts to the +// collection-scope entry check (a collection-permissive "open, filter per record" override would +// fail open there; a typical owner-check override fails closed). +const unenforcedRecordScopedDeleteWarned = new Set(); +function warnUnenforcedRecordScopedDelete(className: string | undefined): void { + const key = className ?? ''; + if (unenforcedRecordScopedDeleteWarned.has(key)) return; + unenforcedRecordScopedDeleteWarned.add(key); + logger.warn?.( + `"${key}" overrides both allowDelete and delete(): query-shaped deletes get NO per-record allowDelete enforcement (the overridden delete() replaces the framework's record-scoped path), only the collection-scope entry check. The overridden delete() must enforce authorization itself.` + ); +} export function snakeCase(camelCase: string) { return ( @@ -702,6 +804,10 @@ function transactional( if (!query) { query = new RequestTarget(); query.id = id; + // carry the collection-ness the argument parsing determined (e.g. a `put(records, context)` + // array form) onto the query, so the resolved resource is marked a collection like it would + // be for an equivalent REST target + if (isCollection) query.isCollection = true; } isCollection = query.isCollection; let resourceOptions; @@ -735,15 +841,16 @@ function transactional( // opposite treatment: // - committed: the prior call ran to completion normally. Safe to silently start a fresh // transaction for this independent call (the case above). - // - aborted due to exceeding storage.maxTransactionOpenTime (#1411, DatabaseTransaction.ts - // abortDueToTimeout(), marked via the `timedOut` poison flag): the whole logical operation - // must fail atomically, not have this write quietly land on a brand-new transaction while - // the earlier write(s) were rolled back. Joining the poisoned transaction here (instead of - // starting fresh) makes the write throw transactionOpenTooLongError via addWrite()/commit()'s - // poison check, correctly propagating the abort to the caller. See - // integrationTests/resources/txn-overtime-atomicity.test.ts. - if (context?.transaction?.open === TRANSACTION_STATE.OPEN || context?.transaction?.timedOut) { - // we are already in a transaction (or it was poisoned by a timeout abort and must fail), proceed + // - aborted: the whole logical operation must fail atomically, not have this write quietly land + // on a brand-new transaction while earlier writes were rolled back. Normal aborts carry the + // `aborted` poison; timeout aborts (#1411) additionally carry `timedOut` so they retain their + // actionable 422. Joining the poisoned transaction makes addWrite()/save()/commit() reject. + if ( + context?.transaction?.open === TRANSACTION_STATE.OPEN || + context?.transaction?.timedOut || + context?.transaction?.aborted + ) { + // we are already in a transaction (or it was poisoned by an abort and must fail), proceed const resource = this.getResource(query, context, resourceOptions); return resource.then ? resource.then(authorizeActionOnResource) @@ -828,6 +935,53 @@ function transactional( return runAction(data); }); } + // Write-side record scoping: collection-shaped writes with an application-overridden + // allow* hook defer their entry check to the point where per-record/per-element + // evaluation is possible (these write paths are async, so unlike the record-scoped + // allowRead, async overrides participate too). Framework defaults keep the single + // entry check — non-overriding tables see zero change (including the collection-scope + // allowCreate/insert RBAC operand for array puts). + // - A query-shaped delete with an overridden allowDelete is enforced per matching + // record inside the framework delete, with FILTER semantics (denied rows are + // skipped). Gated on the resolved delete method carrying the framework's + // enforcesCheckPermission marker: a subclass that overrides delete() loses the + // marker and keeps today's entry check (warned — the entry verdict may not be what + // a record-scoped override means). + // - A put into a collection with an overridden allowUpdate/allowCreate defers into + // the static put action, which sees the RESOLVED body (here it may still be a + // promise): an array body is checked per element (create-vs-update chosen by that + // element's existence; any denial fails the whole write), any other body gets the + // same single check the entry would have run. The action enforces for every class, + // so no capability marker is needed. + // Both write deferrals apply only to tables (supportsRecordScopedWriteAuth): a plain + // Resource subclass has no records for per-record evaluation to be meaningful — its + // overrides were written against the single entry check (e.g. an array-put allowUpdate + // receiving the whole array) and keep it. + const supportsRecordScopedWriteAuth = (resource.constructor as any)?.supportsRecordScopedWriteAuth; + if (options.method === 'delete') { + if ( + supportsRecordScopedWriteAuth && + query.isCollection && + !(resource.allowDelete as any)?.isDefaultAllowDelete + ) { + if ((resource.delete as any)?.enforcesCheckPermission) { + return when(data, (data) => { + return runAction(data); + }); + } + warnUnenforcedRecordScopedDelete((resource.constructor as any)?.name); + } + } else if ( + options.method === 'put' && + supportsRecordScopedWriteAuth && + query.isCollection && + (!(resource.allowUpdate as any)?.isDefaultAllowUpdate || + !(resource.allowCreate as any)?.isDefaultAllowCreate) + ) { + return when(data, (data) => { + return runAction(data); + }); + } // do permission checks, with allow methods let allowed; try { diff --git a/resources/Table.ts b/resources/Table.ts index ee21aa975..6ccab2725 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -1428,27 +1428,20 @@ export function makeTable(options) { * Determine if the user is allowed to create new data in the current resource */ // @ts-expect-error Tables only allow synchronous allowCreate checks. - allowCreate(user: User, newData: Record, context: Context): boolean { - if (this.isCollection) { - const tablePermission = getTablePermissions(user); - if (tablePermission?.insert) { - const attribute_permissions = tablePermission.attribute_permissions; - if (attribute_permissions?.length > 0) { - // if attribute permissions are defined, we need to ensure there is a select that only returns the attributes the user has permission to - const attrsForType = attributesAsObject(attribute_permissions, 'insert'); - for (const key in newData) { - if (!attrsForType[key]) return false; - } - return checkContextPermissions(this.getContext()); - } else { - return checkContextPermissions(this.getContext()); + allowCreate(user: User, newData: Record, _context: Context): boolean { + const tablePermission = getTablePermissions(user); + if (tablePermission?.insert) { + const attribute_permissions = tablePermission.attribute_permissions; + if (attribute_permissions?.length > 0) { + // if attribute permissions are defined, we need to ensure there is a select that only returns the attributes the user has permission to + const attrsForType = attributesAsObject(attribute_permissions, 'insert'); + for (const key in newData) { + if (!attrsForType[key]) return false; } + return checkContextPermissions(this.getContext()); + } else { + return checkContextPermissions(this.getContext()); } - } else { - // creating *within* a record resource just means we are adding some data to a current record, which is - // an update to the record, it is not an insert of a new record into the table, so not a table create operation - // so does not use table insert permissions - return this.allowUpdate(user, newData, context); } } @@ -1841,35 +1834,73 @@ export function makeTable(options) { // `when` settles the embed hook before `save()` so the write is staged first. return when((this as any).update(target, true), () => this.save() as any) as any; } else { - let allowed = true; if (target == undefined) throw new TypeError('Can not put a record without a target'); const context = this.getContext(); - if ((target as any).checkPermission) { - // requesting authorization verification - allowed = this.allowUpdate((context as any).user, record, context); - } - return when(allowed, (allowed) => { - if (!allowed) { - throw new AccessViolation((context as any).user); - } - // standard path, handle arrays as multiple updates, and otherwise do a direct update - if (Array.isArray(record)) { - // Capture each element's operation synchronously (before any async `@embed` - // hook resolves): `#savingOperation` is a single field that parallel writes - // would otherwise clobber, so a deferred `save()` would commit the wrong op - // — e.g. one element's save running before a later element's vector is written. + // standard path, handle arrays as multiple updates, and otherwise do a direct update + if (Array.isArray(record)) { + // When authorization is requested, each element is checked individually (a batch-level + // verdict can't account for per-record state): create-vs-update chosen by that + // element's existence, `this` = a per-row resource so schema properties and + // `super.allow*` work. Any denial fails the whole write — the thrown AccessViolation + // aborts the transaction, so no partial batch is committed. + const checkPermission = (target as any).checkPermission; + const authorizations = record.map((element) => { + const id = element[primaryKey]; + let allowed: any = true; + if (checkPermission) { + const rowResource = new (this.constructor as any)(id, context); + TableResource._updateResource( + rowResource, + primaryStore.getEntry(id, { transaction: txnForContext(context).getReadTxn() }) + ); + try { + allowed = + rowResource.doesExist() === false + ? rowResource.allowCreate((context as any).user, element, context) + : rowResource.allowUpdate((context as any).user, element, context); + } catch { + throw new AccessViolation((context as any).user); + } + } + return when( + allowed, + (allowed) => { + if (!allowed) throw new AccessViolation((context as any).user); + return true; + }, + () => { + throw new AccessViolation((context as any).user); + } + ); + }); + // Keep authorization and writes in separate phases: a late async grant must not save + // after a sibling denial has aborted and closed the transaction. + return Promise.all(authorizations).then(() => { const writes = record.map((element) => { const id = element[primaryKey]; + // Capture each element's operation synchronously (before any async `@embed` + // hook resolves): `#savingOperation` is a single field that parallel writes + // would otherwise clobber, so a deferred `save()` would commit the wrong op + // — e.g. one element's save running before a later element's vector is written. const writePromise = this._writeUpdate(id, element, true); const operation = this.#savingOperation; + this.#savingOperation = null; return when(writePromise, () => this.#saveOperation(operation)); }); - this.#savingOperation = null; return Promise.all(writes) as any; - } else { - const id = requestTargetToId(target as any); - return when(this._writeUpdate(id, record, true), () => this.save() as any); + }); + } + let allowed: any = true; + if ((target as any).checkPermission) { + // requesting authorization verification + allowed = this.allowUpdate((context as any).user, record, context); + } + return when(allowed, (allowed) => { + if (!allowed) { + throw new AccessViolation((context as any).user); } + const id = requestTargetToId(target as any); + return when(this._writeUpdate(id, record, true), () => this.save() as any); }) as any; } // always return undefined @@ -2607,6 +2638,9 @@ export function makeTable(options) { async delete(target: RequestTargetOrId): Promise { if (isSearchTarget(target)) { + if ((target as any).checkPermission && !(this.allowDelete as any)?.isDefaultAllowDelete) { + return this.#deleteWithRecordScopedCheck(target); + } target.select = ['$id']; // just get the primary key of each record so we can delete them for await (const entry of this.search(target)) { this._writeDelete((entry as any).$id); @@ -2632,6 +2666,61 @@ export function makeTable(options) { this._writeDelete(this.getId()); return Boolean(this.#record); } + /** + * Record-scoped allowDelete on a query-shaped delete: the entry check was deferred by the + * authorization wrapper; the application-overridden allowDelete is evaluated once per matching + * record — `this` a per-row resource instance, so schema properties (`this.ownerId`) and + * `super.allowDelete` both work — with FILTER semantics: a denied (or throwing/rejecting — + * fail closed) row is skipped, not fatal. limit/offset are enforced against ALLOWED rows + * (denied rows don't consume pagination slots), mirroring the record-scoped allowRead guard, + * which filters before the query limit applies. + */ + async #deleteWithRecordScopedCheck(target: RequestTarget): Promise { + const context = this.getContext(); + const user = (context as any)?.user; + // search() must not interpret checkPermission (the delete override is the authority on this + // scan, matching the no-read-check semantics query deletes have always had), but the flag + // carries the caller's permission operand, which the default allowDelete reads via + // getTablePermissions — so restore it once search() has synchronously consumed the target. + const checkPermission = (target as any).checkPermission; + const select = target.select; + const limit = target.limit; + const rawOffset = target.offset; + const offset = rawOffset || 0; + let results; + try { + (target as any).checkPermission = false; + target.select = undefined; // the per-record check needs the record's fields, not just $id + target.limit = undefined; + target.offset = undefined; + results = this.search(target); // consumes the scan parameters synchronously + } finally { + // restore what the scan consumed, so the per-record hooks (and any caller) see the + // original target — including the checkPermission permission operand + (target as any).checkPermission = checkPermission; + target.select = select; + target.limit = limit; + target.offset = rawOffset; + } + let allowedCount = 0; + for await (const record of results) { + const id = (record as any)[primaryKey]; + const rowResource = new (this.constructor as any)(id, context); + rowResource.setRecord(record); + let allowed; + try { + allowed = rowResource.allowDelete(user, target, context); + if ((allowed as any)?.then) allowed = await allowed; + } catch { + allowed = false; // fail closed: the denied row is skipped + } + if (!allowed) continue; + if (allowedCount++ < offset) continue; + if (limit !== undefined && allowedCount - offset > limit) break; + this._writeDelete(id); + } + return true; + } _writeDelete(id: Id, options?: any) { const context = this.getContext(); const transaction = txnForContext(context); @@ -4713,6 +4802,22 @@ export function makeTable(options) { // Resource.ts that this class has the per-record machinery to defer collection reads to. (TableResource.prototype.allowRead as any).isDefaultAllowRead = true; (TableResource as any).supportsRowLevelAllowRead = true; + // Same for allowDelete: the table-level default is a record-independent RBAC check and keeps + // its single entry evaluation; an application OVERRIDE on a query-shaped delete is evaluated + // per matching record (see #deleteWithRecordScopedCheck). The enforcesCheckPermission marker on + // the framework delete tells the authorization wrapper the deferred (still-armed) + // checkPermission WILL be enforced — a subclass that overrides delete() loses the marker, so + // its query deletes keep the entry check instead of silently skipping authorization. + // supportsRecordScopedWriteAuth is the write-side analogue of supportsRowLevelAllowRead: only + // tables have records for per-record/per-element evaluation to be meaningful, so the wrapper + // defers write entry checks (and warns about unenforceable delete overrides) only for classes + // carrying it. A plain Resource subclass defines its own class/instance semantics and keeps the + // single entry check its overrides were written against. + (TableResource.prototype.allowDelete as any).isDefaultAllowDelete = true; + (TableResource.prototype.delete as any).enforcesCheckPermission = true; + (TableResource.prototype.allowUpdate as any).isDefaultAllowUpdate = true; + (TableResource.prototype.allowCreate as any).isDefaultAllowCreate = true; + (TableResource as any).supportsRecordScopedWriteAuth = true; // Records can be asked directly (`record.allowRead(user, target, context)`), delegating to the // table's allowRead with `this` = the record. Non-enumerable so it never serializes; defined on // the per-table record prototype alongside computed properties. Engine-level enforcement does @@ -5063,6 +5168,7 @@ export function makeTable(options) { // Inherit never-drop-on-conflict so a source-applied multi-store transaction doesn't // drop the canonical write when a secondary store hits a transient conflict. transaction.next.sourceApply = transaction.sourceApply; + transaction.next.aborted = transaction.aborted; // Inherit the replay marker so a multi-table replay transaction skips validation on // every store, not just the first (harper#1316). transaction.next.isReplay = transaction.isReplay; diff --git a/resources/transaction.ts b/resources/transaction.ts index 906c77496..c8ac77c60 100644 --- a/resources/transaction.ts +++ b/resources/transaction.ts @@ -1,6 +1,11 @@ import type { Context } from './ResourceInterface.ts'; import { _assignPackageExport } from '../globals.js'; -import { DatabaseTransaction, type Transaction, TRANSACTION_STATE } from './DatabaseTransaction.ts'; +import { + DatabaseTransaction, + transactionAbortedError, + type Transaction, + TRANSACTION_STATE, +} from './DatabaseTransaction.ts'; import { AsyncLocalStorage } from 'async_hooks'; export const contextStorage = new AsyncLocalStorage(); @@ -32,6 +37,7 @@ export function transaction( if (typeof callback !== 'function') { throw new TypeError('Callback function must be provided to transaction'); } + if (context?.transaction?.aborted) throw transactionAbortedError(); if (context?.transaction?.open === TRANSACTION_STATE.OPEN && typeof callback === 'function') { return callback(context.transaction); // nothing to be done, already in open transaction } diff --git a/unitTests/resources/recordScopedWriteAuth.test.js b/unitTests/resources/recordScopedWriteAuth.test.js new file mode 100644 index 000000000..bf0c32dec --- /dev/null +++ b/unitTests/resources/recordScopedWriteAuth.test.js @@ -0,0 +1,565 @@ +// Record-scoped WRITE authorization (the write-side corollary to #1786's record-scoped allowRead): +// - An application-overridden allowDelete on a query-shaped delete is evaluated once per matching +// record (`this` = a per-row resource instance) with FILTER semantics — denied rows are skipped. +// - Array puts are authorized per element (create-vs-update chosen by that element's existence) +// with FAIL semantics — any denial aborts the whole write's transaction. +// - Framework defaults keep their single entry check. +require('../testUtils'); +const assert = require('node:assert'); +const { setupTestDBPath } = require('../testUtils'); +const { table } = require('#src/resources/databases'); +const { Resource } = require('#src/resources/Resource'); +const { RequestTarget } = require('#src/resources/RequestTarget'); +const { setMainIsWorker } = require('#js/server/threads/manageThreads'); +const { transaction } = require('#src/resources/transaction'); + +async function fromAsync(iterable) { + const results = []; + for await (const value of iterable) results.push(value); + return results; +} + +function permissionFor(tablePermission) { + return { + role: { + permission: { + test: { + tables: { + WriteAuthDocs: { read: true, insert: true, update: true, delete: true, ...tablePermission }, + }, + }, + }, + }, + }; +} + +const isAccessViolation = (error) => error.statusCode === 403 || /unauthorized/i.test(error.message); + +describe('record-scoped write authorization', () => { + let Docs; + let alice; + + before(async function () { + setupTestDBPath(); + setMainIsWorker(true); + Docs = table({ + table: 'WriteAuthDocs', + database: 'test', + attributes: [ + { name: 'id', isPrimaryKey: true }, + { name: 'owner', indexed: true }, + { name: 'kind', indexed: true }, + { name: 'label' }, + ], + }); + alice = { username: 'alice', ...permissionFor({}) }; + }); + + async function seed(kind, rows) { + for (const row of rows) await Docs.put({ kind, ...row }); + } + async function idsOf(kind) { + const records = await fromAsync(Docs.search(new RequestTarget(`?kind=${kind}`), {})); + return records.map((record) => record.id).sort(); + } + + describe('query-shaped delete with an overridden allowDelete', () => { + it('sync override filters per record: only the rows it allows are deleted', async function () { + class OwnedDelete extends Docs { + allowDelete(user) { + return this.owner === user.username; + } + } + await seed('t1', [ + { id: 't1-a1', owner: 'alice' }, + { id: 't1-a2', owner: 'alice' }, + { id: 't1-b1', owner: 'bob' }, + { id: 't1-b2', owner: 'bob' }, + ]); + const result = await OwnedDelete.delete(new RequestTarget('?kind=t1'), { user: alice, authorize: true }); + assert.strictEqual(result, true); + assert.deepStrictEqual(await idsOf('t1'), ['t1-b1', 't1-b2'], "bob's rows must survive alice's query delete"); + }); + + it('async override participates per record too (the delete path can await)', async function () { + class AsyncOwnedDelete extends Docs { + async allowDelete(user) { + return this.owner === user.username; + } + } + await seed('t2', [ + { id: 't2-a1', owner: 'alice' }, + { id: 't2-b1', owner: 'bob' }, + ]); + await AsyncOwnedDelete.delete(new RequestTarget('?kind=t2'), { user: alice, authorize: true }); + assert.deepStrictEqual(await idsOf('t2'), ['t2-b1']); + }); + + it('a throwing override fails closed: the row is skipped, the request does not error', async function () { + class ThrowyDelete extends Docs { + allowDelete() { + throw new Error('boom'); + } + } + await seed('t3', [ + { id: 't3-a1', owner: 'alice' }, + { id: 't3-b1', owner: 'bob' }, + ]); + const result = await ThrowyDelete.delete(new RequestTarget('?kind=t3'), { user: alice, authorize: true }); + assert.strictEqual(result, true, 'filter semantics: a denied row is skipped, not fatal'); + assert.deepStrictEqual(await idsOf('t3'), ['t3-a1', 't3-b1'], 'no rows may be deleted'); + }); + + it('super.allowDelete composes: the RBAC baseline gates the override', async function () { + class ComposedDelete extends Docs { + allowDelete(user, target, context) { + if (!super.allowDelete(user, target, context)) return false; + return this.owner === user.username; + } + } + await seed('t4', [ + { id: 't4-a1', owner: 'alice' }, + { id: 't4-b1', owner: 'bob' }, + ]); + // alice without the role's delete grant: super denies every row + const aliceNoDelete = { username: 'alice', ...permissionFor({ delete: false }) }; + await ComposedDelete.delete(new RequestTarget('?kind=t4'), { user: aliceNoDelete, authorize: true }); + assert.deepStrictEqual(await idsOf('t4'), ['t4-a1', 't4-b1']); + // with the grant, the per-row owner check applies + await ComposedDelete.delete(new RequestTarget('?kind=t4'), { user: alice, authorize: true }); + assert.deepStrictEqual(await idsOf('t4'), ['t4-b1']); + }); + + it('preserves a target-supplied permission operand for super.allowDelete (REST checkPermission form)', async function () { + class ComposedDelete extends Docs { + allowDelete(user, target, context) { + if (!super.allowDelete(user, target, context)) return false; + return this.owner === user.username; + } + } + await seed('t5', [ + { id: 't5-a1', owner: 'alice' }, + { id: 't5-b1', owner: 'bob' }, + ]); + // REST arms checkPermission with the PERMISSION OBJECT (not `true`); the record-scoped scan + // must restore it so getTablePermissions still sees the operand — the user carries no role. + const target = new RequestTarget('?kind=t5'); + target.checkPermission = alice.role.permission; + await ComposedDelete.delete(target, { user: { username: 'alice' } }); + assert.deepStrictEqual(await idsOf('t5'), ['t5-b1'], 'the operand must reach super.allowDelete per row'); + }); + + it('non-overridden allowDelete keeps the single entry check (no per-record deferral)', async function () { + await seed('t6', [ + { id: 't6-a1', owner: 'alice' }, + { id: 't6-b1', owner: 'bob' }, + ]); + // entry denial: no delete grant fails the whole request + await assert.rejects( + async () => + Docs.delete(new RequestTarget('?kind=t6'), { user: permissionFor({ delete: false }), authorize: true }), + isAccessViolation + ); + assert.deepStrictEqual(await idsOf('t6'), ['t6-a1', 't6-b1']); + // entry grant: the whole matching set is deleted, regardless of row ownership + await Docs.delete(new RequestTarget('?kind=t6'), { user: alice, authorize: true }); + assert.deepStrictEqual(await idsOf('t6'), []); + }); + + it('limit counts ALLOWED rows: denied rows do not consume pagination slots', async function () { + class OwnedDelete extends Docs { + allowDelete(user) { + return this.owner === user.username; + } + } + // interleave owners so denied rows sit between allowed ones + await seed('t7', [ + { id: 't7-1', owner: 'alice' }, + { id: 't7-2', owner: 'bob' }, + { id: 't7-3', owner: 'alice' }, + { id: 't7-4', owner: 'bob' }, + { id: 't7-5', owner: 'alice' }, + ]); + const target = new RequestTarget('?kind=t7'); + target.limit = 2; + await OwnedDelete.delete(target, { user: alice, authorize: true }); + const remaining = await idsOf('t7'); + assert.strictEqual(remaining.filter((id) => id.match(/t7-[24]/)).length, 2, "bob's rows must all survive"); + assert.strictEqual(remaining.length, 3, 'exactly limit=2 ALLOWED rows are deleted'); + }); + + it('a subclass overriding delete() keeps the entry check — no per-record deferral (enforcesCheckPermission gate)', async function () { + // The overridden delete() loses the framework marker, so the wrapper must NOT defer. + // Leg 1: a typical record-scoped override fails CLOSED at collection scope (`this.owner` + // undefined on the collection resource) — the whole request 403s, nothing deleted, and + // the custom delete() is never reached. + let deleteCalls = 0; + class CustomDelete extends Docs { + allowDelete(user) { + return this.owner === user.username; + } + delete(target) { + deleteCalls++; + return super.delete(target); + } + } + await seed('t15', [ + { id: 't15-a1', owner: 'alice' }, + { id: 't15-b1', owner: 'bob' }, + ]); + await assert.rejects( + async () => CustomDelete.delete(new RequestTarget('?kind=t15'), { user: alice, authorize: true }), + isAccessViolation + ); + assert.strictEqual(deleteCalls, 0, 'the entry denial must fail the request before the custom delete runs'); + assert.deepStrictEqual(await idsOf('t15'), ['t15-a1', 't15-b1'], 'nothing may be deleted'); + + // Leg 2: a collection-permissive override (RBAC at collection scope) passes the entry + // check, the custom delete() runs, and — the documented downgrade — NO per-record + // filtering applies: every matching row is deleted, including bob's. + class PermissiveCustomDelete extends Docs { + allowDelete(user, target, context) { + if (this?.owner === undefined || this?.owner === null) return super.allowDelete(user, target, context); + return this.owner === user.username; + } + delete(target) { + deleteCalls++; + return super.delete(target); + } + } + await PermissiveCustomDelete.delete(new RequestTarget('?kind=t15'), { user: alice, authorize: true }); + assert.strictEqual(deleteCalls, 1, 'the custom delete must run once the entry check grants'); + assert.deepStrictEqual( + await idsOf('t15'), + [], + 'with delete() overridden, the entry verdict governs the whole scan (documented downgrade)' + ); + }); + + it('limit=0 deletes nothing (parity with the default query-delete path)', async function () { + class OwnedDelete extends Docs { + allowDelete(user) { + return this.owner === user.username; + } + } + await seed('t13', [ + { id: 't13-a1', owner: 'alice' }, + { id: 't13-a2', owner: 'alice' }, + ]); + const target = new RequestTarget('?kind=t13'); + target.limit = 0; + await OwnedDelete.delete(target, { user: alice, authorize: true }); + assert.deepStrictEqual(await idsOf('t13'), ['t13-a1', 't13-a2'], 'limit=0 must not delete any row'); + }); + }); + + describe('array put authorized per element', () => { + let OwnedPut; + before(function () { + OwnedPut = class extends Docs { + allowUpdate(user) { + // `this` = the element's resource with the EXISTING record loaded + return this.owner === user.username; + } + allowCreate(user, record) { + return record.owner === user.username; + } + }; + }); + + it('a denied element fails the whole write and no partial batch is committed', async function () { + await seed('t8', [ + { id: 't8-a1', owner: 'alice', label: 'original' }, + { id: 't8-b1', owner: 'bob', label: 'original' }, + ]); + await assert.rejects( + async () => + OwnedPut.put( + [ + { id: 't8-a1', owner: 'alice', kind: 't8', label: 'updated' }, + { id: 't8-b1', owner: 'bob', kind: 't8', label: 'updated' }, + ], + { user: alice, authorize: true } + ), + isAccessViolation + ); + const records = await fromAsync(Docs.search(new RequestTarget('?kind=t8'), {})); + assert( + records.every((record) => record.label === 'original'), + 'the aborted transaction must not commit ANY element, including the allowed one' + ); + }); + + it('an all-allowed batch writes every element', async function () { + await seed('t9', [ + { id: 't9-a1', owner: 'alice', label: 'original' }, + { id: 't9-a2', owner: 'alice', label: 'original' }, + ]); + await OwnedPut.put( + [ + { id: 't9-a1', owner: 'alice', kind: 't9', label: 'updated' }, + { id: 't9-a2', owner: 'alice', kind: 't9', label: 'updated' }, + ], + { user: alice, authorize: true } + ); + const records = await fromAsync(Docs.search(new RequestTarget('?kind=t9'), {})); + assert( + records.every((record) => record.label === 'updated'), + 'both allowed elements must be written' + ); + }); + + it('chooses allowCreate for a new element and allowUpdate for an existing one', async function () { + await seed('t10', [{ id: 't10-a1', owner: 'alice', label: 'original' }]); + // new element claiming bob as owner: allowCreate denies for alice + await assert.rejects( + async () => + OwnedPut.put( + [ + { id: 't10-a1', owner: 'alice', kind: 't10', label: 'updated' }, + { id: 't10-new-b', owner: 'bob', kind: 't10', label: 'new' }, + ], + { user: alice, authorize: true } + ), + isAccessViolation + ); + assert.deepStrictEqual(await idsOf('t10'), ['t10-a1']); + // new element owned by alice: allowCreate grants + await OwnedPut.put( + [ + { id: 't10-a1', owner: 'alice', kind: 't10', label: 'updated' }, + { id: 't10-new-a', owner: 'alice', kind: 't10', label: 'new' }, + ], + { user: alice, authorize: true } + ); + assert.deepStrictEqual(await idsOf('t10'), ['t10-a1', 't10-new-a']); + }); + + it('super.allowCreate always enforces insert RBAC for a new element', async function () { + class ComposedCreate extends Docs { + allowCreate(user, record, context) { + return super.allowCreate(user, record, context); + } + } + await assert.rejects( + async () => + ComposedCreate.put([{ id: 't16-new', owner: 'alice', kind: 't16', label: 'new' }], { + user: permissionFor({ insert: false, update: true }), + authorize: true, + }), + isAccessViolation + ); + assert.deepStrictEqual(await idsOf('t16'), [], 'update permission must not authorize an insert'); + }); + + it('record-targeted publish uses insert RBAC because it creates a message entry', async function () { + await seed('t19', [{ id: 't19-record', owner: 'alice', label: 'original' }]); + const message = { id: 't19-message', owner: 'alice', kind: 't19', label: 'message' }; + await assert.rejects( + async () => + Docs.publish('t19-record', message, { + user: permissionFor({ insert: false, update: true }), + authorize: true, + }), + isAccessViolation + ); + await Docs.publish('t19-record', message, { + user: permissionFor({ insert: true, update: false }), + authorize: true, + }); + }); + + async function assertAsyncDenialIsAtomic(kind, invokePut) { + const slowId = `${kind}-slow`; + let releaseSlowAuthorization; + let slowAuthorization; + let lateWriteAttempted = false; + class AsyncDeniedPut extends Docs { + allowUpdate() { + if (this.getId() !== slowId) return false; + slowAuthorization = new Promise((resolve) => { + releaseSlowAuthorization = () => resolve(true); + }); + return slowAuthorization; + } + _writeUpdate(id, ...args) { + if (id === slowId) lateWriteAttempted = true; + return super._writeUpdate(id, ...args); + } + } + await seed(kind, [ + { id: slowId, owner: 'alice', label: 'original' }, + { id: `${kind}-denied`, owner: 'bob', label: 'original' }, + ]); + const batch = [ + { id: slowId, owner: 'alice', kind, label: 'updated' }, + { id: `${kind}-denied`, owner: 'bob', kind, label: 'updated' }, + ]; + await assert.rejects(async () => invokePut(AsyncDeniedPut, batch), isAccessViolation); + assert(releaseSlowAuthorization, 'the slow authorization must have started before the denial'); + releaseSlowAuthorization(); + await slowAuthorization; + await Promise.resolve(); + assert.strictEqual(lateWriteAttempted, false, 'no write may start after another element denies'); + const records = await fromAsync(Docs.search(new RequestTarget(`?kind=${kind}`), {})); + assert(records.every((record) => record.label === 'original')); + } + + it('authorizes the full static array put before staging any writes', async function () { + await assertAsyncDenialIsAtomic('t17', (PutClass, batch) => + PutClass.put(batch, { user: alice, authorize: true }) + ); + }); + + it('authorizes the full direct instance array put before staging any writes', async function () { + await assertAsyncDenialIsAtomic('t18', (PutClass, batch) => { + const context = { user: alice }; + return transaction(context, () => { + const collection = new PutClass(null, context); + const target = new RequestTarget('?'); + target.checkPermission = true; + return collection.put(target, batch); + }); + }); + }); + + it('default hooks keep the single collection-entry check (insert RBAC governs array puts)', async function () { + await seed('t11', [{ id: 't11-a1', owner: 'alice', label: 'original' }]); + // No app override → no per-element deferral: the collection-scope allowCreate (INSERT + // operand) still authorizes the whole batch, exactly as before record scoping. + await assert.rejects( + async () => + Docs.put([{ id: 't11-a1', owner: 'alice', kind: 't11', label: 'updated' }], { + user: permissionFor({ insert: false }), + authorize: true, + }), + isAccessViolation + ); + await Docs.put([{ id: 't11-a1', owner: 'alice', kind: 't11', label: 'updated' }], { + user: alice, + authorize: true, + }); + const [record] = await fromAsync(Docs.search(new RequestTarget('?kind=t11'), {})); + assert.strictEqual(record.label, 'updated'); + }); + + it('async-resolving element resources write the ELEMENT, not the context (legacy arg-shift bug)', async function () { + // When getResource resolves asynchronously, the static put loop used to call + // put(element, request) — the context is not a URLSearchParams, so put() treated the + // CONTEXT as the record body (cyclic object → encoder stack overflow on CI). + let AsyncResolve; + AsyncResolve = class extends OwnedPut { + static getResource(target, context, options) { + return Promise.resolve(super.getResource(target, context, options)); + } + }; + await seed('t14', [{ id: 't14-a1', owner: 'alice', label: 'original' }]); + await AsyncResolve.put( + [ + { id: 't14-a1', owner: 'alice', kind: 't14', label: 'updated' }, + { id: 't14-new', owner: 'alice', kind: 't14', label: 'new' }, + ], + { user: alice, authorize: true } + ); + const records = await fromAsync(Docs.search(new RequestTarget('?kind=t14'), {})); + assert.deepStrictEqual( + records.map((record) => [record.id, record.label]).sort(), + [ + ['t14-a1', 'updated'], + ['t14-new', 'new'], + ], + 'each element body must be written verbatim' + ); + }); + + it('direct instance put(target, array) with checkPermission enforces per element (no wrapper)', async function () { + await seed('t12', [ + { id: 't12-a1', owner: 'alice', label: 'original' }, + { id: 't12-b1', owner: 'bob', label: 'original' }, + ]); + const context = { user: alice }; + await assert.rejects( + async () => + transaction(context, () => { + const collection = new OwnedPut(null, context); + const target = new RequestTarget('?'); + target.checkPermission = true; + return collection.put(target, [ + { id: 't12-a1', owner: 'alice', kind: 't12', label: 'updated' }, + { id: 't12-b1', owner: 'bob', kind: 't12', label: 'updated' }, + ]); + }), + isAccessViolation + ); + const records = await fromAsync(Docs.search(new RequestTarget('?kind=t12'), {})); + assert( + records.every((record) => record.label === 'original'), + 'the aborted transaction must not commit ANY element' + ); + }); + }); + + describe('plain Resource subclasses keep the single entry check (no supportsRecordScopedWriteAuth)', () => { + it('array put: an overridden allowUpdate sees the whole array once, not per element', async function () { + const allowCalls = []; + const putCalls = []; + class PlainEndpoint extends Resource { + allowUpdate(user, data) { + allowCalls.push(data); + return user.username === 'alice'; + } + put(data) { + putCalls.push(data); + } + } + await PlainEndpoint.put( + [ + { id: 'p1', label: 'one' }, + { id: 'p2', label: 'two' }, + ], + { user: alice, authorize: true } + ); + assert.strictEqual(allowCalls.length, 1, 'a plain Resource must keep the single entry check'); + assert(Array.isArray(allowCalls[0]), 'the entry check must receive the whole array, not an element'); + assert.strictEqual(allowCalls[0].length, 2); + assert.strictEqual(putCalls.length, 2, 'the pre-existing per-element put dispatch is unchanged'); + assert( + putCalls.every((data) => !Array.isArray(data)), + 'each dispatched put receives one element' + ); + }); + + it('array put: an entry-check denial still fails the whole request with a 403', async function () { + class DeniedEndpoint extends Resource { + allowUpdate() { + return false; + } + put() { + throw new Error('must not be reached'); + } + } + await assert.rejects( + async () => DeniedEndpoint.put([{ id: 'p1' }], { user: alice, authorize: true }), + isAccessViolation + ); + }); + + it('collection delete: an overridden allowDelete keeps the entry check against the query', async function () { + const allowCalls = []; + let deleted = false; + class PlainDelete extends Resource { + allowDelete(user, target) { + allowCalls.push(target); + return true; + } + delete() { + deleted = true; + return true; + } + } + await PlainDelete.delete(new RequestTarget('?kind=x'), { user: alice, authorize: true }); + assert.strictEqual(allowCalls.length, 1, 'the entry check runs exactly once, against the query'); + assert.strictEqual(deleted, true, 'the custom delete() still runs'); + }); + }); +}); diff --git a/unitTests/resources/transaction.test.js b/unitTests/resources/transaction.test.js index 4c2e12650..89570036c 100644 --- a/unitTests/resources/transaction.test.js +++ b/unitTests/resources/transaction.test.js @@ -5,6 +5,7 @@ const { setupTestDBPath } = require('../testUtils'); const { table } = require('#src/resources/databases'); const { setMainIsWorker } = require('#js/server/threads/manageThreads'); const { transaction } = require('#src/resources/transaction'); +const { DatabaseTransaction } = require('#src/resources/DatabaseTransaction'); const { IterableEventQueue } = require('#src/resources/IterableEventQueue'); const { RocksDatabase } = require('@harperfast/rocksdb-js'); const isLMDB = process.env.HARPER_STORAGE_ENGINE === 'lmdb'; @@ -606,6 +607,35 @@ describe('Transactions', () => { let entity = await TxnTest.get(49); assert.equal(entity.count, 9); }); + it('rejects a prepared write after its transaction is aborted', async function () { + await TxnTest.put(50, { name: 'original' }); + const context = {}; + await assert.rejects( + transaction(context, async (txn) => { + const pending = await TxnTest.update(50, { name: 'must not persist' }, context); + txn.abort(); + return pending.save(); + }), + /transaction that has been aborted/ + ); + assert.equal((await TxnTest.get(50)).name, 'original'); + }); + it('poisons an aborted transaction and its database chain against later writes', async function () { + const txn = new DatabaseTransaction(); + txn.next = new DatabaseTransaction(); + txn.abort(); + assert(txn.aborted); + assert(txn.next.aborted); + assert.throws(() => txn.addWrite({}), /transaction that has been aborted/); + assert.throws(() => txn.next.addWrite({}), /transaction that has been aborted/); + assert.throws(() => txn.commit(), /transaction that has been aborted/); + const context = { transaction: txn }; + await assert.rejects( + async () => TxnTest.put(51, { name: 'must not persist' }, context), + /transaction that has been aborted/ + ); + assert.equal(await TxnTest.get(51), undefined); + }); it('Can update new object and addTo consecutively replication updates', async function () { class WithCountOnGet extends TxnTest { static async get(target) {