diff --git a/packages/authz/package.json b/packages/authz/package.json index 4fd24545..94afc646 100644 --- a/packages/authz/package.json +++ b/packages/authz/package.json @@ -7,5 +7,8 @@ "exports": { ".": "./src/index.ts", "./db": "./src/db.ts" + }, + "dependencies": { + "@wrnexus/db": "workspace:*" } } diff --git a/packages/authz/src/db.ts b/packages/authz/src/db.ts index 8e2aa24c..69ba7578 100644 --- a/packages/authz/src/db.ts +++ b/packages/authz/src/db.ts @@ -1,6 +1,6 @@ import type { Db, Dialect } from "@wrnexus/db"; import { authzMigrationSql } from "./migrations.ts"; -import { scopeKey, type GrantEffect, type PermissionStore } from "./store.ts"; +import { scopeKey, type PermissionStore } from "./store.ts"; import type { AuthzScope, SubjectAssignments } from "./types.ts"; // Re-exported so `@wrnexus/authz/db` is the single entry point for everything @@ -8,72 +8,84 @@ 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};`); - } +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. Do the delete+insert inside a - // transaction so a failed insert can't leave the row missing. - await db.tx(async (tx) => { - await tx.exec( - "DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?", - [subjectId, key, permission], - ); - await tx.exec( - "INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (?, ?, ?, ?)", - [subjectId, key, permission, effect], - ); - }); + await db.exec( + `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], ); }, @@ -81,8 +93,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))]; diff --git a/packages/authz/src/migrations.ts b/packages/authz/src/migrations.ts index 19117af0..e4107c62 100644 --- a/packages/authz/src/migrations.ts +++ b/packages/authz/src/migrations.ts @@ -1,11 +1,17 @@ import type { Dialect } from "@wrnexus/db"; /** - * DDL for the two assignment tables. `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). + * 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 } { +export function authzMigrationSql(dialect: Dialect): { up: string[]; down: string[] } { const id = dialect === "postgres" ? "SERIAL PRIMARY KEY" @@ -13,31 +19,31 @@ 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 = "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"], + }; } diff --git a/packages/authz/src/store.ts b/packages/authz/src/store.ts index a8848814..89c1fe7e 100644 --- a/packages/authz/src/store.ts +++ b/packages/authz/src/store.ts @@ -16,9 +16,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 { diff --git a/packages/authz/test/migrations.test.ts b/packages/authz/test/migrations.test.ts new file mode 100644 index 00000000..81064a4f --- /dev/null +++ b/packages/authz/test/migrations.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, test } from "bun:test"; +import { authzMigrationSql } from "../src/migrations.ts"; + +/** + * The postgres/mysql DDL is generated but never exercised against a real + * server in this repo, so it has to be asserted statically: the id column + * type, the `effect` CHECK constraint (an unrecognised value must not vanish + * from both the grant and deny buckets), the MySQL binary collation (so + * tenant "T1" cannot match "t1" and role "admin" cannot collapse with + * "Admin"), and both UNIQUE constraints, per dialect. + */ +describe("authzMigrationSql", () => { + test("sqlite: autoincrement id, no collation, both constraints", () => { + const { up, down } = authzMigrationSql("sqlite"); + expect(up).toHaveLength(2); + const [assignment, grant] = up; + + expect(assignment).toContain("id INTEGER PRIMARY KEY AUTOINCREMENT"); + expect(assignment).toContain( + "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)", + ); + expect(assignment).not.toContain("COLLATE"); + + expect(grant).toContain("id INTEGER PRIMARY KEY AUTOINCREMENT"); + expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))"); + expect(grant).toContain( + "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)", + ); + expect(grant).not.toContain("COLLATE"); + + expect(down).toEqual([ + "DROP TABLE IF EXISTS _wrn_authz_grant", + "DROP TABLE IF EXISTS _wrn_authz_assignment", + ]); + }); + + test("postgres: SERIAL id, no collation, both constraints", () => { + const { up } = authzMigrationSql("postgres"); + const [assignment, grant] = up; + + expect(assignment).toContain("id SERIAL PRIMARY KEY"); + expect(assignment).toContain( + "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)", + ); + expect(assignment).not.toContain("COLLATE"); + + expect(grant).toContain("id SERIAL PRIMARY KEY"); + expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))"); + expect(grant).toContain( + "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)", + ); + expect(grant).not.toContain("COLLATE"); + }); + + test("mysql: AUTO_INCREMENT id, binary collation on identity columns, both constraints", () => { + const { up } = authzMigrationSql("mysql"); + const [assignment, grant] = up; + + expect(assignment).toContain("id INT AUTO_INCREMENT PRIMARY KEY"); + expect(assignment).toContain("subject_id VARCHAR(255) COLLATE utf8mb4_bin NOT NULL"); + expect(assignment).toContain("scope VARCHAR(255) COLLATE utf8mb4_bin NOT NULL DEFAULT ''"); + expect(assignment).toContain("role VARCHAR(255) COLLATE utf8mb4_bin NOT NULL"); + expect(assignment).toContain( + "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)", + ); + + expect(grant).toContain("id INT AUTO_INCREMENT PRIMARY KEY"); + expect(grant).toContain("subject_id VARCHAR(255) COLLATE utf8mb4_bin NOT NULL"); + expect(grant).toContain("permission VARCHAR(255) COLLATE utf8mb4_bin NOT NULL"); + expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))"); + expect(grant).toContain( + "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)", + ); + }); +}); diff --git a/packages/authz/test/store-conformance.ts b/packages/authz/test/store-conformance.ts index f0660950..a9154d8e 100644 --- a/packages/authz/test/store-conformance.ts +++ b/packages/authz/test/store-conformance.ts @@ -129,6 +129,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" });