diff --git a/docs/plans/2026-08-04-authz-permissions-implementation.md b/docs/plans/2026-08-04-authz-permissions-implementation.md index d015cef8..d2915789 100644 --- a/docs/plans/2026-08-04-authz-permissions-implementation.md +++ b/docs/plans/2026-08-04-authz-permissions-implementation.md @@ -598,6 +598,42 @@ export function runStoreConformance(name: string, makeStore: () => Promise { + await store.assignRole("g1", "viewer"); + // Otherwise a caller who controls the tenant id reaches global scope. + await expect(store.assignmentsFor("g1", { tenantId: "" })).rejects.toThrow(/tenantId/); + await expect(store.assignRole("g1", "admin", { tenantId: "" })).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. + await Promise.all(Array.from({ length: 20 }, () => store.assignRole("u1", "editor"))); + expect((await store.assignmentsFor("u1")).roles).toEqual(["editor"]); + }); + + test("concurrent grants on distinct keys all resolve", async () => { + await Promise.all([ + store.grant("u1", "post:read", "allow"), + store.grant("u1", "post:write", "allow"), + store.grant("u1", "post:delete", "deny"), + ]); + const assignments = await store.assignmentsFor("u1"); + expect(assignments.grants.sort()).toEqual(["post:read", "post:write"]); + expect(assignments.denies).toEqual(["post:delete"]); + }); + + test("a concurrent write is not lost to another method's failure", async () => { + // A store that wraps one method in a transaction on a shared connection + // will roll back this unrelated write and still resolve successfully. + await store.assignRole("victim", "admin"); + await Promise.all([ + store.revokeRole("victim", "admin"), + store.grant("other", "post:read", "allow").catch(() => undefined), + ]); + expect((await store.assignmentsFor("victim")).roles).toEqual([]); + }); + test("listSubjects with no scope returns global assignees only", async () => { await store.assignRole("g1", "viewer"); await store.assignRole("s1", "editor", { tenantId: "t1" }); @@ -646,9 +682,21 @@ export interface PermissionStore { listSubjects(scope?: AuthzScope): Promise; } -/** Global assignments are stored under the empty-string scope key. */ +/** + * 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. + */ export function scopeKey(scope?: AuthzScope): string { - return scope?.tenantId ?? ""; + const tenantId = scope?.tenantId; + if (tenantId === undefined) return ""; + if (tenantId === "") { + throw new Error( + "WRN-AUTHZ-SCOPE: tenantId must not be empty; omit the scope for a global assignment.", + ); + } + return tenantId; } interface Row { @@ -2645,7 +2693,18 @@ import type { Dialect } from "@wrnexus/db"; * string for a global assignment, so the unique constraints work on every * dialect (NULL is not comparable in a UNIQUE index). */ -export function authzMigrationSql(dialect: Dialect): { up: string; down: string } { +/** + * DDL for the two assignment tables, as a list of statements rather than one + * blob: splitting a blob on a separator makes runtime correctness depend on + * source formatting, and only the sqlite driver accepts multi-statement exec. + * + * `scope` holds a tenant id, or the empty string for a global assignment, so + * the unique constraints work on every dialect (NULL is not comparable in a + * UNIQUE index). `effect` is CHECK-constrained: an unrecognised value would + * otherwise be dropped from both the grant and deny buckets on read, silently + * turning a deny into a no-op. + */ +export function authzMigrationSql(dialect: Dialect): { up: string[]; down: string[] } { const id = dialect === "postgres" ? "SERIAL PRIMARY KEY" @@ -2653,33 +2712,33 @@ export function authzMigrationSql(dialect: Dialect): { up: string; down: string ? "INT AUTO_INCREMENT PRIMARY KEY" : "INTEGER PRIMARY KEY AUTOINCREMENT"; const timestamp = dialect === "sqlite" ? "TEXT" : "TIMESTAMP"; - const now = dialect === "sqlite" ? "CURRENT_TIMESTAMP" : "CURRENT_TIMESTAMP"; + // MySQL's default collation is case- and accent-insensitive, which would let + // tenant "T1" match "t1" and collapse roles "admin"/"Admin" onto one row. + const exact = dialect === "mysql" ? " COLLATE utf8mb4_bin" : ""; + const key = `VARCHAR(255)${exact} NOT NULL`; - const up = `CREATE TABLE IF NOT EXISTS _wrn_authz_assignment ( + return { + up: [ + `CREATE TABLE IF NOT EXISTS _wrn_authz_assignment ( id ${id}, - subject_id VARCHAR(255) NOT NULL, - scope VARCHAR(255) NOT NULL DEFAULT '', - role VARCHAR(255) NOT NULL, - granted_by VARCHAR(255), - created_at ${timestamp} NOT NULL DEFAULT ${now}, + subject_id ${key}, + scope ${key} DEFAULT '', + role ${key}, + created_at ${timestamp} NOT NULL DEFAULT CURRENT_TIMESTAMP, CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role) -); - -CREATE TABLE IF NOT EXISTS _wrn_authz_grant ( +)`, + `CREATE TABLE IF NOT EXISTS _wrn_authz_grant ( id ${id}, - subject_id VARCHAR(255) NOT NULL, - scope VARCHAR(255) NOT NULL DEFAULT '', - permission VARCHAR(255) NOT NULL, - effect VARCHAR(16) NOT NULL, - granted_by VARCHAR(255), - created_at ${timestamp} NOT NULL DEFAULT ${now}, + subject_id ${key}, + scope ${key} DEFAULT '', + permission ${key}, + effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny')), + created_at ${timestamp} NOT NULL DEFAULT CURRENT_TIMESTAMP, CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission) -);`; - - const down = `DROP TABLE IF EXISTS _wrn_authz_grant; -DROP TABLE IF EXISTS _wrn_authz_assignment;`; - - return { up, down }; +)`, + ], + down: ["DROP TABLE IF EXISTS _wrn_authz_grant", "DROP TABLE IF EXISTS _wrn_authz_assignment"], + }; } ``` @@ -2698,69 +2757,85 @@ import type { AuthzScope, SubjectAssignments } from "./types.ts"; export { authzMigrationSql } from "./migrations.ts"; /** Create the tables if absent. Production apps should use a real migration. */ -export async function ensureAuthzTables(db: Db, dialect: Dialect = "sqlite"): Promise { - for (const statement of authzMigrationSql(dialect).up.split(";\n\n")) { - const sql = statement.trim(); - if (sql) await db.exec(sql.endsWith(";") ? sql : `${sql};`); - } +/** Create the tables if absent. Production apps should use a real migration. */ +export async function ensureAuthzTables( + db: Db, + dialect: Dialect = db.driver.dialect, +): Promise { + for (const statement of authzMigrationSql(dialect).up) await db.exec(statement); +} + +/** Positional placeholder for the dialect: postgres numbers them, others use "?". */ +function ph(dialect: Dialect, index: number): string { + return dialect === "postgres" ? `$${index}` : "?"; } export function dbPermissionStore(db: Db): PermissionStore { + const dialect = db.driver.dialect; + const p = (n: number) => ph(dialect, n); + // Single-statement upserts. A transaction here would be worse than useless: + // the drivers run BEGIN on one shared connection, so an open transaction + // swallows any concurrent write from another method and discards it on + // rollback - a revoke would resolve successfully while the role survived. + const onConflict = (columns: string, update: string) => + dialect === "mysql" + ? ` ON DUPLICATE KEY UPDATE ${update}` + : ` ON CONFLICT (${columns}) DO UPDATE SET ${update}`; + const onConflictIgnore = (columns: string) => + dialect === "mysql" + ? " ON DUPLICATE KEY UPDATE id = id" + : ` ON CONFLICT (${columns}) DO NOTHING`; + return { async assignmentsFor(subjectId: string, scope?: AuthzScope): Promise { const key = scopeKey(scope); // A request inside a tenant sees global rows plus that tenant's rows. const roleRows = await db.all<{ role: string }>( - "SELECT role FROM _wrn_authz_assignment WHERE subject_id = ? AND (scope = '' OR scope = ?)", + `SELECT role FROM _wrn_authz_assignment WHERE subject_id = ${p(1)} AND (scope = '' OR scope = ${p(2)})`, [subjectId, key], ); - const grantRows = await db.all<{ permission: string; effect: GrantEffect }>( - "SELECT permission, effect FROM _wrn_authz_grant WHERE subject_id = ? AND (scope = '' OR scope = ?)", + const grantRows = await db.all<{ permission: string; effect: string }>( + `SELECT permission, effect FROM _wrn_authz_grant WHERE subject_id = ${p(1)} AND (scope = '' OR scope = ${p(2)})`, [subjectId, key], ); return { roles: roleRows.map((row) => row.role), grants: grantRows.filter((r) => r.effect === "allow").map((r) => r.permission), - denies: grantRows.filter((r) => r.effect === "deny").map((r) => r.permission), + // Anything that is not literally "allow" counts as a deny, so a + // corrupted or mis-cased effect fails closed rather than vanishing. + denies: grantRows.filter((r) => r.effect !== "allow").map((r) => r.permission), }; }, async assignRole(subjectId, role, scope) { - const key = scopeKey(scope); - const existing = await db.all<{ id: number }>( - "SELECT id FROM _wrn_authz_assignment WHERE subject_id = ? AND scope = ? AND role = ?", - [subjectId, key, role], - ); - if (existing.length) return; await db.exec( - "INSERT INTO _wrn_authz_assignment (subject_id, scope, role) VALUES (?, ?, ?)", - [subjectId, key, role], + `INSERT INTO _wrn_authz_assignment (subject_id, scope, role) VALUES (${p(1)}, ${p(2)}, ${p(3)})` + + onConflictIgnore("subject_id, scope, role"), + [subjectId, scopeKey(scope), role], ); }, async revokeRole(subjectId, role, scope) { await db.exec( - "DELETE FROM _wrn_authz_assignment WHERE subject_id = ? AND scope = ? AND role = ?", + `DELETE FROM _wrn_authz_assignment WHERE subject_id = ${p(1)} AND scope = ${p(2)} AND role = ${p(3)}`, [subjectId, scopeKey(scope), role], ); }, async grant(subjectId, permission, effect, scope) { - const key = scopeKey(scope); - // Re-granting replaces the effect, so delete then insert. await db.exec( - "DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?", - [subjectId, key, permission], - ); - await db.exec( - "INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (?, ?, ?, ?)", - [subjectId, key, permission, effect], + `INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (${p(1)}, ${p(2)}, ${p(3)}, ${p(4)})` + + onConflict( + "subject_id, scope, permission", + "effect = " + (dialect === "mysql" ? "VALUES(effect)" : "excluded.effect"), + ), + [subjectId, scopeKey(scope), permission, effect], ); }, async revokeGrant(subjectId, permission, scope) { await db.exec( - "DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?", + `DELETE FROM _wrn_authz_grant WHERE subject_id = ${p(1)} AND scope = ${p(2)} AND permission = ${p(3)}`, [subjectId, scopeKey(scope), permission], ); }, @@ -2768,8 +2843,8 @@ export function dbPermissionStore(db: Db): PermissionStore { async listSubjects(scope) { const key = scopeKey(scope); const rows = await db.all<{ subject_id: string }>( - "SELECT subject_id FROM _wrn_authz_assignment WHERE scope = ? " + - "UNION SELECT subject_id FROM _wrn_authz_grant WHERE scope = ?", + `SELECT subject_id FROM _wrn_authz_assignment WHERE scope = ${p(1)} ` + + `UNION SELECT subject_id FROM _wrn_authz_grant WHERE scope = ${p(2)}`, [key, key], ); return [...new Set(rows.map((row) => row.subject_id))];