diff --git a/packages/authz/src/db.ts b/packages/authz/src/db.ts index 69ba7578..58b04d4d 100644 --- a/packages/authz/src/db.ts +++ b/packages/authz/src/db.ts @@ -7,7 +7,16 @@ import type { AuthzScope, SubjectAssignments } from "./types.ts"; // database-related, including the DDL the CLI scaffolds. export { authzMigrationSql } from "./migrations.ts"; -/** Create the tables if absent. Production apps should use a real migration. */ +/** + * Create the tables if absent. Production apps should use a real migration. + * + * The UNIQUE constraints in this DDL are load-bearing beyond deduplication: + * `grant`/`assignRole` below use ON CONFLICT / ON DUPLICATE KEY, which infers + * its conflict target from them. A hand-rolled migration that recreates these + * tables without `_wrn_authz_grant_unique` (or the assignment equivalent) + * will make those methods reject outright, where the old delete-then-insert + * approach would have silently worked without the constraint. + */ export async function ensureAuthzTables( db: Db, dialect: Dialect = db.driver.dialect, @@ -73,6 +82,10 @@ export function dbPermissionStore(db: Db): PermissionStore { }, async grant(subjectId, permission, effect, scope) { + // `VALUES(effect)` is deprecated as of MySQL 8.0.20 in favour of the + // row-alias form (`... VALUES (...) AS new ON DUPLICATE KEY UPDATE + // effect = new.effect`). Noted here rather than migrated because there + // is no MySQL server in CI to catch its eventual removal. await db.exec( `INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (${p(1)}, ${p(2)}, ${p(3)}, ${p(4)})` + onConflict( diff --git a/packages/authz/src/store.ts b/packages/authz/src/store.ts index 11480bab..3de33d14 100644 --- a/packages/authz/src/store.ts +++ b/packages/authz/src/store.ts @@ -18,16 +18,23 @@ export interface PermissionStore { /** * Global assignments are stored under the empty-string scope key. An OMITTED - * scope means global; an explicitly EMPTY tenantId is refused, because it is - * indistinguishable from global and would let a caller who controls the tenant - * id read and write global assignments. + * scope means global; an explicitly EMPTY or non-string tenantId is refused, + * because an empty string is indistinguishable from global (and would let a + * caller who controls the tenant id read and write global assignments), and a + * non-string value (e.g. `null` from a JSON body or a nullable column) would + * otherwise flow through un-normalised and leave the adapters disagreeing + * about what happened. */ export function scopeKey(scope?: AuthzScope): string { const tenantId = scope?.tenantId; if (tenantId === undefined) return ""; - if (tenantId === "") { + // Guard the TYPE as well as the value: a null from a JSON body or a nullable + // column would otherwise flow through un-normalised and the adapters would + // disagree about what happened - the db rejects on NOT NULL, memory accepts + // an unreachable row. + if (typeof tenantId !== "string" || tenantId === "") { throw new Error( - "WRN-AUTHZ-SCOPE: tenantId must not be empty; omit the scope for a global assignment.", + "WRN-AUTHZ-SCOPE: tenantId must be a non-empty string; omit the scope for a global assignment.", ); } return tenantId; diff --git a/packages/authz/test/db-no-transaction.test.ts b/packages/authz/test/db-no-transaction.test.ts new file mode 100644 index 00000000..60a89e7f --- /dev/null +++ b/packages/authz/test/db-no-transaction.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, test } from "bun:test"; +import type { Db, Dialect, Driver, ExecResult, Row, TxHandle } from "@wrnexus/db"; +import { dbPermissionStore } from "../src/db.ts"; + +/** + * A deterministic regression guard for C1/C2 (round-1 review): grant() used to + * wrap its delete-then-insert in db.tx, and the sqlite driver runs a bare + * BEGIN on one shared, unserialized connection - so an open transaction there + * could sweep in and discard a concurrent bare write from another method. + * Timing-based tests can't reliably prove the absence of that; this can, + * because it needs no concurrency at all - it just asserts the store never + * asks the driver to open a transaction in the first place. + */ +function makeFakeDb(): { db: Db; statements: string[]; transactionCalls: number } { + const statements: string[] = []; + const stats = { transactionCalls: 0 }; + + const driver: Driver = { + dialect: "sqlite" as Dialect, + async query(sql: string): Promise { + statements.push(sql); + return []; + }, + async exec(sql: string): Promise { + statements.push(sql); + return { changes: 0 }; + }, + async transaction(fn: (tx: TxHandle) => Promise): Promise { + // The spy: this must never be called by a transaction-free store. + stats.transactionCalls++; + statements.push("BEGIN"); + return fn(driver); + }, + close() {}, + }; + + const db: Db = { + driver, + async all(sql: string): Promise { + statements.push(sql); + return []; + }, + async one(sql: string): Promise { + statements.push(sql); + return null; + }, + async exec(sql: string): Promise { + statements.push(sql); + return { changes: 0 }; + }, + async tx(fn: (tx: Db) => Promise): Promise { + // Routed through the same driver.transaction spy a real Db would use. + return driver.transaction(() => fn(db)); + }, + async createTable() {}, + close() {}, + }; + + return { + db, + statements, + get transactionCalls() { + return stats.transactionCalls; + }, + }; +} + +describe("dbPermissionStore opens no transaction", () => { + test("the store opens no transaction: a shared-connection rollback would discard concurrent writes from other methods", async () => { + const fake = makeFakeDb(); + const store = dbPermissionStore(fake.db); + + await store.assignmentsFor("u1"); + await store.assignRole("u1", "editor"); + await store.revokeRole("u1", "editor"); + await store.grant("u1", "post:write", "allow"); + await store.revokeGrant("u1", "post:write"); + await store.listSubjects(); + + expect(fake.transactionCalls).toBe(0); + for (const sql of fake.statements) { + expect(sql).not.toContain("BEGIN"); + } + }); +}); diff --git a/packages/authz/test/store-conformance.ts b/packages/authz/test/store-conformance.ts index cb82f99f..f9b41a7d 100644 --- a/packages/authz/test/store-conformance.ts +++ b/packages/authz/test/store-conformance.ts @@ -136,6 +136,15 @@ export function runStoreConformance(name: string, makeStore: () => Promise { + // Same class as the empty-string case: the caller controls this value. + for (const bad of [null, 0, false, {}]) { + await expect(store.assignmentsFor("u1", { tenantId: bad as never })).rejects.toThrow( + /tenantId/, + ); + } + }); + test("concurrent identical assignRole calls all resolve", async () => { // Check-then-act loses this race; the UNIQUE constraint then rejects // every loser even though the desired end state was already reached.