diff --git a/core/src/authorization/store.ts b/core/src/authorization/store.ts index 5e30d0c..8687db3 100644 --- a/core/src/authorization/store.ts +++ b/core/src/authorization/store.ts @@ -382,9 +382,10 @@ export function createAuthorizationStore( revokeGrant: async (input: RevokeGrantInput): Promise => { try { await db.transaction(async (tx) => { - // The tuple columns are immutable once written, so reading them before taking the lock - // is safe; whether the target is still unrevoked is re-decided under the lock by the - // update predicate below. + // This first read exists only to discover the equivalence tuple, which is safe to read + // before the lock because those columns are immutable once written. It deliberately does + // *not* settle whether the revocation may proceed: everything it observed can change + // while this transaction waits for the lock below. const [target] = await tx .select({ permissionId: ekklesiaCoreGrant.permissionId, @@ -410,6 +411,40 @@ export function createAuthorizationStore( await lockEquivalence(tx, target); + // Revalidate the *addressed* grant now that the lock is held. Without this, a revocation + // that paused between the read above and the lock could wake after its target was + // revoked and a fresh equivalent created, and the tuple-matching update below would + // silently revoke that replacement — reporting success for a grant the caller never + // addressed, in an order no serial execution could produce, and losing the not-found + // result an already-revoked target must return. + // + // The check is deliberately narrow and re-asserts every condition the pre-lock read + // relied on: the exact addressed id, still unrevoked, still satisfying the caller's + // explicit/relationship source guard, and still carrying the same equivalence tuple the + // lock was taken for. It is a plain read, not `for update`: the tuple advisory lock + // remains the one create/revoke serialization order, so no second lock ordering — and no + // deadlock path — is introduced. + const [addressed] = await tx + .select({ id: ekklesiaCoreGrant.id }) + .from(ekklesiaCoreGrant) + .where( + and( + eq(ekklesiaCoreGrant.id, input.grantId), + isNull(ekklesiaCoreGrant.revokedAt), + ...(input.sourceKind === undefined + ? [] + : [eq(ekklesiaCoreGrant.sourceKind, input.sourceKind)]), + equivalenceCondition(target), + ), + ) + .limit(1); + if (!addressed) { + throw new AuthorizationStoreError("AUTHORIZATION_GRANT_NOT_FOUND"); + } + + // Only now is the whole equivalence class cleared, which is what still removes genuine + // legacy duplicates — including expired-but-unrevoked ones. Narrowing this update to the + // addressed id instead would not have been a fix; it would have dropped that behavior. const rows = await tx .update(ekklesiaCoreGrant) .set({ @@ -420,7 +455,9 @@ export function createAuthorizationStore( .where(and(equivalenceCondition(target), isNull(ekklesiaCoreGrant.revokedAt))) .returning(); if (rows.length === 0) { - // A concurrent revocation committed between the lookup and the lock. + // Unreachable while the lock is held, since the row confirmed above satisfies this + // same predicate — kept as a fail-closed guard against a writer that bypasses the + // lock, rather than silently auditing nothing. throw new AuthorizationStoreError("AUTHORIZATION_GRANT_NOT_FOUND"); } diff --git a/core/tests/authorization-store.integration.ts b/core/tests/authorization-store.integration.ts index 3bf02d1..ce28307 100644 --- a/core/tests/authorization-store.integration.ts +++ b/core/tests/authorization-store.integration.ts @@ -1085,3 +1085,200 @@ describe.sequential("live Core authorization store — bounded keyset pagination await safeEnd(admin); }); }); + +/** + * A revocation must never succeed by revoking a *replacement* grant it never addressed. + * + * `revokeGrant` discovers the addressed grant's equivalence tuple before taking that tuple's + * advisory lock, because the tuple columns are immutable once written. The all-equivalent update + * that follows matches on the tuple, not on the addressed id — so without an under-lock + * revalidation of the target itself, a revoker that paused between its target read and the lock + * could wake up after the target was revoked and a fresh equivalent created, and then revoke that + * replacement while reporting success. No serial order explains that outcome, and it contradicts + * the rule that an already-revoked target keeps not-found behavior. + * + * Reproducing it needs the interleaving forced, not raced: an ordinary `Promise.all` would depend + * on timing luck. The tuple advisory lock is cooperative, so a test-only connection can hold the + * exact key the store computes — using the identical SQL expression, not a re-derived constant — + * while ordinary `UPDATE`/`INSERT` statements proceed normally. That is the same test-only + * administrative provisioning this suite already uses to seed legacy duplicates and force + * expirations; no production test hook exists to pause the store. + */ +describe.sequential("live Core authorization store — stale revocation under the equivalence lock", () => { + /** Bounded, condition-based wait — never an arbitrary sleep. Returns once a backend is genuinely + * blocked on an advisory lock in this disposable database, which is the only way to know the + * stale revocation has passed its target read and reached the lock. */ + async function awaitAdvisoryLockWait(client: Client): Promise { + const deadline = Date.now() + 20_000; + while (Date.now() < deadline) { + const rows = await safeQuery<{ waiting: number }>( + client, + `select count(*)::int as waiting + from pg_locks l + join pg_stat_activity a on a.pid = l.pid + where l.locktype = 'advisory' + and not l.granted + and a.wait_event_type = 'Lock' + and a.wait_event = 'advisory' + and l.database = (select oid from pg_database where datname = current_database())`, + ); + if (Number(rows[0]?.waiting ?? 0) > 0) { + return; + } + await new Promise((resolve) => setTimeout(resolve, 25)); + } + throw harnessError("The stale revocation never reached the advisory-lock wait."); + } + + /** An independent outer deadline, so a regression can never hang the suite. The timer is always + * cleared, including when the awaited promise rejects. */ + async function withDeadline(operation: Promise, timeoutMs: number): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + operation, + new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(harnessError("The stale revocation did not settle before its deadline.")), + timeoutMs, + ); + }), + ]); + } finally { + clearTimeout(timer); + } + } + + it("rejects a revocation whose addressed target was revoked and replaced while it waited for the tuple lock", async () => { + const database = await createDisposableDatabase("stale_revoke"); + await applyCoreMigrations(database.url); + const admin = await openAdministrativeClient(database.url); + const lockHolder = await openAdministrativeClient(database.url); + const handle = createAuthorizationDrizzleDatabase(database.url); + let staleSettled: Promise | undefined; + let lockHeld = false; + + try { + const subject = parseEkklesiaUserId("stale_subject"); + const manager = parseEkklesiaUserId("stale_manager"); + await seedUser(admin, subject, "stale-subject@example.test"); + await seedUser(admin, manager, "stale-manager@example.test"); + const store = createAuthorizationStore(handle.db); + const actor = createUserActor(manager); + const permission = parsePermissionId("calendar.events.stale"); + + const firstGrant = await store.createGrant({ + actor, + permission, + sourceKind: "explicit", + userId: subject, + }); + + // Hold the exact tuple key the store derives, computed with the identical SQL expression. + await safeQuery(lockHolder, "begin"); + await safeQuery( + lockHolder, + `select pg_advisory_xact_lock(hashtextextended(jsonb_build_array( + $1::text, $2::text, null::text, 'explicit'::text, null::text + )::text, 0))`, + [subject, permission], + ); + lockHeld = true; + + // The stale revoker reads its target successfully, then blocks on the held lock. + const stale = store.revokeGrant({ actor, grantId: firstGrant.id, sourceKind: "explicit" }); + staleSettled = stale.then( + () => ({ status: "fulfilled" as const }), + (error: unknown) => ({ error, status: "rejected" as const }), + ); + await awaitAdvisoryLockWait(admin); + + // Committed while the stale revoker waits: the target is revoked and audited, then a fresh + // equivalent replacement is created — exactly what a concurrent revoker and creator produce. + await safeQuery( + admin, + `update "ekklesia_core_grant" + set revoked_at = now(), revoked_by_actor_type = 'user', revoked_by_actor_id = $2 + where id = $1`, + [firstGrant.id, manager], + ); + await safeQuery( + admin, + `insert into "ekklesia_core_authorization_audit" + (action, actor_type, actor_id, subject_user_id, permission_id, grant_id) + values ('grant.revoke', 'user', $1, $2, $3, $4)`, + [manager, subject, permission, firstGrant.id], + ); + const replacementRows = await safeQuery<{ id: string }>( + admin, + `insert into "ekklesia_core_grant" + (user_id, permission_id, scope, source_kind, source_ref, created_by_actor_type, created_by_actor_id) + values ($1, $2, null, 'explicit', null, 'user', $3) + returning id`, + [subject, permission, manager], + ); + const replacementId = replacementRows[0]?.id as string; + expect(replacementId).toBeDefined(); + + // Release the cooperative lock and let the stale revoker resume. + await safeQuery(lockHolder, "commit"); + lockHeld = false; + + const outcome = (await withDeadline(staleSettled, 30_000)) as { + error?: unknown; + status: "fulfilled" | "rejected"; + }; + + // The addressed target no longer exists in the state it was addressed in. + expect(outcome.status).toBe("rejected"); + expect(outcome.error).toBeInstanceOf(AuthorizationStoreError); + expect(outcome.error).toMatchObject({ code: "AUTHORIZATION_GRANT_NOT_FOUND" }); + + // The replacement it never addressed is untouched, and no audit row claims otherwise. + const replacement = await safeQuery<{ revoked_at: Date | null }>( + admin, + `select revoked_at from "ekklesia_core_grant" where id = $1`, + [replacementId], + ); + expect(replacement[0]?.revoked_at).toBeNull(); + expect( + await safeQuery( + admin, + `select id from "ekklesia_core_authorization_audit" + where action = 'grant.revoke' and grant_id = $1`, + [replacementId], + ), + ).toHaveLength(0); + + // The end state is consistent with a serial order: revoke(G1), create(G2), reject(stale). + expect(await store.hasActiveGrant({ now: new Date(), permission, userId: subject })).toBe( + true, + ); + const effective = await safeQuery<{ n: number }>( + admin, + `select count(*)::int as n from "ekklesia_core_grant" + where user_id = $1 and permission_id = $2 and revoked_at is null`, + [subject, permission], + ); + expect(Number(effective[0]?.n)).toBe(1); + const revokeAudit = await safeQuery<{ grant_id: string }>( + admin, + `select grant_id from "ekklesia_core_authorization_audit" where action = 'grant.revoke'`, + ); + expect(revokeAudit.map((row) => row.grant_id)).toEqual([firstGrant.id]); + } finally { + // The lock is released first, so a failed assertion above can never leave the stale + // revocation blocked forever; every client, pool, promise, and database is then cleaned up. + if (lockHeld) { + await safeQuery(lockHolder, "rollback").catch(() => undefined); + } + if (staleSettled) { + await withDeadline(staleSettled, 30_000).catch(() => undefined); + } + await safeEnd(lockHolder).catch(() => undefined); + await safeEnd(admin).catch(() => undefined); + await handle.close().catch(() => undefined); + await dropDatabase(database.name).catch(() => undefined); + } + }); +});