fix: revalidate grant revocation under lock
`revokeGrant` discovers the addressed grant's equivalence tuple before taking that tuple's advisory lock, which is safe because those columns are immutable. The all-equivalent update that follows then matches on the tuple rather than on the addressed id — and nothing re-checked the addressed grant once the lock was finally held. That allowed a revocation to succeed by revoking a grant it never addressed. A revoker reads its target and stalls before the lock; a second revoker takes the lock, revokes that same target, audits it, commits; a creator takes the lock, sees no effective equivalent, creates a replacement, commits; the first revoker finally acquires the lock, finds its stale tuple still valid, and revokes the replacement — returning success for an already-revoked target that should have been not-found. No serial order produces those three results together. Reproduced deterministically rather than by racing: a test-only connection holds the exact tuple key, computed with the identical SQL expression the store uses, while the stale revocation is proven — via `pg_locks`/`pg_stat_activity`, not assumed — to be blocked on that lock; ordinary administrative statements then commit the revoked target, its audit row, and the replacement, because the advisory lock is cooperative. Before the fix the stale revocation fulfilled, the replacement was revoked, and a `grant.revoke` audit row was written for it. The correction re-reads the addressed grant after the lock is acquired, re-asserting every condition the pre-lock read relied on: the exact id, still unrevoked, still matching the caller's explicit/relationship source guard, and still carrying the tuple the lock was taken for. A miss is AUTHORIZATION_GRANT_NOT_FOUND. It is a plain read, not `for update`, so the tuple advisory lock remains the single create/revoke serialization order and no second lock ordering or deadlock path is introduced. The broad update stays untouched, because narrowing it to the addressed id would have stopped clearing genuine legacy duplicates — the behavior this pass exists to provide. The comment claiming the target's unrevoked state was re-decided by the update predicate described a sequence the code did not implement, and is corrected. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
ekklesia
co-authored by
Claude Opus 5
Claude Opus 5
parent
5c589d8fec
commit
4a34a8203c
@@ -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<void> {
|
||||
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<T>(operation: Promise<T>, timeoutMs: number): Promise<T> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
return await Promise.race([
|
||||
operation,
|
||||
new Promise<never>((_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<unknown> | undefined;
|
||||
let lockHeld = false;
|
||||
|
||||
try {
|
||||
const subject = parseEkklesiaUserId("stale_subject");
|
||||
const manager = parseEkklesiaUserId("stale_manager");
|
||||
await seedUser(admin, subject, "[email protected]");
|
||||
await seedUser(admin, manager, "[email protected]");
|
||||
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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user